Skip to content

Add new box zoom tool, icon and cursor. Hook it into existing GUI. #2360

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

elektronisk
Copy link
Contributor

@elektronisk elektronisk commented Apr 7, 2025

Fixes #229 .

This works fine for me on windows, but I'm unable to test it on tablet.

Note: the assertion in tool_base.cpp is commented because the assertion could be triggered by starting a drag operation (holding left mouse button), then clicking the right mouse button to bring up the pie context menu, and then letting go of the left mouse button, and starting a new drag. I'm not sure what the best way is to deal with the situation. Should I be disabling the pie context menu?

image image
Screen.Recording.2025-04-07.204908.mp4

@dg0yt
Copy link
Member

dg0yt commented Apr 8, 2025

Thank you for the contribution.

I'm unable to test it on tablet.

The new action won't be in the mobile UI until we add it. You can switch to the mobile UI on the PC by enabling "Touch mode" on the welcome screen.

Note: the assertion in tool_base.cpp is commented because the assertion could be triggered by starting a drag operation (holding left mouse button), then clicking the right mouse button to bring up the pie context menu, and then letting go of the left mouse button, and starting a new drag. I'm not sure what the best way is to deal with the situation. Should I be disabling the pie context menu?

You might need to look at when to actual cancel the drag operation. Maybe as soon as another button is pressed.
(Shouldn't there be a similar challenge for drag-to-select?)

…gging, can now reintroduce disabled assertion.

Convert files from CRLF to LF line endings.
@elektronisk
Copy link
Contributor Author

@dg0yt thanks for the feedback.

The new action won't be in the mobile UI until we add it. You can switch to the mobile UI on the PC by enabling "Touch mode" on the welcome screen.

That worked. However I have a hunch that screenspace on a tablet might be constrained, and I don't have a good feel if there is available space for this tool or not. What do you think? Also, should a shortcut key be assigned?

You might need to look at when to actual cancel the drag operation. Maybe as soon as another button is pressed. (Shouldn't there be a similar challenge for drag-to-select?)

Thanks. My latest push uses the setEditingInProgress() during drag to prevent interference from the pie context menu, and restores the assertion in tool_base.h. That seemed like the most straightforward way.

@dg0yt
Copy link
Member

dg0yt commented Apr 8, 2025

That worked. However I have a hunch that screenspace on a tablet might be constrained, and I don't have a good feel if there is available space for this tool or not. What do you think? Also, should a shortcut key be assigned?

Maybe it is not necessary to add it to the mobile UI (now) because it has an alternative: Use two fingers.

@dl3sdo
Copy link
Member

dl3sdo commented Apr 8, 2025

@elektronisk: thank you for your PR, I like the feature.
I noticed that the zoom frame is not always visible, it depends whether objects were selected before:

SometimesNotVisible.mp4

@elektronisk
Copy link
Contributor Author

@elektronisk: thank you for your PR, I like the feature. I noticed that the zoom frame is not always visible, it depends whether objects were selected before:

@dl3sdo Thanks for testing it out. The last commit fixes this.

@elektronisk elektronisk marked this pull request as ready for review April 9, 2025 06:59
@dl3sdo
Copy link
Member

dl3sdo commented Apr 9, 2025

@elektronisk: Tests are now successful, thank you for the fix.

I notice that the zoom tool remains active until the user selects another tool.
Personally I would prefer some easy way to finish zooming (and to change to the previous drawing tool?), e.g. by a left double-click.
What do you think?

@elektronisk
Copy link
Contributor Author

I notice that the zoom tool remains active until the user selects another tool. Personally I would prefer some easy way to finish zooming (and to change to the previous drawing tool?), e.g. by a left double-click. What do you think?

It can make sense to have a hierarchy of tool categories, Edit tools, Draw tools and View tools, where you fall back towards last used draw tool or edit tool when you "leave" the current tool. Using Esc is pretty reasonable for that purpose. It's a more involved change though, affects every tool and deserves separate issue IMO.

Map editor: allow tool change from Box zoom tool, when symbol is selected in symbol widget.
@elektronisk
Copy link
Contributor Author

elektronisk commented Apr 9, 2025

@dl3sdo i did discover functionality for quickly entering draw tool based on selecting a symbol in the symbol widget. The last commit adds this "shortcut" so that it works from this Box zoom tool also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Box Zooming
3 participants