-
Notifications
You must be signed in to change notification settings - Fork 249
Left/right document isolated view with hotkeys #92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| toolbar->AddTool(ID_LEFT_DOC, "Left document", BMP_ARTPROV(wxART_GO_BACK), | ||
| "Show left document (Ctrl <)"); | ||
| toolbar->AddTool(ID_DIFF_DOC, "Diff document", BMP_ARTPROV(wxART_REDO), | ||
| "Show diff document (Ctrl d)"); | ||
| toolbar->AddTool(ID_RIGHT_DOC, "Right document", BMP_ARTPROV(wxART_GO_FORWARD), | ||
| "Show right document (Ctrl >)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These icons seem arbitrary — and semantically incorrect.
| switch ( m_display_mode ) | ||
| { | ||
| case SHOW_LEFT_DOCUMENT: | ||
| // |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh?
| if ( img1 ) | ||
| m_viewer->Set(img1); | ||
| else if ( img2 ) | ||
| m_viewer->Set(img2); | ||
| else | ||
| m_viewer->Set(NULL); | ||
| break; | ||
|
|
||
| case SHOW_RIGHT_DOCUMENT: | ||
| if ( img2 ) | ||
| m_viewer->Set(img2); | ||
| else if ( img1 ) | ||
| m_viewer->Set(img1); | ||
| else | ||
| m_viewer->Set(NULL); | ||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t understand why this code shows left-side image on the right side and vice versa?
| m_offset = wxPoint(0, 0); | ||
| DoUpdatePage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do these reset the offset — seems unrelated?
Please also don’t duplicate code like this, use a helper function.
| EVT_TOOL (ID_LEFT_DOC, DiffFrame::OnShowLeftDocument) | ||
| EVT_TOOL (ID_DIFF_DOC, DiffFrame::OnShowDiffDocument) | ||
| EVT_TOOL (ID_RIGHT_DOC, DiffFrame::OnShowRightDocument) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong indentation, D is not aligned.
| It is also possible to shift the two pages relatively to each other using | ||
| Ctrl-arrows (Cmd-arrows on MacOS). This is useful for identifying translation-only differences. | ||
|
|
||
| You can also use Ctrl < and Ctrl > (Cmd < and Cmd > on MacOS) show the left and right documents respecively. Ctrl d to return back to the diff view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the standard way of referring to combined keypresses, which is Ctrl+D etc.
Sometimes the red/blue diff view is a bit confusing and its easier to just check the left/right documents individually. This PR adds buttons to toggle between 3 modes:
You can also use
Ctrl <andCtrl >to hot switch between the Left/Right view.Ctrl dreturns to the default Diff view.Futher more, the
Ctrl dsets the offsets to 0, 0 (acts as a clear in case you want to quickly reset the offset)Futher Improvements
[ ] Show the names of the current document view (Left: file.pdf/ Right other.pdf / Diff)
[ ] Handle Missing pages (currently the diff doesn't handle missing pages and just shows changes for all subsequent pages)