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

Use ui.message more, capitalize some messages and docstrings #5557

Closed
wants to merge 10 commits into from

Conversation

dkager
Copy link
Collaborator

@dkager dkager commented Nov 29, 2015

Related issue: #5547

Some notes:

  • All messages in the speech module are untouched.
  • Progress bar output should never be done using ui.message because depending on the circumstances this can trap the user in a series of messages, making it hard or impossible to read the dialog's contents.
  • Global commands that deal with speech, e.g. speak selection, are untouched. The rationale is that it makes no sense to not braille anything for a command except when an error occurs. If someone triggers a speech-only command they'll probably not check their braille display for errors.
  • As I wrote in the first commit I'm not sure what to do with the app module messages. Can anyone explain where these occur when you're using the app?
  • In globalCommands there is a bunch of toggles with messages like "input help on" and "input help off". While capitalizing these would provide a more consistent UI, and while it is easy to autoamte, this will probably make translators really unhappy. If this is to be undertaken it would be nice to only provide the toggle's name to translators once and dynamically append on/off. But looking at all open issues this has the lowest possible priority.
  • Generalizing the above, maybe braille.handler.message could capitalize the first letter. Again, this is all rpetty hypothetical.

…em. Also add missing translator comments and do some tiny script doc and capitalization fixes.

Question: should the messages in the Firefox, Thunderbird and Outlook app modules also be brailled?
re nvaccess#5547
…ode name in braille instead of only speaking it. Fixes nvaccess#5239.
@dkager
Copy link
Collaborator Author

dkager commented Nov 29, 2015

With regard to the only instance of speech.speakMessage in core.py @derekriemer wrote:

Maybe. Note. Braille display support may very well not exist yet at this point in nvda's loading process.

This is correct, braille is initialized a few lines down. I assume we probably want to preserve this order.

@jcsteh
Copy link
Contributor

jcsteh commented Nov 30, 2015

In globalCommands there is a bunch of toggles with messages like "input help on" and "input help off". While capitalizing these would provide a more consistent UI, and while it is easy to autoamte, this will probably make translators really unhappy.

I assume, then, that the majority of our messages start with a capital letter? (I suspect this is true also, but I'm not certain and you've been looking at this more recently than I.)

If this is to be undertaken it would be nice to only provide the toggle's name to translators once and dynamically append on/off.

I believe the reason we don't do this is precisely because of other languages. My understanding is that in some languages, you can't just go "input help %s" and expect it to be correct for both on and off. @josephsl might be able to comment more on this.

But looking at all open issues this has the lowest possible priority.

It might be worth splitting that out into a separate pull request after this one is merged so it's easier to take these separately if we wish. My feeling is that if we're going to have to do it at some point, it doesn't matter when; now or later is the same. :)

Generalizing the above, maybe braille.handler.message could capitalize the first letter.

This sounds like a horrible hack to me. If we're worried about braille, we should also consider Speech Viewer, for example. Also, there may be cases where we explicitly don't want the first letter capitalised. I think if we're going to do this, we should just do it properly. :)

braille is initialized a few lines down. I assume we probably want to preserve this order.

Yes, we do. We hope that message doesn't show up too often these days :), but the whole point is that it needs to be as early as possible to be of any use and braille can take longer to initialise than speech (e.g. Bluetooth displays).

@jcsteh
Copy link
Contributor

jcsteh commented Nov 30, 2015

Cover app modules. Still not sure about Firefox/Thunderbird.

The idea behind that code (which doesn't work in newer versions of Firefox AFAIK) is to speak a message when a document is being loaded so the user knows what's going on. My concern is that it could get rather annoying if that hangs around on the display even though the document has finished loading. In contrast, the message happens quite quickly for speech. I think it could be useful, but it would require additional code to dismiss any display message when the document finishes loading, which requires further thought; I don't quite know where we'd put this.

@josephsl
Copy link
Collaborator

Hi,
Regarding on/off toggle: A few years ago, I and other translators raised concerns regarding some messages being hard to translate, and input help toggle was one of them. Specifically, because some languages such as Korean have different contextual meanings for the words "on" and "off", using a single message for these words confused users and was inappropriate in some cases.
In regards to the overall approach of making ui.message replace speech.speakMessage: One potential issue I see is "no next" and "no previous" and friends. I think, to reduce confusion, these object navigation messages should be brailled if and only if braille is tethered to review cursor. I think we should take context and user experience into account when trying to decide which spoken messages should be sent to braille displays.
In regards to capitalization and possible burden for translators: Many translators (including I) use poEdit. Poedit is smart enough to detect fuzzy messages, so it's the matter of marking them as not fuzzy, something that shouldn't take a single translation session (about five to ten minutes a week if done early).
Thanks.
b

@dkager
Copy link
Collaborator Author

dkager commented Nov 30, 2015

I assume, then, that the majority of our messages start with a capital letter?
Yes. The consistent exceptions are the settings toggles and the dynamic messages, such as the mouse cursor change message.

It might be worth splitting that out into a separate pull request after this one is merged so it's easier to take these separately if we wish.
Sure, I already did some changes in this pull request but we can do the capitalization of toggle messages separately.

Generalizing the above, maybe braille.handler.message could capitalize the first letter.
This sounds like a horrible hack to me.
Agreed, just making sure we looked at it from all angles. :)

If we're worried about braille, we should also consider Speech Viewer, for example.
I didn't even think of that. This means capitalizing messages that are strictly spoken too. Fortunately there aren't many of those.

One potential issue I see is "no next" and "no previous" and friends. I think, to reduce confusion, these object navigation messages should be brailled if and only if braille is tethered to review cursor.
Excellent point.
Off-topic: I've wanted to raise the question if "tethered to review" should be required to do object navigation. That is, do you call it "object navigation" or "object review"? Obviously it doesn't move the focus, but these semantics lead to a lot of swithcing between tethering modes in some applications. But for now I will conditionally call ui.message depending on the tethered state.

@dkager
Copy link
Collaborator Author

dkager commented Nov 30, 2015

Cover app modules. Still not sure about Firefox/Thunderbird.
My concern is that it could get rather annoying if that hangs around on the display even though the document has finished loading.
Yes, let's not braille these messages. Display message traps are horrible, which is why I am hesitant to add ui.message to anything that isn't directly triggered by a keyboard command or other NVDA action.

@dkager
Copy link
Collaborator Author

dkager commented Nov 30, 2015

Thinking about the point @josephsl made re messages for object navigation. At first glance the easiest way to suppress certain braille messages depending on what braille is tethered to is to give ui.message two optional parameters speak=True and braille=True. Then you can do:

ui.message(_("No next"), braille=(braille.handler.tether == braille.handler.TETHER_FOCUS))

Or some variation of that. Of course you can also select either ui.message or speech.speakMessage in each script, but that adds quite a few lines of code all solving the same problem.

@jcsteh
Copy link
Contributor

jcsteh commented Nov 30, 2015

ui.message(_("No next"), braille=(braille.handler.tether == braille.handler.TETHER_FOCUS))
Or some variation of that. Of course you can also select either ui.message or speech.speakMessage in each script, but that adds quite a few lines of code all solving the same problem.

Hmm. That depends whether you consider this to be too ugly:

(ui.message if braille.handler.tether == braille.handler.TETHER_FOCUS else speech.speakMessage)(_("No next"))

I guess that is pretty ugly. It's kinda cool, though. :)

It just seems weird to me that ui.message, which is meant for output agnostic messages, should take arguments indicating what output methods should be used. It's like "okay, let's introduce some abstraction for messages. Hmm, now let's allow the caller to disable the abstraction."

@dkager
Copy link
Collaborator Author

dkager commented Dec 1, 2015

