Speech Dictionary dialog: Add a "Remove all" button (#11802) taken over after abandoned #12385#13294
Conversation
|
@seanbudd Given that we're changing api of the |
|
Thanks @lukaszgo1 for working on this. I want to raise two points:
|
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:
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. |
|
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
left a comment
There was a problem hiding this comment.
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:
- Code move from settingsDialog to speechDict, where
git diff master:source/gui/settingsDialog.py <commit-1>:source/gui/speechDict.py --statis just deletions, no insertions - Change of line endings for
source/gui/speechDict.pyfrom CRLF to LF, wheregit diff <commit-1>:source/gui/speechDict.py <commit-2>:source/gui/speechDict.py --ignore-cr-at-eolis empty - Linting from the diff, i.e. a minimal set of changes to eliminate errors from
runlint.bat master. - 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.
| @@ -1,4 +1,5 @@ | |||
| # -*- coding: UTF-8 -*- | |||
| # -*- coding: UTF-8 -*- | |||
There was a problem hiding this comment.
this has been added in for a second time
I agree that this is better done in a separate PR. |
Qchristensen
left a comment
There was a problem hiding this comment.
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.
Sorry about that - it seems this has been done in the previous PR and I missed this.
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.
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. |
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.
Nothing more to do, just a reminder in case you end up changing the commits. |
As a preparatory step for PR nvaccess#13294 the code should be moved from gui\settingsDialogs
In PR nvaccess#13294 it has been decided that LF is prefered for new files going forward.
As part of PR nvaccess#13294 speech dictionary dialogs were separated into their own module. This commit makes the code in the new module compliant with our coding standards.
f1ee51e to
1da86f8
Compare
|
@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. |
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
1da86f8 to
0bf6435
Compare
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:
DictionaryDialogandDictionaryEntryDialogfromgui.settingsDialogsto a new dedicatedgui.speechDictsource/gui/settingsDialogs.pyTesting 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
For Developers
DictionaryDialogandDictionaryEntryDialogare moved fromgui.settingsDialogsto the newgui.speechDictmodule.Code Review Checklist:
Changes from #12385:
gui.speechDict.pyhas been updated to reflect people who have modified the moved code.