Skip to content

Make Python console opening more robust#17392

Merged
seanbudd merged 11 commits into
nvaccess:masterfrom
CyrilleB79:pythonOpen
Nov 13, 2024
Merged

Make Python console opening more robust#17392
seanbudd merged 11 commits into
nvaccess:masterfrom
CyrilleB79:pythonOpen

Conversation

@CyrilleB79

@CyrilleB79 CyrilleB79 commented Nov 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Link to issue number:

Closes #17391

Summary of the issue:

When opening Python console with NVDA+control+Z, NVDA initialize snapshot variables. If an error occurs during the initialization of these variables, the console cannot open.

Description of user facing changes

The Python console will not fail to open anymore in case an error occurs while retrieving snapshot variables. If an exception occurs while retrieving a snapshot variable, this variable is set to None.

Note: I have imagined to display an error dialog if one or more variable cannot be retrieved successfully, but:

  • I have not been able to implement one. My trial lead to a not vocalized dialog, probably because run from the main thread instead of wx one.
  • I also wonder if there are side effect having a dialog here.
    So I'd like to discuss this point before choosing investigating how to implement this design if you agree.

Description of development approach

For each snapshot variable, catch any exception that can occur during its retrieval and initialize the variable to None if retrieval has failed. Also log the corresponding error.

Note: I have added one more #noqa E402. I am tempted to remove all of them but have not done so because I do not know why they have been added in the first place.

Testing strategy:

Manual test:
Steps of #17391.

Known issues with pull request:

None

Code Review Checklist:

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

@coderabbitai summary

@AppVeyorBot

AppVeyorBot commented Nov 11, 2024

Copy link
Copy Markdown
  • PASS: Translation comments check.
  • PASS: License check.
  • PASS: Unit tests.
  • FAIL: Lint check. See test results and lint artifacts for more information.
  • PASS: System tests (tags: installer NVDA).
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/uetfs9seqh3f2jjm/artifacts/output/l10nUtil.exe nvda_snapshot_pr17392-34556,b8192d5a.exe
  • CI timing (mins):
    INIT 0.0,
    INSTALL_START 1.6,
    INSTALL_END 1.2,
    BUILD_START 0.0,
    BUILD_END 31.0,
    TESTSETUP_START 0.0,
    TESTSETUP_END 0.5,
    TEST_START 0.0,
    TEST_END 19.4,
    FINISH_END 0.2

See test results for failed build of commit b8192d5a63

@CyrilleB79 CyrilleB79 marked this pull request as ready for review November 11, 2024 22:22
@CyrilleB79 CyrilleB79 requested a review from a team as a code owner November 11, 2024 22:22
@CyrilleB79 CyrilleB79 requested a review from seanbudd November 11, 2024 22:22
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Nov 12, 2024
seanbudd
seanbudd reviewed Nov 13, 2024
View reviewed changes
Comment thread source/pythonConsole.py Outdated
seanbudd
seanbudd reviewed Nov 13, 2024
View reviewed changes
Comment thread user_docs/en/changes.md Outdated
CyrilleB79 and others added 2 commits November 13, 2024 17:14
@CyrilleB79 @seanbudd
Update user_docs/en/changes.md
e2e59f9 
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@CyrilleB79 @seanbudd
Update source/pythonConsole.py
90518b2 
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@CyrilleB79

CyrilleB79 commented Nov 13, 2024

Copy link
Copy Markdown
Contributor Author

@seanbudd thanks for your review. I'll remove alll other lambda function following your example where not necessary.

In addition, I'd like to know your opinion on:

  • Should we issue an error message if a variable cannot be retrieved? (see "Description of user facing changes") If yes, how to do this?
  • Should I remove #noqa E402's and put the docstring above? See "Description of development approach". If yes, any idea why it was not done so in the first time?

@CyrilleB79 CyrilleB79 marked this pull request as draft November 13, 2024 16:18
@AppVeyorBot

AppVeyorBot commented Nov 13, 2024

Copy link
Copy Markdown

See test results for failed build of commit 4967d080ed

@CyrilleB79 CyrilleB79 marked this pull request as ready for review November 13, 2024 22:18
@CyrilleB79

CyrilleB79 commented Nov 13, 2024

Copy link
Copy Markdown
Contributor Author

@seanbudd, finally, I have removed the noqa's for imports; maybe they appeared during Ruff's introduction.

Just let me know for the message box in case of error.

seanbudd
seanbudd reviewed Nov 13, 2024
View reviewed changes
Comment thread user_docs/en/changes.md Outdated
seanbudd
seanbudd approved these changes Nov 13, 2024
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 @CyrilleB79 - I don't think we should add a warning dialog, I think logging an error is enough

@seanbudd seanbudd merged commit 89ef456 into nvaccess:master Nov 13, 2024
@github-actions github-actions Bot added this to the 2025.1 milestone Nov 13, 2024
@Adriani90 Adriani90 mentioned this pull request Nov 22, 2024
Open
@CyrilleB79 CyrilleB79 deleted the pythonOpen branch November 22, 2024 21:57
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

Assignees

No one assigned

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Milestone

2025.1

Development

Successfully merging this pull request may close these issues.

Unable to open Python console with NVDA+control+Z

3 participants

@CyrilleB79 @AppVeyorBot @seanbudd