Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the gui.messageBox API #13007

Closed
seanbudd opened this issue Oct 29, 2021 · 12 comments · Fixed by #17304 or #17582
Closed

Implement the gui.messageBox API #13007

seanbudd opened this issue Oct 29, 2021 · 12 comments · Fixed by #17304 or #17582
Assignees
Labels
Addon/API Changes to NVDA's API for addons to support addon development. api-breaking-change bug squash target merge-early Merge Early in a developer cycle p2 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority release/blocking this issue blocks the milestone release
Milestone

Comments

@seanbudd
Copy link
Member

seanbudd commented Oct 29, 2021

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

Current problems:

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

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:

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
  • Run COM Registration fixing tool Dialog can't be closed by ALT+F4 or ESC #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 seanbudd added Addon/API Changes to NVDA's API for addons to support addon development. deprecated/2022.1 Label used to track deprecations due for removal in the 2022.1 release labels Oct 29, 2021
@seanbudd seanbudd added this to the 2022.1 milestone Oct 29, 2021
@feerrenrut

This comment was marked as off-topic.

@lukaszgo1

This comment was marked as resolved.

@seanbudd seanbudd added api-breaking-change release/blocking this issue blocks the milestone release and removed deprecated/2022.1 Label used to track deprecations due for removal in the 2022.1 release labels Dec 10, 2021
@seanbudd seanbudd changed the title Improve the gui.messageBox API and usages Determine the gui.messageBox API Feb 7, 2022
@seanbudd seanbudd changed the title Determine the gui.messageBox API Design the gui.messageBox API Feb 7, 2022
@seanbudd seanbudd self-assigned this Feb 9, 2022
@seanbudd

This comment was marked as duplicate.

@lukaszgo1

This comment was marked as resolved.

@lukaszgo1

This comment was marked as resolved.

@XLTechie

This comment was marked as resolved.

@seanbudd

This comment was marked as resolved.

@seanbudd
Copy link
Member Author

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

This comment was marked as duplicate.

seanbudd added a commit that referenced this issue Mar 3, 2022
…essageBoxActive (#13376)

Relates to #13007, as the new design should be backwards compatible, we can implement the API changes as-is for 2022.1.
Follow up to the API proposed in #12984

Summary of the issue:
The API for gui.message had not yet been determined for 2022.1, as per #13007.
As the future API is intended to be backwards compatible, this PR commits to the API proposed in #12984.

Description of how this pull request fixes the issue:
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.

Changes logic gui/message.py to be safer with locking and handling errors.
@seanbudd seanbudd removed their assignment Mar 3, 2022
@seanbudd seanbudd changed the title Design the gui.messageBox API Implement the gui.messageBox API Mar 4, 2022
@seanbudd
Copy link
Member Author

seanbudd commented Mar 8, 2022

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 seanbudd removed this from the 2022.1 milestone Mar 17, 2022
@feerrenrut feerrenrut added p3 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority p2 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority and removed p3 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority labels May 17, 2022
@feerrenrut feerrenrut added the merge-early Merge Early in a developer cycle label Jul 12, 2022
@seanbudd seanbudd added merge-early Merge Early in a developer cycle and removed merge-early Merge Early in a developer cycle labels Jul 12, 2022
@seanbudd seanbudd self-assigned this Jul 18, 2022
@feerrenrut
Copy link
Contributor

feerrenrut commented Oct 25, 2022

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.

@seanbudd seanbudd added this to the 2025.1 milestone May 10, 2024
@SaschaCowley SaschaCowley assigned SaschaCowley and unassigned seanbudd Sep 5, 2024
@SaschaCowley SaschaCowley linked a pull request Oct 18, 2024 that will close this issue
31 tasks
SaschaCowley added a commit that referenced this issue Dec 18, 2024
Closes #13007
Closes #12344 
Closes #12353 

Summary of the issue:
Currently, there are several ways to create message boxes/dialogs in NVDA, all of them with their own drawbacks.
Additionally, subclassing `wx.Dialog` (or using it directly) brings with it the risk of breaking parts of NVDA. See #13007 for in-depth discussion.

Thus, a new API for creating highly customizable dialogs that work well with NVDA is desired.

Description of user facing changes
This PR does not, in itself, introduce any end-user facing changes. However, it lays the groundwork for closing a number of issues.

Description of development approach
Subclassed `wx.Dialog` to add the desired functionality in #13007

Re-implemented the about box to use a `MessageDialog`.

Added a callback field to `gui.blockAction._Context` to allow adding a function to execute when the block condition is met.

Features and tasks
* [x] Rename buttons
* [x] Add custom buttons
* [x] Associate context help with the given message box
* [x] Show the dialog as a modal or a non-modal
* [x] for non modal dialogs: 
  * [x] provide a default return code when dialogs are forced to close (eg via an exit, perform cancel) (WIP)
  * [x] attach a callback to handle return codes
  * [x]  refocus a dialog in the event of needing to close it
  * [x] if a default return code is provided, make the dialog close-able with alt+F4 and escape (WIP)
* [x] for modal dialogs: 
  * [x] if wx.CANCEL is provided, make the dialog close-able with alt+F4 and escape (mostly done)
  * [x] nice to have:
    * [x] if wx.CANCEL is provided, perform this action when dialogs are forced to close.
    * [x] refocus a dialog in the event of needing to close it
* [x] Ensure that the test `Quits from keyboard with about dialog open` is re-enabled and works.
* [x] Block exiting NVDA with modal `MessageDialog`s open.
* [x] Focus open `MessageDialog`s without default actions when exiting.
* [x] Tests
  * [x] Unit tests
* [x] Document the new API in the developer guide
* [x] Migrate the new API from `gui.MessageDialog` to `gui.message`.
* [x] Mark other means of creating message dialogs as deprecated.
  * [x] Re implement their internals to use the new API
    * [x] `gui.message.messageBox`
    * [x] `gui.nvdaControls.MessageDialog`
    * [x] `gui.nvdaControls._ContinueCancelDialog`

Testing strategy:
Unit and manual tests.

Unit tests are extensive, and reach around 85% coverage of the newly added code.

Message box shim tested with a variety of inputs and unittests. This is a fairly straightforward adapter.

`nvdaControls.MessageDialog` tested that screen curtain and add-on install warning dialogs still work as expected. Its private subclass, `_ContinueCancelDialog`, was tested by attempting to run the CRFT.

Known issues with pull request:

Internal dialogs are yet to be migrated. This should be done gradually before the existing means of creating message dialogs are removed.

Slightly odd behaviour when a callback function blocks, and `focusBlockingInstances` is called, where a hidden dialog is focused. This can be worked around, but possibly not without unintended consequences.

Some invalid combinations (namely a button which is set as the default action but set not to close the dialog) are silently changed. While documenting this should be sufficient, logging or raising an exception may be warranted.

Visual proportions of the re-implemented About dialog are different.

The `ctrl+c` behaviour of copying the dialog's title, contents and buttons is not supported.

Future work

* Remove all internal use of deprecated message dialog APIs.
* Remove deprecated message dialog APIs in 2026.1.

Nice to have
* Implement system tests for the new API.
* Comprehensively document a manual test plan, and develop any associated scripts.
* Add the ability to set a callback after a button has been added.
* Add a helper to add a "don't show this message again" checkbox.
michaelDCurran pushed a commit that referenced this issue Dec 20, 2024
This reverts commit 2477780.
Reverts #17304
Issues fixed
Fixes #17560
Fixes #17553
Issues reopened
Reopens #13007
Reopens #12344
Reopens #12353
Reason for revert
This PR broke two important NVDA functions, and there is insufficient time before the Christmas/New Years break to fix the issues, so we are temporarily reverting so that alphas work over the break:
• The CRFT no longer works, as gui.message.MessageDialog's inheritance from ContextHelpMixin was accidentally removed.
• Update checking no longer works, as runScriptModalDialog was modified to increment and decrement _messageBoxCounter, but UpdateAskInstallDialog attempts to restart NVDA before it has been closed, so the update fails as NVDA appears to be in an unsafe state.
@CyrilleB79
Copy link
Collaborator

Reopening as #17304 has been reverted in #17561.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Addon/API Changes to NVDA's API for addons to support addon development. api-breaking-change bug squash target merge-early Merge Early in a developer cycle p2 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority release/blocking this issue blocks the milestone release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants