Improve control validation in the settings dialog

[hwf1324]

This issue was reported by @wmhn1872265132

Steps to reproduce:

Reproduction method 1:

1. Open NVDA Keyboard settings. Deselect all items in the "Select NVDA Modifier Keys" list.
2. Navigate to other categories (such as Mouse settings) through "Categories:".
3. Click the OK button
4. Click the OK button in the warning dialog box.

Reproduction method 2:

1. Open Speech settings Deselect "talk" in the list of modes available in the Cycle speech mode command.
2. Like method 1
3. Click the OK button
4. Click the "No" button in the warning dialog box.

Actual behavior:

The focus jumps back to the corresponding list that needs adjustment (for method 1, it is the "Select NVDA Modifier Keys" list, for method 2, it is the "Modes available in the Cycle speech mode command:" list), but the title of the settings window and the category list remain as the options before clicking "OK" (in this case, it is the mouse settings). Additionally, the GUI does not switch to the corresponding settings panel.

Expected behavior:

The focus jumps back to the corresponding list that needs adjustment (for method 1, it is the "Select NVDA Modifier Keys" list, for method 2, it is the "Modes available in the Cycle speech mode command:" list). The title and category lists of the settings window should also be updated simultaneously (method 1 should be keyboard settings, method 2 should be speech settings). At the same time, the GUI switches to the corresponding settings panel.

NVDA logs, crash dumps and other attachments:

System configuration

NVDA installed/portable/running from source:

portable

NVDA version:

alpha-30253

Windows version:

Name and version of other software in use when reproducing the issue:

Other information about your system:

Other questions

Does the issue still occur after restarting your computer?

Have you tried any other versions of NVDA? If so, please report their behaviors.

If NVDA add-ons are disabled, is your problem still occurring?

Does the issue still occur after you run the COM Registration Fixing Tool in NVDA's tools menu?

The reason for this issue is: calling the SetFocus function of a control in one category tab from another category tab, but without changing the GUI

For the solution, I have several ideas:

1. Change the category before using SetFocus.
   1. Directly use the index of SettingsPanel.
   2. Add a getter to SettingsPanel.
   3. Add an index property to each SettingsPanel when creating catListCtrl.
2. Add a method to display the SettingsPanel.
   1. Add a method to each SettingsPanel to set the visibility of this panel.
   2. Allow the SetFocus of each control under each SettingsPanel to directly switch to the category of the SettingsPanel where the control is located.

I think the last option is the most scalable, but I don't know how to implement it For example, how to hook the SetFocus of all controls in a SettingsPanel only CC: @CyrilleB79, @lukaszgo1, @LeonarddeR, any suggestions?

[lukaszgo1]

Thanks for reporting this issue! I will submit a fix for it during the weekend, though cannot yet comment on which, if any, of your proposed methods I will use.

[CyrilleB79]

What should happen if you both modify NVDA modifier with no key checked and you uncheck Talk speech mode, then validate? I think that we should revert the focusing of the list having a problem. Also, in this case, should we have two error/warning dialogs, one after the other?

Still worse, you may modify your configuration so that:

• The language has been changed in General settings
• No NVDA key is checked in Keyboard settings
• In Speech Settings, Talk speech mode is unchecked, or less than two speech modes are checked in the list. Will you have all the dialogs?

Maybe we should reconsider the moment when the warning/error dialogs show up. At least for the NVDA keys and the speech modes, the dialog should appear when the focus is about to leave the checkable list. Does such pre-focus exit event exist?

[hwf1324]

What should happen if you both modify NVDA modifier with no key checked and you uncheck Talk speech mode, then validate? I think that we should revert the focusing of the list having a problem.

It seems to only react to the first control that does not pass validation. For example, if you modify "Select NVDA Modifier Keys", only a warning for this control will pop up, and vice versa. Of course, the focus does move to the problematic list.

Also, in this case, should we have two error/warning dialogs, one after the other?

I don't think it's necessary. Either gather all the validation errors into one error dialog box, or solve the errors one by one.

Maybe we should reconsider the moment when the warning/error dialogs show up. At least for the NVDA keys and the speech modes, the dialog should appear when the focus is about to leave the checkable list. Does such pre-focus exit event exist?

For keyboard users, this may be possible, but what about mouse users? Of course, this is secondary

[lukaszgo1]

Maybe we should reconsider the moment when the warning/error dialogs show up. At least for the NVDA keys and the speech modes, the dialog should appear when the focus is about to leave the checkable list. Does such pre-focus exit event exist?

