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 profile specific speech dictionaries #11005
Add profile specific speech dictionaries #11005
Conversation
See test results for failed build of commit f32a4f25e0 |
See test results for failed build of commit 9e7debf40a |
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.
Hello.
In the spanish comunity, the speech dictionarys are used for translation audiogames from english to spanish when the game speak using NVDA. I hope that this change request can be implemented. Is good have a dictionary for translation of a game only in this application, for example.
Regards.
Hello, While NVDA does not merge this changes you will be happy to know that the very same code that is to be merged fot that is available on an addon, see https://github.com/marlon-sousa/EnhancedDictionaries |
Hello, Thanks for the addonaries
|
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.
We'd like to introduce this change, however there are a few things to address.
- The latest master will need to be merged in.
- Please add to the PR description an overview of the different dictionary files involved and how they relate to each other and profiles.
- Please also summarize how these new files interact with the dictionary upgrade process.
Thanks for your work here @marlon-sousa, I suspect this will be a popular feature for power users.
source/config/__init__.py
Outdated
@@ -642,6 +644,11 @@ def deleteProfile(self, name): | |||
if trigger._profile == delProfile: | |||
del self._suspendedTriggers[trigger] | |||
|
|||
def getActiveProfile(self): |
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 think this name confuses concepts used. ConfigManager can have many active profiles, they are consulted in order of most recently activated. This function seems to want to get the most recently activated profile, or the most context specific profile. Please update the name, and add some docs to explain this. It would also be nice to extend the docs for self.profiles
to specify that the most recently activated are added to the end of the list.
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.
@feerrenrut as far as I understand how profiles work on NVDA, there is always the base config (a k a default profile) and zero or one other profile in effect at the same time.
The final configuration is then a merge of all default configurations plus the specified configurations on the profile, which will have precedence over these same configurations on the default profile.
Your comment seems to imply that there is a stack of profiles activated, so every configuration is searched, in precedence order, from the upper to the lower profiles, being the default profile the lowest one.
I frankly have never been able to reproduce this kind of behavior, nor has the graphical user interface ever given me this impression, because, from the GUI, it seems clear that only one profile can be activated at any time, not multiple profiles (either you have the default profile activated or some other being either something you manually activated or that the loader decided that should be activated).
So far, up to now, every time I activated a profile their configurations came in effect immediately, and configuration the previous profile specified were discarded.
For example:
- Configure your default profile to use eSpeak American English voice.
- Create another profile, say test1, and configure it to use British English Voice.
- Create another profile, say test2, and do not change the voice settings.
Now, do the following:
- Activate the default probile and read a word. You should hear the American English Voice.
- Now, activate the test1 profile manually and read the same word. You should hear it using the British English Voice.
- Now, Activate the test2 profile, that one you didn't supply voice configuration for. Read the word again, and you will hear it in American English voice.
If, as you stated, there could be multiple profiles active, you would hear the word read in British English Voice, because test2 didn't have voice specific configurations and the profile activated before that, called Test1, has British English as its voice.
Instead, what happened was that, when you activated test2, the test1 profile was immediately unloaded and, as test2 has no voice specific settings, the configuration on the default profile was then used.
This seems to confirm my impression that there may be only one active profile on a given moment, with the default profile being used as base for resolving settings that the active profile didn't specify.
The fact that a stack is chosen to achieve this implementation is an internal topic, and this is why I created a higher level abstraction that will give the caller the *** currently ** active profile, if there is one.
Sorry for the long text, and of course you are the maintainer here so I will end up naming the function whatever you decide, but I can not think on a better name, so if you could please provide it I will happily apply the change.
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.
As you demonstrated in your example, you cannot stack two profiles activated manually.
But you can stack 3 profiles (including default) in the following situation:
- Profile 0: Default profile with English TTS and no indent reporting.
- Profile 1: Application profile, e.g. Notepad++ profile with indent reporting; this profile is triggered when Notepad++ is focused
- Profile 2: Manual profile with voice configured to Spanish TTS. And with a keyboard shortcut to enable it manually.
Now if you go to Notepad++ and press the shortcut to activate the manual profile (profile 2), you will have both indent reported (Notepad++ profile option) and Spanish voice (manual profile option).
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.
You have a good point here.
This is getting confusing, because now NVDA behavior is not deterministic based on a given profile, since the stack below it can not be predicted. Anyways, I can not extend this to the dictionaries, because then one would never know exactly what to expect on pronunciation and substitution rules. I will delete this convenience function and access the top of stack directly when needed.
source/speechDictHandler/__init__.py
Outdated
@@ -49,6 +52,12 @@ class SpeechDict(list): | |||
|
|||
fileName = None | |||
|
|||
def create(self, fileName): | |||
if os.path.exists(fileName): | |||
raise f"can not create dictionary backed by file {fileName}" |
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.
Please raise as an exception type. For a listing of built-in common exceptions see: https://docs.python.org/3/library/exceptions.html
I'd suggest using FileExistsError
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.
Done
Hello @feerrenrut Since this have been opened so many time ago, I will rebase with the latest master and see if anything else changed in the mean time. I say that because this turned into an addon. So I will this weekend:
As soon as it is ready I will let you know and we restart the review process again. |
cc11827
to
b82fe64
Compare
See test results for failed build of commit 892959763c |
source/config/__init__.py
Outdated
@@ -628,6 +630,11 @@ def deleteProfile(self, name): | |||
if trigger._profile == delProfile: | |||
del self._suspendedTriggers[trigger] | |||
|
|||
def getActiveProfile(self): | |||
if globalVars.appArgs.secure: |
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 don't think this should return a different value when in secure mode - while it is indeed impossible to create profiles or triggers when running securely triggers created in the configuration before it is copied to the system config are still applied.
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.
What do you suggest?
My understanding is that if globalVars.appArgs.secure is on, then no custom profiles are allowed, so we return None to signalize that the default profile is used.
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.
My understanding is that if globalVars.appArgs.secure is on, then no custom profiles are allowed,
That is not true. You can test as follows:
- Create a triggered profile for an application and change some settings there.
- Start NVDA providing
--secure
from the CLI. - Check if the trigger for the configure profile has been applied or not.
What do you suggest?
This method should not do anything special when running with globalVars.appArgs.secure
set to `true.
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.
@feerrenrut Can you explain what globalVars.appArgs.secure does exactly?
@lukaszgo1 while I agree with you, in several places where globalVars.appArgs.secure is checked things like saving profiles seem to be disabled.
See, for example, the constructor of class ConfigManager in config/init.py
You will find the following piece of code:
self._shouldWriteProfile: bool = not (globalVars.appArgs.secure or globalVars.appArgs.launcher)
If I cannot save a profile, how am I supposed to switch to it?
Again, I would like to know what does this variable do. I couldn't find documentation.
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.
From the user guide, advanced topics, command line options:
--secure
: Secure mode: disables Python console, profile features such as creation, deletion, renaming profiles etc., update check, some checkboxes in the welcome dialog and in general settings category (e.g. Use NVDA during sign-in, save configuration after exit etc.), as well as logviewer and logging features (used often in secure screens). Note also that this command will disable the possibility to save settings in system config and the gesture map will not be saved on the disk.
The primary use case is when NVDA is run as the "System" user, eg during sign-on. Then the secure argument is set, search for isSecureDesktop = desktopName == "Winlogon"
in nvda.pyw
.
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.
My bad. I should have looked first in the user guide instead ratter than spending more than 20 minutes inspecting the source code.
@lukaszgo1 thanks so much for pointing this out, it is now fixed and hopefully folks won a more elegant way than referencing [-1] every where.
@feerrenrut if there is anything else remaining let me know. Also, feel free to edit or ask some one to edit the documentation, I have done nearly my best but English is not my native language.
ab2830a
to
89dad35
Compare
The same way you're supposed to customize settings for the secure config i.e. by configuring triggers however you like in your standard NVDA copy and then copying them into the system config from the general settings. |
Hello @marlon-sousa I'm just discovering the details of this PR/feature. I would like to discuss two main points. Point 1: Whole dictionary override vs. entry overrideIf a dictionary is attached to a profile (e.g. myProfile), you have chosen to override the whole dictionary of the default profile. Since however the entries of the dictionary of the default profile may be useful everywhere, you have added the possibility to import entries from the dictionary of the default profile into the one of myProfile.
On the contrary, have you considered to override the default dictionary entry by entry, i.e. loading default dic and then loading myProfile's dic, overiding existing entryes? This approach would be quite similar to what is done with symbol files where English symbol file is first loaded and then local (e.g. Portuguese) symbol file is loaded on top of English one, overriding duplicate entries. Point 2: Voice dictionaryIn my view, the voice dictionary's goal is to fix the pronunciation of words that are not pronounced correctly by a voice. Thus I do not understand in which case tying a voice dictionary to a profile could be interesting. Thanks. |
Hello, The rationale for the decision follows:
I can set two specific voices to say words a given way when coding, and make sure this does not apply when reading common text. Voices can have some peculiarities, so I would specify by voice some fixes. Although this is rare, again, remember that these are advanced configurations after all, and thus this gives flexibility at almost no extra cost.
|
I partly disagree with it regarding the example of code reading, where you may need specific entries (in the code dic) to read some keywords or specific code syntax, but you may also need common word entries (from the default dic) to read correctly the comments in the code.
If today's choices do not prevent to implement full sync in the future, I would say that this PR is already a big progress for advanced users who want to configure each app with (or other profiles) with specific dic.
No. The use case where the user want to remove a word from default profile dic is not cover. In this PR, the user has to remove the entry in the default profile dic and in all the dic where he had imported default profile dic. If entry-sync solution is implemented, the user has to remove only the entry from the default profile dic.
It has no extra cost, except NVDA code main tenability. |
See test results for failed build of commit 668f59e316 |
Yep, we agree, and this was the item 2 of my previous response. To recap, since this is an advanced option, I opted to let in the user's hand when, whether and how the sync should be done, while offering an elegant alternative for a case which can be common enough some times.
Again, I don't disagree. The cost of removing manually entries for some dics comes with the benefit of putting sync on user's hands, greater power brings some times greater work. Usually, default dictionaries will grow more than shrink, specially because now it will be possible to add entries that just make sense for specific cases to specific dics. Personally, before the addon, I would find myself adding and removing entries because they would help me one day and get into my way on another, and I ended up adding and removing entries several times. Now, by separating stuff, I almost don't have to remove entries. But there is no right response, both points are valid.
I can't do it. I just think that once we segregate any kind of persistent dictionaries we should segregate them all based on profiles ratter than mixing everything. Temporary dic does not account because oit is not persistent. A user should be able to make the choice, but again this is only my point of view. |
Fixing upper case typo Co-authored-by: Cyrille Bougot <cyrille.bougot2@laposte.net>
8ce6307
to
cbcca13
Compare
Wouldn't it be more reasonable to include now some checkbox for "Ignore default
speech dictionary", and then do the entry based overriding that @CyrilleB79
suggests?
It seems like it could cover both use cases:
* It would allow any updates that occur in the default dictionary, to propagate
to any interested profiles without further tedious and error-prone user action.
* It would let profiles that specifically want to ignore the whole default
dictionary, which is probably the more rare case, to do so easily.
I have not read this PR's code, so I don't know where you would put such a
checkbox, but presumably in whatever dialog lets you manipulate the profile's
dictionary.
|
I dont know, I would need some more days but I am not aware of the time schedule. This could be ready by the weekend but if a merge needs to be made now I think everyone would bennefite from it the way it is ratter than waiting some months.brigado,
Marlon
… Em 24 de mar de 2021, à(s) 17:40, Luke Davis ***@***.***> escreveu:
Wouldn't it be more reasonable to include now some checkbox for "Ignore default
speech dictionary", and then do the entry based overriding that @CyrilleB79
suggests?
It seems like it could cover both use cases:
* It would allow any updates that occur in the default dictionary, to propagate
to any interested profiles without further tedious and error-prone user action.
* It would let profiles that specifically want to ignore the whole default
dictionary, which is probably the more rare case, to do so easily.
I have not read this PR's code, so I don't know where you would put such a
checkbox, but presumably in whatever dialog lets you manipulate the profile's
dictionary.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
From NV Access' point of view, we'd prefer to get the design of this right before it is introduced. Despite being very hard work to think it through, it is entirely possible to do so without making code changes. Once something like this is introduced it can be quite hard to change. I think this would be a very valuable feature for power users, and is worth the effort. In order to come to a common understanding I'd suggest outlining the key user stories you are each thinking about. It may be that they are different in some areas and are common in others. |
Is there the latest news about this PR? |
I think this is a PR that benefits everyone I turn on separate profiles during reading to use specific voice characters and voice speeds, and because of the specificity of the book content I need to correct some readings using the regular expression rules of the voice dictionary that don't need to be corrected in other scenarios, so the PR is useful to me. |
The question isn't whether it's useful. I think we all agree that it is
very useful.
I think the question is whether it is being done in the most effective and
user friendly way with this PR.
|
@marlon-sousa |
I have done several implementations that did not advance because there seem to be a disagreement om how the feature should be implemented. If there is a common, agreed spec I can happily code it, but I will avoid doing yet another implementatiom that will face the same disagreement and require yet another implementation and such and such again. So may be you guys should go backto the issue and specify what you want before I back to this pr again, in the mean time my addon with the very same implementation is working so the community is covered with the feature.
Obrigado,
Marlon
… Em 29 de ago. de 2021, à(s) 22:54, Rowen ***@***.***> escreveu:
@marlon-sousa
Do you have plans to update this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
I agree with your point of view, and I think that such a feature should be merged as soon as possible to benefit more people, and it should not be blocked by the ideas of a few people. |
I have not yet reviewed this PR because it seemed like discussion was ongoing. I'm going to close this PR to make it clear to others that someone new may take this on if they wish to. @marlon-sousa if I have misunderstood, and you would like to work through feedback on this implementation please comment for me to re-open. You may need to provide more reasoning on your design decisions. Understanding your reasoning will make the review process easier. |
Ref, this is not a matter of technical preferences, it is ratter a matter of functional specification. Some want dictionaries merged by default, some do not, the history is here to be read. As far as I am concerned, I have no decision power and I have coded an addon which works, as far as my opinion goes, in a reazonable way. I, however, do not care coding this feature working in anotrer way, but I need to know what way should this be before coding. So again qhis is not about variable naming, it is ratter about how the feature should work, cinse the way it is is not im accordance with some folks but in the other hand nobody comes with a final decision. In the meantime, again, the addon with the current implementation exists and is available for all to use.Obrigado,
Marlon
… Em 1 de set. de 2021, à(s) 06:29, Reef Turner ***@***.***> escreveu:
I have not yet reviewed this PR because it seemed like discussion was ongoing.
The last message from @marlon-sousa indicates that this has stalled and they won't be trying to progress it any further.
I'm going to close this PR to make it clear to others that someone new may take this on if they wish to.
In order to make progress, a clear plan and reasoning for the approach will need to be provided.
@marlon-sousa if I have misunderstood, and you would like to work through feedback on this implementation please comment for me to re-open. You may need to provide more reasoning on your design decisions. Understanding your reasoning will make the review process easier.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
No one can come up with the final decision, only differences, and in the end a useful PR was closed. Many contributors will be disappointed because of this. An open source community should have a more positive and open attitude. |
I understand there are different opinions on how to implement this feature. This isn't a project that NV Access has picked up, we are willing to provide advice on specific aspects as necessary, but the project must be driven by those developing it. This requires developing a functional specification, and getting stake holders to understand the trade-offs and agree to the approach. When there is a lot of discussion, this may also mean summarizing the current status, remaining questions etc. The easier you make it for us to understand and review your work, the more likely it is to be accepted. As I said, if you have lost interest in trying to get this integrated, that is fine, I will leave this closed. But from briefly looking over the discussion here, it doesn't seem like the matter is settled, if you'd like to push this forward I can re-open it. |
In case anyone wanting to take this PR again: According to NV Access, user stories should be written and lead to a clear description of the resulting design with appropriate justifications. And #11005 (comment) from @XLTechie which seemed to provide an interesting design suggestion has remained unanswered:
|
Link to issue number: #3656
Summary of the issue:
NVDA dictionaries are powerful, offering great features such as regular expression processing. However, there is currently no way of applying dictionary processing conditionally.
The way NVDA applies other conditional settings, such as document formatting and others, is through the use of profiles.
Profiles are groups of settings that can, together, be applied conditionally to the screen reader.
However, right until this pull request, dictionary processing wasn't taking in consideration the active profile.
Description of how this pull request fixes the issue:
This pull request implements profile context when processing and creating / editing dictionaries. For a description in terms of behavior, see #3656 (comment)
Bellow is a summary of implementation changes:
Config.ConfigManager
gui.MainFrame
gui.SettingsDialogs.DictionaryDialog
speechDictHandler module
speechDictHandler.SpeechDict class
dictFormatUpgrade module
Dictionary files
This PR Creates specific dictionary for profiles on %appdata%\nvda\speechDicts, where the dictionaries are currently saved. Default dictionary (for default profile) is placed on the root folder, as usual. The voice dictionaries for the default profile reside on voiceDicts.v1 subdirectory.
The profile specific dictionaries are placed in subdirectories with the profile name, inside the speechDicts directory.
The default.dict dictionary for each profile is placed on the profile root directory, while voice dictionaries are placed inside the voiceDicts.v1 for each profile.
Migrations are made for default dictionaries only, following the procedure as before. New dictionaries are placed on profile subfolders, as stated avobe.
Why dictionaries are saved on speechDicts folder instead of on profiles folder
This decision was taken because, although a "profiles" folder already exists, profiles aren't created within subfolders. Instead, they are single files scattered at the same folder.
Ideally, each profile should deserve its own subdirectory where the configuration itself plus additional files could live, but taking this step now would envolve a migration tool and discussion with the community, which would be too much for this functionality.
So, by now, dictionary files will reside on speechDicts subfolder and be separated by profiles, and if and when the profiles folder get under discussion we can write a migration tool.
Testing performed:
All tests above can be performed with voice specific dictionaries. The behaviour will be the same.
Known issues with pull request:
None at this time
Change log entry:
Section: New features, Changes, Bug fixes