Skip to content

Revert change of globalMapScripts in scriptHandler, which is now again a list instead of a set#14066

Merged
seanbudd merged 6 commits into
nvaccess:rcfrom
LeonarddeR:rc_fixScriptHandler
Aug 26, 2022
Merged

Revert change of globalMapScripts in scriptHandler, which is now again a list instead of a set#14066
seanbudd merged 6 commits into
nvaccess:rcfrom
LeonarddeR:rc_fixScriptHandler

Conversation

@LeonarddeR

@LeonarddeR LeonarddeR commented Aug 25, 2022 •
edited by seanbudd
Loading

Copy link
Copy Markdown
Collaborator

Link to issue number:

Fixes #14065

Summary of the issue:

Overriding/remapping existing braille display gestures was broken in NVDA 2022.2.1.

Description of user facing changes

Gesture overrides work again in the mentioned case in #14065

Description of development approach

Revert an uncessary and seemlinly undocumented change in scriptHandler. It would help if we could clear up how this change ended up in this release.

Testing strategy:

Followed the str in #14065 for at least three times, in all cases my remappings worked as expected.

Known issues with pull request:

None known

Change log entries:

Bug fixes

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • Security precautions taken.

@LeonarddeR LeonarddeR requested a review from a team as a code owner August 25, 2022 09:29
@LeonarddeR LeonarddeR requested review from seanbudd and removed request for a team August 25, 2022 09:29
@LeonarddeR

LeonarddeR commented Aug 25, 2022

Copy link
Copy Markdown
Collaborator Author

Additional rationale for the revert. getGlobalMapScripts loops through the map in order of importance, the user gesture map comes first. However, a Python set is not ordered, and therefore the order of scripts that are associated with a gesture is lost and entirely random. Sometimes a user gesture would take prevalence, sometimes a built-in gesture. That also clarifies why I got arbitrary results in #14066. An ordered set would suffice, but python doesn't have one in core

@feerrenrut

feerrenrut commented Aug 25, 2022

Copy link
Copy Markdown
Contributor

Since the ordered requirement isn't obvious where these changes are made, it would be good to add comments to point it out and explain the rationale.

@LeonarddeR I don't expect you to do this, but I want to make a note so it's not missed when we look at this tomorrow.

@seanbudd seanbudd requested a review from feerrenrut August 26, 2022 01:48
seanbudd
seanbudd previously approved these changes Aug 26, 2022
View reviewed changes
@seanbudd seanbudd marked this pull request as draft August 26, 2022 01:50
@seanbudd seanbudd marked this pull request as ready for review August 26, 2022 02:18
feerrenrut
feerrenrut previously approved these changes Aug 26, 2022
View reviewed changes
@AppVeyorBot

AppVeyorBot commented Aug 26, 2022

Copy link
Copy Markdown

See test results for failed build of commit b9ff2fdbb7

@seanbudd seanbudd self-assigned this Aug 26, 2022
@seanbudd seanbudd merged commit 8fc092c into nvaccess:rc Aug 26, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.4 milestone Aug 26, 2022
@seanbudd seanbudd modified the milestones: 2022.4, 2022.2.2 Aug 26, 2022
@seanbudd seanbudd linked an issue Aug 26, 2022 that may be closed by this pull request
Gesture overrides broken in NVDA 2022.2.1 #14065
Closed
@seanbudd seanbudd mentioned this pull request Aug 31, 2022
Closed
@LeonarddeR LeonarddeR deleted the rc_fixScriptHandler branch August 23, 2025 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd left review comments

+1 more reviewer

@feerrenrut feerrenrut feerrenrut left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@seanbudd seanbudd

Labels

None yet

Projects

None yet

Milestone

2022.2.2

Development

Successfully merging this pull request may close these issues.

Gesture overrides broken in NVDA 2022.2.1

5 participants

@LeonarddeR @feerrenrut @AppVeyorBot @seanbudd @nvaccessAuto