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

Fix import error #10

Merged
merged 4 commits into from
Jul 7, 2023
Merged

Fix import error #10

merged 4 commits into from
Jul 7, 2023

Conversation

nvdaes
Copy link
Contributor

@nvdaes nvdaes commented Jul 7, 2023

No description provided.

@nvdaes
Copy link
Contributor Author

nvdaes commented Jul 7, 2023

Fixes issue #9

Copy link
Owner

@CyrilleB79 CyrilleB79 left a comment

Choose a reason for hiding this comment

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

Thanks Noelia for your PR. You've been quicker than I!

@@ -32,7 +32,7 @@

Copy link
Owner

Choose a reason for hiding this comment

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

Remove this comment that is not useful anymore.

@@ -32,7 +32,7 @@

# Temporarily import private function as advised by Sean Budd from NVAccess.
# A public API can be requested by add-on developers 2-3 month after 2022.3.3 release if no issue is found.
from winAPI.sessionTracking import _isLockScreenModeActive
from winAPI.sessionTracking import isLockScreenModeActive
Copy link
Owner

Choose a reason for hiding this comment

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

Importing this new symbol, the add-on will not work with older versions of NVDA (including 2023.1).

Could you use a try/except statement?

Also add a comment to indicate the versions of NVDA, e.g.:

try
    # For NVDA >= 2023.2
    import publicSymbol
except Blablabla:
    # For NVDA < 2023.1
    import _privateSymbol

This allows me to easily clean code when modifying the compatibility range.

@nvdaes
Copy link
Contributor Author

nvdaes commented Jul 7, 2023

Hi Cyrille, please review and test latest changes, where I've tried to address your comments.


try
# For NVDA >= 2023.2
from winAPI.sessionTracking import isLockScreenModeActive as isLockScreen
Copy link
Owner

Choose a reason for hiding this comment

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

Could you just used the new symbol defined in NVDA's core without renaming? It's quite clear and easier to find in the code.

Suggested change
from winAPI.sessionTracking import isLockScreenModeActive as isLockScreen
from winAPI.sessionTracking import isLockScreenModeActive

Of course, it should be renamed to isLockScreenModeActive in the except clause and in the rest of the code.

@nvdaes
Copy link
Contributor Author

nvdaes commented Jul 7, 2023

Hi Cyrille, should be done

@CyrilleB79 CyrilleB79 merged commit c1ae17c into CyrilleB79:master Jul 7, 2023
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.

2 participants