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

Change multiple settings dialogs into one category based dialog #7302

Merged
merged 91 commits into from Apr 30, 2018

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Jun 19, 2017

Link to issue number:

closes #5774
closes #3644
Closes #4040

Summary of the issue:

Based on #577 (comment)

In the current situation, several settings subjects have their own dialog in NVDA. This makes configuring NVDA more complicated than necessary (i.e. you can't update all settings in one go and need to open multiple dialogs, either using the menu or using shortcut keys).

Description of how this pull request fixes the issue:

This pr combines the several configuration manager based dialogs into one single dialog. The resulting dialog has a list view with categories on the left side and a panel containing the settings on the right side.

Known issues with pull request:

From #577 (comment)

  • I didn't do anything about the look yet, so it might look very ugly. We probably need sighted assistance for this, so any thoughts from @feerrenrut are highly appreciated.
  • Currently, all the configuration manager options (e.g. general settings, voice settings) activate the same dialog. This will probably have to go away in the future
  • I would like to have a kind of heading at the top of every panel with the panel's title, but do not know yet how to accomplish this in a look friendly way. @@feerrenrut or @josephsl, ideas?
  • The categories list is a list box, which can be changed into a tree view if desired. We should consider this if speech dictionaries get integrated into this dialog as well.
  • The current implementation has no facilities to integrate add-ons into the system with backwards compatibility (i.e. integrating addons into the new system without any change into the add-ons code is currently not possible). Honestly, I think we should keep it that way and offer people information on how to upgrade their add-on.
  • Sub dialogs and message boxes will probably break
  • Changing the synthesizer or braille display without saving will make the synth or display selection go back to the initial selection when opening the dialog

Todo

  • Decide whether to stick to a ListBox, or should we choose a tree view anyway?
  • Arrange the visual lay out
  • Decide what to include and what not to include in the dialog (e.g. Punctuation/symbol pronunciation, speech dictionaries)

@feerrenrut
Copy link
Contributor

I have had a look at this, I think that a tree view for the categories would make more sense than a list view, even if there is no hierarchy. At the moment the size of the dialog changes with each category. I think we should address this, visually this is a bit jarring. This could be an "interesting" problem to solve. I would suggest that we remove the preferences submenu from the NVDA menu, making it a button that launches the dialog.

I would like to have a kind of heading at the top of every panel with the panel's title, but do not know yet how to accomplish this in a look friendly way.

Would it be acceptable to change the window title to achieve this?

@jcsteh
Copy link
Contributor

jcsteh commented Jul 4, 2017

@feerrenrut commented on 4 Jul 2017, 17:15 GMT+10:

I would suggest that we remove the preferences submenu from the NVDA menu, making it a button that launches the dialog.

I assume you mean a menu item rather than a dialog, but yes, I agree. The problem, though, is that this breaks existing shortcuts to quickly get to specific sections; e.g. NVDA+n, p, r for Braille Settings. As non-standard as it might be, I think overriding the default keyboard handling for the tree view may well be preferable to losing these shortcuts. Discoverability remains an open question, though.

I would like to have a kind of heading at the top of every panel with the panel's title, but do not know yet how to accomplish this in a look friendly way.

Do we actually need to do this? What do similar dialogs in other apps do to show the title of the active page (if anything)?

Would it be acceptable to change the window title to achieve this?

Perhaps, though I think it should be prefixed; e.g. "Preferences: Braille". Out of interest, I just checked foobar2000 and it does this.

@LeonarddeR
Copy link
Collaborator Author

@jcsteh commented on 4 jul. 2017 13:29 CEST:

@feerrenrut commented on 4 Jul 2017, 17:15 GMT+10:

I would suggest that we remove the preferences submenu from the NVDA menu, making it a button that launches the dialog.

I assume you mean a menu item rather than a dialog, but yes, I agree.

So this means speech dictionaries, input gestures, etc. should also go into this dialog?

@jcsteh
Copy link
Contributor

jcsteh commented Jul 4, 2017

@leonardder commented on 5 Jul 2017, 02:36 GMT+10:

So this means speech dictionaries, input gestures, etc. should also go into this dialog?

Blerg. No, I don't think that makes sense actually. If we were starting from scratch, I'd probably have a Preferences menu containing a Settings... item (this dialog) and the rest of the items as they are (Speech dictionaries, etc.). The problem is that this definitely breaks those well-known shortcuts.

I guess the only way to get around this is to somehow alter the way key presses are handled in the NVDA menu. Yuck.

@LeonarddeR
Copy link
Collaborator Author

@jcsteh commented on 5 jul. 2017 00:50 CEST:

The problem is that this definitely breaks those well-known shortcuts.

I guess the only way to get around this is to somehow alter the way key presses are handled in the NVDA menu. Yuck.

If the same order/sequence of key presses is that important to you, I guess we only have two options left. Drop this whole idea and pr altogether, or create a preferences... menu item and make the tree view react properly to the given key presses.
Actually, the latter breaks with add-ons, but we can rename the preferences menu to add-on preferences and only show it when it is populated.

@feerrenrut
Copy link
Contributor

feerrenrut commented Jul 5, 2017 via email

@jcsteh
Copy link
Contributor

jcsteh commented Jul 5, 2017

@feerrenrut commented on 5 Jul 2017, 14:33 GMT+10:

I assume you mean a menu item rather than a dialog

No, I don't think so. However we might be talking about the same thing.

As discussed out-of-band, I meant to say "button" instead of "dialog" here... and we are indeed talking about the same thing. :)

After some extremely lengthy discussion, here's what we think should happen.

  1. The Preferences sub-menu of the NVDA menu should contain a "Settings..." item which opens this new settings dialog.
  2. Items such as General settings, Synthesizer, etc. that previously opened settings dialogs should be removed.
  3. The remaining items in the Preferences sub-menu (Speech dictionaries, Punctuation/symbol pronunciation and Input gestures) should be left as is.

Notes:

  • This does mean we will lose the "NVDA+n p g" style keyboard shortcuts. However, we can't come up with a way to support these which isn't super ugly. Given that this is a major UX change, it's reasonable to expect this to change.
  • As we gain more settings pages, it becomes harder to avoid key conflicts anyway. At this point, typing the first few letters is certainly more intuitive; e.g. "bro" will get you to Browse mode. Eventually, being able to search the settings will make things even more intuitive/efficient.
  • We will keep the existing scripts for opening general settings, etc. and these will open the appropriate page in the new dialog. We won't bind any additional commands, but users could bind a key to directly open, for example, the braille settings as they could before.
  • We considered renaming Preferences to Customization so that the shortcuts completely change instead of just partially; i.e. a "clean slate". We decided against this because:
    1. It conflicts with the accelerator for Configuration profiles.
    2. While it's true that users might be confused as to why NVDA+n p g doesn't work, at least they'll see the new Settings item when they hit NVDA+n p and know that something has changed. In contrast, changing the name would mean the user just hears a ding (or gets nothing) when they press NVDA+n p, and they then have to closely look through the NVDA menu to find the settings.

@LeonarddeR
Copy link
Collaborator Author

@jcsteh commented on 5 jul. 2017 07:51 CEST:

  • We considered renaming Preferences to Customization so that the shortcuts completely change instead of just partially; i.e. a "clean slate". We decided against this because:

    1. It conflicts with the accelerator for Configuration profiles.
    2. While it's true that users might be confused as to why NVDA+n p g doesn't work, at least they'll see the new Settings item when they hit NVDA+n p and know that something has changed. In contrast, changing the name would mean the user just hears a ding (or gets nothing) when they press NVDA+n p, and they then have to closely look through the NVDA menu to find the settings.

I assume you also considered renaming preferences to settings? This is probably something we should not do due to the same reasons as above?

@jcsteh
Copy link
Contributor

jcsteh commented Jul 5, 2017

I assume you also considered renaming preferences to settings?

We didn't, actually, but as you say, I don't think we should do this for the same reasons.

LeonarddeR pushed a commit to BabbageCom/nvda that referenced this pull request Jul 5, 2017
@LeonarddeR
Copy link
Collaborator Author

@feerrenrut, could you have another look? I hopefully improved stuff regarding the layout.

@derekriemer
Copy link
Collaborator

derekriemer commented Jul 6, 2017

what about addons breaking? Should we provide an AddPage method somehow to add a settings page for an addon?

@jcsteh
Copy link
Contributor

jcsteh commented Jul 6, 2017 via email

@feerrenrut
Copy link
Contributor

feerrenrut commented Jul 6, 2017

@LeonarddeR I have had another look now. This seems to be coming along nicely.
There are a couple of things that strike me right now:

  • It think it would be nice to have some vertical separator and space between the categories and the active settings page.
  • We should try to normalize the width of the options dilogs. The general settings page in particular has several very wide options:
    • The language selection label
    • The "Use currently saved settings on the logon and other secure screens (requires administrator privileges)" button

@LeonarddeR
Copy link
Collaborator Author

@feerrenrut commented on 6 jul. 2017 04:52 CEST:

There are a couple of things that strike me right now:

  • It think it would be nice to have some vertical separator and space between the categories and the active settings page.

That shouldn't be that difficult to do, the only question is, how wide should the space be. I will try to come up with something.

  • We should try to normalize the width of the options dilogs. The general settings page in particular has several very wide options:

    • The language selection label
    • The "Use currently saved settings on the logon and other secure screens (requires administrator privileges)" button

Formally, this is not related to this issue, the problem with the current change is that this problem is getting more and more obvious. Would this really block the new dialog from a visual perspective?

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Jul 6, 2017 via email

@derekriemer
Copy link
Collaborator

They would break because it is no longer a preferences submenu, and many add their item to the preferences submenu.

@feerrenrut
Copy link
Contributor

@LeonarddeR I dont think #577 should be blocked by #6455. Fixing the issues in #6455 will improve the NVDA ux generally. But it isn't required for this work. I think you had it right saying:

this problem is getting more and more obvious

The change to put the different settings pages in one dialog highlights the difference in width of the items within those dialogs.

@LeonarddeR
Copy link
Collaborator Author

@feerrenrut: I now added the requested vertical line, checked with object nav and it seems to be as high as the panel, which is what I intended. I also changed the width of the tree view a bit, based on the current items in it, it shouldn't be wider than 100, but correct me if I'm wrong. This should also mean that the panel is more left now.

I also wonder whether it would be possible to get rid of the scrollbar(s) for the tree view, but it seems changing the height doesn't make that much difference.

@LeonarddeR
Copy link
Collaborator Author

@derekriemer commented on 6 jul. 2017 07:07 CEST:

They would break because it is no longer a preferences submenu, and many add their item to the preferences submenu.

You probably missed #7302 (comment)

@derekriemer
Copy link
Collaborator

gotcha. What about providing a way for addons to register a dialog or page with the new dialog? Did I miss that too? sorry, we had a holiday here, and I missed some tickets because I decided to stop using email for github and use notifications instead.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Jul 6, 2017

@derekriemer commented on 6 jul. 2017 11:26 CEST:

gotcha. What about providing a way for addons to register a dialog or page with the new dialog?

At the moment, registering a new panel is as easy as creating a Settingspanel AND DOING GUI.settingsDialogs.NVDASettingsDialog.categoryClasses.append(newClass)

@derekriemer derekriemer changed the title Change multiple settings dialogs into one, category based dialog Change multiple settings dialogs into one category based dialog Jul 6, 2017
@derekriemer
Copy link
Collaborator

removed a comma from the title.

@LeonarddeR
Copy link
Collaborator Author

I cleaned this up a bit and squashed this back into three commits now, but before this is ready for review, there's some work to be done, might do another squash later. Note that this is not the normal procedure for pull requests, since normally they are opened when they are ready for review.

@feerrenrut: I got some assistance from my wife and I got rid of the scrollbars now. My wife commented that it looks a bit strange to have nothing in the left bottom corner of the dialog. Would it be nice to vertically center align the tree view?

I also added control+tab and control+shift+tab support for easier navigation between tabs. This is currently a way to go back to the categories list. I'm a bit hesitant with adding an accelerator to the tree view. Since we have the char_hook on the dialog anyway, I'd rather make F6 switch between the panel and the tree view.

@derekriemer commented on 7 jul. 2017 00:35 CEST:

removed a comma from the title.

Oops, that was me applying dutch rules to English text.

@LeonarddeR
Copy link
Collaborator Author

