Zoombutton #1073
Zoombutton #1073
Conversation
Created ZoomMouseHandler, and an apply_fixed_zoom that takes a zoom target coordinate
…ra white background.
|
Can you use these zoom in/out icons instead (you may have to modify size to be 24x24 to conform to other tool icons):
Note: to get source, click on the icon and then show source in your browser. |
Update ImageCanvasItem.py Fixed some type issues with return value of a function being Optional. Update ImageCanvasItem.py Fixed some more type errors. Update ImageCanvasItem.py Re-attempt at fixing some of the type errors.
Changed icons as per request, renamed the file references and files to be based around 'zoom' rather than 'magifying' as that has extra connotations.
Removing old icon files.
|
I didn't get to this today - but a question anyway: is this ready for me to review? |
Yes it should be ready for a first review, I am still trying to figure if I can add the drag-to-zoom stuff but that will come in as a second PR. |
There was a problem hiding this comment.
After trying this branch, a few comments:
I think we should put a single magnifying glass icon in the tool bar: zoom in by 25% by default; out by 25% if the shift key is held down. It shouldn't have a "plus" or "minus" on the inside. That's a bit of a hidden capability to zoom out (by holding shift) but it is in line with other applications and much less tedious than having two tools.
Should we also use the 'z' key to select the zoom tool?
When clicking with the zoom tool, it doesn't zoom in/out around the point that you click. You'll need to adjust the image_position value such that ImageCanvasItemMapping.map_point_widget_to_image for the mouse point is invariant before and after the zoom. I didn't look closely at the algorithm, but I think you can track the necessary information down from this line:
nionswift/nion/swift/ImageCanvasItem.py
Line 978 in c142d86
Clicking multiple times should keep the click-upon feature exactly under the same spot under the cursor.
Adding some checks to the test for this would be good.
Adjusted icons to have transparent background. Changed zoom to be focussed on the cursor, rather than centering on the cursor.
Fixed an issue with zooming out not keeping cursor held correctly, as well as some typing errors buried in mypy.
|
Can we move this PR forward or close it? Mainly this should be a single 'zoom' tool that zooms in when clicked and zooms out when alt-clicked (Windows). The code looks like it has an Note: The svg/png file for the zoom tool icon are here: #1073 (comment) |
|
I added #1137 to limit the scope of this change and added it to the 16.12 milestone. This PR is very close, just needs to replace the two tools with the one and zoom out on Alt+click. |
Removed references to drag-to-zoom. Changed to single button with modifier (ctrl) for zoom out.
|
Aligned the PR along with the discussed scope. |
As per Kev's review.
Update ImageCanvasItem.py Fixed some type issues with return value of a function being Optional. Update ImageCanvasItem.py Fixed some more type errors. Update ImageCanvasItem.py Re-attempt at fixing some of the type errors.
Changed icons as per request, renamed the file references and files to be based around 'zoom' rather than 'magifying' as that has extra connotations.
Adjusted icons to have transparent background. Changed zoom to be focussed on the cursor, rather than centering on the cursor.
Fixed an issue with zooming out not keeping cursor held correctly, as well as some typing errors buried in mypy.
Removed references to drag-to-zoom. Changed to single button with modifier (ctrl) for zoom out. Removing unnecessary files and whitespace As per Kev's review.
Fixed problems with changing from fill/fit/1:1 on initial zoom. Fixed issue with zooming out sometimes walking in x/y.
…into zoombutton
Removed the duplicate call added with a merge somehow.
Corrected the zoom_in/out functions to be able to be called without specifying a zoom factor by providing a default.
There was a problem hiding this comment.
Thanks for fixing the previous set of issues. I have a couple of minor coding comments; but mostly I've added a test which checks the actual zooming behavior from 'fit' mode with a centered image - and it shows that the zoom is not actually taking place around the cursor position. We'll need a bunch more tests - unfortunately zooming isn't as trivial to get right.
| def test_zoom_tool_in_and_out_around_clicked_point(self): | ||
| # setup | ||
| with TestContext.create_memory_context() as test_context: | ||
| document_controller = test_context.create_document_controller() | ||
| document_model = document_controller.document_model | ||
| display_panel = document_controller.selected_display_panel | ||
| data_item = DataItem.DataItem(numpy.zeros((10, 10))) | ||
| document_model.append_data_item(data_item) | ||
| display_item = document_model.get_display_item_for_data_item(data_item) | ||
| copy_display_item = document_model.get_display_item_copy_new(display_item) | ||
| display_panel.set_display_panel_display_item(copy_display_item) | ||
| header_height = display_panel.header_canvas_item.header_height | ||
| display_panel.root_container.layout_immediate((1000 + header_height, 1000)) | ||
| # run test | ||
| document_controller.tool_mode = "zoom" | ||
| display_panel.display_canvas_item.simulate_press((100, 125)) | ||
| display_panel.display_canvas_item.simulate_release((100, 125)) | ||
|
|
||
| document_controller.tool_mode = "zoom" | ||
| display_panel.display_canvas_item.simulate_press((125, 100), KeyboardModifiers.control) | ||
| display_panel.display_canvas_item.simulate_release((125, 100), KeyboardModifiers.control) |
There was a problem hiding this comment.
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 passed KeyboardModifiers.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:
def test_zoom_tool_in_and_out_around_clicked_point(self):
with TestContext.create_memory_context() as test_context:
document_controller = test_context.create_document_controller()
document_model = document_controller.document_model
display_panel = document_controller.selected_display_panel
data_item = DataItem.DataItem(numpy.zeros((50, 50)))
document_model.append_data_item(data_item)
display_item = document_model.get_display_item_for_data_item(data_item)
display_panel.set_display_panel_display_item(display_item)
header_height = display_panel.header_canvas_item.header_height
display_panel.root_container.layout_immediate((100 + header_height, 100))
# run test. each data pixel will be spanned by two display pixels. clicking at an odd value
# will be a click in the center of a data pixel. zoom should expand and contract around that
# pixel. there is also a check to make sure zooming is not just ending up with the same exact
# zoom, which would also succeed.
self.assertEqual((25, 25), display_panel.display_canvas_item.map_widget_to_image(Geometry.IntPoint(51, 51)))
self.assertEqual((30, 30), display_panel.display_canvas_item.map_widget_to_image(Geometry.IntPoint(61, 61)))
document_controller.tool_mode = "zoom"
display_panel.display_canvas_item.simulate_click((26, 26))
# the zoom tool runs asynchronously, so give it a slice of async time.
document_controller.periodic()
# check results
self.assertEqual((25, 25), display_panel.display_canvas_item.map_widget_to_image(Geometry.IntPoint(51, 51)))
self.assertNotEqual((30, 30), display_panel.display_canvas_item.map_widget_to_image(Geometry.IntPoint(61, 61)))
# zoom out is done by passing the alt key to the canvas item.
display_panel.display_canvas_item.simulate_click((26, 26), CanvasItem.KeyboardModifiers(alt=True))
document_controller.periodic()
# zoom in at the center of a data pixel followed by zoom out in the same spot should end up in the same original mapping.
self.assertEqual((25, 25), display_panel.display_canvas_item.map_widget_to_image(Geometry.IntPoint(51, 51)))
self.assertEqual((30, 30), display_panel.display_canvas_item.map_widget_to_image(Geometry.IntPoint(61, 61)))
There was a problem hiding this comment.
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.
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.
Added a new function to verify Tuples are 'close', with a provided delta or precision. Changed the unit test to work, as well as checking the mouse position is approximately correct. Future Issue needs to verify rendering precision on the layout which may be introducing inaccuracies with mouse->image offsets.
nion/swift/ImageCanvasItem.py
Outdated
| # Apply a zoom factor to the widget, optionally focused on a specific point | ||
| def apply_fixed_zoom(self, zoom_in: bool, coord: typing.Optional[Geometry.IntPoint]) -> None: | ||
| zoom_factor = 0.25 | ||
| self.set_custom_mode() # Put us into custom canvas mode |
There was a problem hiding this comment.
set_custom_mode and zoom_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 calling zoom_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.
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.
| @@ -26,6 +29,16 @@ def setUp(self): | |||
| def tearDown(self): | |||
| TestContext.end_leaks(self) | |||
|
|
|||
| def assertTupleAlmostEqual(self, tuple1, tuple2, delta=None, places=None): | |||
There was a problem hiding this comment.
This has no typing annotations, and should go outside the class. It does not use self.
There was a problem hiding this comment.
It uses Self throughout? The typing annotations need adding and that will be done.
| self.assertTupleAlmostEqual((25, 25), | ||
| display_panel.display_canvas_item.map_widget_to_image(Geometry.IntPoint(51, 51)), | ||
| delta=math.ceil((25*0.1)+1)) | ||
| self.assertTupleAlmostEqual((30, 30), | ||
| display_panel.display_canvas_item.map_widget_to_image(Geometry.IntPoint(61, 61)), | ||
| delta=math.ceil((30*0.1)+1)) |
There was a problem hiding this comment.
Why did you make this section less restrictive? In doing so, I think you've actually added a bug to the tests. If you zoom out around 51,51, then the mapping at 61,61 should NOT be equal (see my original test). But your test has changed it to test to be equal - which means your test is succeeded where it should be failing...?
There was a problem hiding this comment.
Upon further inspection, math.ceil((30*0.1)+1) gives a range of +/- 4 (!) pixels. That's just wrong. It should be far more restrictive than that even if you don't want to check exact floating point numbers. Please tell me this wasn't an AI tool that suggested that code...? A reasonable range would be 10e-3 at first glance.
There was a problem hiding this comment.
This was an error in the unit test case that it was not verifying against the wrong numbers causing the test to fail unexpectedly. This has been resolved, and we can discuss the remaining discrepancy.
Refactored the zoom function to not be a mousehandler. Consolidated functions to remove duplicate code, so now zoom_in and zoom_out call down to a single zoom routine. Fixed an error in the calculation of what zoom level the various display modes are in when switching to custom. Consolidated the display_properties_command into a single call at the end once all parameters are known.
Corrected an error with the fit_mode test where the expected result was set incorrectly. Added detailed breakdown of the maths in comments of the expectation. Added an additional unit test for 1:1 mode. Added typing to assertTupleAlmostEqual.
|
Squashed and merged here c2a389a |
Initial draft pull request to run tests.