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

Allow the use of toga.Window as the main window #2649

Merged
merged 21 commits into from
Jun 25, 2024

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Jun 14, 2024

Builds on #2646; that PR should be reviewed and merged before reviewing this one.

Derived from #2244.

Adds the ability to use toga.Window as the main window of an app. As toga.Window cannot have a toolbar, this is a good indicator that the user wants a minimalist "Simple" app, so the set of menu commands that are installed is also reduced. A SimpleApp example has been added to demonstrate this capability.

We need to know the type of the main window before we create the app commands; this means the app commands must be created after startup() has been invoked. However, app commands are currently created before startup so that the startup() method can customise the default commands.

To allow for customisation, create_app_commands() has been promoted to a public interface method so that users can override create_app_commands() in their apps; but this method is invoke after startup() so that it has sufficient context to create the right set of default commands.

Fixes #1870.

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 freakboy3742 marked this pull request as ready for review June 14, 2024 04:08
@mhsmith mhsmith self-requested a review June 17, 2024 11:14
@mhsmith
Copy link
Member

mhsmith commented Jun 19, 2024

As toga.Window cannot have a toolbar, this is a good indicator that the user wants a minimalist "Simple" app, so the set of menu commands that are installed is also reduced.

On macOS, which is the platform where this makes the greatest difference, doesn't this remove commands which are needed for basic functionality like window management (Hide, Minimize, etc.), and text editing (Cut, Paste, etc.)?

More generally, it looks to me like the existing set of commands is already minimalist enough, so introducing these two variants seems to add a lot of complexity for no clear benefit.

Also, the main_window documentation is about to become a lot more complicated in the next PR, which adds "background" and "session" apps. This would be helped if we eliminate the idea of a "simple app", and make "simple" or "main" purely a property of a window.

docs/reference/api/window.rst Outdated Show resolved Hide resolved
docs/reference/api/mainwindow.rst Outdated Show resolved Hide resolved
docs/reference/api/app.rst Outdated Show resolved Hide resolved
docs/reference/api/app.rst Outdated Show resolved Hide resolved
@freakboy3742
Copy link
Member Author

As toga.Window cannot have a toolbar, this is a good indicator that the user wants a minimalist "Simple" app, so the set of menu commands that are installed is also reduced.

On macOS, which is the platform where this makes the greatest difference, doesn't this remove commands which are needed for basic functionality like window management (Hide, Minimize, etc.), and text editing (Cut, Paste, etc.)?

More generally, it looks to me like the existing set of commands is already minimalist enough, so introducing these two variants seems to add a lot of complexity for no clear benefit.

My original thinking was that a minimal app should be truly as minimal as possible - but you make a good point about Hide, Minimise, Cut, Paste et al. Looking over the list, the only ones that could be considered completely optional are Preferences and Visit Homepage. Visit homepage should arguably exist by default, as the Apple HIG suggests apps should always provide help; "visit homepage" isn't the best manifestation of help, but we need something to provide the default help menu that has the "search" option to find menu items.

We will still need the "minimal vs standard" API for other platforms, because we don't want to create command instances that literally won't (or can't) be used.

I think we can roll this into the description of MainWindow - pointing out that if the platform uses app-level menus, using a MainWindow as the main window may add extra menu items. That then means we can hide the "preferences" item from bare bones apps, and provides a lead in for the introduction of DocumentMainWindow, which will introduce document management commands (new, open, save etc).

Also, the main_window documentation is about to become a lot more complicated in the next PR, which adds "background" and "session" apps. This would be helped if we eliminate the idea of a "simple app", and make "simple" or "main" purely a property of a window.

Agreed - this is a distinction that matters for internal reasons, but doesn't really matter from the perspective of the external API surface. I've simplified the docs to reflect this.

@freakboy3742 freakboy3742 requested a review from mhsmith June 20, 2024 00:15
@mhsmith
Copy link
Member

mhsmith commented Jun 20, 2024

We will still need the "minimal vs standard" API for other platforms, because we don't want to create command instances that literally won't (or can't) be used.

Even if a platform shows no menu bar with a base class Window, the commands could still be used if the app creates a MainWindow later. For example. when Android Studio has no projects open, it shows a window with a list of recent projects. On Windows, this window has no menu bar, but the per-project windows that it opens do.

I think we can roll this into the description of MainWindow - pointing out that if the platform uses app-level menus, using a MainWindow as the main window may add extra menu items. That then means we can hide the "preferences" item from bare bones apps, and provides a lead in for the introduction of DocumentMainWindow, which will introduce document management commands (new, open, save etc).

This still seems like a needless coupling between the app and the initial main_window:

  • It permanently commits the app to a set of commands based on what its initial main_window happens to be, even though main_window can be reassigned later.

  • It adds clutter to the App class by adding a new method which will rarely be overridden, while also adding complexity because it must be overridden to remove default commands, which you would naturally expect to be able to do in startup at the same time as you create your own commands.

  • I don't see how it generalizes to main_window=None, because we would have no idea whether the app's windows are going to be MainWindows or not.

I think all the goals of this change can be achieved in other, more flexible ways:

  • In the case of Preferences, the command can be added or not based on whether App.preferences has been overridden. Why should an app that doesn't have a preferences command be forced to clutter its user experience with a disabled menu item?

  • If DocumentMainWindow is going to have document management commands associated with it, that could be done with Per-window commands #2210, which is context-sensitive, not permanent.

docs/reference/api/app.rst Outdated Show resolved Hide resolved
docs/reference/api/mainwindow.rst Outdated Show resolved Hide resolved
@freakboy3742 freakboy3742 requested a review from mhsmith June 21, 2024 06:20
@freakboy3742
Copy link
Member Author

As discussed in person:

  • I've merged the "minimal" and "standard" app commands back into create_app_commands, which means we can go back to auto-creating the app commands before startup is invoked
  • The default commands are always installed, regardless of the main window type, and whether platform displays app-level menus..
  • I've modified the "Preferences" menu item to only be installed if the user overrides the preferences() method.

Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've modified the "Preferences" menu item to only be installed if the user overrides the preferences() method.

This should be documented at App.preferences and Command.PREFERENCES.

It should also be mentioned in the existing 90.feature.rst change note, which should probably be renamed to removal, because if anyone was using the command before, it will disappear until they implement App.preferences.

docs/reference/api/app.rst Outdated Show resolved Hide resolved
changes/2649.feature.rst Outdated Show resolved Hide resolved
docs/reference/api/mainwindow.rst Outdated Show resolved Hide resolved
docs/reference/api/mainwindow.rst Outdated Show resolved Hide resolved
winforms/src/toga_winforms/app.py Outdated Show resolved Hide resolved
@freakboy3742 freakboy3742 requested a review from mhsmith June 25, 2024 04:13
@mhsmith mhsmith merged commit bf99b38 into beeware:main Jun 25, 2024
32 of 35 checks passed
@freakboy3742 freakboy3742 deleted the simple-app branch June 25, 2024 22:01
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.

Add a Toga.SimpleApp base class
2 participants