Skip to content

Fix for error on Chromium UIA combo-box when restoring Advanced Settings#12302

Merged
seanbudd merged 3 commits into
nvaccess:masterfrom
CyrilleB79:restoreUIAChromium
Apr 19, 2021
Merged

Fix for error on Chromium UIA combo-box when restoring Advanced Settings#12302
seanbudd merged 3 commits into
nvaccess:masterfrom
CyrilleB79:restoreUIAChromium

Conversation

@CyrilleB79
Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #12299

Summary of the issue:

Using .Selection on a combo-box rather than .GetSelection was causing an error when restoring Advanced settings to default.

Description of how this pull request fixes the issue:

Modified .Selection to .GetSelection()

Testing strategy:

Tested restoration with and without the advanced settings panel enabled.
The window is correctly closed when pressing "OK".

Known issues with pull request:

During these tests, I have identified another path that may cause the settings not to be saved. Issue coming soon.
This PR only fixes the combo-box .Selection error that was probably introduced when with Chromium UIA feature.
It does not fixes the issue of advanced settings not saved correctly (probably introduced by Wx 4.1.1)

Change log entry:

None: alpha regression

Code Review Checklist:

This checklist is a reminder of things commonly forgotten in a new PR.
Authors, please do a self-review and confirm you have considered the following items.
Mark items you have considered by checking them.
You can do this when editing the Pull request description with an x: [ ] becomes [x].
You can also check the checkboxes after the PR is created.

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual tests.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.

@CyrilleB79
Copy link
Copy Markdown
Contributor Author

Cc @seanbudd, @XLTechie
Sorry, seems that you already were assigned to / investigating / working on #12299; I have missed some of your comments.
Note that I do not reproduce the secondary issue indicated by @XLTechie in #12299.
@XLTechie is this issue still present with the build of this PR on your side?

@XLTechie
Copy link
Copy Markdown
Collaborator

XLTechie commented Apr 16, 2021 via email

@CyrilleB79
Copy link
Copy Markdown
Contributor Author

Regarding the other issue (let's call it the "Visually modified options not saved"), I describe it here to keep a track. I cannot open a separate issue for now since there is no version of NVDA where it occurs since the error of #12299 happens before and since Wx 4.1.1 was introduced after Chromium UIA option.

STR:

  1. Work on a fresh version of the snapshot of this PR (to have the GetSelection issue fixed).
  2. Open advanced settings panel
  3. Check the checkbox to enable the panel
  4. Modify each option (e.g. check the unchecked checkboxes, change selected items of combo-boxes, etc.)
  5. Press OK
  6. Open again the advanced settings panel
  7. Press the "Restore defaults" button (witout having activated the panel first)
  8. Press OK
  9. Open Advanced settings panel
  10. Check the checkbox to enable the panel
  11. Tab through the various options

Expected behaviour:
At step 11, the options are the default options.

Actual behaviour:
At step 11, the options remained the modified options.
Note that at step 7, the values of combo-boxes and the numeric spin control are visually modified (and probably also the checkboxes but I am not able to guarantee it visually due to low vision). However these values seem not to be taken into account at step 8 when validating the config.

Note:
I have taken last Python 3.7 snapshot (nvda_snapshot_alpha-21877,8e4b189b) and added the fix of this PR. In this modified build, the "Visually modified options not saved" issue is not present.

@seanbudd, there are 2 options:

  1. Merge quickly the fix of this simple PR to close After clicking the "Restore Default" button under the "Advanced" category, you cannot close the settings panel by clicking the "OK" button #12299 and open a new issue for the "Visually modified options not saved"
  2. Keep this PR open until the "Visually modified options not saved issue" is also fixed.
    Let me know what you prefer.

Personally, I prefer the first option, because:

Copy link
Copy Markdown
Collaborator

@OzancanKaratas OzancanKaratas left a comment

Choose a reason for hiding this comment

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

This is great! It worked as expected. @seanbudd, please review.

@seanbudd seanbudd self-assigned this Apr 18, 2021
Copy link
Copy Markdown
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

@CyrilleB79 - no worries at all for taking this up, I was planning on starting this investigation today and I am glad that you've done the work.

After a quick test, the change I've suggested seems to fix the "Visually modified options not saved issue" but I will do some further testing to confirm.

Comment thread source/gui/settingsDialogs.py Outdated
@seanbudd seanbudd added this to the 2021.1 milestone Apr 19, 2021
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@CyrilleB79
Copy link
Copy Markdown
Contributor Author

Oh my bad!
No "Visual modification not saved" bug; just missing parenthesis that were causing self.advancedControls.haveConfigDefaultsBeenRestored() test to fail each time...
Thanks @seanbudd, I have committed your suggestion. This PR should be OK now, saving the advanced settings panel works as expected.

@CyrilleB79 CyrilleB79 closed this Apr 19, 2021
@CyrilleB79 CyrilleB79 reopened this Apr 19, 2021
@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit f94db16c60

Copy link
Copy Markdown
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks @CyrilleB79

@seanbudd seanbudd merged commit 1d038ff into nvaccess:master Apr 19, 2021
@CyrilleB79 CyrilleB79 deleted the restoreUIAChromium branch April 20, 2021 14:35
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.

After clicking the "Restore Default" button under the "Advanced" category, you cannot close the settings panel by clicking the "OK" button

5 participants