I've looked into it, and this is not easily doable. As an alternative I'd suggest to validate when checking / unchecking items, i.e. When unchecking the 'talk' mode warning should be shown, when 'yes' is pressed 'talk' becomes unchecked, when 'no' is chosen it remains checked. For NVDA modifiers when user unchecks the last one warning appears, and the checkbox remains checked. Would that be acceptable?

[CyrilleB79]

Maybe we should reconsider the moment when the warning/error dialogs show up. At least for the NVDA keys and the speech modes, the dialog should appear when the focus is about to leave the checkable list. Does such pre-focus exit event exist?

I've looked into it, and this is not easily doable. As an alternative I'd suggest to validate when checking / unchecking items, i.e. When unchecking the 'talk' mode warning should be shown, when 'yes' is pressed 'talk' becomes unchecked, when 'no' is chosen it remains checked. For NVDA modifiers when user unchecks the last one warning appears, and the checkbox remains checked. Would that be acceptable?

Thanks @lukaszgo1 for looking to how my idea could be implemented. With your proposal, the UX is slightly degraded with respect to my initial idea. Indeed in NVDA modifiers list, when a user has only Insert checked and he wants to use only numpadInsert instead, he has to take care to check numpadInsert before unchecking Insert.

Given the technical difficulty to have my initial idea implemented, your proposal is completely acceptable. Thanks!

[lukaszgo1]

With your proposal, the UX is slightly degraded with respect to my initial idea. Indeed in NVDA modifiers list, when a user has only Insert checked and he wants to use only numpadInsert instead, he has to take care to check numpadInsert before unchecking Insert.

Given the technical difficulty to have my initial idea implemented, your proposal is completely acceptable. Thanks!

I haven't thought about this case, and you're right that this is a regression in behavior, although a slight one. Having looked more closely it seems you can be notified when the given control lost focus, and perform some validation on it, what is problematic is the fact that you are not allowed to set focus to the control which lost it in the event handler. This apparently can be solved using Delayed Action Mechanism, but that is something I have no experience with. There is also a case of additional ways in which settings can be saved i.e. by pressing a button with the mouse, pressing enter or CTRL+s for apply. Given the fact that validating when checking / unchecking is not optimal and requires a fer hunk of work, validation on focus loss is doable but difficult, I'll just submit a PR which disables focusing of the invalid controls, as the remaining behavior is not worse that in 2023.3. I'll also create a separate issue gathering comments from this one, so that if someone has time / skills to improve the validation they will have a starting point. I'll wait with submitting a PR until tomorrow, so that if someone disagrees with the approach for any reason, they have a chance to comment here.

[CyrilleB79]

But will you be able to restore all the error / warning messages in case there are more than one error? E.g. both no NVDA modifier and Talk speech mode not selected.

If not, I prefer your first solution, even if the user may be bothered because he unchecks all NVDA modifiers.

[lukaszgo1]

But will you be able to restore all the error / warning messages in case there are more than one error? E.g. both no NVDA modifier and Talk speech mode not selected.

If not, I prefer your first solution, even if the user may be bothered because he unchecks all NVDA modifiers.

I'm not sure I understand what you mean by 'restore'. Note that even before #15873, when validating panels only the first warning was shown. In the current master the behavior is marginally better, since settings dialog is not closed.

[wmhn1872265132]

It is not advisable to uncheck the box to warn immediately. The most obvious example is Modes available in the Cycle speech mode command: This list also has a judgment that requires two checkboxes to be selected. If you do this, frequent warnings may appear when adjusting this setting.

[wmhn1872265132]

I think the first problem to solve right now is that the category and title of the settings dialog should be displayed correctly after the warning dialog is displayed. Secondly, you should ensure that the dialog box prompting you to change the language and restart is displayed after all warning dialog boxes (if there is a behavior of changing the language). In this way, even if there are multiple warning behaviors, users only need to confirm or adjust one by one according to the prompts.

[lukaszgo1]

It looks like there are several conflicting ideas with regard to how this issue should be solved. There is also a misunderstanding about what behavior has been introduced by #15873, and what issues existed with the GUI validations before. In 2023.3 Validation behaved as follows:

• Only warning from the first invalid settings panel was shown
• After user dismissed the warning settings dialog was closed without saving the config. As a result all changes performed in it were lost
• Due to the above it was (and still is) impossible to have both a validation error and dialog asking about restart caused by language change shown, since if one of the panels was invalid configuration from all of them was not applied.

15873 Introduced the following:

• After user dismisses the validation warning for the first panel settings dialog stays open, therefore selection in all other controls is not lost
• The invalid control is focused, which works only if user didn't change the panel

We can either:

• Revert focusing of the invalid control, which would still be an improvement compared to 2023.3
• Try to activate settings panel with the invalid control
• Rethink the validation so that it applies when the invalid control loses focus by any means i.e. user moves it from the keyboard, clicks another control, tries to change the category or activate Okay / Apply.

[CyrilleB79]

Thanks @lukaszgo1 for this clear summary.

I was wrong in my previous comment: there is no possible sequence of multiple error dialogs. Still there can be a no Talk warning with answer "No" and then an NVDA modifier error dialog; or a no %Talk warning dialog answered "No" and the language change restart dialog. These should be checked.

All your three proposals will already be improvement with respect to 2023.3 and latest master. My order of preference from a UX point of view is:

• Proposal 3: Check the validity when focus is about to go away from the checkable list by any mean (tab, Enter, "Apply", control+tab, equivalent mouse clics). It's the best one since the error is caught as early as possible, when the user still has his/her last action in mind.
• Proposal 2: When the settings dialog is validated, do not close it and refocus the list corresponding to the first error found (switching panel if needed)
• Proposal 1: Just revert the focusing of the checkable list where the error has occurred.

But it should be considered what is technically easily doable or not, not only UX. I'll let you appreciate it yourself.

Even if you implement proposal 1, you or someone may still implement proposal 2 or 3 in the future, if he/she has a good idea or bandwidth for it.

[lukaszgo1]

I have created #15897, where my first proposal is implemented, i.e. invalid controls are no longer focused. I'd suggest to keep this issue open, but morph it to discuss possible improvements for validation.

[hwf1324]

@lukaszgo1 @CyrilleB79

For example, bind the event handler wx.EVT_KILL_FOCUS to the modifierList control on the keyboard panel. When the focus leaves the control, a warning dialog pops up. Press the Ok button, and everything goes smoothly. However, when pressing Esc to cancel the settings dialog, the dialog is destroyed, but a warning dialog pops up at the same time.

Is there any good method?

In makeSettings:

        self.modifierList.Bind(wx.EVT_KILL_FOCUS, self.onFocusLost)

In KeyboardSettingsPanel:

    def onFocusLost(self, event):
        if not self:
            return
        # #2871: check whether at least one key is the nvda key.
        if not self.modifierList.CheckedItems:
            log.debugWarning("No NVDA key set")
            gui.messageBox(
                # Translators: Message to report wrong configuration of the NVDA key
                _("At least one key must be used as the NVDA key."),
                # Translators: The title of the message box
                _("Error"), wx.OK|wx.ICON_ERROR,self)
            self.SetFocus()

Reference: https://wiki.wxpython.org/Surviving%20with%20wxEVT%20KILL%20FOCUS%20under%20Microsoft%20Windows

[XLTechie]

Given the fact that validating when checking / unchecking is not optimal and requires a fer hunk of work, validation on focus loss is doable but difficult,

I can't think of anything better than either of those options.

The one alternative I came up with, although I'm not sure I would say it was better, is to make these options alterable in a sub-dialog, like choosing a braille display or speech synth is. That would mean they would get a button in the main UI, and the sub-dialog couldn't be departed without having a valid setup to pass back to the config UI.

I'm not sure I like this idea at all, but I'm presenting it as a different possible direction.

[hwf1324]

@lukaszgo1

I think the warning for deselecting the "talk" voice mode can be separated, for example:

    def onSpeechModesListChanged(self, evt):
        if (
            evt.GetInt() == self._allSpeechModes.index(speech.SpeechMode.talk)
            and not self.speechModesList.IsChecked(evt.GetInt())
        ):
            if gui.messageBox(
                _(
                    # Translators: Warning shown when 'talk' speech mode is disabled in settings.
                    (
                        "You did not choose Talk as one of your speech mode options. "
                        "Please note that this may result in no speech output at all. "
                        "Are you sure you want to continue?"
                    )
                ),
                # Translators: Title of the warning message.
                _("Warning"),
                wx.YES | wx.NO | wx.ICON_WARNING,
                self,
            ) == wx.NO:
                self.speechModesList.SetCheckedItems(
                    list(self.speechModesList.GetCheckedItems())
                    + [self._allSpeechModes.index(speech.SpeechMode.talk)]
                )

Can be bound to the EVT_CHECKLISTBOX event

[CyrilleB79]

What is remaining in this issue? Is it only to re-focus the corresponding option after having dismissed the error dialog?

For now, I have submitted #16352. It can considered as an alternative solution to the current issue or as a temporary work-around. Just let me know.

[lukaszgo1]

IMO nothing has changed from the last time you've summarized the discussion i.e. it will be preferable to validate the control immediately after it loses focus. #16352 is a nice improvement though.