Custom braille tables (#3304) by JulienCochuyt · Pull Request #10172 · nvaccess/nvda

JulienCochuyt · 2019-09-08T07:10:07Z

Link to issue number:

Fixes #3304.
Fixes #9863.
Supersedes PR #9864.
Addresses #505 (comment)
Provides a workaround for #3470

Summary of the issue:

In NVDA, there is no easy and reliable way for an add-on to provide a new braille table.
For an experienced users wishing to do so there are two options:

Alter manually an existing table in louis/tables.
The new table still has its original name in the settings GUI.
This change is lost upon NVDA updates.
Set the absolute path to the table file in the configuration in lieu of the usual file name.
The settings GUI shows an empty entry for this one.
Forces to manually alter nvda.ini.
Forces to copy in the same directory the whole dependency chain of the new table plus braille-patterns.cti. This is because liblouis default table resolver only looks for tables in a single directory, See Make it easier for add-ons to supply custom braille tables #5489 (comment)

Description of how this pull request fixes the issue:

Add a new brailleTables optional directory in both the user scratchpad directory and the add-on directory structure.
Implement a custom liblouis table resolver which looks up first in these directories, then in louis/tables.
Support reading tables metadata from an optional brailleTables section of the add-on manifest or from a .ini file with the same format found in the brailleTables subdirectory of the scratchpad directory, allowing a user to provide a display names and set output/input/contracted capabilities with no code.
Enforce the existing fallback mechanism to ensure there still is braille output if the configured table cannot be found eg. because an add-on or the scratchpad directory was disabled. This now applies both to the main configuration and individual profiles and also covers braille input.

As is already the case in the default liblouis table resolver, table file names are treated as an identifier.
That is, if a user eg. provides in its scratchpad/brailleTables directory a new version of a table which is imported by other tables, all of those are impacted.
Only one version of a given table file name can be resolved. In that concern, the user scratchpad directory takes precedence over add-ons, which take precedence over the tables provided with NVDA.
It also mean that the manifest file can alter metadata of a table file provided elsewhere.
Eg. adding a manifest to the scratchpad/brailleTables directory is enough to workaround #3470.

Manifest file example as found in the developer guide:

[brailleTables]
[[fr-bfu-tabmod-comp8.utb]]
displayName = French (unified) 8 dot computer braille - Tab mod
contracted = False
output = True
input = True
[[no-no-generic.ctb]]
displayName = Norwegian unofficial computer braille table
contracted = False
output = True
input = True

Example add-on demonstrating the new functionality (please rename .zip to .nvda-addon):
pr10712-customBrailleTables-demo-2021.05.08.zip

Testing strategy:

From a source copy:

added a new table via an add-on
added a new table via the scratchpad directory
replaced an existing table via an add-on
replaced an existing table via the scratchpad directory
altered existing table meta-data via an add-on
altered existing table meta-data via the scratchpad directory
ensured proper fallback when the user selected a new table which was provided by a later disabled add-on
ensured proper fallback when the user selected a new table which was provided by a later removed add-on
ensured proper fallback when the user selected a new table which was provided by the later disabled scratchpad directory

Known issues with pull request:

The current automatic translation of older tables names to newer tables names as of a liblouis update can lead to unexpected behavior if there is collision, but I do not think I am the one to take the decision when to drop this backward-compatibility piece.

Change log entry:

Section: Changes for developers
Add-on can now provide custom braille tables.

Code Review Checklist:

LeonarddeR

Thanks for this huge and nice work! I think I like the switch from json to configobj.

Now I wonder, have you considered storing table data in the add-ons manifest itself? I think this has a major benefit, namely that the add-on manifest parser has support for language inheritance out of the box. This means that coming up with the right translated name for the table is the responsibility of the potential add-on translator, not of the initial author.

LeonarddeR · 2019-09-09T13:31:05Z

+		for addon in addonHandler.getRunningAddons():
+			yield os.path.join(addon.path, "brailleTables")
+
+	return [dir_ for dir_ in candidates() if os.path.isdir(dir_)]


I like this style. Do you think it makes sense to log if the isdir check doesn't pass?

I would guess no.
Having a file named "brailleTables" is damn unlikely, and no one would reasonably expect its content to be interpreted.
The check could even be removed here and such a file would still not break the resolver.

LeonarddeR · 2019-09-09T13:33:05Z

+		)
+
+
+def _getCustomTableDisplayName(fileName: str, tableCfg: dict) -> str:


Have you considered making this a property on CustomTablesManifest?

Actually, no.
The manifest is only one way to supply the config.
Another is to pass a dictionary to brailleTables.loadCustomTablesConfig (made public as of b87f5b2, see #10172 (comment))

Co-Authored-By: Leonard de Ruijter <leonardder@users.noreply.github.com>

Re: nvaccess#10172 (comment)

JulienCochuyt · 2019-09-09T15:38:45Z

Thanks for this huge and nice work! I think I like the switch from json to configobj.

You are very welcome. Thanks for you interest.

Now I wonder, have you considered storing table data in the add-ons manifest itself? I think this has a major benefit, namely that the add-on manifest parser has support for language inheritance out of the box. This means that coming up with the right translated name for the table is the responsibility of the potential add-on translator, not of the initial author.

The add-on manifest could indeed safely contain a "brailleTables" section with the needed configuration. brailleTables._getCustomTableDisplayName even handles the case where "displayName" is a key rather than a section - which fits this use case just fine.

My concern, though, is that ConfigObj does not support formally optional sections.
See DiffSK/configobj#131
This means that the "brailleTables" section could not be specified in the add-on manifest specification unless every single add-on manifest provides it. I guess neither of these alternatives is fine.

Anyway, an add-on can safely add a custom table during its initialization and have its display name translated as usual:
Eg: brailleTables.addTable("somefile.utb", , _("Some fancy name"), output=True, input=True, contracted=False)

Writing this I realize that brailleTables._loadCustomTablesConfig should have its leading underscore removed as the following is also a valid use for an add-on:

brailleTables.loadCustomTablesConfig({
    "file1.utb": {
        "displayName": _("Name of first table"),
        "output": True,
        "input": True,
        "contracted": True
    },
    "file2.utb: {
        "displayName": _("Name of second table"),
        "output": True,
        "input": True,
        "contracted": True
    }
})

EDIT:
Renamed _loadCustomTablesConfig to loadCustomTablesConfig as of b87f5b2.

Re: nvaccess#10172 (comment)

LeonarddeR · 2019-09-10T06:10:13Z

@feerrenrut, could your share your thoughts about what you think would be best to specify metadata for custom braille tables?

DrSooom · 2019-09-20T15:11:24Z

See also: https://github.com/liblouis/liblouis/wiki/Table-discovery-based-on-table-metadata

LeonarddeR · 2019-09-30T09:49:12Z

@JulienCochuyt wrote:

This means that the "brailleTables" section could not be specified in the add-on manifest specification unless every single add-on manifest provides it.

Is this true? Is there anything holding you back from providing a [brailleTables] section and a [[many]] section in there and providing a manifest that doesn't have this section? I gess the validator would just add the empty section just as it adds defaults for optional fields, such as readme.

JulienCochuyt · 2019-09-30T11:16:20Z

@JulienCochuyt wrote:

This means that the "brailleTables" section could not be specified in the add-on manifest specification unless every single add-on manifest provides it.

Is this true? Is there anything holding you back from providing a [brailleTables] section and a [[many]] section in there and providing a manifest that doesn't have this section? I gess the validator would just add the empty section just as it adds defaults for optional fields, such as readme.

It at least is how I understood DiffSK/configobj#131 back then: You can't have a specified optional section. Unspecified sections are supported, but once they are specified, they become mandatory.
Nevertheless, I might try to play a bit more with ConfigObj to try ensure that point.

But anyhow, I doubt bundling a hundreds of tables in an add-on would ever be a reasonable use case, and keep in mind that registering the tables via simple code already allows translators to follow their usual workflow.

LeonarddeR · 2019-09-30T11:23:18Z

... keep in mind that registering the tables via simple code already allows translators to follow their usual workflow.

Problem with this is that there are multiple pathways to do exactly the same. I'd rather have only one public method of bundling tables in add-ons.

JulienCochuyt · 2019-09-30T12:25:29Z

... keep in mind that registering the tables via simple code already allows translators to follow their usual workflow.

Problem with this is that there are multiple pathways to do exactly the same. I'd rather have only one public method of bundling tables in add-ons.

Actually, the support for metadata .ini files was initially merely added to easily add custom tables in the scratchpad directory with no code - extending my personal use case of wanting my own modified fr-bfu-comp8 available when disabling all add-ons. Felt then like providing non-developer end users with as much icing on the cake as possible.
Thought it also provided an easy way to allow building of a minimal add-on with no code that would eg. add the requested no-no-comp table. I never considered an add-on author would really want to provide dozens of tables with localized names - but still made sure to support it.

We're in Python, there isn't a single way to do things, even if a compatriot of yours is told to be the one able to distinguish which is the right one :)

If I understand your point of view, it's hard to advertise the metadata .ini files way as it does not fit with the standard translation process. Though, I can remove it altogether if you are more comfortable with it.

LeonarddeR · 2019-09-30T12:53:21Z

Actually, the support for metadata .ini files was initially merely added to easily add custom tables in the scratchpad directory with no code - extending my personal use case of wanting my own modified fr-bfu-comp8 available when disabling all add-ons. Felt then like providing non-developer end users with as much icing on the cake as possible.

Mm yes, this is an interesting one. This requires a method that doesn't involve add-on manifests.

We're in Python, there isn't a single way to do things, even if a compatriot of yours is told to be the one able to distinguish which is the right one :)

This is true. However, I'd rather advertise one public method to do it, leaving it up to an add-on author to use another method that might be perfectly valid, but isn't the advised one. For example, if we decide that manifests is the way to go, add-on authors can still use the registration method, but they are encouraged to use the manifest method.

If I understand your point of view, it's hard to advertise the metadata .ini files way as it does not fit with the standard translation process.

Yes, this is true. However, I think it is up to either @feerrenrut or @michaelDCurran to crack the nut here.

Though, I can remove it altogether if you are more comfortable with it.

I'd say lets leave it as is until we have some more opinions about this.

LeonarddeR · 2019-10-23T08:11:43Z

Could either @michaelDCurran and @feerrenrut elaborate on what NV access' standpoint is with regard to what implementation to use to define custom tables? See #10172 (comment)

JulienCochuyt · 2021-05-24T08:22:26Z

@seanbudd wrote:

There's some minor merge conflicts in the docs that need to be fixed as well.

Done as of f3be0f9.

# Conflicts: # source/addonHandler/__init__.py # source/braille.py # source/brailleInput.py # source/brailleTables.py # source/gui/settingsDialogs.py

seanbudd · 2022-02-17T01:58:33Z

when resolving this merge conflict, I kept this line where it currently is in master. However, it might need to get moved to handlePostConfigProfileSwitch?

Why do you think it should be there? I don't think it needs to re-initialize after every config update?

This was the only non-trivial change when resolving merge conflicts.
I thought was worth raising, in the case that we need to reload detections when changing braille tables.

stale

feerrenrut

I'm going to need to come back to this next week.

feerrenrut · 2022-02-17T10:58:16Z

+[brailleTables]
+	# The key is the table file name (not the full path)
+	[[__many__]]
+		displayName = string()


Is display name the name of the braille display, or the name of the braille table to be displayed in the GUI, or file name?

This is the name displayed in the GUI. The [[many]] map key is the file name.

This is based on my understanding of the user docs: https://github.com/nvaccess/nvda/pull/10172/files#diff-c9a23ef91eea3598e1128c9564a3a9b511d4941456ca31a24837e81c18b8b5caR853-R868, and the code a little further down this file https://github.com/nvaccess/nvda/pull/10172/files#diff-2fafc40e2d44037b654b987d85ce3c62746cc62261eb9c8dd59268a2d48a9494R768-R771

feerrenrut · 2022-02-17T11:06:56Z

Why do you think it should be there? I don't think it needs to re-initialize after every config update?

burmancomp · 2023-03-09T18:21:04Z

How is it with this pull request? I think this would be useful feature.

LeonarddeR · 2023-08-25T16:41:32Z

@JulienCochuyt any chance you can bring this forward? If not, I think you did some very valuable work here and I'm happy to take this one. I'll do that if I'm not hearing back from you within a month or so.

Adriani90 · 2024-01-20T13:00:47Z

@LeonarddeR I think @JulienCochuyt is no longer that active on Github, at least I didn't see any updates from him since many years. Are you still thinking about taking this up?

Adriani90 · 2024-01-20T13:20:51Z

Just a note, in my view unlike #3304 suggests, I would rather prefer an approach similar to speech dictionaries where user's dictionaries are stored in the user config and where users can edit the dictionaries in a GUI. . But I don't know if this is technically feasible. Nevertheless, this PR definitely would point things in the right direction at least.

seanbudd · 2024-01-30T03:19:29Z

Hi - I'm going to mark this as a draft. I know that the reason this has been abandoned due to a delayed review from NV Access, however I do not see it in our near capacity to fix up the merge conflicts and prepare this for another review.
Given this is now the only item in our ready, unblocked, PR queue, I think we can be confident that this will be reviewed in a timely manner in future if further work is done.

LeonarddeR · 2024-02-21T09:37:01Z

I'm definitely taking this, since I might have a use case for myself in the near future. That's a pretty good stick behind the door if I may say so😉

LeonarddeR · 2024-02-21T17:29:42Z

Superseeded by #16208

Fixes #3304. Fixes #9863. Supersedes PR #9864, #10172. Addresses #505 (comment) Summary of the issue: In NVDA, there is no easy and reliable way for an add-on to provide a new braille table. For an experienced users wishing to do so there are two options: Alter manually an existing table in louis/tables. The new table still has its original name in the settings GUI. This change is lost upon NVDA updates. Set the absolute path to the table file in the configuration in lieu of the usual file name. The settings GUI shows an empty entry for this one. Forces to manually alter nvda.ini. Forces to copy in the same directory the whole dependency chain of the new table plus braille-patterns.cti. This is because liblouis default table resolver only looks for tables in a single directory, See Make it easier for add-ons to supply custom braille tables #5489 (comment) Description of how this pull request fixes the issue: Add a new brailleTables optional directory in both the user scratchpad directory and the add-on directory structure. Support reading tables metadata from an optional brailleTables section of the add-on manifest or from a manifest.ini file with the same format found in the root of the scratchpad directory, allowing a user to provide a display name and set output/input/contracted capabilities with no code. Implement a custom liblouis table resolver that resolves tables based on what is registered in the brailleTables module: When liblouis calls the resolver without a base file specified, the table is looked up from the brailleTables module and either resolved from the add-ons brailleTables directory or the built-in tables directory When liblouis calls the resolver with a base file specified (e.g. when processing includes in tables), the table is looked up from the folder of the base table and/or the built-in tables directory Enforce the existing fallback mechanism to ensure there still is braille output if the configured table cannot be found e.g. because an add-on or the scratchpad directory was disabled. This now applies both to the main configuration and individual profiles and also covers braille input. Note that if an add-on author wants a table to be listed in the GUI, he/she should always define the table in the manifest. Contrary to earlier incarnations of this pr, replacing a table in an add-on (i.e. when it has the same filename as a built-in table) without defining it in the manifest is no longer possible. Therefore it is also not possible to replace unlisted tables that are included by listed tables. For example, if you want to replace spaces.uti as included in nl-comp8.utb, you weel need to both define and bundle a replacement of nl-comp8.utb and spaces.uti in your add-on.

CyrilleB79 · 2024-06-18T08:15:59Z

Since it has been superseded, I remove the "Abandoned" label.

@seanbudd I hope NV Access approves such triage actions; it should allow to filter Abandoned PRs for somewone wanting to take it over.

seanbudd · 2024-06-18T23:02:55Z

Thanks @CyrilleB79

JulienCochuyt added 2 commits September 8, 2019 08:00

Custom braille tables (nvaccess#3304)

053ca71

Unit tests: Initialize brailleTables before braille.

3e0a117

LeonarddeR reviewed Sep 9, 2019

View reviewed changes

JulienCochuyt and others added 8 commits September 9, 2019 16:23

brailleTables: Favor except Exception instead of bare except.

0193c71

Co-Authored-By: Leonard de Ruijter <leonardder@users.noreply.github.com>

settingsDialogs: Remove dev comment.

24f5ed1

Co-Authored-By: Leonard de Ruijter <leonardder@users.noreply.github.com>

brailleTables: Remove dev log

ea43238

Re: nvaccess#10172 (comment)

louisHelper: Add debug log of tables directories at initialization.

af30589

Re: nvaccess#10172 (comment)

brailleTables: Use an f-string instead of str.format

6fb372f

Re: nvaccess#10172 (comment)

brailleTables: Add a comment to explain slice-replace construct.

deb3836

Re: nvaccess#10172 (comment)

brailletables: Update no more valid part of docstring.

284b5c9

Re: nvaccess#10172 (comment)

brailleTable: Log an error for missing custom tables directory.

fb0d965

Re: nvaccess#10172 (comment)

JulienCochuyt added 3 commits September 9, 2019 17:39

brailleTables: Make loadCustomTablesConfig public.

b87f5b2

Re: nvaccess#10172 (comment)

Custom braille tables: Add a few more type hints.

5010f35

Merge branch 'master' into i3304-customBrailleTables

f147742

LeonarddeR closed this Sep 30, 2019

LeonarddeR reopened this Sep 30, 2019

Merge branch 'master' into i3304-customBrailleTables

6f248ec

michaelDCurran mentioned this pull request Oct 25, 2019

Gracefully handle regular expression errors in speech dictionaries #10421

Merged

Merge branch 'master' into i3304-customBrailleTables

66dab97

feerrenrut added Addon/API Changes to NVDA's API for addons to support addon development. component/braille labels Apr 8, 2020

seanbudd added the blocked/merge-conflicts Merge conflicts exist on this PR label Sep 22, 2021

Merge branch 'master' into i3304-customBrailleTables

7be07a8

# Conflicts: # source/addonHandler/__init__.py # source/braille.py # source/brailleInput.py # source/brailleTables.py # source/gui/settingsDialogs.py

seanbudd reviewed Feb 17, 2022

View reviewed changes

feerrenrut removed the blocked/merge-conflicts Merge conflicts exist on this PR label Feb 17, 2022

feerrenrut reviewed Feb 17, 2022

View reviewed changes

feerrenrut added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 18, 2022

seanbudd added the blocked/needs-code-review label Jul 5, 2022

seanbudd added the stale PR is stale/old - NV Access has missed reviewing this or this is an abandoned draft label Apr 28, 2023

seanbudd self-requested a review June 20, 2023 01:51

seanbudd added merge-early Merge Early in a developer cycle blocked/merge-conflicts Merge conflicts exist on this PR labels Jun 21, 2023

This comment has been minimized.

Sign in to view

Adriani90 mentioned this pull request Jan 20, 2024

Problems with Spanish Braille table #10246

Open

seanbudd marked this pull request as draft January 30, 2024 03:19

seanbudd added the Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. label Jan 30, 2024

LeonarddeR mentioned this pull request Feb 21, 2024

Support for custom braille tables #16208

Merged

14 tasks

LeonarddeR closed this Feb 21, 2024

CyrilleB79 removed the Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. label Jun 18, 2024

		)


		def _getCustomTableDisplayName(fileName: str, tableCfg: dict) -> str:

Uh oh!

Conversation

JulienCochuyt commented Sep 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entry:

Code Review Checklist:

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JulienCochuyt commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LeonarddeR commented Sep 10, 2019

Uh oh!

DrSooom commented Sep 20, 2019

Uh oh!

LeonarddeR commented Sep 30, 2019

Uh oh!

JulienCochuyt commented Sep 30, 2019

Uh oh!

LeonarddeR commented Sep 30, 2019

Uh oh!

JulienCochuyt commented Sep 30, 2019

Uh oh!

LeonarddeR commented Sep 30, 2019

Uh oh!

LeonarddeR commented Oct 23, 2019

Uh oh!

JulienCochuyt commented May 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

burmancomp commented Mar 9, 2023

Uh oh!

LeonarddeR commented Aug 25, 2023

Uh oh!

This comment has been minimized.

Adriani90 commented Jan 20, 2024

Uh oh!

Adriani90 commented Jan 20, 2024

Uh oh!

seanbudd commented Jan 30, 2024

Uh oh!

LeonarddeR commented Feb 21, 2024

Uh oh!

LeonarddeR commented Feb 21, 2024

JulienCochuyt commented Sep 8, 2019 •

edited

Loading

JulienCochuyt commented Sep 9, 2019 •

edited

Loading