Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed inappropriate pending install item when accessing NVDA menu through systray #14523

Merged
merged 9 commits into from
Jan 13, 2023

Conversation

CyrilleB79
Copy link
Collaborator

@CyrilleB79 CyrilleB79 commented Jan 9, 2023

Link to issue number:

None.
Issue reported on a French mailing list.

Summary of the issue:

When accessing NVDA menu via the systray (enter or right click on the NVDA icon), the item "Install pending update" is present even if there is nothing to install.

In the code, it appears that the function to evaluate whether this item should be present or not is called only when accessing NVDA menu via NVDA+N shortcut, not when accessing it via the systray icon.

Description of user facing changes

The item "Install pending update" will not be present anymore in NVDA menu when there is no pending update available.<

Description of development approach

Moved the corresponding piece of code so that it is called in both scenarios.

Note: I have not written deprecation code since 2023.1 is an API breaking release and since this function is probably not used by so many add-ons (if any). I feel that in the case of functions/methods rarely used by add-on authors it's not worth making NVDA's core code heavier with deprecation functions when not necessary. If NVAccess wishes to maintain a deprecated method evaluateUpdatePendingUpdateMenuItemCommand in MainFrame

Testing strategy:

Manual tests with a portable NVDA created from appVeyor build of this PR:

  1. Launch NVDA
    • Open NVDA menu via NVDA+N and check that there is no pending install item in the menu
    • Open NVDA menu via systray icon and check that there is no pending install item in the menu
  2. Check for update via NVDA menu, download the available (alpha) update but do not install it
    • Open NVDA menu via NVDA+N and check that there is a pending install item in the menu
    • Open NVDA menu via systray icon and check that there is a pending install item in the menu
  3. In the userConfig\updates folder, delete the .exe file:
    • Open NVDA menu via NVDA+N and check that there is no pending install item in the menu
    • Open NVDA menu via systray icon and check that there is no pending install item in the menu

Known issues with pull request:

None

Change log entries:

Bug fixes
When accessing NVDA menu via its icon in the notification area, NVDA will not suggest a pending update anymore when none is available.

Changes for developers:
gui.MainFrame.evaluateUpdatePendingUpdateMenuItemCommand has been removed; use gui.MainFrame.sysTrayIcon.evaluateUpdatePendingUpdateMenuItemCommand instead.

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.

@CyrilleB79 CyrilleB79 marked this pull request as ready for review January 9, 2023 21:00
@CyrilleB79 CyrilleB79 requested a review from a team as a code owner January 9, 2023 21:00
@CyrilleB79 CyrilleB79 requested a review from seanbudd January 9, 2023 21:00
@@ -524,8 +514,17 @@ def __init__(self, frame: MainFrame):

self.Bind(wx.adv.EVT_TASKBAR_LEFT_DOWN, self.onActivate)
self.Bind(wx.adv.EVT_TASKBAR_RIGHT_DOWN, self.onActivate)


def evaluateUpdatePendingUpdateMenuItemCommand(self):
Copy link
Member

Choose a reason for hiding this comment

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

If this is something we don't want or expect add-on authors to use, it should be prefixed with an underscore

Suggested change
def evaluateUpdatePendingUpdateMenuItemCommand(self):
def _evaluateUpdatePendingUpdateMenuItemCommand(self):

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise, we should use deprecations and retain the alias. The current policy is to retain aliases where possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@seanbudd, it's not very clear to me. Let me rephrase the issues:

There are two evaluateUpdatePendingUpdateMenuItemCommand functions (actually, it's a move):

  • gui.MainFrame.evaluateUpdatePendingUpdateMenuItemCommand that was in NVDA code base and that I remove
  • gui.MainFrame.sysTrayIcon.evaluateUpdatePendingUpdateMenuItemCommand that was created in this PR.

Questions:

  1. In NVDA code base, I do not need gui.MainFrame.evaluateUpdatePendingUpdateMenuItemCommand anymore. Do you recommend to deprecate it or not; it does not cause any problem to keep it; deprecate it would just be useful if we want to clen up the code in the future.
  2. I have created gui.MainFrame.sysTrayIcon.evaluateUpdatePendingUpdateMenuItemCommand. Do you recommend to make it private instead?

Copy link
Member

Choose a reason for hiding this comment

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

It's a situation of one or the other.
If this is something we want to remove from the API, as it is not expected to be used by API consumers and/or it is not stable, it should be made private.

Otherwise, it should be deprecated and the alias should remain.
I think in this case, keeping the alias is more appropriate as the code should be stable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@seanbudd, sorry it is still unclear to me. Are you answering to question 1 or question 2? Also not clear when you are talking of evaluateUpdatePendingUpdateMenuItemCommand in gui.MainFrame or in gui.SysTrayIcon

Anyway, let's go step by step. I have restored gui.MainFrame.evaluateUpdatePendingUpdateMenuItemCommand (in d84a1c4) to avoid unneeded change in the API. Note however that this function is not used in the core anymore.

Now let me know if I should implement none, one or both of the following mutually unrelated changes:

  • Change 1: Deprecate gui.MainFrame.evaluateUpdatePendingUpdateMenuItemCommand given it will not be used in NVDA's core code anymore.
  • Change 2: make gui.SysTrayIcon.evaluateUpdatePendingUpdateMenuItemCommand
    Note that these two changes are unrelated and none, one, the other or both can be implemented.
    Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

These are mutually exclusive approaches based on our API policy.

If you wanted to make gui.SysTrayIcon.evaluateUpdatePendingUpdateMenuItemCommand private, you would make it as an API breaking change and not retain gui.MainFrame.evaluateUpdatePendingUpdateMenuItemCommand as an alias.

Otherwise, you retain the alias with a deprecation notice.
I suggest making change 1, with a deprecation notice.

source/gui/__init__.py Outdated Show resolved Hide resolved
@AppVeyorBot
Copy link

See test results for failed build of commit a3ebf6b0db

source/gui/__init__.py Show resolved Hide resolved
@@ -524,8 +514,17 @@ def __init__(self, frame: MainFrame):

self.Bind(wx.adv.EVT_TASKBAR_LEFT_DOWN, self.onActivate)
self.Bind(wx.adv.EVT_TASKBAR_RIGHT_DOWN, self.onActivate)


def evaluateUpdatePendingUpdateMenuItemCommand(self):
Copy link
Member

Choose a reason for hiding this comment

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

These are mutually exclusive approaches based on our API policy.

If you wanted to make gui.SysTrayIcon.evaluateUpdatePendingUpdateMenuItemCommand private, you would make it as an API breaking change and not retain gui.MainFrame.evaluateUpdatePendingUpdateMenuItemCommand as an alias.

Otherwise, you retain the alias with a deprecation notice.
I suggest making change 1, with a deprecation notice.

Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@AppVeyorBot
Copy link

See test results for failed build of commit d9b29b205e

@AppVeyorBot
Copy link

See test results for failed build of commit 0c06b85cd6

@seanbudd seanbudd merged commit bc164e2 into nvaccess:master Jan 13, 2023
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Jan 13, 2023
@CyrilleB79 CyrilleB79 deleted the noPendingInstall branch January 13, 2023 12:38
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.

4 participants