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

Zooming and panning #6441

Merged
merged 5 commits into from
Jan 19, 2021
Merged

Conversation

TurboGraphxBeige
Copy link
Contributor

@TurboGraphxBeige TurboGraphxBeige commented Jan 11, 2021

Added more complete documentation about zooming and panning.

Copy link
Collaborator

@DelazJ DelazJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TurboGraphxBeige
Thanks for providing the docs' update. Much appreciated. I did not really go through the description as I have some "concerns" with the legibility of the new structure. Would like to get your feedback on my suggestion below. Thanks

docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@DelazJ DelazJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better, thanks @TurboGraphxBeige
I miss the "zoom to native resolution" tool.
There's also a need to add link references in https://docs.qgis.org/testing/en/docs/user_manual/introduction/qgis_gui.html#layer, and align occurrences of "zoom to layer(s)" in the whole docs (this can be done in a follow-up PR, to keep backport easier, if I understand @SrNetoChan plans)

docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
- Layer Panel Contextual Menu
- Usage
* - |pan| :sup:`Pan`
- :menuselection:`View --> Pan Map`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- :menuselection:`View --> Pan Map`
- :menuselection:`Pan Map`

Since the header already mentions it, it looks overkill to repeat at each row. Same for the Layers column.
I'm halved whether we should show the icon in this column too, to visually match what people should look for in the GUI (or a first icon column?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be nice to show the icons in the View Menu column. I would show them smaller than in the Map Navigation Toolbar column (already the case in the View menu). Is there a way to override the image size or I need to re-declare a smaller image for each icons?

.. |panSmall| image:: /static/common/mActionPan.png
:width: 1em

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaik, you'd need to declare the new references, either:

  • in the substitution.rst file and add it in the substitutions list at the end of this file
  • or above the substitution list, to limit their scope to this file

But I wonder if this size distinction is really necessary: up to now icons were assigned (almost) the same size, regardless they are on a toolbar or in a menu. Adding a smaller icon for menus (and applying everywhere for coherence and consistency), I'm not sure of how the doc would render but ultimately, I'm afraid it'd add burden on writers (duplicate list of icons and remember which one to use).
I suggested the separate columns for menu and toolbar because I thought some actions were exclusive to menu vs toolbar(*) and had different label or usage. but it's not the case, so probably no need to repeat labels over the columns. What about something like https://docs.qgis.org/testing/en/docs/user_manual/working_with_vector/attribute_table.html#introducing-the-attribute-table-interface (sorry to suggest another formatting. Just trying to find the best easy-to-understand-and-maintain scenario)

* Btw, the top message of qgis/QGIS#40649 should be updated to reflect the actual behavior before it's harvested in 3.18 changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what you think of the new table structure in the recent commit. I think we're getting there. I also added the "Zoom to Native Resolution" tool. However, I would appreciate a suggestion for the "Usage" column. Honestly, I have never used it and have no idea what it really does.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes LGTM. Let's get feedback for the usage and merge. Thanks @TurboGraphxBeige

Copy link

@jakimowb jakimowb Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Zoom to Native Resolution" means to zoom to that zoom level, where each pixel of the active raster layer covers one screen pixel. This way no raster pixel get's "hidden", as it is the case in lower map scales.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK: it is only for raster layers and matches 1 pixel of raster to 1 pixel of screen.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks guys. @TurboGraphxBeige, your choice 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's it. Feel free to rephrase it if needed.

docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
docs/user_manual/introduction/general_tools.rst Outdated Show resolved Hide resolved
Co-authored-by: Harrissou Sant-anna <delazj@gmail.com>
@TurboGraphxBeige
Copy link
Contributor Author

TurboGraphxBeige commented Jan 19, 2021

I'll wait for this PR to be merged then I'll make another PR for the 3.18 related stuff (and fix #6406):

@TurboGraphxBeige TurboGraphxBeige changed the title Zooming and panning, doc for PR #40460 and #40857 (fixes issue #6406) Zooming and panning Jan 19, 2021
@DelazJ
Copy link
Collaborator

DelazJ commented Jan 19, 2021

Asked on the dev list

@DelazJ DelazJ merged commit 19518b5 into qgis:master Jan 19, 2021
DelazJ added a commit to DelazJ/QGIS-Documentation that referenced this pull request Jan 19, 2021
…g_3_18

Zooming and panning

(cherry picked from commit 19518b5)
@TurboGraphxBeige TurboGraphxBeige deleted the zooming_and_panning_3_18 branch January 19, 2021 21:32
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.

None yet

5 participants