Skip to content

Scintilla: use UTF-8 decoding to convert font name to Unicode - #9893

Merged
feerrenrut merged 3 commits into
nvaccess:threshold_py3_stagingfrom
josephsl:i9892
Jul 9, 2019
Merged

Scintilla: use UTF-8 decoding to convert font name to Unicode#9893
feerrenrut merged 3 commits into
nvaccess:threshold_py3_stagingfrom
josephsl:i9892

Conversation

@josephsl

@josephsl josephsl commented Jul 7, 2019

Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #9892

Summary of the issue:

Format info command fails in Notepad++ and other Scintilla controls because font name is a bytes-like object.

Description of how this pull request fixes the issue:

Font name is decoded using UTF-8.

Testing performed:

Tested with Python 3 source code version of NVDA and notepad++, ensuring font name and other attributes are announced.

Known issues with pull request:

None

Change log entry:

None

Additional context:

Originally, text utils was considered, but passing buffer.raw results in NULL characters being included in format info output. Thus directly decode buffer.value in UTF-8 mode.

josephsl added 2 commits July 7, 2019 14:35
…ccess#9892.

When a user attempts to obtain font info and other formatting attributes from Scintilla controls such as Notepad++, type error is thrown becasue font name is returned as bytes. Thus use text utils module to convert this into Unicode.
@josephsl
josephsl requested review from LeonarddeR and feerrenrut July 7, 2019 21:42

@LeonarddeR LeonarddeR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think using textUtils is a bit overkill here, I think you can just decode the value from the string buffer from utf_8. this function is textUtils is mainly there to support cases where we have to deal with raw buffers, containing null characters. The scintilla docs say that the font name string is null terminated, though.

In fact, what you're doing now is probably going to include null characters in the output for the font name. For this not to happen, you will need to know the actual number of characters for the font in the buffer.

If you insist on using textUtils, see my comment below, but I think it is unnecessary.

Comment thread source/NVDAObjects/window/scintilla.py Outdated
finally:
winKernel.virtualFreeEx(self.obj.processHandle,internalBuf,0,winKernel.MEM_RELEASE)
formatField["font-name"]=fontNameBuf.value
formatField["font-name"]=textUtils.getTextFromRawBytes(fontNameBuf.raw,numChars=fontNameLength)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

From the docs:

Under Windows, only the first 32 characters of the name are used, the name is decoded as UTF-8, ...

Suggested change
formatField["font-name"]=textUtils.getTextFromRawBytes(fontNameBuf.raw,numChars=fontNameLength)
formatField["font-name"]=textUtils.getTextFromRawBytes(fontNameBuf.raw,numChars=fontNameLength, encoding="utf_8")

@josephsl

josephsl commented Jul 8, 2019 via email

Copy link
Copy Markdown
Contributor Author

@LeonarddeR

LeonarddeR commented Jul 8, 2019 via email

Copy link
Copy Markdown
Collaborator

Reviewed by Leonard de Ruijter (Babbage): if using text utils, make sure to pass in encoding keyword (utf_8). One workaround for using text utils is passing in buffer.value, but that is an overkill. Thus resort to using a simple buffer.value.decode call.
@josephsl

josephsl commented Jul 8, 2019

Copy link
Copy Markdown
Contributor Author

Hi,

In the end, I decided to make this simpler through buffer.value.decode call. One way for text utils solution is passing buffer.value, but it is overkill.

Thanks.

@josephsl josephsl changed the title Scintilla: use text utils module to convert font name to Unicode Scintilla: use UTF-8 decoding to convert font name to Unicode Jul 8, 2019

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @josephsl

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.

4 participants