-
-
Notifications
You must be signed in to change notification settings - Fork 634
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
Add options to set translation and input braille tables according to NVDA's language #17314
base: master
Are you sure you want to change the base?
Conversation
… autoBrailleLang
… autoBrailleLang
… autoBrailleLang
See test results for failed build of commit 0d25bc42d7 |
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.
I like this a lot!
source/braille.py
Outdated
tableName = config.conf["braille"]["translationTable"] | ||
# #6140: Migrate to new table names as smoothly as possible. | ||
newTableName = brailleTables.RENAMED_TABLES.get(tableName) | ||
if newTableName: | ||
tableName = config.conf["braille"]["translationTable"] = newTableName | ||
if tableName != self._table.fileName: | ||
try: | ||
self._table = brailleTables.getTable(tableName) | ||
if config.conf["braille"]["translationTable"] == "auto": | ||
self._table = brailleTables.getDefaultTableForCurLang(brailleTables.TableType.OUTPUT) |
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.
Is this correct? Shouldn't this be?
self._table = brailleTables.getDefaultTableForCurLang(brailleTables.TableType.OUTPUT) | |
tableName = brailleTables.getDefaultTableForCurLang(brailleTables.TableType.OUTPUT) |
That means instead of the else block, you can just load tableName.
source/gui/settingsDialogs.py
Outdated
) | ||
self.inTableForCurLang = self.inTables[inTableForCurLangIndex] | ||
# Translators: An option in Braille settings to select a braille table automatically, according to the current language. | ||
inTableChoices = [_("Automatic (%s)" % self.inTableForCurLang.displayName)] |
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.
Same as above
Co-authored-by: Leonard de Ruijter <[email protected]>
See test results for failed build of commit 4a72f93aeb |
@LeonarddeR , thanks for your review!
UnboundLocalError: cannot access local variable 'table' where it is not associated with a value Perhaps we should revert this suggestion? |
I assumed that @LeonarddeR suggestion was complete, but now I've committed a fix and this should be ready. |
pre-commit.ci run. |
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.
Looks good and will be a welcome improvement for users.
Thanks Quentin. |
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.
Just small things, feel free to ignore them if you think your code is more appropriate.
tableName = config.conf["braille"]["translationTable"] | ||
# #6140: Migrate to new table names as smoothly as possible. | ||
newTableName = brailleTables.RENAMED_TABLES.get(tableName) | ||
if newTableName: | ||
tableName = config.conf["braille"]["translationTable"] = newTableName | ||
if config.conf["braille"]["translationTable"] == "auto": |
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.
tableName already contains the name of config at this point, so it is not necessary to recheck the config here.
if config.conf["braille"]["translationTable"] == "auto": | |
if tableName == "auto": |
# #6140: Migrate to new table names as smoothly as possible. | ||
tableName = config.conf["braille"]["inputTable"] | ||
newTableName = brailleTables.RENAMED_TABLES.get(tableName) | ||
if newTableName: | ||
tableName = config.conf["braille"]["inputTable"] = newTableName | ||
table = config.conf["braille"]["inputTable"] | ||
if table != self._table.fileName: | ||
if config.conf["braille"]["inputTable"] == "auto": |
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.
Idem as above
if config.conf["braille"]["inputTable"] == "auto": | |
if tableName == "auto": |
Link to issue number:
Fixes issue #17306
Summary of the issue:
By default, NVDA sets braille tables according to its language. When NVDA's language is changed, by default braille tables are modified accordingly, which may be confusing for users.
Description of user facing changes
In the Braille settings panel, combo boxes for tables have an option to set them according to NVDA's language, so that now this is optional. The automatically selected tables corresponding to the current language are described in parentheses.
Description of development approach
New options have been added to the Braille settings panel. braille, brailleInput and brailleTables modules have been modified so that the "auto" option in configuration can be optional.
Testing strategy:
Tested manually checking that options are presented in the gui and that braille tables are set accordingly.
Known issues with pull request:
None
Code Review Checklist:
@coderabbitai summary