nvda icon indicating copy to clipboard operation
nvda copied to clipboard

Implement the gui.messageBox API

Open seanbudd opened this issue 2 years ago • 11 comments

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

Current problems:

  • Cannot rename buttons / add custom buttons (addon author request)
    • #12344
    • #12353
  • An open message dialog blocks exiting NVDA, and isn’t refocused
  • An open message dialog without wx.OK or wx.CANCEL as an option cannot be closed with Alt+F4/Escape
    • #10799
  • An open message dialog blocks other input (example: opening the NVDA menu with the about dialog open)
    • #12353
    • #8709
  • Dialogs cannot have context help associated with them
    • #12355
  • unprotected open modals crash NVDA or cause undefined behaviour when exiting: #13579
  • Ensure that the test Quits from keyboard with about dialog open is re-enabled and works. Tagged with # Excluded to be fixed still (#12976), however that issue is closed.

Requirements for the API

A developer should be able to:

  • rename buttons
  • associate context help with the given message box
  • show the dialog as a modal or a non-modal
  • for non modal dialogs:
    • provide a default return code when dialogs are forced to close (eg via an exit, perform cancel)
    • attach a callback to handle return codes
    • refocus a dialog in the event of needing to close it
    • if a default return code is provided, make the dialog close-able with alt+F4 and escape
  • for modal dialogs:
    • if wx.CANCEL is provided, make the dialog close-able with alt+F4 and escape
    • nice to have:
      • if wx.CANCEL is provided, perform this action when dialogs are forced to close.
      • refocus a dialog in the event of needing to close it

Constraints from wxPython

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

  • if a default return code is provided, make the dialog close-able with alt+F4 and escape
    • this is the behaviour already when wx.CANCEL is an option for the wx.MessageBox/wx.MessageDialog. If Yes/No is required, we should not accept alt+F4 and escape
    • in #10799 wx.CANCEL should replace wx.NO and the buttons should be renamed appropriately

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

  • rename buttons
  • associate context help with the given message box

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 :

  • provide a default return code when dialogs are forced to close (eg via an exit, perform cancel)
  • attach a callback to handle return codes
  • refocus a dialog in the event of needing to close it
  • if a default return code is provided, make the dialog close-able with alt+F4 and escape

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:

  • gui.IsInMessageBox was a boolean, this has been replaced by a function gui.message.isModalMessageBoxActive, which tracks if a messageBox is open in a safer manner. Done in #13376 .

In a future release

  • Create a new gui.MessageDialog class, encourage migration to using it instead of gui.messageBox.
    • gui.messageDialog should not subclass wx.MessageDialog, and instead be a custom wx.Dialog subclass
    • override Show and ShowModal to track instances to track in IsInMessageBox
  • #10799 wx.CANCEL should be replaced with wx.NO to enable esc/alt+f4, with the buttons renamed

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

seanbudd avatar Oct 29 '21 01:10 seanbudd

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

feerrenrut avatar Nov 03 '21 05:11 feerrenrut

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.

lukaszgo1 avatar Nov 03 '21 11:11 lukaszgo1

Current problems:

  • Cannot rename buttons / add custom buttons (addon author request)
    • #12344
    • #12353
  • An open message dialog blocks exiting NVDA, and isn’t refocused
  • An open message dialog without wx.OK or wx.CANCEL as an option cannot be closed with Alt+F4/Escape
    • #10799
  • An open message dialog blocks other input (example: opening the NVDA menu with the about dialog open)
    • #12353
    • #8709
  • Dialogs cannot have context help associated with them
    • #12355

Requirements for new design:

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

  • We should be able to rename buttons
  • We should be able to associate context help with the given message box

Requirements that are invalid:

  • 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 wx.MessageBox/wx.MessageDialog. If Yes/No is required, we should not accept alt+F4 and escape
    • #10799 should be closed, wx.CANCEL should replace wx.NO and the buttons should be renamed appropriately

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.

  • An open dialog should avoid blocking input in some circumstances
  • We should be able to refocus a dialog in the event of needing to close it
  • We should be able to provide a default action when dialogs are forced to close (eg via an exit, perform cancel)
  • An open dialog with no default closing action should be refocused when exiting NVDA

Suggestions moving forward:

In 2022.1:

  • Replace gui.messageBox with a gui.messageDialog class
  • #10799 should be closed, either as is or wx.CANCEL should be replaced with wx.NO to enable esc/alt+f4
  • Add developer documentation for when to use gui.messageDialog as opposed to other dialogs

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

  • This should not subclass wx.MessageDialog, and instead be a custom Dialog subclass
  • This should replace uses of wx.MessageDialog which do not need to block exit
  • Update developer documentation for when to use gui.messageDialog as opposed to gui.messageDialogNonBlocking

Reference documentation

seanbudd avatar Feb 13 '22 23:02 seanbudd

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 avatar Feb 14 '22 11:02 lukaszgo1

* 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.

lukaszgo1 avatar Feb 17 '22 16:02 lukaszgo1

"#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.

XLTechie avatar Feb 17 '22 23:02 XLTechie

@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 avatar Feb 18 '22 00:02 seanbudd

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 avatar Feb 18 '22 00:02 seanbudd

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 avatar Feb 22 '22 02:02 seanbudd

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

  • gui.message.messageBox - blocking
  • gui.runScriptModalDialog - non-blocking modal with a callback
  • nvdaControls.MessageDialog - a dialog subclass that could potentially form the base of the new design.

seanbudd avatar Mar 08 '22 04:03 seanbudd

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.

feerrenrut avatar Oct 25 '22 08:10 feerrenrut