-
Notifications
You must be signed in to change notification settings - Fork 853
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 support for "returning" a result value from screens. #2321
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is roughly how it should work. Having got this going and constructed test code to go with it (outwith of this commit, not unit testing code, just a test app to try out the ideas), I wanted to get this onto the forge for further mulling over tomorrow. The one sneaky/questionable thing here is that I'm sort of dumpster-diving the screen stack to get the "parent" screen, to make the callback in context. This both feels right and feels like a cheat. On the other hand it's public for a reason, right? Right?
It is possible for the same instance of a screen to get pushed onto the screen stack multiple times; as such we really need to keep track of all the callback requests. So here I register a callback for every screen push and clean it up on every screen pop; with those without callbacks being no-ops.
Under normal circumstances the code wouldn't encounter this problem as there's always a default screen; but a handful of tests that were testing the screen stack broke after the recent additions relating to result callbacks. This cleans up that problem.
Just go with a single dismiss method.
willmcgugan
requested changes
Apr 19, 2023
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 think we will need to discard callbacks for switch screens.
As per the observation here: Textualize#2321 (review)
Co-authored-by: Will McGugan <willmcgugan@gmail.com>
@willmcgugan Good call. Done: 9123a80 |
willmcgugan
approved these changes
Apr 19, 2023
This was referenced Apr 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is still in the "let's see if we like this approach" stage, although I think @willmcgugan and myself are liking this approach. Testing and review of these changes is highly encouraged. Beyond docstrings there are no documentation changes at the moment. This will need a slight reworking of the
Screen
guide so it's better that we're happy with the approach and the code before we write this up fully.Please review without worrying about documentation for now.
This PR seeks to satisfy #2267 and does take the suggested callback approach. The alternative was to go with a "use messages" story that, while working well, places more work on the developer using Textual and potentially results in more boilerplate.
Changes of note here are:
App
, aScreen
can now be typed. The type signifies the type of the result that it will "return".App.push_screen
has acquired acallback
parameter. If provided the callback function should be a function that takes a single argument, that is the type of the type given to the screen being pushed.Screen
has acquired a new method calleddismiss
. This is a thin wrapper aroundApp.pop_screen
. Called with no parameter it performs the same as ifApp.pop_screen
were called. If called with a parameter the callback provided withApp.push_screen
is called and then the screen is popped.Screen
has also acquired anaction_dismiss
method to allow for easy dismissal of a screen from within an action.Other things to keep in mind:
As an illustration of how this might be used, here's a small program that uses a
ModalScreen
to ask the user a question and then act on their answer:Screen.Recording.2023-04-18.at.14.44.50.mov