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

Fix tooltip punctuation consistency (issue #963) and make tool tooltips localizable #999

Closed

Conversation

SeaLiteral
Copy link

Fix #963 by removing periods at the end of some tooltips, supporting localizable tooltips for the Tool class (which used to only fetch tooltips from class docstrings) and adding such tooltips to the built in tools.

@nsmarkop
Copy link
Contributor

Should the direction here be making everything have periods, or removing them? At least Microsoft's UX guide has tooltips (well, "infotips") using periods: https://docs.microsoft.com/en-us/windows/desktop/uxguide/ctrl-tooltips-and-infotips

@SeaLiteral
Copy link
Author

SeaLiteral commented Aug 13, 2018

A lot of programmes (e.g. LibreOffice.org Writer) have one-word-and-one-shortcut tootips like New (Ctrl+N) or Bold (Ctrl+B). But those are pretty short and some of those in Plover are longer. GIMP has longer tooltips like "Select by Cilir Tool: Select regions with similar colors Shift+O", which still don't have periods. I mostly use Linux, but I think Plover should try to be consistent with other open source applications rather than trying to do whatever CaseCat is doing. And that page you link to doesn't say that tooltips should or shouldn't have periods. It just happens to have one on the example in the screenshot. That being said, the multi-line tooltip in machine_options.py is pretty long and contains more than one sentence, so that one should probably keep the period.

self.arpeggiate.setToolTip(_(
            'Arpeggiate allows using non-NKRO keyboards.\n'
            '\n'
            'Each key can be pressed separately and the\n'
            'space bar is pressed to send the stroke'
))

Edit: Code was copy-pasted twice.

@nsmarkop
Copy link
Contributor

I'm not sure why you randomly brought up CaseCat. You can find many examples of programs, open source or not, using either of the styles (or in the case of what I linked to, both -- one for "tooltips" and one for "infotips"). My intent was just to provide one example doing something different than proposed here.

This is likely something that would benefit from Ted or Benoit making a decision for what they want so a standard can be enforced for future development.

@SeaLiteral
Copy link
Author

SeaLiteral commented Aug 13, 2018

On the page you linked, there are two screenshots: an "infotip" with a period and a tooltip without one. And yeah, mentioning another steno programme was random. And at least it should be possible to find the strings, at the very least by running Plover and pointing at everything in every window Plover can open and using grep to find them (or looking for them in the pot file, but the pot file doesn't mention which strings are tooltips, and it might have to be regenerated). That being said, I personally prefer letting it depend on their length, so the multi-sentence ones should have punctuation and single-word tooltips should, in my opinion, not. As for the medium-length ones, I found it easier to remove them than to add them, but let's see what the others say. And it seems one of the multi-line ones has periods and one doesn't, I guess I should fix that.

Edit: That last sentence refers to code that wasn't there. I got confused by the then-wrong code in my comment above.

@SeaLiteral
Copy link
Author

Such a guideline would also be useful for plugin developers.

@benoit-pierre
Copy link
Member

I'm for make the tooltips proper phrase, meaning they should end with a .. Especially since the goal is to have long helpful tooltip that explain what a button/feature does rather than just a substitute for the lack of label in some cases.

@@ -11,6 +11,7 @@ class AddTranslationDialog(Tool, Ui_AddTranslationDialog):
''' Add a new translation to the dictionary. '''

TITLE = _('Add Translation')
TOOLTIP = _('Add a new translation to the dictionary')
Copy link
Member

Choose a reason for hiding this comment

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

How about this instead:

    __doc__ = _('Add a new translation to the dictionary.')

?

Copy link
Author

Choose a reason for hiding this comment

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

Docstrings used to be read-only, but are changeable now. But their goal is to work as a type of comments that can be fetched automatically when you're trying to call a function but want to get reminded what the function does or how it's meant to be used. By encoding them in an unusually way, we're stopping IDEs from showing them, but fetching the docstring is just fetching the doc attribute, so creating an attribute called TOOLTIP means anyone looking at the code can guess what that tooltip is rather than wondering why we're setting the docstring that way.

Copy link
Member

@benoit-pierre benoit-pierre left a comment

Choose a reason for hiding this comment

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

As already mentioned, I'm for keeping a terminating ..

As for the issue of tools, either use __doc__ = _('Tooltip text.'), and Tool.TOOLTIP is unnecessary, or we add Tool.TOOLTIP and we might as well remove those useless docstrings.

@SeaLiteral
Copy link
Author

Docstrings are supposed to be read-only in Python, so setting them to the return value of a function won't work. We could do exec("class Test:\n '''"+_('Tooltip text)+"'''") and it would work, but, really? There are some more elegant ways to do this, but they do feel like they were meant to be known to the average programmer, so I think it might be better to use the TOOLTIP if it's set and the docstring if no TOOLTIP is specified, for backwards compatibility with Plugins written before this.

Speaking of Plugins, we might want to have a better tooltip than "Plugins Manager". Why even have a tooltip when the name of the tool already appears under the icon? Also, are plugins meant to be localizable?

@benoit-pierre
Copy link
Member

benoit-pierre commented Aug 21, 2018

Docstrings are supposed to be read-only in Python, so setting them to the return value of a function won't work

Did you test it?

@benoit-pierre
Copy link
Member

With 4.0.0 not yet released, let's not get bogged down by backward compatibility.

@benoit-pierre
Copy link
Member

GUI plugins should be localizable using plover.gui_qt.i18n.get_gettext(...), yes.

@SeaLiteral
Copy link
Author

How about strings that aren't tooltips? Some of them look like sentences. For instance there's the {stroke} maps to {translation}, which has the additional problem that it might look like the period is a part of the translation.
hadbeen
Note: There are several files I haven't added the periods to yet. I'm just writing this question before I finish editing those. I'll write back when there are no files missing periods as far as I know.

Note 2: I'm showing a "{stroke} maps to {translation}" screenshot that I originally took to show something else, it's not meant to show a smiley here, but this was faster to find than take another screenshot. I hope that's okay.

@SeaLiteral
Copy link
Author

Answer to question: Docstrings used to be read-only.

And now, back to the main topic: Using plover.pot to spot tooltips I think I got all of them now. You might want to look at this one from gui_qt/config_window.py. I edited the capitalization to fit:

Set the display order of dictionaries:
- top-down: Match the search order; highest priority first.
- bottom-up: Reverse search order; lowest priority first.

I'm editing a screenshot that contained it so that it doesn't nearly get as wide as the screen.
Screenshot with the tooltip moved to be below the option rather than outside the window
The tooltip matches capitalization of selectable options, but might not fit the punctuation inside the tooltip.

It's worth noting that right above that option is an opacity option that would have capitalization issues if it weren't because numbers are neatly exempt from that. If we change the punctuation in the former, we might need to do so in the latter for consistency.

Set the translation dialog opacity:
- 0 makes the dialog invisible.
- 100 is fully opaque.

Tooltips: Should be okay, but should the ones mentioning option names match the capitalization of the option names?
Punctuation in other strings: How do we deal with {stroke} maps to {translation} (we don't want the period to look like it's a part of the translation, which the background colour partially solves, but visually impaired users might be unable to see that) and {translation} maps to stroke {strokes} (where we don't need to worry about the period being mistaken for a key, but we still want those two messages to be consistent with each other).

@SeaLiteral
Copy link
Author

Also, I'm going to remove the code that gets the docstrings if there isn't a tooltip.

It might have been just in earlier versions of Python that docstring changing was hard to do, but normal docstrings are readable by things like IDEs whereas the ones set programatically are not. And then we might as well use Tool.TOOLTIP instead.

@SeaLiteral
Copy link
Author

And that's done now, including changing a comment. We might want to make changes to some plugins as well, but that's outside Plover.

@SeaLiteral
Copy link
Author

I think I've made the changes. I might post issues and pull requests to plugins where I notice they need to be changed after this (since their tooltips in the main window won't show once this PR gets merged, but other than that, the plugins should still work). But I think plugins shouldn't change their tooltips until Plover does.

@morinted
Copy link
Member

This looks good to me -- we could do a squash-and-merge -- anything else that needs updating?

@SeaLiteral SeaLiteral closed this Feb 10, 2019
@SeaLiteral
Copy link
Author

I'm closing and reopening the PR to fix the problem that seems to have been caused by a new pip release.

@SeaLiteral SeaLiteral reopened this Feb 10, 2019
@SeaLiteral
Copy link
Author

That doesn't seem to work. Also, Pierre wrote something while I was doing this. Closing again.

@SeaLiteral
Copy link
Author

So there's two main changes here:

  • Make tooltip punctuation consistent between files
  • Support localizable tooltips

I think 4066b60 ("Make tooltip into sentence (and add period)") falls into the first category even if it's changing the wording in order to change the punctuation. And those things I changed and then changed back, I should just merge the change with the change back?

And then I suppose I should end up with two commits: one for punctuation changes and one for localizability.

@SeaLiteral SeaLiteral force-pushed the fix-tooltip-periods branch 2 times, most recently from 4c37910 to 8cdc169 Compare February 16, 2019 19:45
@SeaLiteral SeaLiteral changed the title Fix tooltip punctuation consistency (issue #963) Fix tooltip punctuation consistency (issue #963) and make tool tooltips localizable Feb 16, 2019
@SeaLiteral SeaLiteral force-pushed the fix-tooltip-periods branch 4 times, most recently from e7c078a to 1222ca7 Compare February 20, 2019 14:02
@SeaLiteral
Copy link
Author

SeaLiteral commented Feb 20, 2019

This is the list of commits as it shows now. I'm showing which of the two changes (punctuation and code) they relate to:

  • Make tooltip punctuation consistent between files (punctuation) 65a5206
  • Change where tooltips go to allow localization (TOOLTIP variable) 96615d4
  • Add periods to tooltips. And make a tooltip into a sentence (both) e22cd62
  • Add period to tooltip and remove docstring (both) 9cb455a
  • Add periods to tooltips (punctuation) ef46898
  • Make tool tooltips localizable (TOOLTIP variable) 9da99aa
  • re-add period (TOOLTIP variable!) 1222ca7

The last one re-adds a period that I'd removed while adding the TOOLTIP member variable. Hence why I group it with TOOLTIP variable list even though it's a punctuation change.

There's also the e22cd62 one that paraphrases a tooltip to make it a proper sentence with a full stop at the end in paper_tape.py.

Now, even when I gave a string a name and changed the punctuation in that string at the same time, it's pretty easy to see both changes without one making the other hard to understand. So I think we could merge the whole thing into a single commit.

Make punctuation in tooltips consistent between files. If one contains a whole sentence or several, end it with a full stop (period).

Also, give the Tool class a TOOLTIP member variable and put tooltips there rather than in docstrings, since docstrings were meant as a type of comments shown development tools rather than the return value of a function, removing the docstrings that are no longer used as tooltips.

Remove a comment about using docstrings as tooltips, and add one that mentions that the TOOLTIP member variable is used for tooltips shown in the main window.
@SeaLiteral
Copy link
Author

Since the tooltip text editing and the localizability fix don't make each other harder to understand in the diffs, I've merged it all into one commit. I also looked at the files to check I hadn't somehow lost any of the changes while merging stuff.

@benoit-pierre benoit-pierre mentioned this pull request Apr 1, 2021
22 tasks
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.

Tooltip format is inconsistent
4 participants