Provide speech symbols file name path on creation #17346
Conversation
WalkthroughThe pull request introduces several changes across multiple files. It updates the minor version number of NVDA software in Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
source/characterProcessing.py (1)
240-244: LGTM! Consider adding type hints for the return type.The addition of the optional filename parameter to the constructor is a good change that allows setting the filename during initialization. This fixes the issue where the filename was only set during load(), enabling saving without loading first.
Consider adding a return type hint to the constructor:
- def __init__(self, filename: str | None = None): + def __init__(self, filename: str | None = None) -> None:user_docs/en/changes.md (1)
Line range hint
12-24: Consider adding version numbers for dependenciesWhen listing updated dependencies like eSpeak-NG and LibLouis, consider including the specific version numbers in the highlights section for better clarity.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
source/buildVersion.py(1 hunks)source/characterProcessing.py(2 hunks)user_docs/en/changes.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
source/buildVersion.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/characterProcessing.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 (5)
source/buildVersion.py (1)
68-68: Verify if the minor version increment is justified.
While the changes in this PR fix important issues with speech dictionaries and dialog behavior, let's verify if these changes are significant enough to warrant a minor version bump from 0 to 1. According to semantic versioning principles, minor version increments typically indicate new functionality added in a backward-compatible manner.
Let's analyze recent changes to confirm the scope:
✅ Verification successful
Let me check the changelog to better understand the changes that justify this version bump.
Minor version increment is justified
The version bump from 2024.4.0 to 2024.4.1 is appropriate as this is a patch release that fixes two specific bugs:
- A fix for Outlook on certain Windows 10 systems
- A critical bug fix for saving speech dictionaries and dialog closure (nvda will fail to close the pronounciation/symbol pronounciation dialog when the ok button is envoked. #17344)
The changes involve providing the speech symbols filename path during creation, which fixes the speech dictionary saving functionality. This type of bug fix is correctly reflected in a patch version increment (0 to 1) rather than a minor version bump.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check recent changes to assess version bump justification
# Get recent commits and their impact
git log -n 10 --oneline
# Check for other significant changes in this PR
git diff HEAD~1 --name-status
# Look for other version-related changes
rg -g '!*.pyc' -g '!*.pyo' "2024.4."
Length of output: 3202
Script:
#!/bin/bash
# Check the changelog for details about the changes
cat user_docs/en/changes.md | head -n 20
# Look at the actual code changes in the relevant commit
git show f40c93c1
Length of output: 2118
source/characterProcessing.py (1)
829-829: LGTM! The filename is now properly initialized.
Passing the formatted path to the SpeechSymbols constructor ensures the filename is set immediately upon creation, which works in conjunction with the constructor changes to fix the saving issue.
user_docs/en/changes.md (3)
1-7: LGTM: Clear changelog header and introduction
The header and introduction provide a good overview of the latest changes. The formatting follows standard Markdown conventions.
8-11: LGTM: Well-organized highlights section
The highlights section effectively summarizes the key changes in this release, making it easy for users to quickly understand the major updates.
Line range hint 25-1000: LGTM: Well-structured changelog entries
The changelog entries are well organized with:
- Clear version headers
- Consistent categorization (New Features, Changes, Bug Fixes, etc.)
- Issue/PR references
- Detailed descriptions
|
For me this sounds good, but I'm not able to test this. I get the following error: CRITICAL - main (09:37:46.570) - MainThread (6652): |
|
Sorry, I didn't mereged this into rc but into master. I'm recreating the virtual environment on my machine to test properly. |
Co-authored-by: Noelia Ruiz Martínez <nrm1977@gmail.com>
|
I'll test the artifact when appveyord creates it. |
|
Also, in
2024.4.1 doesn't exist in the server as an API version, so add-ons cannot be retrievet when trying the artifact. |
|
lets hope this issue is resolved. looking forward to testing the alpha snapshot!!! |
|
@nvdaes - no, we just need to update |
Provide speech symbols file name path on creation (#17346)
Link to issue number:
Fixes #17344
Summary of the issue:
The
SpeechSymbolsonly sets the filename to write to when the dictionary is loaded.Before 826ef91, dictionary files were always loaded before they were saved.
Afterwards, dictionaries are saved without being loaded.
When trying to save dictionary files the following error occurs, due to no file name being set
Description of user facing changes
Users can now save the speech dictionary dialog.
Add-ons which use custom speech dictionaries are also saved correctly now.
Description of development approach
Create optional parameter to set filename path for speech dictionaries.
Testing strategy:
Manual testing issue described in #17345
Manually testing Perky Duck add-on which uses a custom speech dictionary
Known issues with pull request:
None
Code Review Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Version Update