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
Conversation
…d display ERROR_OLD_WIN_VERSION message, rather than raising UnicodeDecodeError.
The appveyor build nvda_snapshot_pr8771-16089,8e12d46c.exe is tested on Windows Vista Japanese and worked as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@LeonarddeR Thank you for the review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of isinstance instead of hasattr in my previous comment was intentional, see below.
source/winUser.py
Outdated
@@ -514,6 +514,10 @@ def FindWindow(className, windowName): | |||
IDCANCEL=3 | |||
|
|||
def MessageBox(hwnd, text, caption, type): | |||
if hasattr(text, 'decode'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use isinstance(text, bytes)
instead. Otherwise, this will lead to unnecessary decoding on python 2 unicode strings.
source/winUser.py
Outdated
@@ -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'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvda_snapshot_pr8771-16091,73deebfe.exe is tested as well.
…d display ERROR_OLD_WIN_VERSION message, rather than raising UnicodeDecodeError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the review comments. Looks all good now.
Strangely, this problem was described a year ago, but then it was suggested to close it: |
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.
It makes difficult for supporters to solve the problem of the users.
Description of how this pull request fixes the issue:
Testing performed:
Known issues with pull request:
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.