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

Removes the words "Toolbar" and "Panel" from sub-menu names [needs-docs] #7184

Closed
wants to merge 2 commits into from

Conversation

SrNetoChan
Copy link
Member

@SrNetoChan SrNetoChan commented Jun 5, 2018

Description

Following the idea of removing the word "Panel" in their titles (e.g., Layers Panel --> Layers), IMHO, there's no reason to have the words "Toolbar" and "Panel" in every sub-menu item.

For the Panel submenu, the word "Panel" is added automatically. I propose to remove that. I removed the Toolbar word manually and changed the tooltip capitalization.

Before

image

image

image

After

image

image

image

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@borysiasty
Copy link
Member

borysiasty commented Jun 5, 2018

There is one nasty side-effect of simplifying the main menu actions.
Until now, if you type . Layers in the Locator, you'll get a Layers Panel result that switches on and off the Layers panel - it simply triggers the appropriate menu action. Now, if you rename the action to just Layers, the Locator search result will be confusing for users: they will get just an enigmatic Layers, so they clicks it and - surprise - an important panel disappears.

Actually we simplified those actions in Polish localization, and now I'm thinking about reverting it because of this Locator surprise.

For that (and only that) reason I'm -1 for this change unless we find another way to prevent users from hiding panels unintendedly - especially the Layers panel ;) Of course we could hardcode exceptions for the panel/toolbar switching actions (so they are not accessible by the Locator) but... yeah ;)

@DelazJ
Copy link
Contributor

DelazJ commented Jun 5, 2018

@borysiasty your comment reminds me #6971 (comment) and I wonder if @nyalldawson workaround would work here

@borysiasty
Copy link
Member

Ah! Completely forgot it. That would be a solution.

@SrNetoChan
Copy link
Member Author

@borysiasty I must confess that I didn't thought about that. And yes, appending the tool tip could work. Or, in the case of menus, its immediate parent (although I am not sure if that is possible.

@borysiasty
Copy link
Member

@SrNetoChan The tooltips should work out of the box. The parent menu title seems to be accessible as well (just tested in the Python console as action.associatedWidgets()[0].title()), but imho it's enough to just add the tooltips.

@nyalldawson
Copy link
Collaborator

I think we could address the locator issue by adding a new property to these actions, say "locator_string" which is set to "toggle %1 panel". Then locator could use this string (if present) instead of the action's text. It would fix the issue with this pr and also help make actions more usable in locator, because other ambiguous actions could manually have the locator string set to something more helpful.

@SrNetoChan
Copy link
Member Author

@nyalldawson should I add that locator_string property on this PR already?

@nyalldawson
Copy link
Collaborator

@SrNetoChan up to you, what you'd need to do is retain the code you've removed which adds the panel/toolbar suffix, and change it to

a->setProperty( "locator_string", tr("Toggle %1 Panel").arg( a->text() ) );

Then I'll make the required changes in the locator filter itself to use this...

@nyalldawson
Copy link
Collaborator

Actually, scratch that - let me double check that this approach will work first!

@nirvn
Copy link
Contributor

nirvn commented Jun 6, 2018

-1 to remove toolbar and panel words from the main window's right click menu, it's really confusing otherwise, we settled that a long time ago.

@SrNetoChan
Copy link
Member Author

@nirvn the right click menu already has those words on top of each list. IMHO I think that is enough and people will get used to it.

@nirvn
Copy link
Contributor

nirvn commented Jun 6, 2018

@SrNetoChan , I'm not sure. As your screenshot actually shows, the "Toolbars" and "Panels" nearly blends in entirely with the background (i.e. gray on gray). Since those labels are actually dynamically added when user right click, I'd suggest you not removing that code 😄

@SrNetoChan
Copy link
Member Author

@nirvn only "panels" are added dynamically once (for all menus I believe). "toolbar" is hard-coded in the widget title.

@nirvn
Copy link
Contributor

nirvn commented Jun 6, 2018

@SrNetoChan , OK, what about dynamically adding "toolbar" too? That'd work for me. Don't forget to harmonize whatever decision is made here across to the layout designer window.

@SrNetoChan
Copy link
Member Author

I just understood now that the "Panel" word append is triggered by the right-clicking the main window (I thought it was triggered also when looking into the sub-menus). The problem is that, once it's triggered, it will show also on the Panels sub-menu items, which was the main target of this PR.

Meanwhile, I consider this behavior to be a bug, as menus items will show differently depending on the users' previous steps. This probably needs to be solved even if we choose not to merge this PR.

I wonder if we could add the Panel and Toolbar words only for the context help

@SrNetoChan
Copy link
Member Author

@nyalldawson @borysiasty I was trying the locator bar, and none of the toolbars or panels are found by it. Maybe I am missing something?

@borysiasty
Copy link
Member

About adding the Panel/Toolbar word dynamically... it's untranslatable in some languages.

I know we Poles always cause trouble ;) but I'm afraid there may be many other languages where it doesn't work well. By adding Toolbar to Layers you change the role of Layers to a noun adjunct, so its form may change. For example, in Polish Layers are Warstwy, while Layers Panel is Panel warstw (note the removed last letter of Layers). So it's safer to hardcode the full strings.

We fixed it very simply - by translating the Panel and Toolbar to an empty string, so in fact we have the short version, and - to be clear - I'm happy with it (except this Locator problem, but it may be fixed in another PR). Btw. the same problem is with "Add %1" for adding items in the Layout manager - we only keep the "Add items" menu, and shortened all its children.

So I'm personally ok with it, just want you to know in some languages it forces us to use the short version anyway ;)

@borysiasty
Copy link
Member

borysiasty commented Jun 6, 2018

@SrNetoChan - you need to use a dot prefix, and you will find it in the Action result group with a magnifier icon. Note it actually doesn't find the toolbars themselves, rather the main menu actions for hiding/showing them. See the first item:
zrzut_20180606_114825

@SrNetoChan
Copy link
Member Author

Btw I took the screenshot above in QGIS 3.0, where the panel menu entries are in the short form (Layers). In master it would be Layers Panel. So it seems we change it back and forth ;)

As I said before, both in master and 3.0, QGIS loads without the Panels word in the menu items. If you use the right-click on the main window once, it will append the Panel word to action text for the context menu, but will also have consequences on the menu items and in the locator results.

@DelazJ
Copy link
Contributor

DelazJ commented Jun 6, 2018

I rather like the idea of removing those redundant "toolbar" and "panel" that crowd the sub menu and contextual one. But I agree with @nirvn that with @SrNetoChan screenshots not showing the headers, this will be an issue. Should we not instead see how we can improve visibility of the headers in the contextual menu?

In master it would be Layers Panel. So it seems we change it back and forth ;)

No Layers panel in master either. still Layers until I open the contextual menu (as said by @SrNetoChan - good catch!).

@borysiasty
Copy link
Member

@SrNetoChan - sorry for the confusion, I've found your explanation and removed that comment a minute after I wrote it ;)

The hardly visible headers is just Gnome issue, isn't it? I don't think we have anything to do with it - anyone unhappy with his system settings can just change them. The same headers are very strong in KDE and Windows.

@nyalldawson nyalldawson added the UX label Jun 10, 2018
@nyalldawson
Copy link
Collaborator

So what's the outcome here? Can someone sum-up the tasks required?

@SrNetoChan
Copy link
Member Author

SrNetoChan commented Jun 14, 2018

@nyalldawson I would still be in favor of simply removing the words "Panel" and "Toolbar" from both the menus and right-click context menu (that is, merge the PR as is). Seems that the visibility issue on the right-click context menu is only on Gnome and that on others systems the words are perfectly visible above each list. for example, this is how it looks on windows.

image

Anyway, based on @nirvn opinion, I tried (without success) to append the words dynamically just to the right-click menu, but the way it is done it affects the menu items too. Also, currently on master for the Panels menu this dynamic mechanism is introducing a bug, since the name of the menu item differs depending on if the user had right-clicked the main window or not during that QGIS section.

@SrNetoChan
Copy link
Member Author

Oh, if we decide to go added with this PR the tasks missing are:

  • Add a locator_string property for each panel and toolbar (this must be done manually since the code I removed only work when the user right-clicks the main window);
  • Change the locator to show locator_string instead of the window_title property

As for the locator_string, I wonder if we should keep only:
Layers panel

or use something like:
Open Layers panel
Toogle Layers panel
Open|close layers panel

@borysiasty
Copy link
Member

borysiasty commented Jun 16, 2018

I'd go with Toggle Layers panel, and Open|Close would be my second choice.

Layers panel is still not so clear (one may expect some kind of focus on the panel, and will be confused by its disappearance), and Open Layers panel is simply misleading (especially this particular panel is usually open). (EDIT: This particular panel is double misleading ;) )

@nyalldawson
Copy link
Collaborator

Ping @probot

@stale
Copy link

stale bot commented Jul 12, 2018

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@stale stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 12, 2018
@stale
Copy link

stale bot commented Jul 19, 2018

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

@stale stale bot closed this Jul 19, 2018
@borysiasty borysiasty added the Worth a Revival Closed but has some interest to be resurrected label Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Uh oh! Seems this work is abandoned, and the PR is about to close. Worth a Revival Closed but has some interest to be resurrected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants