Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Zoombutton #1073
Zoombutton #1073
Changes from 31 commits
be0caf6
a5c13c6
3eca6b2
45833be
94b264e
8058d57
b7c3cd3
01ebe1f
623072c
901fc1d
617ba24
72ed328
6495018
fd5e5f4
21469fc
6f1a70d
853ac50
7761dff
f740530
64c4d88
6ff51a5
deef9bc
1cd42b3
439c3ab
214b008
44c0da4
6955eef
08cb7d2
1f02377
02cd65b
2c1da07
3379976
26fa492
3dd78a5
b5f9fc1
5836671
e3248f2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
set_custom_mode
andzoom_in
add an undo command, which is not what we want. This is going to take some refactoring, but essentially we want to calculate everything and then have a single command at the end of the calculation. It's also a strange way to implement this overall function by setting of the image position followed by callingzoom_in
. Why not just calculate the desired mode, position and zoom and set it directly using__apply_display_properties_command
?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 was having some problems with setting all 3 parameters in one go, that depending on the previous mode 'strange' it would act irratically. Further investigation seems that it may have been a red herring and it was the transition to 'custom' causing problems. See external discussion and related to Issue 1169.
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.
Unfortunately, this test is not testing anything nor even doing the zoom. As a quick explanation, the zoom tool is using an async reactor to handle the mouse tracking; but the test would need to call
document_controller.periodic()
after each mouse click to run the reactor. In addition, the keyboard modifiers values being passedKeyboardModifiers.control
are actually pointers to abstract functions (not even calling the functions - just pointers to them).Here is a test which actually performs the zoom and checks if it nominally works correctly (it fails unfortunately). We'll need several more tests like this to cover the other cases:
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.
After looking this further, I don't think the current layout system is going to be able to support proper zoom. We'll need something like the test above, but there will be limitations because the layout uses the canvas size for zooming, and canvas size is rounded to integer boundaries. Let's do the best we can with the current limitations, but zooming in/out multiple times is going to be off center due to rounding errors. We will fix that in a future PR by changing the layout and composition to accommodate fractional coordinates via scaling and translation.
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.
Indeed, you can see this if you zoom in really far (single digit displayed pixels on a full size canvas) you can really see that. I think there are two distinct problems I identified:
Since it renders entire pixels at large zooms, even with the zoom factor being saved correctly number of pixels rendered doesn't always agree.
Again, when zoomed in a lot the 'coordinate' clicked becomes a more complex question. Was the click on the top left of a pixel, the center of the pixel, or does it need to have 'full resolution' of where abouts in the pixel did you click.