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

[lukaszgo1]

First of all many thanks to @JulienCochuyt for most of this work. Since the previous PR #12385 seemed to be abandoned and it introduces backwards incompatible changes I've decided to fix the remaining issue so that this work can be hopefully included in 2022.1

Link to issue number:

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

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.

For Developers

• DictionaryDialog and DictionaryEntryDialog are moved from gui.settingsDialogs to the new gui.speechDict module.

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

Changes from #12385:

• All commits preserving backwards compatibility are dropped
• As requested by @seanbudd during the review copyright header of gui.speechDict.py has been updated to reflect people who have modified the moved code.

[lukaszgo1]

@seanbudd Given that we're changing api of the DictionaryDialog in this release anyway it makes sense to include this one in 2022.1. When updating the copyright header I've inspected the history for the moved code manually rather than just copying the entire copyright header from the settingsDialogs as there is no guarantee that it reflects all people who have contributed to the code.

[CyrilleB79]

Thanks @lukaszgo1 for working on this.

I want to raise two points:

1. When some code is moved, I think that NVAccess (at least @feerrenrut) used to separate the work in two commits, one dedicated to code moving and the other one to implement the PR. Then they merge the PR without squashing. If this way to do is confirmed by NVAccess folks, would you mind rework the commit history to keep only these two commits and ask this PR to be merged, not squash-merged?
2. Is there any requirement or just common usage in NVDA to avoid multiple selection operations in lists? If it has been so in the past, is it still a requirement for today? I feel that people are quite accostumated to multi-selection lists in todays life (Windows Explorer, e-mail list in mail client, etc.) This does not seem to cause issues to blind people. I would be interested to hear NVAccess' opinion on this topic.

[lukaszgo1]

1. When some code is moved, I think that NVAccess (at least @feerrenrut) used to separate the work in two commits, one dedicated to code moving and the other one to implement the PR. Then they merge the PR without squashing. If this way to do is confirmed by NVAccess folks, would you mind rework the commit history to keep only these two commits and ask this PR to be merged, not squash-merged?

I would be happy to rework the commit history in whatever way is decided as the most optimal. If it is indeed decided that the commit history needs to be modified it seems to me that more logical course of action would be to split this into three rather than two commits:

• First commit for moving the code
• Second commit for linting
• Third commit - for an actual implementation of remove all button.

2. Is there any requirement or just common usage in NVDA to avoid multiple selection operations in lists? If it has been so in the past, is it still a requirement for today? I feel that people are quite accostumated to multi-selection lists in todays life (Windows Explorer, e-mail list in mail client, etc.) This does not seem to cause issues to blind people. I would be interested to hear NVAccess' opinion on this topic.

While I personally agree that multi selections on lists are pretty handy this should be implemented in a follow up if it is to be implemented at all. The dialog should be substantially modified to support this so that the "edit" button is available only when the single entry is selected.

[CyrilleB79]

Yes, both points in your replies make sense and I fully agree with each one. Let's see what NVAccess says about commit history. And regarding the multi-selection it should actually be discussed in a separate issue (and maybe PR) since the question arises for various NVDA dialogs: dictionaries, add-on manager, profile dialog, etc.

[seanbudd]

It appears that in the commit with the code move, there is a change of line endings (CRLF to LF).

(i11802-resetSpeechDictionaries)
$ git diff master:source/gui/settingsDialogs.py HEAD:source/gui/speechDict.py --ignore-cr-at-eol --stat
 source/gui/{settingsDialogs.py => speechDict.py} | 4524 +---------------------
 1 file changed, 134 insertions(+), 4390 deletions(-)

(i11802-resetSpeechDictionaries)
$ git diff master:source/gui/settingsDialogs.py HEAD:source/gui/speechDict.py --stat
 source/gui/{settingsDialogs.py => speechDict.py} | 4904 ++--------------------
 1 file changed, 324 insertions(+), 4580 deletions(-)

Ideally this would be merged with 4 commits:

1. Code move from settingsDialog to speechDict, where git diff master:source/gui/settingsDialog.py <commit-1>:source/gui/speechDict.py --stat is just deletions, no insertions
2. Change of line endings for source/gui/speechDict.py from CRLF to LF, where git diff <commit-1>:source/gui/speechDict.py <commit-2>:source/gui/speechDict.py --ignore-cr-at-eol is empty
3. Linting from the diff, i.e. a minimal set of changes to eliminate errors from runlint.bat master.
4. Changes introduced for this PR.

I think optionally we can merge as just 2 commits (move+changes), but this process makes reviewing much easier.

I would suggest doing this on a separate branch so that you can diff the final commit with this PR.

It would be great if you could preserve Julien's co-authorship. Due to the nature of the merge commit, it would be helpful to provide a small description referencing this PR in each commit message body.

Based on earlier reviewing, I don't expect any need to change the approach, but in that case, additional commits are fine, and can be squashed into commit 4. I will do this when updating the changelog.

[seanbudd]

this has been added in for a second time

[seanbudd]

While I personally agree that multi selections on lists are pretty handy this should be implemented in a follow up if it is to be implemented at all. The dialog should be substantially modified to support this so that the "edit" button is available only when the single entry is selected.

I agree that this is better done in a separate PR.
If this is a change people are interested in, it would be great to have an issue to track it and discuss any reasoning to not go ahead with the change.

[Qchristensen]

I'll leave the discussion about code and number of commits to others, but just from the documentation point of view, the user guide change is concise and clear, I'm happy with that.

[lukaszgo1]

It appears that in the commit with the code move, there is a change of line endings (CRLF to LF).

Sorry about that - it seems this has been done in the previous PR and I missed this.

2. Change of line endings for `source/gui/speechDict.py` from CRLF to LF, where `git diff <commit-1>:source/gui/speechDict.py <commit-2>:source/gui/speechDict.py --ignore-cr-at-eol` is empty

Is this really necessary - wouldn't it be simpler to stick with CRLF for now and convert the entire code base at some future point? If it is decided that LF is preferred for the new files going forward then this should be documented in our coding standards.

It would be great if you could preserve Julien's co-authorship.

GitHub shows both me and @JulienCochuyt and authors of these commits. Is there anything more to do in that regard?

I'll wait for the decision about CRLF -> LF before rebasing this code.

[seanbudd]

Is this really necessary - wouldn't it be simpler to stick with CRLF for now and convert the entire code base at some future point? If it is decided that LF is preferred for the new files going forward then this should be documented in our coding standards.

We've already decided that we are looking to move to LF. However, changing to LF isn't necessary as part of this PR. It's just better if it wasn't mixed with the other commits, if you wanted to keep the CRLF to LF change.

GitHub shows both me and JulienCochuyt and authors of these commits. Is there anything more to do in that regard?

Nothing more to do, just a reminder in case you end up changing the commits.

[lukaszgo1]

@seanbudd All done. I've divided this into 4 commits as you've suggested and, hopefully, provided clear commit messages referencing this PR for each one.

[seanbudd]

Thanks @lukaszgo1 - LGTM