@feerrenrut: Coming back to this, I think this is ready for review already, unless we really want an accelerator for the tree view. However, layout review is something that can be done as part of the code review.

@beqabeqa473
Copy link
Contributor

beqabeqa473 commented Apr 9, 2018 via email

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Apr 9, 2018 via email

@beqabeqa473
Copy link
Contributor

beqabeqa473 commented Apr 9, 2018 via email

@LeonarddeR
Copy link
Collaborator Author

@leonardder commented on 9 apr. 2018 16:31 CEST:

Thanks for catching that one. However, this bug also exists in master,

I horribly lied there. Just pushed a commit that fixes this and restores the old behaviour. I have no idea why this has been changed, actually. Probably in order not to reload a currently active synthesizer when it's already active.

@LeonarddeR
Copy link
Collaborator Author

@michaelDCurran commented on 7 apr. 2018 21:33 CEST:

But personally... I think users can just get used to pressing tab one
more time to get to the Voice combo box in the first place.

When pressing ctrl+NVDA+v, it is two tabs to the voice combo box, or pressing alt+v. People who would like to change voices more quickly really should consider using the synthesizer settings ring.

Moving the change synthesizer button further in the tab order will create an asymmetry with the braille settings dialog. In there, the braille display selection combo box has always been the first option.

@michaelDCurran
Copy link
Member

michaelDCurran commented Apr 10, 2018 via email

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

I've had a look over the rest of these changes, it looks good to me. @michaelDCurran if you are also happy, can we re-incubate this?

michaelDCurran added a commit that referenced this pull request Apr 12, 2018
@LeonarddeR LeonarddeR mentioned this pull request Apr 20, 2018
@michaelDCurran michaelDCurran merged commit df535cc into nvaccess:master Apr 30, 2018
@nvaccessAuto nvaccessAuto added this to the 2018.2 milestone Apr 30, 2018
@DrSooom
Copy link

DrSooom commented May 6, 2018

Based on Master 15089,482e916c: Why is the menu entry "Settings..." in the NVDA menu called "Settings..." and not "NVDA Settings..."? In the manual the new dialog is called "11.1. NVDA Settings".

@josephsl
Copy link
Collaborator

josephsl commented May 6, 2018 via email

@DrSooom
Copy link

DrSooom commented May 6, 2018

EN path: NVDA menu -> Preferences -> Settings...
DE path: NVDA-Menü -> Einstellungen -> Einstellungen...
Hint: The DE path is currently not translated into German in Master. It's my translation.

The words "preferences" and "settings" mean the same in German – Einstellungen. And as the manual call it "NVDA Settings", translating this in other languages might cause less irritations.

New EN path: NVDA menu -> &Preferences -> NVDA &Settings...
New DE path: NVDA-Menü -> &Einstellungen -> NVDA-&Einstellungen...

@feerrenrut
Copy link
Contributor

Since there is less ambiguity (or at least there is more than one word) in English, and brevity is desirable, I would suggest keeping the English as is. Happy for the German translation to be changed if it makes it clearer. Perhaps we can update the # Translators: ... comment in the source to make this more apparent to translators.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented May 7, 2018 via email

@DrSooom
Copy link

DrSooom commented May 7, 2018

The words "Voreinstellungen" and "Optionen" would be possible but the first one sounds a little bit strange here. Well, and the second one change the whole wording - and the manual has to change to the new wording too.

So the final question is: Have the translators the permission to add the prefix "NVDA" (or "NVDA-") for the "Settings..." menu entry if it is necessary in their languages? On German - as well as in English - most of the people will say "Öffne die NVDA-Einstellungen und gehe dort zu ..." (EN: "Open the NVDA settings and go to ..."). In the end some languages have the NVDA-prefix here and some not. Would this a suitable solution for you? And as long as Add-on can enter menu items anywhere in the NVDA menu, adding the NVDA-prefix could reduce irritations as well. But I also understand that the NVDA-prefix isn't necessary in all languages at all.

@josephsl
Copy link
Collaborator

josephsl commented May 7, 2018 via email

@LeonarddeR LeonarddeR added the BabbageWork Pull requests filed on behalf of Babbage B.V. label Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BabbageWork Pull requests filed on behalf of Babbage B.V. component/NVDA-GUI
Projects
None yet