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

pop_until_active does not pop screens #5166

Open
jakubziebin opened this issue Oct 24, 2024 · 5 comments
Open

pop_until_active does not pop screens #5166

jakubziebin opened this issue Oct 24, 2024 · 5 comments

Comments

@jakubziebin
Copy link

jakubziebin commented Oct 24, 2024

Hello, consider the following MRE:

from __future__ import annotations

from textual.app import App, ComposeResult
from textual.binding import Binding
from textual.screen import Screen
from textual.widgets import Footer, Label


class FirstScreen(Screen):
    BINDINGS = [
        Binding("n", "second_screen", "Second screen"),
    ]

    def compose(self) -> ComposeResult:
        yield Label("First screen. Press 'n' to go to the second screen.")
        yield Footer()

    def action_second_screen(self) -> None:
        self.app.push_screen(SecondScreen())


class SecondScreen(Screen):
    BINDINGS = [
        Binding("n", "third_screen", "Third screen"),
    ]

    def compose(self) -> ComposeResult:
        yield Label("Second screen. Press 'n' to go to the third screen.")
        yield Footer()

    async def action_third_screen(self) -> None:
        # going back to first screen and then replacing it with the third screen
        # self.app.get_screen works only with "installed" screens
        self.app.screen_stack[-1].pop_until_active()
        await self.app.switch_screen(ThirdScreen())


class ThirdScreen(Screen):
    def compose(self) -> ComposeResult:
        yield Label("Third screen.")
        yield Label(f"The screen stack looks like: {self.app.screen_stack}")
        yield Label("but should be: [Screen(id='_default'), ThirdScreen()]")


class MyApp(App):
    def on_mount(self) -> None:
        self.push_screen(FirstScreen())


MyApp().run()

version: 0.83.0

The problem is observed in situations where the next line must have the screen stack updated using the pop_until_active method.
In general, we are looking for a simple way to clear the screen stack. As it is read-only, we do not want to modify any protected attributes. It would be great to have a method to do this, it would be very helpful in applications with an onboarding process for example.

Copy link

We found the following entry in the FAQ which you may find helpful:

Feel free to close this issue if you found an answer in the FAQ. Otherwise, please give us a little time to review.

This is an automated reply, generated by FAQtory

@jakubziebin jakubziebin changed the title problem with pop_until_active() method pop_until_active does not drop off screens Oct 24, 2024
@jakubziebin jakubziebin changed the title pop_until_active does not drop off screens pop_until_active does not pop screens Oct 24, 2024
@willmcgugan
Copy link
Collaborator

The last item in the screen stack is always the active screen. self.app.screen_stack[-1].pop_until_active() doesn't make much sense, since screen_stack[-1] already the active screen.

I'd be surprised if you need pop_until_active at all. I suspect you could use modes

@jakubziebin
Copy link
Author

jakubziebin commented Oct 25, 2024

My mistake. In MRE, of course, it's about -2. In the example below I used instance lookup instead. After the change - incorrect behavior is still observed so the issue is justified.

I guess I can also use modes, however, it seems too complicated in my case to have multiple screen stacks and I feel like pop_unitl_active method should just work fine (if this bug wasn't there).

With modes, I still need to be able to manage the screen stack and dump a few screens or restore the stack to its initial state.
The second part seems to be achievable by dynamically removing and adding a new mode like (however pop_until_active still finds a use-case in modes):

self.app.remove_mode("some_mode")
self.app.add_mode("some_mode", SomeScreen)

However I would like it to be possible to manage screens in a better way. In the current form it seems to be insufficient for applications with multiple screens as it is quite limited. I believe that the mentioned public method should simply work as expected, and there could be additional screen management options apart from it. The above-mentioned stack clearing is a common situation even with modes, and the multiple screen pops to the expected screen offered by pop_until_active is also a non-imaginary use-case.

from __future__ import annotations

from textual.app import App, ComposeResult
from textual.binding import Binding
from textual.screen import Screen
from textual.widgets import Footer, Label


class FirstScreen(Screen):
    BINDINGS = [
        Binding("n", "second_screen", "Second screen"),
    ]

    def compose(self) -> ComposeResult:
        yield Label("First screen. Press 'n' to go to the second screen.")
        yield Footer()

    def action_second_screen(self) -> None:
        self.app.push_screen(SecondScreen())


class SecondScreen(Screen):
    BINDINGS = [
        Binding("n", "third_screen", "Third screen"),
    ]

    def compose(self) -> ComposeResult:
        yield Label("Second screen. Press 'n' to go to the third screen.")
        yield Footer()

    async def action_third_screen(self) -> None:
        """Going back to FirstScreen and then replacing it with the ThirdScreen"""
        for screen in self.app.screen_stack:
            if isinstance(screen, FirstScreen):
                # self.app.get_screen works only with "installed" screens
                # so we need to refer by index or do a loop like this
                screen.pop_until_active()
        await self.app.switch_screen(ThirdScreen())


class ThirdScreen(Screen):
    def compose(self) -> ComposeResult:
        yield Label("Third screen.")
        yield Label(f"The screen stack looks like: {self.app.screen_stack}")
        yield Label("but should be: [Screen(id='_default'), ThirdScreen()]")


class MyApp(App):
    def on_mount(self) -> None:
        self.push_screen(FirstScreen())


MyApp().run()

@willmcgugan
Copy link
Collaborator

When you call pop_until_active it doesn't occur immediately. It runs after the message handler exists, to avoid blocking the message loop. If you use call_later with your ThirdScreen self.call_later(self.app.switch_screen, ThirdScreen()), it should work as you expect.

I would really like to understand why you are working with screens in this way. Without understanding what you are trying to achieve, I can't help you with a potentially better solution or confidently make changes to the API.

For instance:

With modes, I still need to be able to manage the screen stack and dump a few screens or restore the stack to its initial state.

Why? What is in those screens? You seem to be insisting you need certain functionality that I have never needed, and nobody other than yourself and @mzebrak has requested. This suggests that A) you may have a fundamental misunderstanding regarding screens and modes, or B) I don't comprehend your use case. At this point, I don't know which is the case. Until I do, I can't make any changes to the API.

This is essentially an XY Problem.

So please, describe your app. What is in the screens that you are pushing? Why do you want to discard a bunch of them at once?

@mzebrak
Copy link

mzebrak commented Nov 28, 2024

@willmcgugan Okay, I’ll try to explain as best as I can.

You seem to be insisting you need certain functionality that I have never needed, and nobody other than yourself and @mzebrak has requested

It seems to me that the lack of need for such methods and not many issues about it is due to the fact that a large part of applications written using Textual can provide most of the functionality using a single screen or a very limited number of them (I mean, let's say up to 3 screens). When I say this, I don't have any bad intentions, I just think the main reason is that it is not tested enough by the developers yet.

I mean the applications like:

  • htop / top (not Textual, of course, but in this style)
  • trogon
  • elia
  • posting
  • frogmouth

So please, describe your app.

Due to the fact that the problem we are trying to solve is to create an application that is a desktop wallet (named Clive) for the blockchain world (Hive ecosystem), which supports over 100 types of operations, we need to provide a lot of functionalities in our application. Hive is a social blockchain, so apart from transfers, there are functionalities such as savings, voting, posts, comments, etc., and even custom operations. I think that Clive can be compared –in some way– to Amazon or any online store application because the main idea is similar - you add some operations to a cart (transaction), then sign it and broadcast them to the network (pay for them, order). However, we cannot generically categorize all operations together because they are so different from each other and require different things to be shown close to them regarding the network state.

If you so desire, you can run Clive and see what it looks like or take a look at the implementation. This is an open-source project. We are constantly working on it and changing it, but this will allow you to get the outline of what we want to create. Links here:


I think there are 2 approaches that can be taken in Textual when you need to deal with the fact the entire screen/terminal content changes drastically:

  1. Using separate screen implementations, which is IMO a very nice feature of Textual and a good idea to solve such a problem.

    This means that, like it or not, you have to face the additional “problem” of managing a stack of screens. And there is a small number of methods for making this possible: push_screen/pop_screen/pop_until_active / modes

  2. Constantly replacing some container on the main screen and thus constantly running unmount/mount - thus no need to manage the stack.

    But this has disadvantages because the screen stack itself is very useful - in some places it is worth having the ability to go back.

We use the 1st. approach and unfortunately run into many screen management issues. This issue is one of them.

If you use call_later with your ThirdScreen self.call_later(self.app.switch_screen, ThirdScreen()), it should work as you expect.

Thanks for the workaround, but we didn't use this solution because we finally decided to switch and use MODES in some situations, but I appreciate it. We now have 3 modes. One for the “create profile wizard”, one for the “login page” and one for giving access to the entire app after “logging in”. Btw with modes, there is a similar problem like “the need to restore a mode in its initial state (initial screen)” which can be now workarounded by remove_mode and add_mode but I think we’ll create another issue for that.

But modes do not solve all the problems.
I still think pop_until_active makes sense and we still use it in some situations.

Let's take a closer look at the Amazon web application because I think it has some analogy.

You have a home page (dashboard) and then a category page, product pages, a shopping cart screen, a finalization screen, some settings screens, etc. In our case, all of this is a separate Screen.

Now notice that you always have a header at the top with a big Amazon logo and by pressing it you can always go back to the home page no matter where you are currently. In my opinion, this is nothing more than just a bunch of pop_screen until you land on the dashboard (home page).

Why do you want to discard a bunch of them at once?

I mean, I don't want to keep too many screens in the history (screen stack), because we want the application to behave in such a way that it simply clears the (screen stack) history from time to time aka. pops screen(s) after some specific action. I believe that this is already a specificity of some applications built with Textual that at some point some screens should be discarded.

Keeping a large number of screens in a stack would probably also cause problems in the form of poorer performance, and as far as I know, non-active screens are also updated (can receive reactive updates).

I also don't feel like MODE is the solution there, because I just want the user to be able to return step by step from this structure by pressing the "ESC" button:

  • Cart/Summary
  • Single operation
  • Operations
  • Dashboard

So on the one hand, I would like to have the benefit of a stack (keep these screens in one stack and not several modes), to be able to return through each of them up to the dashboard via "Back". The dashboard should always be stacked as the 1st screen in a “logged in” mode. On the other hand, I want, for example, on the Cart / Summary screen to be able to click on the header and automatically drop the screens to the Dashboard. I can achieve this with just a single screen stack with the ability to pop to a certain screen.

You may say: Ok, so store Dashboard in another mode than Operations and all the following ones. This is not a solution since at some point you'll have exactly the same problem when you decide you want to drop screens until Operations while being in the Summary.

When you call pop_until_active it doesn't occur immediately. It runs after the message handler exists, to avoid blocking the message loop

It would be really great if it could return some AwaitComplete that would work and won't freeze the app. It looks like its current implementation means this method has the same issue as described in: #5008

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

No branches or pull requests

3 participants