Drop unsupported language switching commands for OneCore by seanbudd · Pull Request #13779 · nvaccess/nvda

seanbudd · 2022-06-09T02:50:51Z

Link to issue number:

Summary of the issue:

OneCore keeps a cache of voices for NVDA in the registry.
OneCore fails to automatic language switch when remnants of a previously installed OneCore language exist in the cache.
This causes speech to not be announced.

This is only an issue for installed copies, as portable copies bypass the NVDA OneCore cache.

Description of how this pull request fixes the issue:

Drop the language change command if a given language is unsupported by OneCore.

Testing strategy:

Manual testing

Using the following HTML sample, test language switching.
Replace the language code "zh" as appropriate.

data:text/html,<p>Hello</p><p lang="zh">world</p>.

Using installed oneCore languages 'en_au', 'fr_fr', 'en_us', 'zh_hk'

Automatic language switching is successful for the following language codes:

"en" (to en_US)
"en_AU"
"en-AU"
"en-au"
"en_AU"
"fr-fr"
"fr" (to fr_FR)
"zh" (to zh_hk)
"zh_hk"

Automatic language switching reverts to the default voice for the following language codes:

"FakeLang_fakeLocale" (illegitimate language)
"en-CA" (unsupported locale)
"ta" (unsupported language)

Known issues with pull request:

None

Change log entries:

Bug fixes

- Fix automatic language switching failing with OneCore when trying to switch to a formerly installed language. (#13732)

Code Review Checklist:

Pull Request description:
- description is up to date
- change log entries
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
API is compatible with existing add-ons.
Documentation:
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English

AppVeyorBot · 2022-06-09T04:34:15Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/q8hib590o91gqcjh/artifacts/output/nvda_snapshot_pr13779-25531,700ad289.exe

See test results for failed build of commit 700ad28992

feerrenrut

Overall looks good.

feerrenrut · 2022-06-09T04:51:39Z


 import os
-import sys
+from typing import Any, Callable, Generator, List, Optional, Set, Tuple, Union


We get better diffs for subsequent updates if these are one per line:

Suggested change

from typing import Any, Callable, Generator, List, Optional, Set, Tuple, Union

from typing import (

Any,

Callable,

Generator,

List,

Optional,

Set,

Tuple,

Union,

)

Fixed in e630724

feerrenrut · 2022-06-09T04:55:08Z

+			defaultLanguage: str,
+			availableLanguages: Set[str],


Please add docs for these, examples are specifically helpful. Are they expected to be 'en' or 'en-au' etc.

These are both fully qualified OneCore language codes, e.g. "en_US".

feerrenrut · 2022-06-09T04:56:02Z

+			rate: float,
+			pitch: float,
+			volume: float,


I assume these are expected to be 0->1 values, if you can confirm please add to the doc string.

This is actually from 0-100, but supports decimal changes

feerrenrut · 2022-06-09T05:01:21Z

+	_ocSpeechToken: Optional[ctypes.POINTER]
+	_queuedSpeech: List[Union[str, Tuple[Callable[[ctypes.POINTER, float], None], float]]]


Shouldn't this typing be in the init method? Doesn't having it here indicate these are expected to be used as class variables?

Fixed in e630724

CyrilleB79 · 2022-06-09T19:42:06Z

Hello
I have just tested nvda_snapshot_pr13779-25537,4aba6cc9.exe:

From the launcher, the test is OK. I get a warning for each English text, what is normal due to my broken English voices.
From a portable copy, I get a critical error: nvda-old.log
However, the issue is also present on last alpha portable (nvda_snapshot_alpha-25538,f3445949.exe): cf. nvda-old.log

seanbudd · 2022-06-10T00:02:14Z

@CyrilleB79 - it looks like you've found a new regression in 2022.2 for portable copies of NVDA.
This needs a new issue and investigation.

feerrenrut

Assuming the build passes.

seanbudd added 2 commits June 9, 2022 12:31

Drop unsupported language switching commands

9ff13db

add support for lang without locale

be15db0

seanbudd requested a review from a team as a code owner June 9, 2022 02:50

seanbudd requested a review from feerrenrut June 9, 2022 02:50

add information to isVoiceValid

1261b8e

seanbudd added 2 commits June 9, 2022 14:37

fix lint

755afa7

Add information on portable/installed

5d04085

feerrenrut suggested changes Jun 9, 2022

View reviewed changes

address review comments

e630724

seanbudd requested a review from feerrenrut June 10, 2022 00:02

feerrenrut approved these changes Jun 10, 2022

View reviewed changes

update changes

f829a53

seanbudd mentioned this pull request Jun 10, 2022

Start 2022.3 dev cycle #13785

Merged

3 tasks

Merge branch 'master' into fixOneCoreLangSwitching

ffb0549

feerrenrut approved these changes Jun 10, 2022

View reviewed changes

seanbudd merged commit 4eb30c9 into master Jun 10, 2022

seanbudd deleted the fixOneCoreLangSwitching branch June 10, 2022 07:36

nvaccessAuto added this to the 2022.2 milestone Jun 10, 2022

CyrilleB79 mentioned this pull request Jun 10, 2022

NVDA portable crashes #13786

Closed

		_ocSpeechToken: Optional[ctypes.POINTER]
		_queuedSpeech: List[Union[str, Tuple[Callable[[ctypes.POINTER, float], None], float]]]

Uh oh!

Conversation

seanbudd commented Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Manual testing

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Jun 9, 2022

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrilleB79 commented Jun 9, 2022

Uh oh!

seanbudd commented Jun 10, 2022

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seanbudd commented Jun 9, 2022 •

edited

Loading