Hmm. That depends whether you consider this to be too ugly:
(ui.message if braille.handler.tether == braille.handler.TETHER_FOCUS else speech.speakMessage)(_("No next"))
Having had far too much fun with Haskell I don't think it's too ugly, though I guess it is a bit bewildering. Probably the clearest way to handle this is to use speech.speakMessage and then conditionally also call (the inconsistently named) braille.handler.message. More code but also self-explanatory code that way. Or just make object navigation work in either tether mode, because taking irrational, biased shortcuts that way is totally how to do things. :)

@josephsl
Copy link
Collaborator

josephsl commented Dec 1, 2015

Hi, I would vote for a separate call to braille.handler.message for these messages to show that ui.message will not solve all problems and to contextualize some messages.

Cheers,

Joseph

From: Davy Kager [mailto:notifications@github.com]
Sent: Tuesday, December 1, 2015 8:35 AM
To: nvaccess/nvda
Cc: josephsl
Subject: Re: [nvda] Use ui.message more, capitalize some messages and docstrings (#5557)

Hmm. That depends whether you consider this to be too ugly:
(ui.message if braille.handler.tether == braille.handler.TETHER_FOCUS else speech.speakMessage)(_("No next"))
Having had far too much fun with Haskell I don't think it's too ugly, though I guess it is a bit bewildering. Probably the clearest way to handle this is to use speech.speakMessage and then conditionally also call (the inconsistently named) braille.handler.message. More code but also self-explanatory code that way. Or just make object navigation work in either tether mode, because taking irrational, biased shortcuts that way is totally how to do things. :)


Reply to this email directly or view it on GitHub #5557 (comment) .Image removed by sender.

@dkager
Copy link
Collaborator Author

dkager commented Dec 4, 2015

Another thought:

def reviewMessage():
    return speech.speakMessage if braille.handler.tether == braille.handler.TETHER_REVIEW else ui.message
...
reviewMessage()(_("No next"))

Would this be too ugly too? I don't want to make people reading this code go 'Wut?' but I also don't like repeating the same if-statement over and over. The idea is that the rule for brailling these messages depends on the focus mode and applies to both review and object navigation, though I'm not sure if brailling "top" and "bottom" all the time when tethered to review would get excessive.

@dkager
Copy link
Collaborator Author

dkager commented Dec 10, 2015

Some commands, such as script_review_top and script_review_bottom speak a message when the action is performed. This is not currently brailled. But should it even be spoken? I'm led to believe that speech users like brevity. If you use one of these scripts and hear speech you know that the command worked. Do you first need to hear "top" or "bottom"? In braille this wouldn't be efficient because you'd have to wait a bit before seeing the actual top line in review mode. Is this standard behavior for all (virtual) navigation commands?

@jcsteh
Copy link
Contributor

jcsteh commented Dec 10, 2015

reviewMessage()(_("No next"))

Would this be too ugly too?

Hmm, they're both pretty horrible. :)

I don't want to make people reading this code go 'Wut?' but I also don't like repeating the same if-statement over and over. The idea is that the rule for brailling these messages depends on the focus mode and applies to both review and object navigation

True enough. I'm not against having a reviewMessage utility function somewhere. If it's only ever going to be used by globalCommands, it should go in that module, but if we think it might get used elsewhere, it could go in the review module.

though I'm not sure if brailling "top" and "bottom" all the time when tethered to review would get excessive.

I think that would get rather annoying, since it's not as "quick" in braille as it is in speech.

Some commands, such as script_review_top and script_review_bottom speak a message when the action is performed. This is not currently brailled. But should it even be spoken?

I agree it's superfluous, though interestingly, no one has ever complained. I believe this was modelled on other screen readers (ASAP, Speakup) that use this set of numpad review commands. At this point, I think I'd prefer to leave speech alone, but I certainly agree brailling this would be irritating.

@jcsteh
Copy link
Contributor

jcsteh commented Dec 22, 2015

Are you planning to do the reviewMessage change as suggested in my last comment? Also, this now has conflicts. :( I'll try to keep on top of this one so it doesn't constantly bitrot.

@jcsteh
Copy link
Contributor

jcsteh commented Dec 22, 2015

Apart from the conflict and the question around the review message stuff, this all looks good to me.

@dkager
Copy link
Collaborator Author

dkager commented Dec 22, 2015

Yes, I was planning to add the reviewMessage function, probably in the review module because globalCommands currently only has commands and no auxiliary functions. Should have time this week.

From: James Teh [mailto:notifications@github.com]
Sent: Tuesday, December 22, 2015 07:54
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvda] Use ui.message more, capitalize some messages and docstrings (#5557)

Apart from the conflict and the question around the review message stuff, this all looks good to me.


Reply to this email directly or view it on GitHub #5557 (comment) . https://github.com/notifications/beacon/AC9y8UYl1pcKpfPg6CrSnpCJz1DFxV-7ks5pSOrtgaJpZM4GrAjR.gif

…s a message if braille is tethered to review. Use this new function in globalCommands.
@dkager
Copy link
Collaborator Author

dkager commented Dec 25, 2015

I ended up putting reviewMessage in the ui module, because it avoids some extra imports in other modules and because it goes well with browseableMessage.
The commands that move the review cursor are fairly straight-forward, but I'm unsure what to do with other review commands. For example, commands to move focus to the navigator object or vice versa, and the command to move the mouse to the navigator object. On one hand I would say that if braille isn't tethered to review, you don't need any messages these commands may generate to be shown in braille since you have no context for them. On the other hand I don't want to deprive users of information about potential errors. Similarly, when you change speech settings with scripts you still get braille feedback too. Maybe messages like "No focus" and "Move to focus" should then also be brailled regardless of tethering. Or maybe not. Any thoughts?

@josephsl
Copy link
Collaborator

Hi,

I think there are messages that should be brailled regardless of tether value, including “no focus” and so on (unless others object to this).

One suggestion: if you don’t mind doing this, can you update the copyright header at the top of the ui module? If not, let us know if I or someone should do it.

From: Davy Kager [mailto:notifications@github.com]
Sent: Friday, December 25, 2015 7:07 AM
To: nvaccess/nvda nvda@noreply.github.com
Cc: josephsl joseph.lee22590@gmail.com
Subject: Re: [nvda] Use ui.message more, capitalize some messages and docstrings (#5557)

I ended up putting reviewMessage in the ui module, because it avoids some extra imports in other modules and because it goes well with browseableMessage.
The commands that move the review cursor are fairly straight-forward, but I'm unsure what to do with other review commands. For example, commands to move focus to the navigator object or vice versa, and the command to move the mouse to the navigator object. On one hand I would say that if braille isn't tethered to review, you don't need any messages these commands may generate to be shown in braille since you have no context for them. On the other hand I don't want to deprive users of information about potential errors. Similarly, when you change speech settings with scripts you still get braille feedback too. Maybe messages like "No focus" and "Move to focus" should then also be brailled regardless of tethering. Or maybe not. Any thoughts?


Reply to this email directly or view it on GitHub #5557 (comment) .

@dkager
Copy link
Collaborator Author

dkager commented Dec 25, 2015

So you'd say braille movement messages such as "Top" only if tethered to review, and all others regardless of tether mode? What about the activate message (e.g. NVDA+enter)? I changed this to use the new ui.reviewMessage but am having second thoughts. Probably a sign that I should quit and go celebrate Christmas. :)

@josephsl
Copy link
Collaborator

Hi,

I think NvDA+ENTER message could be brailled (if needed). As for review top and bottom, I’d say if only tethered to review.

As for having second thoughts, I think sometimes, a good rest helps, especially when you are writing a major feature change. I do agree that, because of hlidays, it’s better to rest.

Merry Christmas.

From: Davy Kager [mailto:notifications@github.com]
Sent: Friday, December 25, 2015 7:54 AM
To: nvaccess/nvda nvda@noreply.github.com
Cc: josephsl joseph.lee22590@gmail.com
Subject: Re: [nvda] Use ui.message more, capitalize some messages and docstrings (#5557)

So you'd say braille movement messages such as "Top" only if tethered to review, and all others regardless of tether mode? What about the activate message (e.g. NVDA+enter)? I changed this to use the new ui.reviewMessage but am having second thoughts. Probably a sign that I should quit and go celebrate Christmas. :)


Reply to this email directly or view it on GitHub #5557 (comment) .

@dkager
Copy link
Collaborator Author

dkager commented Dec 25, 2015

Haha, fair enough. I'll definitely look at the complete diff again after the holidays.
Merry Christmas!

@dkager
Copy link
Collaborator Author

dkager commented Jan 8, 2016

One more thing I thought of: should NVDA+tab also show the output in braille? The information it provides can be very useful, but maybe it's too verbose for a quick message?

@josephsl
Copy link
Collaborator

josephsl commented Jan 8, 2016

Hi, I think there’s no need for this, as braille output already provides all the necessary information.

From: Davy Kager [mailto:notifications@github.com]
Sent: Friday, January 8, 2016 12:55 PM
To: nvaccess/nvda nvda@noreply.github.com
Cc: josephsl joseph.lee22590@gmail.com
Subject: Re: [nvda] Use ui.message more, capitalize some messages and docstrings (#5557)

One more thing I thought of: should NVDA+tab also show the output in braille? The information it provides can be very useful, but maybe it's too verbose for a quick message?


Reply to this email directly or view it on GitHub #5557 (comment) .

@dkager
Copy link
Collaborator Author

dkager commented Jan 8, 2016

Not all, though of course I can’t think of a scenario where it doesn’t right now. But there have been cases where speech appeared to provide more info with this command than was visible in braille. I’ll revisit this if/when I have something more substantial.

From: josephsl [mailto:notifications@github.com]
Sent: Friday, January 08, 2016 22:00
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvda] Use ui.message more, capitalize some messages and docstrings (#5557)

Hi, I think there’s no need for this, as braille output already provides all the necessary information.

From: Davy Kager [mailto:notifications@github.com]
Sent: Friday, January 8, 2016 12:55 PM
To: nvaccess/nvda <nvda@noreply.github.com mailto:nvda@noreply.github.com >
Cc: josephsl <joseph.lee22590@gmail.com mailto:joseph.lee22590@gmail.com >
Subject: Re: [nvda] Use ui.message more, capitalize some messages and docstrings (#5557)

One more thing I thought of: should NVDA+tab also show the output in braille? The information it provides can be very useful, but maybe it's too verbose for a quick message?


Reply to this email directly or view it on GitHub #5557 (comment) .


Reply to this email directly or view it on GitHub #5557 (comment) . https://github.com/notifications/beacon/AC9y8bi2V2tkZ2I4--2koXLFLfmcdfRBks5pYBrLgaJpZM4GrAjR.gif

@jcsteh
Copy link
Contributor

jcsteh commented Jan 11, 2016

One more thing I thought of: should NVDA+tab also show the output in braille? The information it provides can be very useful,

Ideally, it should. It actually does report more than normal focus reporting (in either braille or speech). For example, it always reports the selected/not selected state if the item is selectable, even if it wouldn't normally be included in a focus report. It also reports roles (e.g. list item, table cell, menu item, unknown) and states (e.g. focused, invisible) which would otherwise be silenced. Seeing the full role and state names (as spoken) could also be useful for braille users who aren't yet familiar with the abbreviations. Ideally, we'd do this for the navigatorObject_current script as well.

Unfortunately, this "ideal" thing isn't so simple to do because speech.speakObject is used to do this and that speaks the message itself rather than returning a string. speech.speakObject does some stuff for editable text that can't be returned in a simple string, but that stuff isn't needed for braille. speech.speakObjectProperties/speech.getSpeechTextForProperties could be refactored a bit so you could get a string, but having to call those twice isn't terrific, although caching mitigates that a bit. I guess we could call it the second time only if braille is enabled.

but maybe it's too verbose for a quick message?

I don't think so. If you scroll braille, the message timer gets reset, which means if the message is long, users should have time to read it.

@dkager
Copy link
Collaborator Author

dkager commented Jan 21, 2016

Fixed another merge conflict.

@dkager
Copy link
Collaborator Author

dkager commented Jan 21, 2016

I don't think so. If you scroll braille, the message timer gets reset, which means if the message is long, users should have time to read it.
This is true, though I often find myself frantically scrolling to prevent the message from timing out. Interesting thought: add a little bit of time to the timeout for every display length of braille being shown. Still, for the NVDA+tab case this point is moot since that doesn't use speech.speakMessage and thus can't be ported to braille as easily.

jcsteh pushed a commit that referenced this pull request Mar 16, 2016
… only spoken are now brailled as well.

Also, fix inconsistent capitalisation and add translator comments for some messages.
Fixes #5557.
@nvaccessAuto
Copy link

Incubated in 55a554d.

@derekriemer
Copy link
Collaborator

I don't think so. If you scroll braille, the message timer gets reset,
which means if the message is long, users should have time to read it.

This is true, though I often find myself frantically scrolling to
prevent the message from timing out. Interesting thought: add a
little bit of time to the timeout for every display length of
braille being shown.

Any interest in this being pursued?

Still, for the NVDA+tab case this point is moot since that doesn't
use speech.speakMessage and thus can't be ported to braille as easily.


Reply to this email directly or view it on GitHub
#5557 (comment).


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.com
Awesome little hand built weather app!
http://django.derekriemer.com/weather/

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

@dkager
Copy link
Collaborator Author

dkager commented Apr 2, 2016

Interesting thought: add a little bit of time to the timeout for every display length of braille being shown.

Any interest in this being pursued?

I think there isn't a good heuristic for this. Some text is easier to read than others, and sometimes you are only interested in the first few words of a long line. Maybe it depends more on the category of text, i.e. 'No next heading' is expected on a webpage and doesn't need to stay 4 seconds whereas a status bar probably needs closer inspection. But there are no good rules to follow here. The solution seems to be to set a long timeout and dismiss messages manually, which is easy enough.

@dkager
Copy link
Collaborator Author

dkager commented Apr 2, 2016

Speaking of 'no next heading', those messages still aren't capitalized. Did we decide to exclude/postpone that or did I miss something?

@jcsteh
Copy link
Contributor

jcsteh commented Apr 3, 2016 via email

@dkager
Copy link
Collaborator Author

dkager commented Apr 4, 2016

Either that, or I skipped it because there are many ‘no next’ and ‘no previous’ and I wasn’t sure if adding a capital would cause all of them to be marked as needing new translations.

From: James Teh [mailto:notifications@github.com]
Sent: Sunday, April 3, 2016 23:58
To: nvaccess/nvda nvda@noreply.github.com
Cc: Davy Kager mail@davykager.nl
Subject: Re: [nvaccess/nvda] Use ui.message more, capitalize some messages and docstrings (#5557)

I didn't exclude any of the patch, so I guess it was missed or somehow
got excluded during a conflict resolution a while ago...?


You are receiving this because you authored the thread.
Reply to this email directly or #5557 (comment) view it on GitHub https://github.com/notifications/beacon/AC9y8WP9622xoiNIOoMt7fD2wnUMMpAEks5p0DfXgaJpZM4GrAjR.gif

@jcsteh
Copy link
Contributor

jcsteh commented Apr 4, 2016 via email

dkager added a commit to dkager/nvda that referenced this pull request Apr 14, 2016
@dkager
Copy link
Collaborator Author

dkager commented Apr 14, 2016

The commit above contains fixes for all the capitalization and punctuation inconsistencies I could easily find. It is quite a diff, which is the reason for not including it in the original PR. Most of it is very repetitive: 'no next', 'no previous' or on/off toggles.

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