search

nvaccess / nvda

NVDA, the free and open source Screen Reader for Microsoft Windows
https://www.nvaccess.org/
Other
2.12k stars 637 forks source link

allow users to set key commands for dialogues which haven't one. #4898

Closed nvaccessAuto closed 9 years ago

nvaccessAuto commented 9 years ago

Reported by bdorer on 2015-02-08 23:17 Users should be able to create key commands for all dialogs where it makes sense including: Braille, review settings, input gestures, addons manager, dictionarries, and so on.

Sometimes you can't see the advantage for a key command, but I had several situations where I had created some. Blocking #465, #3285, #3604, #4873

nvaccessAuto commented 9 years ago

Comment 1 by jteh on 2015-02-09 02:15 There are a few tickets related to this, so let's address them all in this one.

I'm happy for these to be added but unbound. A sentence should be added to the User Guide for each affected section which says something like: "There is no default shortcut to access this dialog, but you can add one using the Input Gestures dialog." where Input Gestures dialog is a link to the appropriate section.

Note that there's a patch on ticket:3285#comment:12 which addresses the add-ons dialog. This could be used a base for this work. It includes a default binding, but I do not think we should have a default binding. Please include the User Guide updates if you submit a patch.

Thanks!

nvaccessAuto commented 9 years ago

Comment 2 by Joseph Lee <joseph.lee22590@... on 2015-02-09 03:18 In [b0c1bd5faa2322a7d29c1e14d5e06262a6020ee9]:

GUI: dialogs such as preferences can now be given custom gestures to access where previously it was necessary to open them from preferences menu. re #4898.
Specifically:
- Global commands: for missing shortcut keys, added scripts to access them with no commands bound to them.
- User guide: for preferences dialogs that does not have their own shortcut keys, added a note (as part of the heading) that shortcut keys are not assigned. For dialogs such as add-ons manager and speech dictionaries, added a note saying custom gestures should be added to access them from anywhere. Lastly, added same page links to jump to input gestures dialog section.
  Since someone has already worked on add-ons manager script, I'll ask that person to take a look at this code in hopes of merging the patches together. Thanks.
nvaccessAuto commented 9 years ago

Comment 3 by leonarddr on 2015-02-09 08:46 I think you can safely merge t3285 from http://bitbucket.org/leonardder/nvda.git into t4898. It incorporates the add-on manager script and leaves it unbound.

nvaccessAuto commented 9 years ago

Comment 4 by leonarddr on 2015-02-09 13:05 Additional things which should be implemented or changed in my opinion:

nvaccessAuto commented 9 years ago

Comment 5 by leonarddr on 2015-02-09 13:31 Branch t4898 on http://bitbucket.org/leonardder/nvda.git now contains:

nvaccessAuto commented 9 years ago

Comment 7 by nvdakor on 2015-02-10 02:34 Hi, The code looks fine to me. I'd rather not merge it myself, as I think Jamie is an authority on that (he and Mick does code reviews, so I'd wait to hear from them before proceeding). But once we get this merged, would it be possible to continue working from t4898 alone? That way we can combine our work and you can be recognized as an author. For the user guide side of things, I took care of it unless you wish to edit some parts of it that relates to this ticket. As for speech viewer shortcut, I'd rather not make that available for assignment - add-ons can do that on their own. This is because the check state would not be announced to you and it may not be a good idea to bound every menu option to scripts, as it may clutter input gestures list. Besides, speech viewer is used in presentations where the presenter may wish to let the audience read what NVDA is saying, and there is a possibility that someone may cause this window to disappear by pressing some keys. Same can be said about some other options such as log viewer (in case of log viewer, a command does open it but that is used in some specific situations). Thanks for the code.

nvaccessAuto commented 9 years ago

Comment 8 by leonarddr (in reply to comment 7) on 2015-02-10 08:33 Replying to nvdakor:

As for speech viewer shortcut, I'd rather not make that available for assignment - add-ons can do that on their own. This is because the check state would not be announced to you

In the current code, NVDA announces enabling and disabling of speech viewer

and it may not be a good idea to bound every menu option to scripts, as it may clutter input gestures list.

What is the current input gestures list policy? I don't think a long list is a huge problem, furthermore, we have the ability to filter the list.

Besides, speech viewer is used in presentations where the presenter may wish to let the audience read what NVDA is saying, and there is a possibility that someone may cause this window to disappear by pressing some keys. Same can be said about some other options such as log viewer (in case of log viewer, a command does open it but that is used in some specific situations).

I see. I'll wait for Jamie to decide about Speech viewer. It is quite easy to undo those commits if requested.

nvaccessAuto commented 9 years ago

Comment 9 by jteh (in reply to comment 7) on 2015-02-20 06:44 Replying to nvdakor:

But once we get this merged, would it be possible to continue working from t4898 alone?

I don't really understand. Once this is merged, there hopefully won't be any changes, unless there are bugs, in which case, yes, they should be fixed in t4898.

As for speech viewer shortcut, I'd rather not make that available for assignment...

I agree with comment:8.

Besides, speech viewer is used in presentations where the presenter may wish to let the audience read what NVDA is saying, and there is a possibility that someone may cause this window to disappear by pressing some keys.

That's a valid point, but if they don't want that to happen, they shouldn't bind it. This is perhaps a good argument for not binding it by default, but I don't think it is relevant as to whether it can be bound at all.

nvaccessAuto commented 9 years ago

Comment 10 by jteh on 2015-02-20 07:06 Thanks! Review:

In globalCommands

Typo: tempoary -> temporary

nit: There are some double blank lines; e.g. before and after the speech viewer toggle function. Please kill one of the blank lines in these cases.

+ # Translators: The message announced when toggling the speech viewer visibility. I'm concerned translators might interpret this comment as meaning the message needs to cover both enabling and disabling. It's probably best to be explicit in this case; e.g.

# Translators: The message announced when disabling speech viewer.

In the User Guide


+Note that not all preferences dialogs can be accessed with dedicated shortcut keys.

This is keyboard specific. It should probably say "input gestures", but since input gestures is a term many users won't be familiar with, it should give a quick example. Something like this:

Note that by default, not all preferences dialogs can be accessed with dedicated input gestures (keyboard shortcuts, touch gestures, etc.).

linguistic: On the next line, "doesn't" should be "don't".

Despite my own thoughts in comment:1 (sorry!), I'm wondering whether the "(shortcut key unassigned)" text is really necessary. There's already a note at the top explaining that some aren't assigned and how to do this, which I think is a good (and better) idea. I'm thinking this might be enough. Aside from redundancy, "shortcut key unassigned" is also keyboard specific; it doesn't cover other types of input gestures.

nvaccessAuto commented 9 years ago

Comment 11 by Joseph Lee <joseph.lee22590@... on 2015-02-20 07:19 In [d037fbdd60066248b8e1e9eb5896e179f022471f]:

User guide and global commands: linguistic and spelling fixes, redundant information removed and wording fixes. re #4898
Code review by: James Teh (jamie@nvaccess.org)
nvaccessAuto commented 9 years ago

Comment 12 by leonarddr on 2015-02-20 09:18 Also updated my branch to apply the points made in @jteh's code review.

nvaccessAuto commented 9 years ago

Comment 13 by jteh on 2015-02-20 09:50 This is starting to get confusing. Joseph, I assume your branch does not contain some of Leonard's fixes... or are they both the same?

nvaccessAuto commented 9 years ago

Comment 14 by leonarddr (in reply to comment 13) on 2015-02-20 09:55 Replying to jteh:

This is starting to get confusing. Joseph, I assume your branch does not contain some of Leonard's fixes... or are they both the same?

I assume that you used my branch for the code review? That branch included everything in Joseph's branch as well as what I described in comment:5. I guess Joseph applied all the things mentioned in the code review which were available in his branch. I merged in those changes and applied the points spesific to my branch. In any case, my branch is up to date with Joseph's, so if he could merge in mine we're all set and synchronised.

nvaccessAuto commented 9 years ago

Comment 15 by nvdakor (in reply to comment 14) on 2015-02-20 10:20 Replying to leonarddr:

Replying to jteh:

This is starting to get confusing. Joseph, I assume your branch does not contain some of Leonard's fixes... or are they both the same?

I assume that you used my branch for the code review? That branch included everything in Joseph's branch as well as what I described in comment:5. I guess Joseph applied all the things mentioned in the code review which were available in his branch. I merged in those changes and applied the points spesific to my branch. In any case, my branch is up to date with Joseph's, so if he could merge in mine we're all set and synchronised.

When I worked on the code, I figured this ticket had better information and included ideas from the other ticket (the other branch that Leonard has worked on). Thus I worked on t4898 and Leonard based his code from the branch for the other ticket. The user guide changes and basic global commands edits are mine, and Leonard worked on add-ons manager and speech viewer script, but I'd give more credit to Leonard for bringing this up in the first place, so I'll back off. Thus let's unify the code based on Leonard's branch, since it has updated code. Thanks.

nvaccessAuto commented 9 years ago

Comment 16 by jteh on 2015-02-20 10:31 You've both done nice work. I just wanted to establish which branch I should be using. I was using Leonard's and it looks like that's the one I should keep using. When I get to reviewing this again, I'll sync the NV Access copy with Leonard's. Thanks.

nvaccessAuto commented 9 years ago

Comment 17 by leonarddr on 2015-02-20 12:21 I'm considering creating an unbound gesture for temporary disabling all configuration profile triggers. Would that fit under this ticket, or should i file a new one for it? I have a text editing profile which reports several pieces of text information, which i'd like to disable at some times. May be we could also include #4873 in this and make this one ticket for all gesture requests currently open?

nvaccessAuto commented 9 years ago

Comment 18 by jteh on 2015-02-20 12:42 Please file the profile one separately, as that requires a slightly new code path (albeit a relatively trivial one) and thus special testing.

nvaccessAuto commented 9 years ago

Comment 19 by jteh on 2015-02-20 12:47 I guess #4873 can be combined here. It probably needs a new script category though.

nvaccessAuto commented 9 years ago

Comment 20 by leonarddr (in reply to comment 19) on 2015-02-20 13:19 Replying to jteh:

I guess #4873 can be combined here. It probably needs a new script category though.

What would you suggest? Just document formatting? Also, should they really be global? Being able to disable reporting of links from the windows desktop doesn't make any sense. Don't know what other module to use though.

nvaccessAuto commented 9 years ago

Comment 21 by jteh on 2015-02-23 01:32 "Document formatting" is fine with me.

nvaccessAuto commented 9 years ago

Comment 22 by leonarddr on 2015-02-23 08:24 All right, it's all in t4898 now in my repository.

nvaccessAuto commented 9 years ago

Comment 24 by bdorer (in reply to comment 22) on 2015-04-19 21:22 @jamie would this fit into 2015.2 or do you have to review leonards last commits? If it's to late for 2015.2 I'd change release to 2015.3 so it woun't be vorgotten.

Replying to leonarddr:

All right, it's all in t4898 now in my repository.

nvaccessAuto commented 9 years ago

Comment 25 by jteh on 2015-04-19 22:50 Changes: Milestone changed from None to 2015.3

nvaccessAuto commented 9 years ago

Comment 27 by James Teh <jamie@... on 2015-04-20 02:01 In [bb6ea40acf8e60eed6bab634b48aded8c831860b]:

Merge branch 't4898' into next

Incubates #4898.

Changes: Added labels: incubating

nvaccessAuto commented 9 years ago

Comment 28 by jteh on 2015-04-20 02:03 Actually, barring any issues, I think we can just squeeze this into 2015.2.

Note that I made a few minor tweaks and squashed this all into one commit in our t4898 branch. Changes: Milestone changed from 2015.3 to 2015.2

nvaccessAuto commented 9 years ago

Comment 30 by leonarddr on 2015-04-23 06:45 There is only one issue with disabling the speech viewer using a hotkey. Speech should say 'speech viewer disabled.', however speech is somehow silenced and only speaks part of the message in my case.

nvaccessAuto commented 9 years ago

Comment 31 by James Teh <jamie@... on 2015-05-01 12:20 In [bd6c06f721c2f9eefed38024ba1ca4873fe2f949]:

It is now possible to assign input gestures (keyboard commands, touch gestures, etc.) for all NVDA preferences dialogs and document formatting options using the Input Gestures dialog.

Fixes #4898.
Authors: Joseph Lee joseph.lee22590@gmail.com, Leonard de Ruijter mail@leonardder.nl

Changes: Removed labels: incubating State: closed

nvaccessAuto commented 9 years ago

Comment 32 by JamaicanUser on 2015-05-02 15:34 There is an option called "report comments, but this is not apart of document formatting dialog. why?