Skip to content

Update liblouis to version 3.8, add debug logging functionality and cleanup old workarounds that are no longer necessary#9015

Merged
michaelDCurran merged 14 commits into
nvaccess:masterfrom
BabbageCom:liblouis3.8
Dec 6, 2018
Merged

Update liblouis to version 3.8, add debug logging functionality and cleanup old workarounds that are no longer necessary#9015
michaelDCurran merged 14 commits into
nvaccess:masterfrom
BabbageCom:liblouis3.8

Conversation

@LeonarddeR

@LeonarddeR LeonarddeR commented Dec 4, 2018

Copy link
Copy Markdown
Collaborator

Link to issue number:

Closes #9013
Closes #4435
Closes #4554

Description of this pull request

  1. Updates liblouis to version 3.8, thereby dealing with renamed tables. Also added grade 2 tables for Arabic and Spanish, and an Arabic 8 dot computer braille table.
  2. Add the possibility to send liblouis' log output to the NVDA log. This is the reason why we are at liblouis commit 90a808bf, and not at 3.8.0 itself.
    The log output can be enabled by setting config.conf['debugLog']['louis'] to True. While @jcsteh suggested in Send liblouis log output to the NVDA log #4554 (comment) to throw everything in at level debug, I think I've been able to come up with a sane mapping of louis levels to NVDA levels.
  3. Now we are introducing more liblouis specific functionality into NVDA, I introduced a new louisHelper module. I added a translate method to that new module as well, to act as a convenience wrapper for louis.translate. The braille module now uses louisHelper.translate, and that function does the underlying magic to have NVDA work with the translation result in a correct way. I also investigated whether some workarounds needed to fix braille cursor position incorrect on words after letter/capital/number/math signs when expand to computer braille enabled #2947 and Braille and web: NVDA is not able to select text when matching a particular type of heading #2466 were still necessary, and it looks like that they're fixed upstream. What also changed since then is that Liblouis now has a decent way of providing patches and getting assistance with bugs, thanks to github.
  4. Something " I'd like to discuss and which is implemented in this pr as well, is signing liblouis dlls with the NV Access certificate. Now there are several liblouis dll builds in the wild, such as those from Microsoft, Freedom Scientific and even others, it might make sense to do this in order for NVDA specific liblouis builds to be recognized as such. Note that Microsoft also signs liblouis dlls that are included with Windows 10.

Testing performed:

Unit tests, chose the new tables from the braille settings panel.

Known issues with pull request:

Change log entry:

@ikrami1

ikrami1 commented Dec 4, 2018

Copy link
Copy Markdown

there is also some major improvements to the arabic grade1 table which are not mentioned in the release notes, including more compatibility with the unified Arabic braille code, fixing many problems regarding back-translation, and improving the overall readability of the table. i wish we see this Merged in NVDA as soon as possible>

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

This won't be in NVDA 2018.4, but 2019.1 should work. Usually we don't mention fixes or improvements in liblouis in the NVDA what's new.

@dpy013

dpy013 commented Dec 4, 2018

Copy link
Copy Markdown
Contributor

@LeonarddeR Thanks to leonardder's mood work liblouis3.8 This version of Chinese Braille also has a lot of modifications. I hope you can see him in the next alpha test build and test thank@

@LeonarddeR LeonarddeR changed the title Update liblouis to version 3.8 Update liblouis to version 3.8, add debug logging functionality and cleanup old workarounds that are no longer necessary Dec 4, 2018
@LeonarddeR LeonarddeR requested a review from dkager December 4, 2018 18:35
@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

I updated the initial description of this pr, as I've been able to expand it in a major way.
@dkager: since this pr involves some major changes, I'd like to have your review if possible. Don't feel forced to do it, though.

Comment thread source/louisHelper.py Outdated
# Set the log level to all.
# The NVDA logging callback will filter messages appropriately,
# i.e. error messages will be logged at the error level.
louis.setLogLevel(louis.LOG_ALL)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are you sure there is not a performance decrease with this? If there are a lot of log calls in Liblouis at level debug, there will be many switches from c to Python, even if the log callback Python function returns early. I certainly agree we should enable for warning and error though.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I haven't experienced any noticeable performance quirks. I guess we can do either of 4 things:

  1. Leave as is
  2. Initialize liblouis debug at louis level DEBUG, in which case the number of message drops significantly (i.e. as far as I know, all messages involved with a translation that succeeds, are logged below level debug and thus won't be logged in that case.
  3. Do not initialize louis logging at all when the louis flag in the configuration is False. This requires a restart of NVDA for changes to the louis flag to take effect.
  4. Revert the config spec change. Instead, initialize louis logging at level_none, in which case nothing is logged at all. However... that only adds the possibility to change the logging level at runtime.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good to know you're not feeling any performance decrease. I'd still vote for going with option 2. Assuming there is no good reason why those messages below debug need to be easily accessed on any copy of NVDA.

@michaelDCurran michaelDCurran merged commit ec877fe into nvaccess:master Dec 6, 2018
@nvaccessAuto nvaccessAuto added this to the 2018.4 milestone Dec 6, 2018
@dkager

dkager commented Dec 11, 2018

Copy link
Copy Markdown
Contributor

Cool! I think setting the liblouis log level to off when louisHelper terminates is not strictly necessary, but presumably it doesn't hurt either.

@LeonarddeR

LeonarddeR commented Dec 11, 2018 via email

Copy link
Copy Markdown
Collaborator Author

@DrSooom

DrSooom commented Feb 14, 2019

Copy link
Copy Markdown

Is this PR already fully included into NVDA 2018.4? I ask because the changelog file wasn't updated.

@lukaszgo1

Copy link
Copy Markdown
Contributor

It is included in 2019.1. It was just added to the wrong milestone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

8 participants