Zoombutton

[Tiomat85]

Initial draft pull request to run tests.

[cmeyer]

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.

[cmeyer]

I know this is still draft, and I'm only reviewing the cursor icons.

[cmeyer]

I didn't get to this today - but a question anyway: is this ready for me to review?

[Tiomat85]

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.

[cmeyer]

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

image_canvas_rect = calculate_origin_and_size(scroll_area_canvas_size, self.__data_shape, self.__image_canvas_mode, self.__image_zoom, self.__image_position)

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.

[cmeyer]

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 if False... in it, too, that should be removed if unused.

Note: The svg/png file for the zoom tool icon are here: #1073 (comment)

[cmeyer]

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.

[Tiomat85]

Aligned the PR along with the discussed scope.

[Ion-e]

Draft looks good to me.

[cmeyer]

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.

[cmeyer]

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)))

[cmeyer]

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.

[Tiomat85]

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.

[cmeyer]

Comments say it all.

[cmeyer]

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?

[Tiomat85]

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.

[cmeyer]

This has no typing annotations, and should go outside the class. It does not use self.

[Tiomat85]

It uses Self throughout? The typing annotations need adding and that will be done.

[cmeyer]

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...?

[cmeyer]

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.

[Tiomat85]

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.

[cmeyer]

Squashed and merged here c2a389a