Skip to content
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

Added fit in view action #2206

Merged
merged 3 commits into from Oct 23, 2019
Merged

Added fit in view action #2206

merged 3 commits into from Oct 23, 2019

Conversation

@mateosss
Copy link
Contributor

mateosss commented Oct 21, 2019

From #2184 discussion.

It implements the "Fit In View" action.

It uses Ctrl-Shift-J (like gimp) however it may have been better to use 5 (like inkscape) as 2, 4, 6 and 8 are already used for viewport navigation. Unfortunately when assigning 5 as the action shortcut QAction::event: Ambiguous shortcut overload: 5 is printed and I wasn't able to find the cause.

Another thing to consider is that I reused zoom-original.png (same as actionZoomNormal), is there a place to get a proper icon for this action?

Copy link
Owner

bjorn left a comment

That's a great addition, thanks!

It uses Ctrl-Shift-J (like gimp) however it may have been better to use 5 (like inkscape) as 2, 4, 6 and 8 are already used for viewport navigation. Unfortunately when assigning 5 as the action shortcut QAction::event: Ambiguous shortcut overload: 5 is printed and I wasn't able to find the cause.

In Inkscape, the 5 makes sense but in Tiled the other numbers are not used for navigation so it doesn't really fit. In fact you're getting shortcut ambiguity problems with numbers because they are used to restore a certain tile selection.

Another thing to consider is that I reused zoom-original.png (same as actionZoomNormal), is there a place to get a proper icon for this action?

I think the other icons are from the GNOME icon theme, but they appear to have changed in the meantime. One option would be to update all of them to the current versions of that theme. I can push this to your branch.

I left a few inline comments.

src/tiled/mapview.cpp Outdated Show resolved Hide resolved
src/tiled/mainwindow.cpp Outdated Show resolved Hide resolved
src/tiled/mainwindow.ui Outdated Show resolved Hide resolved
<string>Fit In View</string>
</property>
<property name="shortcut">
<string>Ctrl+Shift+J</string>

This comment has been minimized.

Copy link
@bjorn

bjorn Oct 22, 2019

Owner

In general I think it's good to copy shortcuts from other applications, though I wonder a little about the reasoning about this one in GIMP. But it's fine until we find a better option. Personally, since + and - are used for zooming, I could imagine using * or / for this action.

bjorn added 2 commits Oct 22, 2019
These are copied from the GNOME icon theme, as noted in the AUTHORS.md
file.
* Be more clear that it only applies to maps and disable the action
  when a tileset is selected.

* Fixed the area that it fits into the view in case multiple maps are
  visible as part of a world.
@bjorn
bjorn approved these changes Oct 23, 2019
@bjorn bjorn merged commit 808b0c4 into bjorn:master Oct 23, 2019
1 of 3 checks passed
1 of 3 checks passed
build
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Oct 23, 2019

I've made the changes that I requested and some other tweaks and merged this now, because I'm working towards string freeze for Tiled 1.3. Thanks for your work on this!

@mateosss

This comment has been minimized.

Copy link
Contributor Author

mateosss commented Oct 23, 2019

Sorry I couldn't make time during the week. But that's great thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.