search

nvaccess / nvda

NVDA, the free and open source Screen Reader for Microsoft Windows
https://www.nvaccess.org/
Other
2.12k stars 638 forks source link

Implement the gui.messageBox API #13007

Open seanbudd opened 3 years ago

seanbudd commented 3 years ago

See also: #12344, #12353, #10799, #8709, #13579

Current problems:

Requirements for the API

A developer should be able to:

Constraints from wxPython

Support across wx.MessageBox, wx.MessageDialog and wx.Dialog for the following:

wx.Dialog or wx.MessageDialog is required to add the following:

wx.MessageBox is a function that doesn't support this. Using wx.MessageDialog directly also can implement this.

wx.Dialog with .Show is required to add the following :

wx.Dialog.ShowModal is blocking, so .Show must be used. wx.MessageBox and wx.MessageDialog doesn't support .Show(), but wx.Dialog does.

Suggestions moving forward:

In 2022.1:

In a future release

Reference documentation

Additional context

Fix up #12984

12976

Draft Sample API

class MessageBoxReturnCode(IntEnum):
    OK = wx.OK
    YES = wx.YES
    NO = wx.NO
    CANCEL = wx.CANCEL

class MessageDialog(
        DpiScalingHelperMixinWithoutInit,
        ContextHelpMixin,
        wx.Dialog,
        metaclass=SIPABCMeta
):
    def __init__(
        self, 
        parent: wx.Window,
        message: str,
        caption: str = wx.MessageBoxCaptionStr,
        style: int = wx.OK | wx.CENTER,
        **kwargs,
    ) -> None:
        super().__init__(parent, message, caption, style, **kwargs)

    def Show(self) -> None:
        """Show a non-blocking dialog.
        Attach buttons with button handlers"""
        pass

    def defaultAction(self) -> None:
           return None

    @staticmethod
    def CloseInstances() -> None:
        """Close all dialogs with a default action"""
        pass

    @staticmethod
    def BlockingInstancesExist() -> bool:
        """Check if dialogs are open without a default return code
        (eg Show without `self._defaultReturnCode`, or ShowModal without `wx.CANCEL`)"""
        pass

    @staticmethod
    def FocusBlockingInstances() -> None:
        """Raise and focus open dialogs without a default return code
        (eg Show without `self._defaultReturnCode`, or ShowModal without `wx.CANCEL`)"""
        pass
feerrenrut commented 3 years ago

See also: https://github.com/nvaccess/nvda/issues/8453

lukaszgo1 commented 3 years ago

When working on this it would be also nice to allow associating context help with a given messageBox. For one example in which this would be useful see PR #12355 where @XLTechie attempted to add context help to the COM registration Fixing Tool message dialog.

seanbudd commented 2 years ago

Current problems:

Requirements for new design:

The following requirements can be done by subclassing wx.MessageDialog, and would be API breaking:

Requirements that are invalid:

The following requirements cannot be based on the wx.MessageDialog class. This is because the function wx.MessageDialog.ShowModal is blocking and the function wx.MessageDialog.Show (as non-modal) is not available for wx.MessageDialogs. Instead a new wx.Dialog subclass is required.

Suggestions moving forward:

In 2022.1:

In any future release, create a new gui.messageDialogNonBlocking class

Reference documentation

lukaszgo1 commented 2 years ago

It seems you've forgotten to add "We should be able to associate context help with the given message box" to the list of requirements. This has been requested in #12355.

