Tidy advanced gui panel by feerrenrut · Pull Request #9239 · nvaccess/nvda

feerrenrut · 2019-02-04T09:43:05Z

Summary of the issue:

With the introduction of the advanced panel in the settings dialog #9200, there were a couple of UI issues, the text did not wrap properly, and the warning message was only announced when the dialog received focus (not when moving from the category list to the panel itself).
Several times during development, old settings dialog windows were not being destroyed, this meant that the settings dialog could not be opened a second time. The check for multiple instances was trying to serve two purposes:

Stop users from opening a second settings dialog while one was already open, changing settings from two places at once could have unintended and confusing outcomes.
Warn developers that dialogs are not closing / being disposed of correctly.

Description of how this pull request fixes the issue:

The long text is now wrapped at the width of the panel. The "Warning!" text is larger and bold.

The warning text is announced when the panel receives focus.

A state is appended to the settings dialog instances, this keeps track of whether the dialog has been destroyed and it is removed when the dialog is deleted. We can then give the standard "user friendly" error when the settings dialog is already open, and log an error when the window is destroyed but not deleted.

Testing performed:

The settings dialog should be reported as prior versions.

The panel description should be reported when the dialog gets focus and the focused control is:

a control on the document formatting panel
a control on the advanced panel
This description should only be reported once, regardless of whether the change in focus is due to alt+tab (swapping applications), ctrl+tab (swapping settings categories), or moving from the category list to the settings panel

Known issues with pull request:

Due to the delays of garbage collection, if the NVDA settings dialog is closed and opened again quickly, an error may be added to the log.

Alternatives:

The text could be used as a label for a checkbox which enables / disables the rest of the panel. The checkbox must be checked before modifications can be made. Perhaps it's state is saved in the configuration?

Change log entry:

Section: New features, Changes, Bug fixes

Changes:

 - Descriptive text is now announced for the advanced settings panel and the document formatting panel (#9239).

Attempt to use a known window ID to force a name change event to be spoken. Otherwise the warning is spoken when the dialog looses and gets focus again.

michaelDCurran · 2019-02-04T09:50:55Z

I think it should be a warning checkbox at the beginning of the Advanced panel that remembers its state for the lifetime of NVDA, but is not saved in the config. This is similar to Firefox.

LeonarddeR · 2019-02-04T10:01:03Z

I second this idea, however, I wouldn't put the warning on the checkbox itself if possible.

If I"m correct, you can do the following:

Create another static box (grouping) called warning
Add a static text with the warning to that grouping. I recall that magic will automatically assign that static text to accDescription on the grouping, but that's the weak part in my idea, I'm not entirely sure.
You can then add a check box with a relatively short label.

This reverts commit 3ffa482.

DrSooom · 2019-02-04T16:44:10Z

• The text could be used as a label for a checkbox which enables / disables the rest of the panel. The checkbox must be checked before modifications can be made. Perhaps it's state is saved in the configuration?

This … no, a checkbox alone, which value is stored in the "nvda.ini" doesn't solve this GUI issue at all, but a button called "Understood" like in Firefox when entering "about:config". All settings in this panel are unaccessible (= write protection) unless this button has pressed.

Well, the checkbox for automatically pressing this button (= disables this button and makes all other settings writable) can be added in this panel too. But we should prevent the normal user for changing something in this panel without showing him the warning first. So a combination of a button and a checkbox should be the best way to solve this GUI issue.

Takes a mapping of the prop and value to override.

…his.

This is handy when working on the NVDA GUI.

feerrenrut · 2019-02-06T17:22:28Z

So after trying lots of options with little success, I think the only viable option is to use the AccPropServer to:

override the description for the panel
set the panel role to property page

I am then setting the name of the panel using self.SetName, because overriding it with AccPropServer did not result in it being spoken!

One remaining issue is that the alt+tabbing to the dialog results in getDialogText running (in behaviours.py ) which essentially makes the dialog speak twice. For now I have resolved this by removing controlTypes.ROLE_PROPERTYPAGE from the collection of child roles included. We will need a better solution for this.

This result is that we can have these bits of "dialog explaining text" announced.
See the updated description for full test results.

LeonarddeR · 2019-02-06T18:19:31Z

Is this considered ready for review, or are you still considering changing some things?

LeonarddeR · 2019-02-06T18:55:17Z

Nevertheless, I have tested the code. Here are some thoughts you might want to consider:

Removing event_gainFocus from the NVDA appModule broke the configuration profile changes.
Have you considered subclassing wx.StaticBox in a way that implements an AccPropServer for accDescription on the StaticBox, instead of setting the description on the panel? This requires setting the "warning" label as a label on the StaticBox, instead of adding a separate static text that contains the label. The advantage of this is that the warning is read again when pressing shift+tab from the UI Automation group. It is not read when entering the property page from the ok button, though.
Using ROLE_PROPERTYPAGE for settings panels is a nice improvement. Make sure that you do not add an accPropServer on the VoiceSettingsPanel and BrailleSettingsSubPanel though. One way to accomplish this is: only add a server on a panel when it has a title.
Rather than giving every settings panel its own AccPropServer instance, you might want to consider registering one accPropServer on MultiCategorySettingsDialog.container, making that a property page and providing the accDescription based on the currently active settings panel. When switching categories, you will have to call SetLabel on the container. Hopefully that will trigger event_namechange on it.

feerrenrut · 2019-02-07T10:51:52Z

Is this considered ready for review

To start with I wanted to get some feedback on whether this actually improves things, find out what issues there might be from a UX consideration. If we can agree on what is possible, and that it improves the UX then we can get it ready for an actual review.

broke the configuration profile changes.

Ah, yes. I'll take a look at that.

One way to accomplish this is: only add a server on a panel when it has a title.

Good idea.

you might want to consider registering one accPropServer on MultiCategorySettingsDialog.container

Yes, probably a good idea. Though, do you know of any disadvantages to having multiple accPropServers registered?

Hopefully that will trigger event_namechange on it.

I was hoping to be able to remove stuff from the NVDA app module. It might not be possible yet.

LeonarddeR · 2019-02-07T11:11:34Z

Though, do you know of any disadvantages to having multiple accPropServers registered?

Not really, except for the fact that systems tend to break when an accPropServer isn't properly unregistered, which could possibly happen if NVDA crashes or when one of the panels refuses to load due to an error within the panel's class. While this is not likely to happen in stable versions, the breakage seems to be pretty major sometimes (#8916).

Hopefully that will trigger event_namechange on it.

I was hoping to be able to remove stuff from the NVDA app module. It might not be possible yet.

I'm pretty sure that gainFocus can be limited to do profile name changes only. I recall that I tested setting the current category's name on the container and then stopped with that because the name wasn't announced automatically when I changed categories, but that probably was because of the panel not having the role of property page.

Simplifies the approach to speaking profile changes in NVDA settings.

Having a non space description overrides the getDialogText behaviour (in behaviours.py).

feerrenrut · 2019-02-07T13:20:12Z

I've pushed a few fixes the concerns raised. I also found how to override the getDialogText behavior. This was causing the dialog to be spoken (up to first focused child). Now the dialog is speaking the current profile as the description. However, this information is also in the title. I am looking for a way to get the accessible name for the dialog to be spoken instead of the title. Another possibility, might be to change the getDialogText behavior so that when encountering a focused property page with a description the recursion stops there.

LeonarddeR · 2019-02-07T13:46:58Z

+				panel,
+				propertyAnnotations={
+					oleacc.PROPID_ACC_ROLE: oleacc.ROLE_SYSTEM_PROPERTYPAGE,  # change the role from pane to property page
+					oleacc.PROPID_ACC_DESCRIPTION: panel.panelDescription,  # set a description


What I've just noticed is that the NVDAObjects.behaviors.Dialog overlay is also applied to property pages, so it looks like you can safely remove the accDescription override for panels like this and only override the accRole. I have commented out this line and it did not result in different behavior.

Hmm, yeah, you are right. The hard question is what to do about the dialog description. Either we need a way to stop the getDialogText on these functions, or a way to override the dialog title.

What part of the dialog description do you mean in this case?

I really like the setup where the config profile is pronounced as part of the dialog description.

How about doing something like this:

Only have NVDA Settings in the dialog title.

Add a statictext with "editing {profilename}". Then, add the categories list and all other controls. This will make the profile name the first child on the dialog, which will be automatically announced using getDialogText. You no longer have to override accDescription for the dialog

When the current profile name changes, update the particular static text with the new profile name. Do something like this on the dialog:
winUser.user32.NotifyWinEvent(winUser.EVENT_OBJECT_DESCRIPTIONCHANGE, settingsDialog.Handle,winUser.OBJID_CLIENT,winUser.CHILDID_SELF)
On the appModule, the only thing you need to do is override event_descriptionChange on the dialog to speak even when the dialog isn't the focus object.

This has the advantage that the visual behavior and non visually perceived behavior match much more than before.

The difficult part with what you are proposing is finding a way to make the profile text look right within the dialog. It's not obvious how to achieve that.

What part of the dialog description do you mean in this case?

I meant, after alt tab to the dialog. NVDA walks through to the focused control, speaking all static text until it gets there. I now realise that as long as PROPID_ACC_DESCRIPTION is set to something other than white space this will not happen.

I like the idea of using the accessible description for the dialog to announce the profile name. Next I'm going to look into why NVDA does not seem to be getting the PROPID_ACC_NAME for the dialog. Inspect.exe shows it, but its not in NVDA's dev info.

The difficult part with what you are proposing is finding a way to make the profile text look right within the dialog. It's not obvious how to achieve that.

Isn't it possible to position it centered just below the window title?

It's possible, I'll have to play with it to see how it looks. Because it's not editable it might look a little strange. I have been thinking it makes sense to be able to edit profiles that are not actually active, so maybe it should be a control at some stage.

feerrenrut · 2019-02-08T09:55:28Z

I haven't worked out why yet, but it seems that inspect.exe will give the name that I set via the acc prop server. However NVDA dev info gives the window title.

The windows title bar includes the profile name. This gets spoken as "NVDA Settings: (normal configuration) dialog" Instead we want this spoken as: "NVDA Settings dialog normal configuration" The other benefit of the description being set is that when the dialog receives focus, it uses the description rather than the getDialogText implemenation in behaviours.py. In the settings dialog, more is spoken by getDialogText than is necessary, because we have a property page with a description which also gets spoken. This is required, so that the explanation of the settings panels is spoken when moving from the category list to the panel.

The debugging comment is not necessary anymore. The wxInspection tool is going now part of another PR

lukaszgo1 · 2019-02-21T22:48:56Z

What do you mean by saying

As much as this is a loss of information when pressing NVDA+tab,

I was talking about NVDA +t. Currently to check which profile we are editing or in which category of the settings we are it is enough to check the title. bar. With the try build from this PR it is nessesary to alt+tab from and alt+tab back to the dialog. Adding to this the fact that when alt+tabbing to any of the settings dialog your position is lost I believe that it isn't an acceptable solution.

michaelDCurran · 2019-02-21T22:55:45Z

Sorry, I meant NVDA+t. However, I do take your point that the category cannot be read on demand anymore. But, I'm not entirely sure of the use case where you would be in the settings dialog and not know the category. either you alt+tabbed in there and it spoke as part of the focus ancestry as focus was inside the property page, or you had set the category from the list yourself. I'm not convinced this problem is enough to block this change?

lukaszgo1 · 2019-02-21T22:59:28Z

I don't know if it is an showstopper or not. I am merely pointing out that this change makes retrieving information about currently editing profile and the current category more difficult than it was before.

feerrenrut · 2019-02-25T09:23:11Z

Thanks for testing @lukaszgo1, and you raise a good point. Not something that had been considered yet. We would like to get this merged in, since it's holding back the release. For now we will go with the current approach. Please see #9321 which could be an approach to resolving this.

lukaszgo1 · 2019-02-25T11:35:50Z

I don't believe that #9321 would improve this situation. because there is no way to reread this control on demand like the title bar.
What about merging this as is and for the next release simply make the content of title bar like it was before this pr?

feerrenrut · 2019-02-25T11:53:58Z

there is no way to reread this control on demand like the title bar.

I figured if there was an actual control, focus can be moved back to the control to read it. Obviously this is not as quick as just using NVDA+t. It should be noted that when the profile being edited changes, it is still reported. Perhaps a script to report the current profile, those who depend on this set a gesture?

lukaszgo1 · 2019-02-25T15:45:55Z

I don't think that not having the active config profile and setting category dialog in the title bar is a big regression however it was nice to have and was intuitive. Introducing additional gesture to reread active profile is in my view an overkill. Is it really technically impossible to have the title bar to contain these information? If it is then simply leaving it as is seems to be the best solution.

feerrenrut · 2019-02-25T17:14:10Z

Is it really technically impossible to have the title bar to contain these information?

It's not technically impossible, but then there are other problems that would need to be solved. Right now, we are using the description of the dialog to report the configuration profile. If this is empty, then NVDA will try to describe the dialog automatically. On panels like the advanced panel or the document formatting panel, this results in the panel description being read twice. This happens because the panel has a description that matches the static text that describes the dialog, this is so the description is read when the panel gets focus. NVDA's dialog description logic reads all static text starting at the dialog title, descending down until it gets to the first active control.

Adding a command to report the active profile is easy. I have a draft pull request: #9325

feerrenrut · 2019-02-25T17:26:51Z

Actually, while testing this again I have noticed that setLabel is also overwriting the visible title.

LeonarddeR · 2019-02-25T18:56:01Z

I guess that for now, we should leave the title as is in current Alpha, i.e. remove the title changes from this pr.

LeonarddeR · 2019-02-26T07:46:37Z

NVDA's dialog description logic reads all static text starting at the dialog title, descending down until it gets to the first active control.

I'm wondering whether this behaviour of getDialogText is right. If there is a property page in the dialog that has a description retrieved by getDialogText, and that property page has a focusable (not focused) descendant, than the description of the property page shouldn't end up in the description of the dialog at all, since the user can have it read by focusing a decendant of the property page.

Alternatively, there is some logic in this code that stops adding grouping descriptions to the dialog text. Overall, it is pretty complex and error prone.

No longer stop a user from accessing settings when there is a destroyed but not deleted window. Instead raise an error. Also check for this error on when terminating the GUI.

Overriding with an empty description so that panel descriptions are not reported twice (when getDialogText runs). The panel descriptions are still reported when the panel gets focus.

Include the category name, so that it is easier to know what category one is in by using NVDA+t

feerrenrut · 2019-02-26T16:02:28Z

I think for now we will be best to rely on the NVDA appmodule to override this behaviour. I believe this is ready for review.

LeonarddeR

Some cosmetic comments that aren't really important and shuldn't block this.

* Make the warning bigger, and wrap the long text explaining the dialog. * Add checkbox to enable the advanced controls - This helps to ensure that users are aware of what they are changing. - api.processPendingEvents is used before calling enable on the panel containing the controls, otherwise NVDA had a mismatch with the reported and actual state of the checkbox. * Add a restore defaults button to the advanced panel. - This allows users who have previously changed settings, restore them to recommended settings. * Change settings panels to use role "property page" - Now when they get focus they are announced as "general property page" * Speak extra info on advanced and document formatting categories. - This is essentially a property page description. It is the static text at the start of the panel. * Created a simplified, reusable acc prop server - Takes a mapping of the prop and value to override. * Fix multi-instance error for settings dialog. - No longer stop a user from accessing settings when there is a destroyed but not deleted window. Instead raise an error. Also check for this error on when terminating the GUI. * Override the description property for the NVDA settings dialog - Overriding with an empty description so that panel descriptions are not reported twice (when getDialogText runs). The panel descriptions are still reported when the panel gets focus.

feerrenrut added 2 commits February 2, 2019 11:12

Make the warning bigger, and wrap the long text explaining the dialog.

17c94f6

Speak extra info on advanced panel

3ffa482

Attempt to use a known window ID to force a name change event to be spoken. Otherwise the warning is spoken when the dialog looses and gets focus again.

feerrenrut added 3 commits February 4, 2019 11:19

Revert "Speak extra info on advanced panel"

e8f3323

This reverts commit 3ffa482.

Fix typo in comment.

57d2a74

Add checkbox to enable the advanced controls

4961b63

feerrenrut added 7 commits February 5, 2019 21:10

Try formatting the advanced panel in a group

4962c61

Implement accProp server for panels / dialogs.

dc89b9c

Experiment with stopping dialog text for propertypage.

1b087db

reusable acc prop server

113432b

Takes a mapping of the prop and value to override.

Other adjustments to speaking of NVDA settings.

209f639

Get rid of speak dialog test, we will need a different approach for t…

60f0127

…his.

Add a script to start the wx inspection tool.

fd81a36

This is handy when working on the NVDA GUI.

Get rid of unused accprop server class

dbb5ece

feerrenrut added 3 commits February 7, 2019 12:36

Restore profile change announcement.

dd260e8

Simplifies the approach to speaking profile changes in NVDA settings.

Don't announce property page for speech and braille sub panels

2b54499

Speak the configuration profile as dialog description.

8180e65

Having a non space description overrides the getDialogText behaviour (in behaviours.py).

LeonarddeR reviewed Feb 7, 2019

View reviewed changes

feerrenrut added 3 commits February 12, 2019 09:34

Merge remote-tracking branch 'origin/master' into tidyAdvancedGuiPanel

ce7d5ca

remove debugging comment and wxInspection tool

343adf7

The debugging comment is not necessary anymore. The wxInspection tool is going now part of another PR

feerrenrut mentioned this pull request Feb 25, 2019

Allow editing of inactive profiles #9321

Open

fix incorrect checkbox state change reporting

b539db1

clean up log message

8383f47

feerrenrut requested a review from michaelDCurran February 25, 2019 12:04

LeonarddeR reviewed Feb 25, 2019

View reviewed changes

Comment thread source/gui/settingsDialogs.py

feerrenrut mentioned this pull request Feb 25, 2019

add reportActiveConfigurationProfile command #9325

Merged

feerrenrut added 3 commits February 26, 2019 14:12

Diagnostics for dialogs that are kept alive

feae6c8

No longer stop a user from accessing settings when there is a destroyed but not deleted window. Instead raise an error. Also check for this error on when terminating the GUI.

Override the description property for the NVDA settings dialog

44c2c29

Overriding with an empty description so that panel descriptions are not reported twice (when getDialogText runs). The panel descriptions are still reported when the panel gets focus.

Revert format of the title

d5557e6

Include the category name, so that it is easier to know what category one is in by using NVDA+t

LeonarddeR approved these changes Feb 26, 2019

View reviewed changes

Comment thread source/appModules/nvda.py Outdated

Comment thread source/gui/nvdaControls.py

Comment thread source/gui/settingsDialogs.py Outdated

Comment thread source/gui/settingsDialogs.py

Comment thread source/gui/settingsDialogs.py Outdated

michaelDCurran approved these changes Feb 26, 2019

View reviewed changes

Comment thread source/appModules/nvda.py Outdated

Comment thread source/gui/settingsDialogs.py

Review actions

43985c2

LeonarddeR approved these changes Feb 27, 2019

View reviewed changes

feerrenrut merged commit ad8e391 into master Feb 27, 2019

nvaccessAuto added this to the 2019.1 milestone Feb 27, 2019

feerrenrut deleted the tidyAdvancedGuiPanel branch January 17, 2020 08:58

Uh oh!

Conversation

feerrenrut commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Alternatives:

Change log entry:

Uh oh!

michaelDCurran commented Feb 4, 2019

Uh oh!

LeonarddeR commented Feb 4, 2019

Uh oh!

DrSooom commented Feb 4, 2019

Uh oh!

feerrenrut commented Feb 6, 2019

Uh oh!

LeonarddeR commented Feb 6, 2019

Uh oh!

LeonarddeR commented Feb 6, 2019

Uh oh!

feerrenrut commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LeonarddeR commented Feb 7, 2019

Uh oh!

feerrenrut commented Feb 7, 2019

Uh oh!

LeonarddeR Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

feerrenrut Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

LeonarddeR Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

feerrenrut Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

LeonarddeR Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

feerrenrut Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

feerrenrut commented Feb 8, 2019

Uh oh!

lukaszgo1 commented Feb 21, 2019

Uh oh!

michaelDCurran commented Feb 21, 2019 via email

Uh oh!

lukaszgo1 commented Feb 21, 2019

Uh oh!

feerrenrut commented Feb 25, 2019

Uh oh!

lukaszgo1 commented Feb 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feerrenrut commented Feb 25, 2019

Uh oh!

Uh oh!

lukaszgo1 commented Feb 25, 2019

Uh oh!

feerrenrut commented Feb 25, 2019

Uh oh!

feerrenrut commented Feb 25, 2019

Uh oh!

LeonarddeR commented Feb 25, 2019 via email

Uh oh!

LeonarddeR commented Feb 26, 2019

Uh oh!

feerrenrut commented Feb 26, 2019

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feerrenrut commented Feb 4, 2019 •

edited

Loading

feerrenrut commented Feb 7, 2019 •

edited

Loading

lukaszgo1 commented Feb 25, 2019 •

edited

Loading