Left/right document isolated view with hotkeys #92
Conversation
diff-pdf.cpp
Outdated
| 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.
These icons seem arbitrary — and semantically incorrect.
There was a problem hiding this comment.
I am open to suggestions regarding the icons. I wanted something to indicate the left document (left arrow) right document (right arrow) and diff (I am using the redo arrow as its sort of returning to the default comparison mode)
I am not familiar with wx icons so if you have better suggestions I am open to change this :)
diff-pdf.cpp
Outdated
| switch ( m_display_mode ) | ||
| { | ||
| case SHOW_LEFT_DOCUMENT: | ||
| // |
| 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.
I don’t understand why this code shows left-side image on the right side and vice versa?
There was a problem hiding this comment.
In my head img1 is the Left document and img2 is the right document.
The idea is if we are in the SHOW_RIGHT_DOCUMENT mode, we try showing the right document, however if the image doesn't exist (if the documents aren't the same length, we just show the left document. Seemed to make most sense in my usecase scenario, however I could change this to just display some kind of default image (like Page, not available....)
diff-pdf.cpp
Outdated
| m_offset = wxPoint(0, 0); | ||
| DoUpdatePage(); |
There was a problem hiding this comment.
Why do these reset the offset — seems unrelated?
Please also don’t duplicate code like this, use a helper function.
There was a problem hiding this comment.
I agree, there should be a dedicated button (shortcut for this)
In my use case scenario I found it usefult to use the the Ctrl+D shortcut to not only return to the the diff mode but also the reset the offset. I removed it in the update
diff-pdf.cpp
Outdated
| 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.
Wrong indentation, D is not aligned.
There was a problem hiding this comment.
Fixed the indent.... would you like me to set up a clang-format to keep the style consistent?
README.md
Outdated
| 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.
Please use the standard way of referring to combined keypresses, which is Ctrl+D etc.
There was a problem hiding this comment.
Made the update. I tried staying consistent with the existing convention in the code. Perhaps this should be fixed also for the other combined key-presses for the zoom and offset:
toolbar->AddTool(ID_ZOOM_IN, "Zoom in", BMP_ZOOM_IN,
"Make the page larger (Ctrl +)");
toolbar->AddTool(ID_ZOOM_OUT, "Zoom out", BMP_ZOOM_OUT,
"Make the page smaller (Ctrl -)");
toolbar->AddTool(ID_OFFSET_LEFT, "", BMP_OFFSET_LEFT,
"Offset one of the pages to the left (Ctrl left)");
toolbar->AddTool(ID_OFFSET_RIGHT, "", BMP_OFFSET_RIGHT,
"Offset one of the pages to the right (Ctrl right)");
toolbar->AddTool(ID_OFFSET_UP, "", BMP_OFFSET_UP,
"Offset one of the pages up (Ctrl up)");
toolbar->AddTool(ID_OFFSET_DOWN, "", BMP_OFFSET_DOWN,
"Offset one of the pages down (Ctrl down)");
|
Hi @vslavik thanks for the review. I will try addressing some of the points you raise directly in the comments, and will push an update. |
2 participants
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)