-
-
Notifications
You must be signed in to change notification settings - Fork 675
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
Finalize API for Document-based apps #2666
Conversation
018608a
to
eb65ee9
Compare
ccbc45b
to
0e3d025
Compare
I agree backwards compatibility isn't a concern – as far I can tell from GitHub and Discord, the old API has never been used outside of Podium. However, I think using the old API as a starting point has resulted in a design that's more complicated than necessary. In particular, I don't see why Document and DocumentMainWindow are separate classes, when the bulk of the implementation of a Document subclass would be about manipulating the window anyway. I'd also like to revisit the idea discussed in #2244 of moving the documents API into a DocumentApp subclass which can be tested entirely in the core, with the help of an First let's look at the command-related methods in the App class, which are the main reason for that class's expansion in this PR and #2688. We started off with just one such method ( So I suggest we abandon this pattern of adding commands by overriding App methods. Instead, we can make it possible to create a Command by passing nothing but one of the standard IDs, e.g. The rest of the API would then look roughly like this: class App:
def on_open_file(self, path):
"""Event handler, which only fires on macOS. Overridable in the same way as
on_exit and on_running.
"""
# The remaining 3 classes can all be documented on the same page:
class DocumentApp(App):
@property
def document_types(self) -> dict[DocumentType, type[DocumentWindow]]:
"""The first item in the dictionary is the default type, and the docs should
explain what the effects of that are.
"""
def startup(self):
"""
Add the file management commands. There's no need for them to be connected
to public methods; they can just be described on the documentation page.
New: Create and show a DocumentWindow of the correct type, passing no `path`
argument.
The command for the default type would be created using Command.NEW as
discussed above, and the other ones would be copied from it with different
text and id, and no shortcut. If there's only one document type, its
command text should be left at the original value of "New".
Open: Display a dialog, and based on the filename extension that comes back,
create and show a DocumentWindow of the correct type, passing a `path`
argument.
Additionally, if there's an existing DocumentWindow that contains an
unmodified untitled document, which is the initial condition on
Windows and GTK, then close that window.
Save, Save As, Save All: Call the corresponding DocumentWindow methods,
as in the current PR.
The code currently in _create_initial_windows would also be moved here. And
we can set main_window = None, so the subclass doesn't have to.
Docstring should say that if you override this method in a subclass, you must
call the base class implementation.
"""
def on_open_file(self, path):
"""Event handler implementation; does the same thing as the Open command,
except with no dialog."""
class DocumentType:
name: str
extensions: list[str]
class DocumentWindow(MainWindow, ABC):
"""Including 'Main' in the name seems unnecessary."""
def __init__(..., document_type, path, ...):
"""If a path was provided, call `read`.
Subclasses should override this method, passing through all arguments
and adding a `content` argument."""
@property
def document_type(self) -> DocumentType:
"""This property is read-only and required in the constructor."""
@property
def path(self) -> Path | None:
"""Setting this property sets the window title to `path.stem` or 'Untitled'."""
def save(self):
"""If `write` is not overridden, do nothing. Later, when we have per-window
commands, the constructor would omit the save commands in this case.
Else if `self.path` is None, call `save_as`.
Otherwise, call `write`."""
def save_as(self):
"""If `write` is not overridden, do nothing.
Otherwise, set self.path from a SaveFileDialog, then call `write`.
The behavior of replacement_filename should be moved into SaveFileDialog,
if it isn't already implemented by the platform (I think it is on Windows).
"""
@abstractmethod
def read(self):
"""Read self.path into the window."""
@overridable
def write(self):
"""Write the window content to self.path."""
pass
@property
def modified(self) -> bool:
"""Whether the document has been modified since the last open or save."""
return False On macOS:
If we go this way, I think it makes sense to close #2688 and do all the commands in this PR. |
Primarily because there can be a many-to-one or one-to-many relationship between the windows for a document, and the document as a concept. The simple case is definitely 1-1 (e.g., a text editor has one window per text document); but that's not the only way a document-based app can be organised. Podium is an example of the many-to-one case - a slide deck is a single document, but there can be multiple views of that slide deck (the slide view, the presenter view, an editing panel for slide content ...) that might be visible at the same time. One of those is nominated as the "main" document window - in the Podium case, this is then set up so that closing the "main" window closes all the other windows related to that document. There could also be a one-to-many relationship. A VSCode-style IDE could be modelled as one window managing multiple documents. There might be better ways to represent this specific case (e.g., treating an IDE window as single "project" document), but a one-to-many relationship isn't out of the question. Aside from data modelling, there's also separation of concerns. How a document is displayed doesn't need to be coupled to how it is stored. This sort of conceptual separation is at the core of MVC or MVVC style data modelling. Consider the case where the app wants a new document to be opened into an existing document window - you really want to preserve a single window instance, but make the window display a different document. It's almost certainly possible to manufacture this in a single DocumentWindow setup, but having the objects separate makes the separation of concern a lot clearer.
That doesn't match my understanding of where we ended up with #2244 - in that discussion, the general drive was away from specialist app classes, and towards incorporating the behaviours of the app classes as capabilities of a single App class. Aside from making testing easier (as we don't need to dance around creating a second App instance for testing purposes), the biggest argument for this was that having a "type" of application was unnecessarily limiting. What if I want a status icon app that is able to manage documents? What if I need an app that does have a true "main" window, but also needs to manage documents. Keeping "document management" as a feature of a single app class rather than a specialised type of app makes it a lot easier to accomodate any type of app behavior the end user may want.
I'm sure the documentation could be improved, and I'm definitely open to suggestions on reorganization. I've tried to keep most of the discussion of document-based behavior in the documentation for Document, with references in DocumentMainWindow and App linking to the Document docs as soon as the topic goes beyond "command line handling" and into document-based specifics. I'm also open to alternate naming (or a naming conventions) for the "capabilities" that can be user-defined (like preferences, new, open etc). I definitely acknowledge that Using Following this pattern, In both these cases, the However, as I said, I acknowledge there's some naming overload, so I wouldn't object to renaming these methods - As for cut/copy/paste - those specific commands aren't going to be simple, because they're going to require interaction with the operating system to ensure that they continue to work for text fields etc. My guess is that copy/paste is more likely to be a widget-level feature that is related to focus handling - so a user-space handlers replaced at the Command level is likely to be implausible.
If I'm understanding this correctly, you're suggesting that it would be possible to construct a Command without an action; but if the ID was a "known" ID, then you'd get a the default behavior for that ID - and this becomes the preferred way to register new commands, rather than overriding. One issue I see with this approach is all the other arguments to Command (shortcut, group, section etc). These are subtly different on each platform; so a "Preferences" or "New" command defined in user space would need a way to inherit/obtain these extra arguments. I guess it might be possible to have a similar "default" approach like the action; but if we do this, we're essentially making ID a "magic" argument to the constructor that implies other values. However, my real problem here is that I don't see what we gain by taking this approach to command management. Specific method naming notwithstanding, I don't see that overriding a method is an especially unintuitive mechanism for accessing (and enabling) customised file handling, especially when And, in the "default" case of document handling, there's no need to override anything - declaring a document type enables the default menus for open, save etc; and the default behaviors are all tied to the document, and how the document is read/written. So - adopting a
If I'm reading this right, the key points of difference are:
I've addressed (1) and (2) above; (1) doesn't address separation of concerns or N-N entity relationships; (2) requires more code to achieve the same result, and requires an (arguably) artificial distinction between a "document" app and all other apps. (3) is a fairly minor API difference; it wouldn't be too hard to incorporate into this PR by making path an optional argument to the Document constructor.
This would also require that users explicitly invoke the
My thinking here was naming consistency - |
Summarising in person discussion between myself and @mhsmith: The problems:
The solution we came to:
As an end-user, there will be no change to the API if the user chooses to use default document types handling, or the user doesn't want any of the default document management commands. However, an end-user could also add:
to add a New and Save handler to their app with default registration, modify the action for the About command, and remove the visit webpage menu item. There may be modifications to error handling required to ensure that missing commands don't cause errors. |
It's currently called
To support this, it should be possible to assign the
It might be cleaner to close the untitled window and open a new one, to avoid requiring all document windows to accept changes to the Either way, to determine whether it's safe to discard an untitled document, we would require a |
(It would help if I didn't forget the name of the APIs I've added and confuse the issue with new names....) I think my intention with
I'm not sure if you'd be changing a single doc property for a multiple-doc window. My inclination is that you'd have a window manage a list/set of documents, with a However, I don't think we need to resolve that design decision now; we can defer until we want to add a tabbed document window.
Possibly; although it might be sufficient to document that
I'm not sure this is necessarily true. My thinking was that this could be tied into on_close handling; the only difference between an That said, there's also some differences in behavior between platforms around what will cause a new window to be created (i.e., when does "Open" create a new window, and when does it change the content of the existing window). On macOS, every "New" and "Open" creates a new window (AFAICT - it definitely does for TextEdit). GTK's text editor has an Open that always opens in the existing window; but "New" creates a new window (technically, there's "New Window" and "New Document"; new Document creates a second tab in the same window). Windows Notepad is mostly the same as GTK, but it differentiates between "New" and "New Window". "New" creates a new document in the current window; "New Window" creates a new document in a new window. On GTK and Windows, "Open" cares if the existing content has been modified; macOS doesn't. This seems consistent with the "you can have no windows open" approach taken by macOS. At the very least, there's some variation in logic required for the "new" and "open" menu items. |
Co-authored-by: Malcolm Smith <smith@chaquo.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this is the last set of significant design comments, I promise. 🙂
# CLOSE_ON_LAST_WINDOW is a proxy for the GTK/Windows behavior of loading content | ||
# into the existing window. This is actually implemented by creating a new window | ||
# and disposing of the old one; mark the current window for cleanup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this means apps will be much less capable on GTK and Windows than on macOS, because they'll only support one document at a time. So I don't think we should hard-code this single-window behavior in the general case – we should leave that up to the Document subclass.
I feel like most cross-platform developers would prefer to stick with the same document-window relationship behavior across all platforms. For example, on both Mac and Windows, LibreOffice always opens one document per window but allows multiple windows, while VS Code always uses a single window per workspace with multiple documents in tabs. And an app that really does intend to support only one document at a time, probably wants that on all platforms as well.
The "replace" behavior should be retained for the special case of replacing an unmodified untitled document, but moved to open
so it can also be triggered by other ways of opening a document, e.g. an "Open Recent" menu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that a document app should definitely use (or, at least, be able to use) multiple windows, regardless of platform.
There's an edge case you're missing in your analysis, though - New. On GTK/Windows, File > New always creates a new window; File > Open replaces the existing window.
From my survey of platform behavior when I set this up, on Windows, most apps that I saw (e.g., notepad) have distinct "New" and "New Window" commands, with "New" being a replacing operation by default. That's a little difficult to accomodate cleanly when there are potentially multiple document types, because you end up with 2 New commands for every document type, so your File menu gets busy really quickly. Open is always a replacing operation.
The behaviour is less consistent on GTK (shocking nobody), but most apps (even Terminal) have an app-level "New window" command in addition to an "open" that has replacing behavior; if there's a "new document" option, it often creates a second tab in the same window (which isn't something we're in a place to support anyway).
So - I opted for a single "New" that is effectively "new window" on all platforms. For the "single document window" case, this seems the most consistent without adding an explicit second new command; and in a multiple-document window (such as an IDE), it should be possible to incorporate "open tab rather than replace" behavior with some relatively minor affordances in the Document class for the multi-document app, plus a user-added "New window" command.
It's also possible for a "single document window" app on GTK/Windows to circumvent the replacement behavior by deleting the _replace
attribute of the current window before creating the new window in the Document's create()
method. This isn't an official API - we'd possibly want to find a cleaner way to formalise this if we want to make it official - but it's at least possible.
The other possible missing piece that GTK and Windows don't currently have is the analog of "drag document onto app" or "double click on icon". On macOS, this opens a new document window in the same app instance; I'm not sure what (if any) mechanisms exist to make this sort of behavior work on GTK and Windows, but in the meantime, you'll get 2 app instances with distinct document windows.
Regarding the larger "meta" point - I'm not sure I agree that cross-platform consistency is as important as platform-native consistency, provided the consequences for the developer aren't invasive. Replace behavior feels alien on macOS; non-replace behavior feels alien on macOS and GTK. Given that platform consistency is one of the key design principles of Toga, and we've got a reasonably clean way of accomodating both in a way that only affects how the user uses the app, not how it is built, it seems reasonable to me to retain the platform consistency.
And - as a note related to your other review comment about merging create
and open
- it occurs to me that there's possibly an option here for formalising how replace
works by making replace
an optional method on the Document class that is only invoked if it is defined and the platform supports replacing behaviour, and then pass the replacement request to the creation of the document. I don't think we need to resolve that design decision right now - I think it fits better in a discussion about formally adding multi-document apps - but it's worth noting that this approach wouldn't be possible if create
and open
were merged.
|
||
if TYPE_CHECKING: | ||
from toga.app import App | ||
from toga.window import Window | ||
|
||
|
||
class Document(ABC): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible simplifications of the Document class:
-
create
,open
, andread
will only be called once, in that order. So what if we mergedcreate
andread
into a single overridable method that's responsible for both creating the UI and loading a file into it? For consistency with the other classes, the best name for this method would beopen
. The code currently inopen
can then move to the constructor, which would once again take apath
argument which can be None for a new document. -
Similarly, the overridable
write
method would be renamed tosave
, and the code currently insave
can move toDocumentWindow.save
. -
Separately,
Document.show
seems unnecessary, and also incomplete since there's noDocument.hide
. Suggest removing this method and saying that whoever creates the window is also responsible for showing it, just like everywhere else in Toga.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible simplifications of the Document class:
I'm not sure these simplifications are actually possible (or, to the extent they're possible, they won't represent an improvement).
open()
does more than just read the file; it also does path normalisation, error handling, window title handling, and the initial dirty state management. read()
is then the extension point that the user can implement so that they only need to deal with the "get the data from the file and populate the window" portion, and can implement that with known the pre-condition that the file must exist. The only restructure of this that I can see would require either:
- every user implementation of a unified
open()
to callsuper().open()
and potentially handle the fact that open might raise an exception or not require opening a file at all; or - pulling the current logic of
open()
into the place whereopen()
is being invoked... which means we've got the same code, just wrapped up into a more complexDocumentWindow.open()
implementation.
The same is true of save()
and write()
.
Separating create()
from open()
is necessary to support untitled documents. Otherwise, we'd need to modify open()
to handle path=None
(or similar) as a "don't open a file" case. Sure, this is possible, but it's a bit messy IMHO. It also requires the user to convolve "creating the window" logic with "reading the file" logic (which makes testing a little harder), and makes operations like "refresh the content of the current window in response to changes on disk" harder (an edge case, to be sure, but not a completely unreasonable one).
A standalone show()
is needed to support deferring making the window visible until after it's fully loaded. Without it, when opening a new document, you'd see an untitled window that is then populated with content (which might take time and result in visible redraws); or, in case of a failed open, a temporary window that then disappears.
I can't think of a good use case for a hide()
method; but for the sake of parity, it's not hard to implement.
Co-authored-by: Malcolm Smith <smith@chaquo.com>
Derived from #2244.
Finalizes the Document-based app interface.
This unfortunately involves some backwards incompatible changes; I can't see any obvious way to retain compatibility with the old API and also allow for the flexibility needed to support GTK, Windows and "new/save" functionality. The changes aren't that invasive... I hope?
Summary of changes:
document_type
dictionary.on_close
handler, rather than a specialised interface. The defaulton_close
handler tracks modification status of the document; a document can be marked as modified using thetouch()
API.There's no longer a need for an implementation-level document. The only backend that was using it was macOS; and it turns out we're not actually using any of the functionality that might make that interesting (like CoreData support). AFAICT, we can meet all the same app requirements using Python's file handling, and the standard cocoa command line handling API.
The testbed has been updated to use an automated window cleanup mechanism, rather than requiring each test to explicitly close windows that have been opened. This also ensures that garbage collection is enforced in the GUI thread (this was being done in the Window tests, but not in other examples). This is an extended version of what was included in #2682; this PR also removes the try/finally blocks that were manually implementing window cleanup.
Fixes #2209.
PR Checklist: