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

If running on the Japanese version of Windows XP or Vista, NVDA should displays the alert of OS version requirements #8771

Merged
merged 4 commits into from Sep 25, 2018

Conversation

@nishimotz
Copy link
Contributor

@nishimotz nishimotz commented Sep 22, 2018

Link to issue number:

Closes #7586

Summary of the issue:

If running on the Japanese version of Windows XP or Vista, NVDA raises error as follows.

See the logfile 'C:\Users\***\AppData\Local\Temp\***.tmp\app\nvda_noUIAccess.exe.log' for details
Traceback (most recent call last):
  File "nvda.pyw", line 77, in <module>
UnicodeDecodeError: 'ascii' codec can't decode byte 0x8e in position 0: ordinal not in range(128)

It makes difficult for supporters to solve the problem of the users.

Description of how this pull request fixes the issue:

  • if ctypes.FormatError() returns mbcs string, it should be decoded.
  • the code is expected to work with both Python 2.7 and Python 3.7.

Testing performed:

  • this workaround is already done for NVDA Japanese version 2018.3.1jp.

Known issues with pull request:

  • similar workaround may be necessary for other parts of NVDA, so we should avoid the duplication.

Change log entry:

Section: Bug fixes

If running on the Japanese version of Windows XP or Vista, NVDA now displays the alert of OS version requirements as expected.

…d display ERROR_OLD_WIN_VERSION message, rather than raising UnicodeDecodeError.
@nishimotz
Copy link
Contributor Author

@nishimotz nishimotz commented Sep 22, 2018

The appveyor build nvda_snapshot_pr8771-16089,8e12d46c.exe is tested on Windows Vista Japanese and worked as expected.

Copy link
Collaborator

@leonardder leonardder left a comment

Personally, I'd rather have this addressed at the winUser.MessageBox level. You can add:

if isinstance(text, bytes)
    text = text.decode("mbcs")

Also, you could do the same to caption.

Than in NVDA.pyw, you can revert your change and change the MessageBox call as follows:

winUser.MessageBox(0, ctypes.FormatError(winUser.ERROR_OLD_WIN_VERSION), None, winUser.MB_ICONERROR)

…d display ERROR_OLD_WIN_VERSION message, rather than raising UnicodeDecodeError.
@nishimotz
Copy link
Contributor Author

@nishimotz nishimotz commented Sep 22, 2018

@leonardder Thank you for the review.
nvda_snapshot_pr8771-16090,2938b4c1.exe is tested and worked as well.

Copy link
Collaborator

@leonardder leonardder left a comment

The use of isinstance instead of hasattr in my previous comment was intentional, see below.

@@ -514,6 +514,10 @@ def FindWindow(className, windowName):
IDCANCEL=3

def MessageBox(hwnd, text, caption, type):
if hasattr(text, 'decode'):

This comment has been minimized.

@leonardder

leonardder Sep 22, 2018
Collaborator

Please use isinstance(text, bytes) instead. Otherwise, this will lead to unnecessary decoding on python 2 unicode strings.

@@ -514,6 +514,10 @@ def FindWindow(className, windowName):
IDCANCEL=3

def MessageBox(hwnd, text, caption, type):
if hasattr(text, 'decode'):
text = text.decode('mbcs')
if caption and hasattr(caption, 'decode'):

This comment has been minimized.

@leonardder

leonardder Sep 22, 2018
Collaborator

You can safely use hasattr on a None, so the if caption check is not required. Furthermore, also avoid using hasattr here and use an instance check instead.

This comment has been minimized.

@leonardder

leonardder Sep 22, 2018
Collaborator

Actually, at least on python 3, I'm pretty sure that you must ommit the if caption check. If you don't and you provide an empty byte string, ctypes might throw an error, telling you that you can't throw an empty byte string into a function that expects a wide character string.

This comment has been minimized.

@nishimotz

nishimotz Sep 22, 2018
Author Contributor

nvda_snapshot_pr8771-16091,73deebfe.exe is tested as well.

…d display ERROR_OLD_WIN_VERSION message, rather than raising UnicodeDecodeError.
Copy link
Collaborator

@leonardder leonardder left a comment

Thanks for addressing the review comments. Looks all good now.

@michaelDCurran michaelDCurran merged commit 7b133e6 into nvaccess:master Sep 25, 2018
1 check was pending
1 check was pending
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
@nvaccessAuto nvaccessAuto added this to the 2018.4 milestone Sep 25, 2018
@kvark128
Copy link
Contributor

@kvark128 kvark128 commented Oct 2, 2018

Strangely, this problem was described a year ago, but then it was suggested to close it:
#7586

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants