Make it easier for add-ons to supply custom braille tables #5489
Conversation
* Rename braille.TABLES to braille.tables and turn it into a list. * Make the always-included patterns table path a constant.
…table paths from being computed on every translation or input.
…s when specifying an invalid table. Requires liblouis >= 2.6.3 (nvaccess#5137).
dkager
changed the title
|
I'm holding back on this for now because I'm looking at doing contracted braille input (#2439) for 2016.1 and that will require some refactoring of the way we handle tables. For that, we need to be able to quickly look up whether a table is contracted or not, so a list is not a good data structure. It'll probably become a dict internally, but I'm also considering adding an addTable function or similar which just handles the gory details, thus allowing for default arguments and making this a bit more future proof. There is still probably code I'll borrow from this, though. |
|
Yeah, that's pretty much what I thought when I heard the latest podcast. :) |
source/braille.py
Outdated
| @@ -21,9 +21,10 @@ | |||
|
|
|||
| #: The directory in which liblouis braille tables are located. | |||
| TABLES_DIR = r"louis\tables" | |||
| PATTERNS_TABLE = os.path.join(TABLES_DIR, "braille-patterns.cti") | |||
There was a problem hiding this comment.
nit: UNICODE_BRAILLE_TABLE or something might be a better name for this. IMO, braille-patterns.cti is a terrible name. This table allows Unicode braille characters to be used anywhere to produce raw dots.
|
I think most of this code still applies, actually. The only thing that might change is the underlying data structure. While I think it's simpler to just have the table list and data in the braille module (rather than having a separate module), I think I'd prefer to have code specific to setting the input table in the brailleInput module. Common code can still go in braille. In practical terms, this means:
|
source/braille.py
Outdated
| return table | ||
|
|
||
| def _getTableList(self, table): | ||
| return [self._getTablePath(table), PATTERNS_TABLE] |
There was a problem hiding this comment.
This worries me because of liblouis silliness. From the liblouis documentation about the table list (square brackets used for emphasis):
liblouis knows where to find all the tables that have been distributed with it. So you can just give a table name such as en-us-g2.ctb and liblouis will load it. You can also give a table name which includes a path. [If this is the first table in a list, all the tables in the list must be on the same path.] You can specify a path on which liblouis will look for table names by setting the environment variable LOUIS_TABLEPATH. This environment variable can contain one or more paths separated by commas. On receiving a table name liblouis first checks to see if it can be found on any of these paths. If not, it then checks to see if it can be found in the current directory, or, [if the first (or only) name in a table list, if it contains a path name, can be found on that path]. If not, it checks to see if it can be found on the path where the distributed tables have been installed.
According to this, it will ignore the path specified for any tables other than the first. So, if the first table is within an add-on, that means braille-patterns.cti won't be found. The stuff about the environment variable and distribution tables doesn't apply; I'm pretty sure you can't change the environment for a running process, only for subsequent processes, and we don't want to compile in an absolute path for distribution tables.
All of this said, if this is indeed the case, surely you would have seen this problem when testing? Maybe the documentation is actually wrong?
Regarding mixing absolute and relative paths: |
|
OK, refactored the code (as yet untested). This adds a few elements to the brailleInput module that could be considered as unnecessary boilerplating or as the cornerstones for future things to come. In particular, brailleInput now has a constructor and also responds to configuration profile switches. |
I don't think it's worth it. I've made this point in the past and no one seems to care. If we do it now, we'd probably end up renaming 90% of the tables directory. :)
The problem is that the distribution directory isn't relevant here. It will change across instances of NVDA, so we don't/can't set it.
Interesting that your tests seem to work. I wonder how it's finding the correct tables. That suggests it doesn't ignore the path if specified in subsequent tables. Did inclusion of dist tables work from custom tables?
In the end, this may be the simplest solution. That wasn't available when I originally wrote this code.
More likely a bug in liblouis itself, since the wrapper just calls liblouis functions. |
|
I’ll do more extensive testing with the current liblouis and the t3304 branch later today to see what does and what doesn’t work. We should probably also add lou_setDataPath to the Python wrapper if that proves to be the easiest and most portable solution. I now also recall that liblouis looked in some hard-coded path, I believe “liblouis/tables”, which doesn’t work for NVDA since it uses “louis/tables”. From: James Teh [mailto:notifications@github.com] Good point. This table should also have extension uti (uncontracted), not cti (contracted). Many tables don't follow these semantics very well. The question is if this is important enough to suggest renaming the table upstream, e.g. to unicode-braille.uti. I don't think it's worth it. I've made this point in the past and no one seems to care. If we do it now, we'd probably end up renaming 90% of the tables directory. :) Regarding mixing absolute and relative paths: The problem is that the distribution directory isn't relevant here. It will change across instances of NVDA, so we don't/can't set it. My tests were consistent with this, but we definitely need to clear it up before using this code. Interesting that your tests seem to work. I wonder how it's finding the correct tables. That suggests it doesn't ignore the path if specified in subsequent tables. Did inclusion of dist tables work from custom tables? You can set the search path, though not from Python, with lou_setDataPath. In the end, this may be the simplest solution. That wasn't available when I originally wrote this code. One remaining issue: occasionally when changing translation tables I get an access violation error from liblouis. I can't think of anything in this patch that would cause that, other than a buggy Python wrapper for liblouis. More likely a bug in liblouis itself, since the wrapper just calls liblouis functions. — |
|
Looking at the liblouis code I remember why I didn't use lou_setDataPath: it always appends "liblouis\tables". Given that NVDA uses "louis\tables" this doesn't work very well. On Linux it probably makes more sense, you'd use something like "/usr/local/share" as data path. Maybe we could suggest to have these semantics changed a bit so braille.py can safely pass table names without a path. That is: Another path liblouis tries, only on Windows, is the program path, which presumably is the path where nvda.exe (or nvda.pyw) lives. Then if you prefix "louis\tables", which is what happens in braille.py, you end up with a relative path that can be resolved. But this isn't as efficient since the program path is checked after the data path. It also doesn't explicitly say that liblouis tables are stored in "louis\tables". |
While this would certainly be nice, I fear this would be a backwards incompatible change.
It also tries the current directory, which again is the program directory for NVDA.
Indeed. However, this means there is still a path and the documentation suggests that the path of any table other than the first is ignored. Note that it says "path", not "absolute path", which suggests that relative paths are ignored as well. |
|
I’m told fixing the liblouis documentation on this topic is on the to-do list. :) This still leaves the question if splitting the code between braille and brailleInput was an improvement. It’s probably more extensible if nothing else. Either version should work with recent versions of liblouis, though. From: James Teh [mailto:notifications@github.com] Maybe we could suggest to have these semantics changed a bit so braille.py can safely pass table names without a path. That is: louis.setDataPath(r"louis\tables") While this would certainly be nice, I fear this would be a backwards incompatible change. Another path liblouis tries, only on Windows, is the program path, which presumably is the path where nvda.exe (or nvda.pyw) lives. It also tries the current directory, which again is the program directory for NVDA. Then if you prefix "louis\tables", which is what happens in braille.py, you end up with a relative path that can be resolved. Indeed. However, this means there is still a path and the documentation suggests that the path of any table other than the first is ignored. Note that it says "path", not "absolute path", which suggests that relative paths are ignored as well. — |
Heh. I didn't think of this when I suggested it. However, it's worth noting that #2439 also requires BrailleInputHandler to have a constructor, as well as setting a variable indicating whether the table is contracted or not. Therefore, I think it still makes sense for this stuff to go in brailleInput. |
|
Well, that’s how it is in the topic branch now, and I also got to test it. It works! Oh, and even tables without contractions, such as the Dutch literary braille standard, will be a lot of fun to backtranslate. Signs for capitals, numbers, emphasis… This is one ambitious task. :) From: James Teh [mailto:notifications@github.com] This adds a few elements to the brailleInput module that could be considered as unnecessary boilerplating or as the cornerstones for future things to come. In particular, brailleInput now has a constructor and also responds to configuration profile switches. Heh. I didn't think of this when I suggested it. However, it's worth noting that #2439 #2439 also requires BrailleInputHandler to have a constructor, as well as setting a variable indicating whether the table is contracted or not. Therefore, I think it still makes sense for this stuff to go in brailleInput. — |
|
Now that contracted braille input (#6449) has been merged, this needs to be refactored. Sorry. :( On the flip side, it should be a lot cleaner. There is already a |
There is one change that I feel made things a lot less clean, the property and setter for |
|
What side effects are you referring to? Right now, it updates the config,
but that seems like a reasonable side effect; we're essentially updating
the model and that in turn updates the backend config in a consistent way.
|
|
It updates the config, which as you said could be seen as reasonable. But I would also like to do error handling in tables and falling back to default tables in one place. For example, renamed tables in config profiles aren't handled correctly. It seems cleaner to have one function |
|
That seems like a reasonable argument. That's technically a public
property, so we should probably map the property to the function and warn
about deprecation.
|
|
I'm also not sure if falling back to another table should be silent or if it should pop up an error in the UI. I would go for silent, but with an error written to the log of course. |
|
Agreed on silent but logged.
|
|
@dkager: I'm afraid this has lots of conflicts, again. Could you provide a status update for this? |
|
This is too old. Closing and starting fresh when time permits. |
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.
Fixes nvaccess#3304. Fixes nvaccess#9863. Supersedes PR nvaccess#9864, nvaccess#10172. Addresses nvaccess#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 nvaccess#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.
Related issue: #3304