Honor useSpellingFunctionality parameter when reporting shortcuts by CyrilleB79 · Pull Request #15572 · nvaccess/nvda

CyrilleB79 · 2023-10-03T21:12:48Z

Link to issue number:

Closes #15566
Fix-up of #14900 / #15373.

Summary of the issue:

In #14900 / #15373., spelling mode was used (CharacterModeCommand) no matter the state of the "Use spelling functionality" option.

The use spelling functionality exists because some old SAPI synth do not support spelling mode correctly. For them, this option needs to be honored.

Description of user facing changes

Use spelling functionality option will now be honored when reporting shortcuts.

Description of development approach

See logic in the code.
If use spelling functionality option is disabled, the key is just reported as is.

Testing strategy:

Manual check:

Checked that reporting shortcuts is still OK
Checked in the log that CharacterModeCommand is used only when the "Use spelling functionality" option is enabled.

Known issues with pull request:

None

Change log

None: unreleased issue.

Code Review Checklist:

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

AppVeyorBot · 2023-10-03T22:03:41Z

PASS: Translation comments check.
FAIL: Unit tests. See test results for more information.
PASS: Lint check.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/1d0omui1vmlevpqj/artifacts/output/nvda_snapshot_pr15572-29504,2daba6ed.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.0,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 7.8,
FINISH_END 0.1

See test results for failed build of commit 2daba6eda6

CyrilleB79 · 2023-10-06T09:20:56Z

@seanbudd or any other reviewer:
I have had to fix the unit tests. Not sure however that function patching is the correct way to do this. Let me know if another approach is required.

lukaszgo1

Pretty nice overall!

lukaszgo1 · 2023-10-06T14:56:43Z

 class Test_getKeyboardShortcutSpeech(unittest.TestCase):

+	@classmethod
+	def setUpClass(cls):


Have you considered using unittest.mock.patch for replacing the function during the tests? When used as a decorator on these test classes you'll have much less of boilerplate code.

lukaszgo1 · 2023-10-06T14:58:50Z

It would be nice to have additional unit test confirming that when shouldUseSpellingFunctionality returns False, character mode commands are not used.

…r when Use spelling functionality is disabled.

CyrilleB79 · 2023-10-07T14:33:22Z

@lukaszgo1, many thanks for the review!

I am not used to unittest.mock.patch (and more generally nor to mock), but that's a really nice tool. Could you please double check that I am using it correctly?

AppVeyorBot · 2023-10-07T14:57:08Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/6y2xnbo5uxoahl6d/artifacts/output/nvda_snapshot_pr15572-29549,19e37f8b.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 21.7,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 4.8,
FINISH_END 0.1

See test results for failed build of commit 19e37f8b1f

CyrilleB79 · 2023-10-07T19:57:19Z

@lukaszgo1, I think you are a user of MathPlayer.
Could you manual test the MathPlayer code that has been modified in this PR?

AppVeyorBot · 2023-10-07T20:23:41Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/9qbjf49du9qy7mgw/artifacts/output/nvda_snapshot_pr15572-29551,eb162c00.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 21.5,
TESTSETUP_START 0.0,
TESTSETUP_END 0.2,
TEST_START 0.0,
TEST_END 4.9,
FINISH_END 0.2

See test results for failed build of commit eb162c0024

seanbudd

Thanks @CyrilleB79

AppVeyorBot · 2023-10-09T01:32:34Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ovqjfv9lk6rx47jj/artifacts/output/nvda_snapshot_pr15572-29560,8f1fa7a3.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.2,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 7.8,
FINISH_END 0.1

See test results for failed build of commit 8f1fa7a337

CyrilleB79 added 2 commits October 3, 2023 22:59

Honor useSpellingFunctionality parameter when reporting shortcuts

a0df691

Also honor the option in MathPlayer

7df3a5e

CyrilleB79 mentioned this pull request Oct 3, 2023

Updated shortcut key reporting doesn't respect use spelling functionality option #15566

Closed

CyrilleB79 added 2 commits October 6, 2023 10:39

Merge branch 'master' into fixNoSpelling

1856833

Fixed unit tests

87ad0ea

CyrilleB79 marked this pull request as ready for review October 6, 2023 09:18

CyrilleB79 requested a review from a team as a code owner October 6, 2023 09:18

CyrilleB79 requested a review from seanbudd October 6, 2023 09:18

lukaszgo1 reviewed Oct 6, 2023

View reviewed changes

Rewrite UT with unittest.mock.patch decorator and add an test case fo…

2641927

…r when Use spelling functionality is disabled.

Fix

89b5f40

Bump CI

9c09c6b

seanbudd reviewed Oct 9, 2023

View reviewed changes

Comment thread source/speech/shortcutKeys.py

Comment thread tests/unit/test_speechShortcutKeys.py Outdated

Comment thread tests/unit/test_speechShortcutKeys.py Outdated

Apply suggestions from code review

112059a

seanbudd approved these changes Oct 9, 2023

View reviewed changes

seanbudd reviewed Oct 9, 2023

View reviewed changes

Comment thread tests/unit/test_speechShortcutKeys.py Outdated

fix lint

90dfb9e

seanbudd merged commit 3284d85 into nvaccess:master Oct 9, 2023

nvaccessAuto added this to the 2024.1 milestone Oct 9, 2023

CyrilleB79 deleted the fixNoSpelling branch October 9, 2023 17:36

Uh oh!

Conversation

CyrilleB79 commented Oct 3, 2023

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Change log

Code Review Checklist:

Uh oh!

AppVeyorBot commented Oct 3, 2023

Uh oh!

CyrilleB79 commented Oct 6, 2023

Uh oh!

lukaszgo1 left a comment

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

CyrilleB79 commented Oct 7, 2023

Uh oh!

AppVeyorBot commented Oct 7, 2023

Uh oh!

CyrilleB79 commented Oct 7, 2023

Uh oh!

AppVeyorBot commented Oct 7, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Oct 9, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants