Skip to content

Fixup of PR 13082: fix a misspell in DefaultAppArgs and no longer use globalVars.appArgs in a boolean context (#13386)#13386

Merged
seanbudd merged 3 commits intonvaccess:masterfrom
lukaszgo1:FakeAppArgsFixUp
Feb 24, 2022
Merged

Fixup of PR 13082: fix a misspell in DefaultAppArgs and no longer use globalVars.appArgs in a boolean context (#13386)#13386
seanbudd merged 3 commits intonvaccess:masterfrom
lukaszgo1:FakeAppArgsFixUp

Conversation

@lukaszgo1
Copy link
Copy Markdown
Contributor

@lukaszgo1 lukaszgo1 commented Feb 23, 2022 •
edited
Loading

Link to issue number:

Fix-up of PR #13082

Summary of the issue:

PR #13082 added fake implementation of globalVars.appArgs which is used in cases where command line arguments are not parsed. It introduced two mistakes however:

  1. The fake class was named DefautAppArgs (note the missing "l")
  2. NVDA's logHandler uses appArgs in a boolean context to check if log viewer should be opened, determine if log fragment can be marked for copy etc. The new class was always True in a boolean context.

Description of how this pull request fixes the issue:

  1. The class is renamed to DefaultAppArgs.
  2. logHandler no longer checks boolean value of appArgs. This is safe since:
    • Both logHandler.Logger.markFragmentStart and logHandler.Logger.getFragment are used only in NVDA and they are invoked by the user action (even before Store current NVDA's language in globalVars regardless if it has been provided from the command line or not. #13082 appArgs was always truthy in that context).
    • The check for appArgs being falthy in logHandler.Logger._log introduced in 2827a34 can also be safely removed since even for NVDA_slave activateLogViewer is set to False (only two cases where it is set to True occur when user invokes log viewer via keyboard command). Also while this is not clarified in this commit at a guess I'd say this check has been added not for any additional security but just to avoid AttributeError being raised when trying to access globalVars.appArgs.secure when logging from slave.

Testing strategy:

None needed.

Known issues with pull request:

None known

Change log entries:

None needed - at a point where add-ons are loaded appArgs was always truthy.

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

@lukaszgo1
Fixup of PR 13082: fix a misspell in the class name and make the fake…
358bf7d 
… instance of globalVars.apppArgs falsy in a logical context
@lukaszgo1 lukaszgo1 requested a review from a team as a code owner February 23, 2022 20:56
@lukaszgo1 lukaszgo1 requested a review from seanbudd February 23, 2022 20:56
@seanbudd

This comment was marked as resolved.

Sign in to view
@seanbudd seanbudd added this to the 2022.1 milestone Feb 24, 2022
@lukaszgo1
Copy link
Copy Markdown
Contributor Author

lukaszgo1 commented Feb 24, 2022

I think the correct approach would be to instead remove the use of globalVars.appArgs as a boolean. I am not certain that an additional check here is required, but it appears that logViewer needs 2 globalVars: bool(globalVars.appArgs.logFileName) == True and globalVars.appArgs.secure == False

I do not really understand why we need to check if the value of globalVars.appArgs is truthy or falsy either ((perhaps for NVDA_slave?). @michaelDCurran Would you be able to shed some light on this?
Since, as I said above, I do not understand what these checks are for I've decided to overload the operator just to be on the safe side.

If this approach is used we may need to add a note in changes for developers about the change in "truthiness" of appArgs.

At the stage where add-ons or plugins from the scratchpad are loaded globalVars.appArgs was always truthy, so this is not API breaking in any sense.

@lukaszgo1 lukaszgo1 changed the title Fixup of PR 13082: fix a misspell in the class name and make the fake instance of globalVars.apppArgs falsy in a logical context Fixup of PR 13082: fix a misspell in the class name and no longer use the fake instance of globalVars.apppArgs in a boolean context Feb 24, 2022
@lukaszgo1
Copy link
Copy Markdown
Contributor Author

lukaszgo1 commented Feb 24, 2022

@seanbudd I've removed the overloaded operator and appArgs is no longer used as a boolean in the logHandler. The PR description explains why this can be safely done.

seanbudd
seanbudd approved these changes Feb 24, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks @lukaszgo1

@seanbudd seanbudd merged commit 618bfb6 into nvaccess:master Feb 24, 2022
@seanbudd seanbudd changed the title Fixup of PR 13082: fix a misspell in the class name and no longer use the fake instance of globalVars.apppArgs in a boolean context Fixup of PR 13082: fix a misspell in DefaultAppArgs and no longer use globalVars.appArgs in a boolean context (#13386) Feb 24, 2022
@lukaszgo1 lukaszgo1 deleted the FakeAppArgsFixUp branch February 24, 2022 23:40
@josephsl josephsl mentioned this pull request Apr 22, 2022
Merged
5 tasks
This was referenced May 12, 2022
Merged
Merged
@CyrilleB79 CyrilleB79 mentioned this pull request Jun 20, 2022
Merged
5 tasks
seanbudd pushed a commit that referenced this pull request Jun 21, 2022
@CyrilleB79
Fixes in the change log for NVDA 2022.2 (#13817)
ec56c3c 
When checking the change log translation, I have found a wrong reference in an item of the change log. I have thus checked all the refs for 2022.2.

Link to issue number:
None
Follow-up of various issues.

Summary of the issue:
Some of the GitHub references in the change log were targetting wrong issue or PR.

Description of user facing changes
The references have been fixed in the change log document.

Description of development approach
Fixed the following references:

Fixup of PR 13082: fix a misspell in DefaultAppArgs and no longer use globalVars.appArgs in a boolean context (#13386) #13386 replaced by Windows 10/11 Calculator: allow NVDA to announce more operations by suppressing only a limited number of keyboard commands #13383
Revert "Update to py2exe 0.11.0.1 (#13066)" #13508 replaced by Windows 11 Design Elements Are Not Reported by NVDA When Mouse Tracking Is On #13506
Fixup of PR 13082: fix a misspell in DefaultAppArgs and no longer use globalVars.appArgs in a boolean context (#13386) #13386 (bis) replaced by Windows 11 Notepad: status bar is not announced #13688
Also, I have replaced #13276 (Libre Office issue) by #13277 (associated PR) since the issue description is not related at all with the change for developers.
@ghost ghost mentioned this pull request Jul 12, 2022
Closed
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

api-breaking-change bug/regression

Projects

None yet

Milestone

2022.1

Development

Successfully merging this pull request may close these issues.

3 participants

@lukaszgo1 @seanbudd @feerrenrut