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

michaelDCurran · 2018-02-14T22:30:23Z

Link to issue number:

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.

…anguage from setLanguage into a new getWindowsLanguage function so that it can be called separately.

…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.

LeonarddeR · 2018-02-15T12:03:50Z

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 · 2018-02-18T22:28:54Z

Checking for availableVoices takes about 600 ms on some machines. Doing this in check will be not practical.

feerrenrut · 2018-03-06T04:52:33Z

source/languageHandler.py

+		localeName=locale.windows_locale[windowsLCID]
+	except KeyError:
+		# #4203: some locale identifiers from Windows 8 don't exist in Python's list.
+		# Therefore use window' own function to get the locale name.


typo: misssing 's' in "window's"

feerrenrut · 2018-03-06T04:53:54Z

source/languageHandler.py

+		# #4203: some locale identifiers from Windows 8 don't exist in Python's list.
+		# Therefore use window' own function to get the locale name.
+		# Eventually this should probably be used all the time.
+		buf=ctypes.create_unicode_buffer(32)


Perhaps use a variable for this value of 32 something like buffer_size, then reuse it below.

feerrenrut · 2018-03-06T04:55:44Z

source/languageHandler.py

+		# Eventually this should probably be used all the time.
+		buf=ctypes.create_unicode_buffer(32)
+		try:
+			ctypes.windll.kernel32.LCIDToLocaleName(windowsLCID,buf,32,0)


Could do the same with 0 name it as dwFlags this will get optimised away, and helps a reader to know what the parameter is without looking up the function.

feerrenrut · 2018-03-06T04:58:57Z

source/synthDrivers/oneCore.py

+	def _getDefaultVoice(self):
+		"""
+		Finds the best available voice that can be used as a default.
+		It first tries finding a voice with the same language and country as the user's configured Windows language (E.g. en_AU), else one that matches just the language (E.g. en), else simply the first available.


Could you split this line into two shorter ones?

feerrenrut · 2018-03-06T07:53:19Z

source/synthDrivers/oneCore.py

+		language=language.replace('-','_')
+		return VoiceInfo(ID,name,language=language)
+
+	def _getAvailableVoices(self):


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

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.

LeonarddeR · 2018-03-06T08:04:25Z

source/languageHandler.py

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


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

michaelDCurran · 2018-03-06T09:57:26Z

What do you see as the advantages to using GetUserPreferredUILanguages? Are you imagining that for Onecore speech we might try each language until we get a voice? If we were only interested in the first language, I assume there is no advantage to this function over UserDefaultUILanguage?

LeonarddeR · 2018-03-06T10:38:44Z

@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 · 2018-03-06T17:32:05Z

Do you know if there is a function to convert an lcid to a localeName? It would be nice if we are going to switch to the other function that we also allow Windows itself to convert from lcid to locale name, such as when receiving the language ID from MS Word, so that the results are the same for both functions. Currently we map from lcid to localeName with a hard-coded list. 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.

LeonarddeR · 2018-03-06T18:39:44Z

@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 · 2018-03-06T18:51:15Z

Err... don't mind me. I was commenting in the middle of the night :) But yes, in future we could switch to using getUserPreferredUILanguages and also remove the hard-coded stuff from lcidToLocaleName.

michaelDCurran · 2018-03-06T18:58:10Z

@feerrenrut: review actions have been addressed.

michaelDCurran · 2018-03-12T08:20:16Z

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.

feerrenrut

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

michaelDCurran added 2 commits February 15, 2018 06:43

languageHandler: split out the code that fetches the user's Windows l…

ec8b95e

…anguage from setLanguage into a new getWindowsLanguage function so that it can be called separately.

michaelDCurran requested a review from feerrenrut February 14, 2018 22:30

michaelDCurran force-pushed the i7553 branch from 682b50b to a1e2a6d Compare February 15, 2018 19:43

feerrenrut reviewed Mar 6, 2018

View reviewed changes

LeonarddeR reviewed Mar 6, 2018

View reviewed changes

michaelDCurran added 2 commits March 7, 2018 04:52

Address review actions.

b8b80a4

Merge branch 'master' into i7553

d7f6ef5

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

Incubates #7999

b2a2f81

nvaccessAuto added the incubating label Mar 12, 2018

nvaccessAuto assigned michaelDCurran Mar 12, 2018

michaelDCurran merged commit 9134677 into master Mar 28, 2018

nvaccessAuto removed the incubating label Mar 28, 2018

nvaccessAuto added this to the 2018.2 milestone Mar 28, 2018

feerrenrut reviewed Apr 11, 2018

View reviewed changes

seanbudd mentioned this pull request Jul 28, 2021

Try next synth if oneCore consistently fails #12647

Merged

8 tasks

seanbudd deleted the i7553 branch July 29, 2021 07:56

seanbudd mentioned this pull request Aug 3, 2021

Use GetUserPreferredUILanguages for determining Windows Languages to use #12707

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

michaelDCurran commented Feb 14, 2018

LeonarddeR commented Feb 15, 2018

michaelDCurran commented Feb 18, 2018 via email

feerrenrut Mar 6, 2018

feerrenrut Mar 6, 2018

feerrenrut Mar 6, 2018

feerrenrut Mar 6, 2018

feerrenrut Mar 6, 2018

michaelDCurran Mar 6, 2018

LeonarddeR Mar 6, 2018

michaelDCurran commented Mar 6, 2018 via email

LeonarddeR commented Mar 6, 2018

michaelDCurran commented Mar 6, 2018 via email

LeonarddeR commented Mar 6, 2018

michaelDCurran commented Mar 6, 2018 via email

michaelDCurran commented Mar 6, 2018

michaelDCurran commented Mar 12, 2018

feerrenrut left a comment

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

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

Conversation

michaelDCurran commented Feb 14, 2018

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

LeonarddeR commented Feb 15, 2018

michaelDCurran commented Feb 18, 2018 via email

feerrenrut Mar 6, 2018

Choose a reason for hiding this comment

feerrenrut Mar 6, 2018

Choose a reason for hiding this comment

feerrenrut Mar 6, 2018

Choose a reason for hiding this comment

feerrenrut Mar 6, 2018

Choose a reason for hiding this comment

feerrenrut Mar 6, 2018

Choose a reason for hiding this comment

michaelDCurran Mar 6, 2018

Choose a reason for hiding this comment

LeonarddeR Mar 6, 2018

Choose a reason for hiding this comment

michaelDCurran commented Mar 6, 2018 via email

LeonarddeR commented Mar 6, 2018

michaelDCurran commented Mar 6, 2018 via email

LeonarddeR commented Mar 6, 2018

michaelDCurran commented Mar 6, 2018 via email

michaelDCurran commented Mar 6, 2018

michaelDCurran commented Mar 12, 2018

feerrenrut left a comment

Choose a reason for hiding this comment