lukaszgo1 commented 2 years ago 
* A dialog should be close-able with alt+F4 and escape, performing the default action

  * this is the behaviour already when `wx.OK` is an option for the MessageBox/MessageDialog. If Yes/No is required, we should not accept alt+F4 and escape
  * I think [Run COM Registration fixing tool Dialog can't be closed by ALT+F4 or ESC #10799](https://github.com/nvaccess/nvda/issues/10799) should be closed, or `wx.CANCEL` should replace `wx.NO`

In many cases where we have a dialog with yes/no in Windows (for example when closing a new notepad document with unsaved changes) there are 3 buttons: yes, no and cancel. In such cases escape executes action bound to the cancel button however in such cases having a separate control for canceling is necessary as it performs different action than activating the no button. Changing "no" to "cancel" in the COM registration fixing tool dialog is counter intuitive IMO, and allowing to customize what control should activate when ESC or ALT+F4 is pressed can be useful regardless. Besides for users the current behavior is inconsistent as usually ESC cancels the operation. While I don't think this should be part of the initial redesign keeping #10799 open but assigned P4 as it is now is the best compromise as making the dialog close with ESC or ALT+F4 does no charm and improves the UX.

XLTechie commented 2 years ago

"#10799 should be closed, either as is or wx.CANCEL should be replaced with wx.NO to enable esc/alt+f4

I agree with @lukaszgo1 that replacing No/Yes with Cancel/Yes, would be less than ideal. My desire for the COM Reg Fix dialog, is to replace yes_no with Cancel and OK, but re-styling OK to "Continue". That would give escape capability, and make clear what is going to happen (A "continuance" of the action described in the dialog).

I haven't done it because (1) it needed a new kind of dialog not provided by gui.messageBox, and (2) I've been non-contributory for the last seven months, and all the work I've already done to redesign gui.messageBox with (some of) these desired capabilities, is now out of date and needs to be re-examined.

I would request that #10799 stay open. It's not my issue, but I think we can solve it in spirit once this work is done.

seanbudd commented 2 years ago

@lukaszgo1

and allowing to customize what control should activate when ESC or ALT+F4 is pressed can be useful regardless.

This is not an option when using wx.MessageBox or using/subclassing wx.MessageDialog. A wx.MessageDialog with a wx.YES/wx.NO option is required to block until a decision is made, this is by design. The appropriate wx pattern is to use wx.YES/wx.CANCEL and rename the buttons as appropriately. NVDA performs a cancel action when people choose "No".

I suggest we go ahead with @XLTechie 's suggestion.

seanbudd commented 2 years ago

See also: #8453

I think this is unrelated as these controls don't use wx.MessageBox/wx.MessageDialog. The progress dialog is an wx.ProgressDialog and log viewer is a wx.Frame.

seanbudd commented 2 years ago

Proposed API


class MessageBoxReturnCode(IntEnum):
    OK = wx.OK
    YES = wx.YES
    NO = wx.NO
    CANCEL = wx.CANCEL

def messageBoxShowModal(
        message: str,
        caption: str = wx.MessageBoxCaptionStr,
        style: int = wx.OK | wx.CENTER,
        parent: Optional[wx.Window] = None,
) -> MessageBoxReturnCode:
    """Display a message dialog.
    @param message: The message text.
    @param caption: The caption (title) of the dialog.
    @param style: Same as for wx.MessageBox.
    @param parent: The parent window.
    @return: Same as for wx.MessageBox.
    """
    from gui import mainFrame
    messageDialog = MessageDialog(
        parent or mainFrame,
        message,
        caption,
        style,
    )
    return messageDialog.ShowModal()

def messageBoxShowWithCallback(
        message: str,
        caption: str = wx.MessageBoxCaptionStr,
        style: int = wx.OK | wx.CENTER,
        parent: Optional[wx.Window] = None,
        defaultReturnCode: Optional[MessageBoxReturnCode] = None,
        callback: Optional[Callable[[MessageBoxReturnCode], MessageBoxReturnCode]] = None,
) -> None:
    """Display a message dialog.
    @param message: The message text.
    @param caption: The caption (title) of the dialog.
    @param style: Same as for wx.MessageBox.
    @param parent: The parent window.
    @param defaultReturnCode: Choose a default MessageBoxReturnCode.
    If None, closing will be blocked by this modal being open.
    @param callback: Called on close, this should return the MessageBoxReturnCode return code provided.
    """
    from gui import mainFrame
    messageDialog = MessageDialog(
        parent or mainFrame,
        message,
        caption,
        style,
        defaultReturnCode,
        callback,
    )
    messageDialog.Show()

class MessageDialog(
        DpiScalingHelperMixinWithoutInit,
        ContextHelpMixin,
        wx.Dialog,
        metaclass=SIPABCMeta
):
    def __init__(
        self, 
        parent: wx.Window,
        message: str,
        caption: str = wx.MessageBoxCaptionStr,
        style: int = wx.OK | wx.CENTER,
        defaultReturnCode: Optional[int] = None,
        callback: Optional[Callable[[int], int]] = None,
        **kwargs,
    ) -> None:
        super().__init__(parent, message, caption, style, **kwargs)
        self._callback = callback
        self._defaultReturnCode = defaultReturnCode

    def ShowModal(self) -> MessageBoxReturnCode:
        """Create a blocking modal that requires user input.
        Return a MessageBoxReturnCode that needs to be handled"""
        pass

    def Show(self) -> None:
        """Show a non-blocking dialog.
        Can attach a callback to perform an action on button presses.
        Attach buttons as per ShowModal however use self._callback
        to handle each button."""
        pass

    @staticmethod
    def CloseInstancesWithDefaultReturnCodes() -> int:
        """Close all dialogs with a default return code
        (eg Show with `self._defaultReturnCode`, or ShowModal with `wx.CANCEL`)"""
        pass

    @staticmethod
    def BlockingInstancesExist() -> bool:
        """Check if dialogs are open without a default return code
        (eg Show without `self._defaultReturnCode`, or ShowModal without `wx.CANCEL`)"""
        pass

    @staticmethod
    def FocusBlockingInstances() -> None:
        """Raise and focus open dialogs are open without a default return code
        (eg Show without `self._defaultReturnCode`, or ShowModal without `wx.CANCEL`)"""
        pass
seanbudd commented 2 years ago

We have similar functions in NVDA that should be assimilated into a final design:

feerrenrut commented 2 years ago

Ensure that the test Quits from keyboard with about dialog open is re-enabled and works.

It was tagged with # Excluded to be fixed still (#12976), however that issue is closed.

I've added this to the description.