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

Ensure Onecore synthDriver still loads when the configured voice is missing #7999

Merged
merged 4 commits into from Mar 28, 2018

Conversation

Projects
None yet
4 participants
@michaelDCurran
Contributor

michaelDCurran commented Feb 14, 2018

Link to issue number:

Fixes #7553

Summary of the issue:

If the Onecore synth driver is set to a particular voice, and then that voice is uninstalled (by the user or Windows itself), the synth driver will fail to load.

Description of how this pull request fixes the issue:

When the Onecore synth driver is initialized, it now sets a default voice that it knows definitely exists. To choose this voice, it tries to match the user's Windows language as best as possible. Failing this, it just chooses the first valid voice available in the list of voices.
To get the user's Windows language, some code has been split out from languageHandler.setLanguage into a new languageHandler.getWindowsLanguage function.
The refactoring of the voice handling for the Onecore synth driver also means that now changing the voice is much faster than it used to be, as all voices are only fetched once.

Testing performed:

  • Loaded the Onecore synth driver, switched to a particular voice, and saved configuration.
  • Manually uninstalled that voice, and started NVDA again. NVDA defaulted back to the first voice for my configured language. Previously it would have failed to load the driver.

Known issues with pull request:

Change log entry:

Bug fixes:

  • The Windows Onecore Voices synth driver no longer fails to load if the configured voice has been uninstalled.
  • Changing voices in the Windows Onecore Voices synth driver is now a lot faster.

michaelDCurran added some commits Feb 14, 2018

languageHandler: split out the code that fetches the user's Windows l…
…anguage from setLanguage into a new getWindowsLanguage function so that it can be called separately.
Onecore synthDriver: Ensure it does not fail to load if the configure…
…d voice has been uninstalled. Also majorly speed up changing voices.

Specifically:
* When fetching the list of voices from Onecore speech, also tet the language for each voice.
* Choose the best default voice on initialization, trying to match the user's Windows language as best as possible. If this fails, just choose the first valid voice it can find.
* Validate and filter the list of available voices as early as possible. This ensures that available voices are not fetched more than once, as the actual call to get the voices from Onecore can sometimes take up to 600 ms or more. thus this was majorly slowing down changing voices.

@michaelDCurran michaelDCurran requested a review from feerrenrut Feb 14, 2018

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Feb 15, 2018

Collaborator

I've tested the OneCore synthesizer on Windows Server 2016, which had no voices installed by that time. Initializing the synth failed with a weird WindowsError. I'd say this was expected, but may be you could check for available in the check function while at it?

Collaborator

leonardder commented Feb 15, 2018

I've tested the OneCore synthesizer on Windows Server 2016, which had no voices installed by that time. Initializing the synth failed with a weird WindowsError. I'd say this was expected, but may be you could check for available in the check function while at it?

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Feb 18, 2018

Contributor
Contributor

michaelDCurran commented Feb 18, 2018

Show outdated Hide outdated source/languageHandler.py Outdated
Show outdated Hide outdated source/languageHandler.py Outdated
Show outdated Hide outdated source/languageHandler.py Outdated
Show outdated Hide outdated source/synthDrivers/oneCore.py Outdated
language=language.replace('-','_')
return VoiceInfo(ID,name,language=language)
def _getAvailableVoices(self):

This comment has been minimized.

@feerrenrut

feerrenrut Mar 6, 2018

Contributor

I dont think this will be cached by default, is that a problem? It seems like this might be a slow operation.

@feerrenrut

feerrenrut Mar 6, 2018

Contributor

I dont think this will be cached by default, is that a problem? It seems like this might be a slow operation.

This comment has been minimized.

@michaelDCurran

michaelDCurran Mar 6, 2018

Contributor

It is very slow, yes. However, the SynthDriver base class has an availableVoices property that by default calls _getAvailableVoices. This property is cached. As long as we never call _getAvailableVoices we are okay.

@michaelDCurran

michaelDCurran Mar 6, 2018

Contributor

It is very slow, yes. However, the SynthDriver base class has an availableVoices property that by default calls _getAvailableVoices. This property is cached. As long as we never call _getAvailableVoices we are okay.

"""
Fetches the locale name of the user's configured language in Windows.
"""
windowsLCID=ctypes.windll.kernel32.GetUserDefaultUILanguage()

This comment has been minimized.

@leonardder

leonardder Mar 6, 2018

Collaborator

Now we are no longer required to support Windows XP, I'd like to suggest using GetUserPreferredUILanguages

@leonardder

leonardder Mar 6, 2018

Collaborator

Now we are no longer required to support Windows XP, I'd like to suggest using GetUserPreferredUILanguages

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Mar 6, 2018

Contributor
Contributor

michaelDCurran commented Mar 6, 2018

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Mar 6, 2018

Collaborator

@michaelDCurran commented on 6 mrt. 2018 10:57 CET:

What do you see as the advantages to using GetUserPreferredUILanguages?
If we were only interested in the first language, I assume there is no
advantage to this function over UserDefaultUILanguage?

The advantage of using GetUserPreferredUILanguages is that you don't have to do a subsequent call to LCIDToLocaleName. The comments above that call suggest that it is not yet clear what to do by default, either quering the locale id in the locale.windows_locale dictionary, or calling LCIDToLocaleName.

As part of #7949, I switched from GetDateFormat to GetDateFormatEx, because of Microsoft stating their preference of using a locale name over an locale id. From the GetDateFormat page is the following quote:

Note: For interoperability reasons, the application should prefer the GetDateFormatEx function to GetDateFormat because Microsoft is migrating toward the use of locale names instead of locale identifiers for new locales. Any application that will be run only on Windows Vista and later should use GetDateFormatEx.

Note that I'm only suggesting this as an alternative, I'd be perfectly ok with it if you keep it as it is.

Collaborator

leonardder commented Mar 6, 2018

@michaelDCurran commented on 6 mrt. 2018 10:57 CET:

What do you see as the advantages to using GetUserPreferredUILanguages?
If we were only interested in the first language, I assume there is no
advantage to this function over UserDefaultUILanguage?

The advantage of using GetUserPreferredUILanguages is that you don't have to do a subsequent call to LCIDToLocaleName. The comments above that call suggest that it is not yet clear what to do by default, either quering the locale id in the locale.windows_locale dictionary, or calling LCIDToLocaleName.

As part of #7949, I switched from GetDateFormat to GetDateFormatEx, because of Microsoft stating their preference of using a locale name over an locale id. From the GetDateFormat page is the following quote:

Note: For interoperability reasons, the application should prefer the GetDateFormatEx function to GetDateFormat because Microsoft is migrating toward the use of locale names instead of locale identifiers for new locales. Any application that will be run only on Windows Vista and later should use GetDateFormatEx.

Note that I'm only suggesting this as an alternative, I'd be perfectly ok with it if you keep it as it is.

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Mar 6, 2018

Contributor
Contributor

michaelDCurran commented Mar 6, 2018

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Mar 6, 2018

Collaborator

@michaelDCurran commented on 6 Mar 2018, 18:32 CET:

Do you know if there is a function to convert an lcid to a localeName?

Yes, this is LCIDToLocaleName and it is already used in the languageHandler code.

Currently we map from lcid to localeName with a
hard-coded list.

Yes, but when that fails, the code falls back to LCIDToLocaleName.

Either way though, I'd be tempted to leave changing the functions at all
for this particular PR as I'm not totally convinced that Windows 7
actually contains a complete list of locale names. We'd have to write
some kind of test for that I guess, even if it was just to prove it once.

Hmm yes. But that ought to be tested on a Windows 7 system. I agree, let's keep it as it is for this particular pr.

Collaborator

leonardder commented Mar 6, 2018

@michaelDCurran commented on 6 Mar 2018, 18:32 CET:

Do you know if there is a function to convert an lcid to a localeName?

Yes, this is LCIDToLocaleName and it is already used in the languageHandler code.

Currently we map from lcid to localeName with a
hard-coded list.

Yes, but when that fails, the code falls back to LCIDToLocaleName.

Either way though, I'd be tempted to leave changing the functions at all
for this particular PR as I'm not totally convinced that Windows 7
actually contains a complete list of locale names. We'd have to write
some kind of test for that I guess, even if it was just to prove it once.

Hmm yes. But that ought to be tested on a Windows 7 system. I agree, let's keep it as it is for this particular pr.

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Mar 6, 2018

Contributor
Contributor

michaelDCurran commented Mar 6, 2018

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Mar 6, 2018

Contributor

@feerrenrut: review actions have been addressed.

Contributor

michaelDCurran commented Mar 6, 2018

@feerrenrut: review actions have been addressed.

michaelDCurran added a commit that referenced this pull request Mar 12, 2018

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Mar 12, 2018

Contributor

Incubating to give people a good chance to test before 2018.2. But @feerrenrut not entirely sure if you were had finished with this. I addressed all your review comments.

Contributor

michaelDCurran commented Mar 12, 2018

Incubating to give people a good chance to test before 2018.2. But @feerrenrut not entirely sure if you were had finished with this. I addressed all your review comments.

@michaelDCurran michaelDCurran merged commit 9134677 into master Mar 28, 2018

@nvaccessAuto nvaccessAuto added this to the 2018.2 milestone Mar 28, 2018

@feerrenrut

I looked over this again, and I am happy with the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment