Speech Dictionary dialog: Add a "Remove all" button (#11802) by JulienCochuyt · Pull Request #12385 · nvaccess/nvda

JulienCochuyt · 2021-05-09T23:30:07Z

Link to issue number:

Fixes #11802

Summary of the issue:

The Speech Dictionary dialog lacks a "Remove all" button to ease clearing a whole dictionary.

Description of how this pull request fixes the issue:

Split both the DictionaryDialog and DictionaryEntryDialog from gui.settingsDialogs to a new dedicated gui.speechDict
Redeem copyright holders' names by blaming the history of source/gui/settingsDialogs.py
Linted the result in a separate commit for easier review and history tracking
Added a "Remove all" coherent with the "Restore to factory default" as found on the Input Gestures dialog

Testing strategy:

On a source copy, filled and emptied various default/voice/temp dictionaries.

Known issues with pull request:

Interestingly, the "Remove" button (removing a single entry) was historically designed to handle the removal of all the selected entries in a multi-selection list control. This idea seems to have been abandoned as the list control is explicitly set to support only a single selection - presumably for easier user interaction.

Change log entries:

New features
The Speech Dictionary dialog now features a "Remove all" button to help clear a whole dictionary.

Code Review Checklist:

AppVeyorBot · 2021-05-09T23:38:47Z

Translation comments missing or unexpectedly included.
Build (for testing PR)

See test results for failed build of commit 7481ce6eb1

Redeem copyright holders' names by blaming the history of `source/gui/settingsDialogs.py`

Remove "Dictionary Entry Error" from the expected `checkPot` failures

XLTechie · 2021-05-10T02:37:54Z

This was in my list of things to do as well, so happy to see it.

Added a "Remove all" coherent with the "Restore to factory default" as

found on the Input Gestures dialog Can you talk some about why you have named the button as you have, instead of "Restore to factory defaults" like the Input Gestures dialog?

JulienCochuyt · 2021-05-10T06:04:02Z

@XLTechie wrote:

Can you talk some about why you have named the button as you have, instead of "Restore to factory defaults" like the Input Gestures dialog?

Sure. The two possible alternative labels were actually proposed by @Qchristensen in #11802.
In the Input Gestures dialog, the action is not to remove all bindings, but really to remove all additions/changes/deletions made by the user to the standard bindings as they are listed in the user guide. After restoring to factory defaults, there still are bindings in the list, but only the standard ones. Furthermore, due to how things are implemented under the hood, this action cannot be reverted and the user is warned about it in the confirmation prompt.
On the other hand, in speech dictionaries, you really just empty the list. The list does not come pre-filled with default entries when you first install NVDA. The actual speech still gets affected by the internal dictionaries of each synthesizer/voice, but these never appear in the list and can only be superseded, not edited nor removed. Furthermore, this action can here be undone by simply dismissing the dialog by pressing Cancel afterwards.
By stating I've made them coherent with each other, I was more referring to visual layout: Both buttons appear right aligned under the list they act upon, separated from the other single-entry-action buttons which are left aligned.

Do you agree?

XLTechie · 2021-05-10T07:26:21Z

Julien Cochuyt wrote:

Do you agree?

Yes, that reasoning makes sense. It was what I expected, but in case you had another aspect to the consideration, I wanted to hear it. That aspect turns out to be that it can be reverted by the cancel button. I hadn't considered that, but I like it, and the button name is good given how it is going to operate in my opinion. Thank you for doing this work.

lukaszgo1 · 2021-05-19T10:05:51Z

-		self.typeRadioBox.SetSelection(DictionaryEntryDialog.TYPE_LABELS_ORDERING.index(type))
-
-
-class DictionaryDialog(SettingsDialog):


While I agree DictionaryDialog should be eventually moved out of gui\settingsDialogs it would break backwards compatibility - various add-ons expect it either in the gui as it is star imported there or in the gui\settingsDialogs. Unles you expect this to be merged before 2021.1 is finalized this part should be reverted IMO.

On the other hand, 2021.1 already brings API changes.
Still, I restored as of 579cbf6 module-level import in gui of DictionaryDialog, as late-importing did not bring much benefit towards the wider API change.
@feerrenrut, may we please have your view here?

I don't believe this will be in 2021.1, unless another dev sees reason to target this to beta instead. I think the a change like in 579cbf6 is good, and should be done to maintain backwards compatibility. A comment should be added though to note this is a deprecation and should only be removed with a .1 release.

we can import both of these removed classes into this module for now to maintain compatibility until 2022.1. Linting warnings can be ignored with a noqa comment to remove this in 2022.1

Done as of 143bcde and fec7595.

…hDictionaries

Re: nvaccess#12385 (comment)

seanbudd · 2021-05-24T04:52:13Z

Hi Julien, Reverting this to a draft as it breaks API compatibility

Re: nvaccess#12385 (comment)

…hDictionaries

JulienCochuyt · 2021-05-24T09:36:06Z

@seanbudd wrote:

Hi Julien, Reverting this to a draft as it breaks API compatibility

API compatibility issues addressed.
Shall I re-set as ready for review?

AppVeyorBot · 2021-05-24T09:57:40Z

System test failure
Build (for testing PR)

See test results for failed build of commit 02c99d6a11

…hDictionaries

seanbudd · 2021-06-22T05:57:00Z

+# Copyright (C) 2006-2021 NV Access Limited, Michael Curran, Peter Vágner, Mesar Hameed, Joseph Lee,
+# Aaron Cannon, Reef Turner, Ethan Holliger, Julien Cochuyt, Thomas Stivers, Cyrille Bougot


This needs to be updated, referencing the discussions here: #12386

seanbudd · 2022-02-01T01:01:52Z

Thanks @JulienCochuyt for your work on this. I am closing this PR as it has been followed up in #13294

nvaccess#13294 Link to issue number: Fixes nvaccess#11802 Supersedes nvaccess#12385 Summary of the issue: The Speech Dictionary dialog lacks a "Remove all" button to ease clearing a whole dictionary. Description of how this pull request fixes the issue: Split both the DictionaryDialog and DictionaryEntryDialog from gui.settingsDialogs to a new dedicated gui.speechDict Redeem copyright holders' names by blaming the history of source/gui/settingsDialogs.py Linted the result in a separate commit for easier review and history tracking Added a "Remove all" coherent with the "Restore to factory default" as found on the Input Gestures dialog

…er after abandoned #12385 Fixes #11802 Supersedes #12385 Summary of the issue: The Speech Dictionary dialog lacks a "Remove all" button to ease clearing a whole dictionary. Description of how this pull request fixes the issue: Split both the DictionaryDialog and DictionaryEntryDialog from gui.settingsDialogs to a new dedicated gui.speechDict Redeem copyright holders' names by blaming the history of source/gui/settingsDialogs.py Linted the result in a separate commit for easier review and history tracking Added a "Remove all" coherent with the "Restore to factory default" as found on the Input Gestures dialog

JulienCochuyt requested a review from a team as a code owner May 9, 2021 23:30

JulienCochuyt requested a review from michaelDCurran May 9, 2021 23:30

JulienCochuyt added 3 commits May 10, 2021 01:50

Speech Dictionary dialogs: Split into their on gui.speechDict module

bef5f0b

Redeem copyright holders' names by blaming the history of `source/gui/settingsDialogs.py`

Follow-up of 7a1c015: Lint the result of the split

dfacd78

Remove "Dictionary Entry Error" from the expected `checkPot` failures

Speech Dictionary dialog: Add a "Remove all" button (nvaccess#11802)

5d1b14a

JulienCochuyt force-pushed the i11802-resetSpeechDictionaries branch from f0291dd to 5d1b14a Compare May 9, 2021 23:52

lukaszgo1 suggested changes May 19, 2021

View reviewed changes

JulienCochuyt added 2 commits May 19, 2021 13:20

Merge remote-tracking branch 'upstream/master' into i11802-resetSpeec…

2866542

…hDictionaries

Speech Dictionary dialogs: import at module level in gui

579cbf6

Re: nvaccess#12385 (comment)

seanbudd marked this pull request as draft May 24, 2021 04:52

JulienCochuyt added 3 commits May 24, 2021 11:28

Restore backward compatibility and add deprecation notice

143bcde

Re: nvaccess#12385 (comment)

Merge remote-tracking branch 'upstream/master' into i11802-resetSpeec…

9412677

…hDictionaries

Fix linting

fec7595

Merge remote-tracking branch 'upstream/master' into i11802-resetSpeec…

ad49944

…hDictionaries

seanbudd marked this pull request as ready for review May 25, 2021 00:29

seanbudd self-requested a review June 11, 2021 01:44

seanbudd reviewed Jun 22, 2021

View reviewed changes

lukaszgo1 mentioned this pull request Jan 31, 2022

Speech Dictionary dialog: Add a "Remove all" button (#11802) taken over after abandoned #12385 #13294

Merged

5 tasks

seanbudd closed this Feb 1, 2022

		self.typeRadioBox.SetSelection(DictionaryEntryDialog.TYPE_LABELS_ORDERING.index(type))


		class DictionaryDialog(SettingsDialog):

		# Copyright (C) 2006-2021 NV Access Limited, Michael Curran, Peter Vágner, Mesar Hameed, Joseph Lee,
		# Aaron Cannon, Reef Turner, Ethan Holliger, Julien Cochuyt, Thomas Stivers, Cyrille Bougot

Uh oh!

Conversation

JulienCochuyt commented May 9, 2021

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

AppVeyorBot commented May 9, 2021

Uh oh!

XLTechie commented May 10, 2021 via email

Uh oh!

JulienCochuyt commented May 10, 2021

Uh oh!

XLTechie commented May 10, 2021 via email

Uh oh!

lukaszgo1 May 19, 2021

Choose a reason for hiding this comment

Uh oh!

JulienCochuyt May 19, 2021

Choose a reason for hiding this comment

Uh oh!

seanbudd May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanbudd May 24, 2021

Choose a reason for hiding this comment

Uh oh!

JulienCochuyt May 24, 2021

Choose a reason for hiding this comment

Uh oh!

seanbudd commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JulienCochuyt commented May 24, 2021

Uh oh!

AppVeyorBot commented May 24, 2021

Uh oh!

seanbudd Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

seanbudd commented Feb 1, 2022 • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seanbudd May 20, 2021 •

edited

Loading

seanbudd commented May 24, 2021 •

edited

Loading

seanbudd commented Feb 1, 2022 •

edited by feerrenrut

Loading