Skip to content

Move some global vars to private variables#14037

Closed
seanbudd wants to merge 2 commits into
masterfrom
makeGlobalVarsPrivate
Closed

Move some global vars to private variables#14037
seanbudd wants to merge 2 commits into
masterfrom
makeGlobalVarsPrivate

Conversation

@seanbudd

@seanbudd seanbudd commented Aug 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Link to issue number:

Summary of the issue:

Add-on API authors may mistakenly use objects cached in globalVars directly, instead of through api.get/set methods.
This means that insecure objects could be set or returned to the user.

Description of user facing changes

None

Description of development approach

Removed variables from globalVars.

The following have been made removed and made private.
Instead, set and get these variables via the equivalent api.get/set method:

  • desktopObject
  • focusAncestors
  • focusObject
  • focusDifferenceLevel
  • foregroundObject
  • mouseObject
  • navigatorObject
  • reviewPosition

The following have been removed. These variables were previously unset and used:

  • mouseOldY
  • mouseOldX
  • lastProgressValue

reviewPositionObj has been removed as it was unused. It can be accessed with the equivalent getReviewPosition().obj.

Testing strategy:

Known issues with pull request:

API breaking change

Change log entries:

For Developers

- Removed variables from ``globalVars``:
  - The following have been made removed and made private.
  Instead, set and get these variables via the equivalent ``api.get/set`` method:
    - ``desktopObject``
    - ``focusAncestors``
    - ``focusObject``
    - ``focusDifferenceLevel``
    - ``foregroundObject``
    - ``mouseObject``
    - ``navigatorObject``
    - ``reviewPosition``
    -
  - The following have been removed. These variables were previously unset and used:
    - ``mouseOldY``
    - ``mouseOldX``
    - ``lastProgressValue``
    -
  - ``reviewPositionObj`` has been removed as it was unused. It can be accessed with the equivalent ``getReviewPosition().obj``.
  -

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.

@seanbudd seanbudd added security Addon/API Changes to NVDA's API for addons to support addon development. audience/nvda-dev PR or issue is relevant to NVDA / Add-on developers api-breaking-change labels Aug 19, 2022
@seanbudd seanbudd added this to the 2023.1 milestone Aug 19, 2022
@seanbudd seanbudd marked this pull request as ready for review August 19, 2022 07:15
@seanbudd seanbudd requested a review from a team as a code owner August 19, 2022 07:15
@seanbudd seanbudd requested a review from feerrenrut August 19, 2022 07:15
@AppVeyorBot

This comment was marked as resolved.

Sign in to view
@seanbudd seanbudd added the merge-early Merge Early in a developer cycle label Aug 19, 2022
feerrenrut
feerrenrut suggested changes Aug 19, 2022
View reviewed changes

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will need to be changed to preserve backwards compatibility.
We don't advise reading / writing to these variables, a deprecation warning is fine. Note 2023.3 may be too soon to remove these.

@AppVeyorBot

This comment was marked as off-topic.

Sign in to view
@Brian1Gaff

This comment was marked as off-topic.

Sign in to view
@seanbudd

seanbudd commented Aug 22, 2022

Copy link
Copy Markdown
Member Author

@feerrenrut I'm uncertain how to preserve backwards compatibility here. While it is easy to preserve "getting" these values with __getattr__, I'm not sure how we can maintain setting these attributes.

@seanbudd

This comment was marked as off-topic.

Sign in to view
@feerrenrut

This comment was marked as outdated.

Sign in to view
@seanbudd seanbudd self-assigned this Aug 22, 2022
This was referenced Aug 23, 2022
Merged
Merged
@seanbudd

seanbudd commented Aug 23, 2022

Copy link
Copy Markdown
Member Author

Due to the inability to retain backwards compatibility here (refer to #14049), this is closed in favour of #14050, which outlines a longer term strategy to eventually deprecate these variables.

@seanbudd seanbudd closed this Aug 23, 2022
seanbudd added a commit that referenced this pull request Aug 26, 2022
@seanbudd
Add notes on limitations to retaining backwards compatibility (#14049)
5a4512e 
See also #14050

Summary of the issue:
#14037 was closed because there was no way to preserve backwards compatibility.
This is the case for any module level variable which the NVDA API expects to support assignment.

Description of user facing changes
None

Description of development approach
Documentation has been added:

to warn developers about writing code which allows for future changes without breaking backwards compatibility.
keep track of cases which we cannot safely retain backwards compatibility, so that we can clearly define when API breaking changes are necessary
seanbudd added a commit that referenced this pull request Aug 30, 2022
@seanbudd
Mark globalVars for deprecation (#14050)
4721336 
Supersedes #14037
See also #14049

Summary of the issue:
#14037 was closed because there was no way to preserve backwards compatibility.
This is the case for any module level variable which the NVDA API expects to support assignment (see also #14049).

globalVars contains many NVDA state variables which should not be changed by add-on authors.
As a result, globalVars should eventually be deprecated, in favour of encapsulation of these variables.
This will make retaining backwards compatibility possible in the future.

Description of user facing changes
None

Description of development approach
Added a deprecation warning and strategy to the docstring of globalVars.

Encapsulated some less used globalVars to provide an example of what the future API should look like.

runningAsSource was moved from globalVars as this is an unreleased variable. (#14015)
_allowDeprecatedAPI was moved as it is internal code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@feerrenrut feerrenrut feerrenrut requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@seanbudd seanbudd

Labels

Addon/API Changes to NVDA's API for addons to support addon development. api-breaking-change audience/nvda-dev PR or issue is relevant to NVDA / Add-on developers blocked merge-early Merge Early in a developer cycle security

Projects

None yet

Milestone

2023.1

Development

Successfully merging this pull request may close these issues.

4 participants

@seanbudd @AppVeyorBot @Brian1Gaff @feerrenrut