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

Add app-level dialogs. #2669

Merged
merged 17 commits into from
Jun 26, 2024
Merged

Add app-level dialogs. #2669

merged 17 commits into from
Jun 26, 2024

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Jun 19, 2024

At present, all dialogs are defined at the Window level. This means you need to have a window instance to display any dialog.

However, not all dialog requests are necessarily window specific; and with the introduction of session-based and background apps (#2651), there will be situations where there isn't a window to which a dialog can be attached.

This PR modifies the API for dialogs to allow for app-based dialogs. It does this by deprecating the older "functional" dialog methods on Window, in favor of surfacing the dialogs as public classes; these classes can be displayed by calling await window.dialog(some dialog) and app.dialog(some dialog).

On macOS and GTK, window-based dialogs are presented as "sheets" attached to the window; app-based dialogs are presented as free floating. Windows presents all dialogs at the app level. iOS and Android always have a window, so app-level dialogs are presented in that context.

This change necessitated a fairly major rework of dialog testing. The old API displayed the dialog on creation, but returned an object that could be awaited; the new API doesn't display the dialog until an async show method is invoked.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@freakboy3742
Copy link
Member Author

@mhsmith There's something odd going on with the macOS M1 testbed... it's passing 100%, then reporting a failure. I'm guessing this is a segfault on cleanup or coverage; I need to investigate further.

However, I'd be interested in your high level thoughts on this as an approach.

What I've done here is the obvious approach - replicate the Window dialog APIs onto App. A different approach would be to expose dialogs as a first class module, then expose either:

  • window.show_dialog(InfoDialog(...)) and app.show_dialog(InfoDialog(...)); or
  • InfoDialog(...).show(window) and InfoDialog(...).show()

I'm not especially enthused about the spelling of either of these APIs... the former feels like creating a temporary object for no reason; the latter doesn't feel consistent with the rest of the Toga API. I'm open to other suggestions if you have them.

That said - the one notable upside to dialogs being a first-class modules is that we get a standalone documentation page for dialogs, rather than needing to find a way to direct people to the app and window pages.

It also gives us an opportunity to fully deprecate dialog on_result handling, which massively simplifies the typing interface. Of course, we'll need to carry the old window API entry points for a while, so it's not a complete win.

@mhsmith
Copy link
Member

mhsmith commented Jun 19, 2024

the one notable upside to dialogs being a first-class modules is that we get a standalone documentation page for dialogs, rather than needing to find a way to direct people to the app and window pages.

I think that would be very useful, because the App and Window pages are already getting quite large.

InfoDialog(...).show seems reasonably consistent, as Window has a show method as well. But if calling it with zero arguments would display a non-modal dialog, then the most obvious way of using the method would be the wrong way for most use cases. Instead, how about making show take a required argument called context, which can be either a Window or an App?

@freakboy3742 freakboy3742 marked this pull request as ready for review June 21, 2024 06:20
@freakboy3742 freakboy3742 marked this pull request as draft June 21, 2024 06:20
@freakboy3742 freakboy3742 marked this pull request as ready for review June 24, 2024 05:42
docs/reference/api/resources/dialogs.rst Outdated Show resolved Hide resolved
docs/reference/api/resources/dialogs.rst Outdated Show resolved Hide resolved
docs/reference/api/resources/dialogs.rst Outdated Show resolved Hide resolved
docs/reference/api/resources/dialogs.rst Outdated Show resolved Hide resolved
core/src/toga/window.py Outdated Show resolved Hide resolved
docs/reference/api/resources/dialogs.rst Outdated Show resolved Hide resolved
core/src/toga/dialogs.py Outdated Show resolved Hide resolved
core/src/toga/dialogs.py Outdated Show resolved Hide resolved
core/src/toga/dialogs.py Outdated Show resolved Hide resolved
core/src/toga/window.py Outdated Show resolved Hide resolved
docs/reference/api/resources/dialogs.rst Outdated Show resolved Hide resolved
core/src/toga/dialogs.py Outdated Show resolved Hide resolved
core/src/toga/dialogs.py Outdated Show resolved Hide resolved
core/src/toga/app.py Outdated Show resolved Hide resolved
core/src/toga/window.py Outdated Show resolved Hide resolved
core/src/toga/window.py Outdated Show resolved Hide resolved
core/src/toga/window.py Outdated Show resolved Hide resolved
core/src/toga/window.py Outdated Show resolved Hide resolved
core/src/toga/window.py Outdated Show resolved Hide resolved
@freakboy3742 freakboy3742 merged commit d6ed2c0 into beeware:main Jun 26, 2024
32 of 35 checks passed
@freakboy3742 freakboy3742 deleted the app-dialogs branch June 26, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants