Skip to content

Add extension point to notify when browse mode state becomes active or inactive#15450

Merged
seanbudd merged 10 commits into
nvaccess:masterfrom
nvdaes:browseModeStateChange
Sep 21, 2023
Merged

Add extension point to notify when browse mode state becomes active or inactive#15450
seanbudd merged 10 commits into
nvaccess:masterfrom
nvdaes:browseModeStateChange

Conversation

@nvdaes

@nvdaes nvdaes commented Sep 14, 2023 •
edited by seanbudd
Loading

Copy link
Copy Markdown
Collaborator
  • Add extension point to notify browseMode changes
  • Update developer guide

Link to issue number:

fixes #14969

Summary of the issue:

Sometimes, it would be desirable perform actions when browse mode state changes. For example, this may be useful to activate or deactivate a profile when entering or exiting browse mode.

Description of user facing changes

  • Add-ons and other components can be notified about browse mode state changes.

Description of development approach

A treeInterceptorHandler.post_browseModeStateChange extension point has been added. The browseMode parameter represents the state of browse mode (True or False).

Testing strategy:

  • NVDA seems not to produce additional errors.
  • Checked that a profile can be activated/deactivated programatically when browseMode is changed with NVDA+space, navigating the web, and opening the menu bar and other applications.

Known issues with pull request:

None.

Change log entries:

For Developers
Added treeInterceptorHandler.post_browseModeStateChange extension points, to be notified about state changes in browse mode (#14969)

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.

@nvdaes

nvdaes commented Sep 14, 2023

Copy link
Copy Markdown
Collaborator Author

cc: @LeonarddeR @XLTechie

LeonarddeR
LeonarddeR reviewed Sep 14, 2023
View reviewed changes
Comment thread source/api.py Outdated
obj.treeInterceptor=treeInterceptorObject
else:
obj.treeInterceptor=None
if obj.treeInterceptor:

@LeonarddeR LeonarddeR Sep 14, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Isn't this block executed on every focus change? I guess that's problematic.

@seanbudd seanbudd Sep 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed, this needs needs to be changed. It's probably worth caching the previous browse mode value

@nvdaes

nvdaes commented Sep 14, 2023

Copy link
Copy Markdown
Collaborator Author

Isn't this block executed on every focus change? I guess that's problematic.

Seems not to have undesirable effects. Anyway, if you have another suggestion, please let me know.

@LeonarddeR

LeonarddeR commented Sep 14, 2023

Copy link
Copy Markdown
Collaborator

I'd rather see the extension point only called on a change.
May be it should be called in browseMode.reportPassThrough instead? This will however not be notified when moving in and out of a window that has browse mode enabled.

@nvdaes

nvdaes commented Sep 14, 2023

Copy link
Copy Markdown
Collaborator Author

I'd rather see the extension point only called on a change.
May be it should be called in browseMode.reportPassThrough instead? This will however not be notified > when moving in and out of a window that has browse mode enabled.

For this reason, for now the best way to achieve the possibility of being notified about changes is this one. Hope a best way can be found if this is problematic.

@nvdaes nvdaes marked this pull request as ready for review September 14, 2023 18:38
@nvdaes nvdaes requested a review from a team as a code owner September 14, 2023 18:38
@nvdaes nvdaes requested a review from seanbudd September 14, 2023 18:38
seanbudd
seanbudd reviewed Sep 20, 2023
View reviewed changes
Comment thread source/treeInterceptorHandler.py
@seanbudd

seanbudd commented Sep 20, 2023

Copy link
Copy Markdown
Member

Could you please update the title to be more descriptive of the change?

@seanbudd seanbudd marked this pull request as draft September 20, 2023 03:55
@nvdaes nvdaes changed the title browseModeStateChange Add extension point to notify when browse mode state becomes active or inactive Sep 20, 2023
@nvdaes

nvdaes commented Sep 20, 2023

Copy link
Copy Markdown
Collaborator Author

@LeonarddeR and @seanbudd agree about this:

Isn't this block executed on every focus change? I guess that's problematic.

I think that we can use the globalVars.oldFocusObject and create a variable with its treeInterceptor, if the old object exist. Then, if focusObject.treeInterceptor is not equal to the oldTreeInterceptor, then we can trigger the extension point. Otherwise, it should be triggered just when a tree interceptor exist and the the _set_passThrough function can take charge of this. I'll test this in a few hours after job.

@LeonarddeR

LeonarddeR commented Sep 20, 2023

Copy link
Copy Markdown
Collaborator

This sounds good to me. It might be good to have a look into reportPassThrough as well. May be that can be simplified based on this new extension point.

@AppVeyorBot

AppVeyorBot commented Sep 20, 2023

Copy link
Copy Markdown

See test results for failed build of commit f3ea2c38d2

@nvdaes

nvdaes commented Sep 20, 2023

Copy link
Copy Markdown
Collaborator Author

It might be good to have a look into reportPassThrough as well. May be that can be simplified based on this new extension point.

Yes, though for now my tests are failing. Maybe in a new PR? Or, if you have suggestion, let me know please. I'll mark this as ready for review.

@nvdaes nvdaes marked this pull request as ready for review September 20, 2023 19:44
seanbudd
seanbudd reviewed Sep 20, 2023
View reviewed changes
Comment thread source/treeInterceptorHandler.py Outdated
@nvdaes

nvdaes commented Sep 20, 2023

Copy link
Copy Markdown
Collaborator Author

why does this use the underscored _passThrough but the changes in api.py uses self.passThrough

My mistake. Fixed.

@nvdaes nvdaes requested a review from LeonarddeR September 20, 2023 23:54
@nvdaes nvdaes requested a review from seanbudd September 20, 2023 23:55
seanbudd
seanbudd approved these changes Sep 21, 2023
View reviewed changes

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @nvdaes

@seanbudd seanbudd merged commit 0f18da3 into nvaccess:master Sep 21, 2023
@nvaccessAuto nvaccessAuto added this to the 2024.1 milestone Sep 21, 2023
@seanbudd seanbudd mentioned this pull request Sep 25, 2023
Merged
seanbudd added a commit that referenced this pull request Sep 25, 2023
@seanbudd
Fix up changes entry under incorrect release (#15509)
662b342 
Fixup of #15271 #15450

Change log entries were mistakenly placed under 2023.3 instead of 2024.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

@LeonarddeR LeonarddeR Awaiting requested review from LeonarddeR

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2024.1

Development

Successfully merging this pull request may close these issues.

extensionPoints: Notify when NVDA is in browse mode

5 participants

@nvdaes @LeonarddeR @seanbudd @AppVeyorBot @nvaccessAuto