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

A way to mount a composed widget, but so that its children are mounted separately (ft. Grid) #3483

Closed
mzebrak opened this issue Oct 9, 2023 · 15 comments

Comments

@mzebrak
Copy link

mzebrak commented Oct 9, 2023

AFAIK now there is no way to deal with such a situation, and I noticed that it would be a very useful possibility. For example, in this simple example - when we want to have a some common part of the code, so TitledInput is created, but it can't be used in a grid correctly.

And the yield from <>.compose() is a problem because the parent widget is not mounted and
yield <> is also a problem as it is mounted as a whole and not separate parts.

from textual import on
from textual.app import App, ComposeResult
from textual.containers import Grid
from textual.widget import Widget
from textual.widgets import Input, Label, TextLog


class TitledInput(Widget):
    DEFAULT_CSS = """
    Label {
        background: green;  # not working because this one is never mounted
    }
    """

    def __init__(self, title: str) -> None:
        super().__init__()
        self.title = title

    def compose(self) -> ComposeResult:
        yield Label(self.title)
        yield Input()

    @on(Input.Changed)
    def update_logs(self, event: Input.Changed) -> None:
        self.app.query_one(TextLog).write(f"{self.title} changed: {event.value}")


class YieldFromComposeApp(App[None]):
    DEFAULT_CSS = """
    TextLog {
        height: 10;
        background: $panel !important;
    }
    
    Grid {
        background: $primary;
        margin: 5;
        grid-size: 2;
        grid-columns: 15 1fr;
        grid-rows: 4;
        height: auto !important;
    }
    
    Label {
        padding: 1;
            background: red;
    }
    """

    def compose(self) -> ComposeResult:
        yield TextLog()
        with Grid():
            yield from self._get_titleds_1()
            # yield from self._get_titleds_2()
            # yield from self._get_titleds_3()
            # yield from self._get_titleds_4()

    def _get_titleds_1(self) -> ComposeResult:
        """This one works partially, because the TitledInput is not mounted.

        This `yield from` is not quite valid because TitledInput won't be added to the list of nodes, so title is red.
        But this is the layout we want to achieve, (with green titles and working TextLog).
        """
        yield from TitledInput("username:").compose()
        yield from TitledInput("password:").compose()

    def _get_titleds_2(self) -> ComposeResult:
        """Would be really nice if this one worked as expected, but it doesn't."""
        yield from TitledInput("username:")
        yield from TitledInput("password:")

    def _get_titleds_3(self) -> ComposeResult:
        """Layout is broken, but titles are green.

        can't do `yield TitledInput()` because I need to have both its child in the grid (to be able to align them properly)
        """
        yield TitledInput("username:")
        yield TitledInput("password:")

    def _get_titleds_4(self) -> ComposeResult:
        """This one works partially, but is quite hacky also. Would be nice if there was a better way like `_get_titleds_2`.

        Setting CSS to explicitly width=0, height=0 instead of display=False also doesnt work.
        """
        titled_1 = TitledInput("username:")
        titled_2 = TitledInput("password:")

        titled_1.display = False
        titled_2.display = False

        self.mount(titled_1)
        self.mount(titled_2)

        yield from titled_1.compose()   # .children causes nothing to appear
        yield from titled_2.compose() 


if __name__ == "__main__":
    YieldFromComposeApp().run()
@Textualize Textualize deleted a comment from github-actions bot Oct 9, 2023
@willmcgugan
Copy link
Collaborator

It is not at all clear what you want to do here. I'm going to guess you want a grid with title in the first column and input in the next? To do that you would need separate widgets for label / input, not a compound widget. Then your compose would look like the following:

yield Label("input 1")
yield Input(id="input1")
yield Label("input 1")
yield Input(id="input2")

@mzebrak
Copy link
Author

mzebrak commented Oct 9, 2023

It is not at all clear what you want to do here. I'm going to guess you want a grid with title in the first column and input in the next? To do that you would need separate widgets for label / input, not a compound widget. Then your compose would look like the following:

yield Label("input 1")
yield Input(id="input1")
yield Label("input 1")
yield Input(id="input2")

Of course I know I can do it without the compund widget and it will work. That's why this issue was created - because it would be useful to be able to do this with the compund widget also.

The point is that I want to have it in a compound widget - the mentioned example is simple to be easy to read, but in a real application I don't want to have different logic in 1000 places, I want to have it in a common class.

I mean creating complex controls (custom widgets) like Input with Select combined - an example is the currency selection (value is given via Input and currency is selected via Select). In the grid, I want to be able to control width of both columns, i.e. have access to the children of the compund widget, and the compund widget itself will then consist of :

def compose(self) -> ComposeResult:
    yield Input()
    yield Select()

now imagine that I need to add the @on(Select.Changed) logic there - it's great, I can do it just in one place, and in your example I have to include it in 1000 other places.

@willmcgugan
Copy link
Collaborator

@mzebrak In order to assist you, we need to know what you are trying to achieve. Not what you tried that didn't work.

https://label.dev/articles/xy-problem/

@mzebrak
Copy link
Author

mzebrak commented Oct 9, 2023

@mzebrak In order to assist you, we need to know what you are trying to achieve. Not what you tried that didn't work.

https://label.dev/articles/xy-problem/

As I clearly indicated - I want to create a custom widget that consists of Input and Select combined... - an example is the currency selection. Why are you referring me to the x-y problem article for this reason?

@davep
Copy link
Contributor

davep commented Oct 9, 2023

Something akin to this?

from textual import on
from textual.app import App, ComposeResult
from textual.containers import Grid
from textual.widgets import Label, Input, Log

class TitledInput(Grid):

    DEFAULT_CSS = """
    TitledInput {
        grid-size: 2;
        height: auto;
    }

    Label {
        background: green;
    }
    """

    def __init__(self, title: str) -> None:
        super().__init__()
        self.title = title

    def compose(self) -> ComposeResult:
        yield Label(self.title)
        yield Input()

class KindaGridApp(App[None]):

    def compose(self) -> ComposeResult:
        yield Log()
        for n in range(4):
            yield TitledInput(f"Input {n}")

    @on(Input.Changed)
    def update_logs(self, event: Input.Changed) -> None:
        self.query_one(Log).write_line(f"{event.control.parent.title} changed: {event.value}")

if __name__ == "__main__":
    KindaGridApp().run()

davep added a commit to davep/textual-sandbox that referenced this issue Oct 9, 2023
@mzebrak
Copy link
Author

mzebrak commented Oct 9, 2023

Something akin to this?

from textual import on
from textual.app import App, ComposeResult
from textual.containers import Grid
from textual.widgets import Label, Input, Log

class TitledInput(Grid):

    DEFAULT_CSS = """
    TitledInput {
        grid-size: 2;
        height: auto;
    }

    Label {
        background: green;
    }
    """

    def __init__(self, title: str) -> None:
        super().__init__()
        self.title = title

    def compose(self) -> ComposeResult:
        yield Label(self.title)
        yield Input()

class KindaGridApp(App[None]):

    def compose(self) -> ComposeResult:
        yield Log()
        for n in range(4):
            yield TitledInput(f"Input {n}")

    @on(Input.Changed)
    def update_logs(self, event: Input.Changed) -> None:
        self.query_one(Log).write_line(f"{event.control.parent.title} changed: {event.value}")

if __name__ == "__main__":
    KindaGridApp().run()

thanks for your help, not entirely, it's a bit more complicated... an external grid container is required - the point is that in our application we can create about 100 operations that you add to the cart - these operations are nothing more than a form, but a little more advanced.

this might give you some context:
image

which compose consists of:

def create_left_panel() -> ComposeResult:
    yield BigTitle("Transfer to account")
    with ScrollableContainer(), Grid():
        to_label, to_input = AccountNameInput().compose()

        yield InputLabel("from")
        yield Label(self.name)
        yield Static()
        yield to_label
        yield to_input
        yield KnownAccount(to_input)
        yield from AssetAmountInput().compose()
        yield Static()
        yield from MemoInput().compose()

There are spaces for entering specific values/or selecting them. One of such places is value and currency (a single compound widget that we have used so far as yield from compose - of course, which meant it was not mounted and this widget was only a wrapper, because e.g. self.quetry does not work in it because it is not mounted.)

It was quite a short-sighted decision when, as you can expect, the lack of the ability to mount the widget is rather a bad thing in the long run (e.g. when input validators appeared, and we now can't use them), we would like to react to Messages and have access to other things that are available when the widget is only mounted.

Now, since in our case the external with Grid: turned out to be the best option - among other things, to equalize the titles of the inputs, on the other hand, it is problematic, because it is not possible to control the arrangement of such a compound widget.

=== EDIT:
The line of reasoning looked something like this:

  1. there is a need to have labeled Inputs (title) and everything aligned nicely
  2. put everything in a grid because it can be easily adjusted
  3. new compound widgets have been created, we have a problem, how to align them inside a grid?
  4. yield from compose
  5. parent widget is not mounted -> we have a problem because the widget is not mounted and in Textual it common all things have to be mounted for other things to work.

@davep
Copy link
Contributor

davep commented Oct 9, 2023

I think I may still be struggling to comprehend the issue being described here, because as I look at the above I sense the broad approach I suggested here would be applicable.

@willmcgugan
Copy link
Collaborator

yield from widget.compose() simply isn't going to work. If you yield a widget, it will compose its children by default. Mounting a widget will mount all its children.

@willmcgugan
Copy link
Collaborator

willmcgugan commented Oct 9, 2023

I wonder if you are struggling with composing nested widgets? This code implements a form.

Screenshot 2023-10-09 at 17 37 48

@mzebrak
Copy link
Author

mzebrak commented Oct 10, 2023

yield from widget.compose() simply isn't going to work. If you yield a widget, it will compose its children by default. Mounting a widget will mount all its children.

This is exactly what is this issue about - the possibility of 'yielding' so that this widget was taken into account by the textual (it was mounted), and instead of it itself - yield its children

I'm not saying that it has to be done with yield from widget.compose(), even better would be yield from widget or introduce some other widget class that behaves this way by default and yield widget will work that way, but yield from widget seems to make more sense anyway (it clearly expresses what we want to achieve).

I wonder if you are struggling with composing nested widgets? This code implements a form.

Unfortunately, I still see that maybe you don't understand me and I don't know why you are trying to abandon the use of compound widget when even a similar example is covered in the documentation. In the example you link to, a completely different approach is used, the elements are stripped down into prime factors . This is what I want to avoid.

Instead, I'm asking about a way/new feature to do this using a compound widget.

Why compound widget?
Since you explicitly mention this approach in the documentation, even my example coincides with the one included in the documentation - compound widget combining Label and Input here.

Why not break it down into prime factors as you suggest - because, as I have already mentioned, since these are elements related to each other and only collected in a group in the compound widget class - I want their logic to be included here as well, and it does not have to be externalized. It's just that since Textual allows feature compound widgets - I would like to use it, I don't want to decompose it... for me it is one whole - one of my own widget/component.

I also want to reuse it in many places, I don't need several elements for this like separate Label, Input, and ErrorLabel and yield 3 times.
I want to have them collected together in my compound widget.

So where is the problem?
It arises when we want to position them in a rational way, e.g. to ensure that the Label of each compound widget has the same width on a specific Screen - it would be helpful to place everything in the Grid, but since these are compound widgets in their current form, we cannot refer to them separately in the grid.

So everything boils down to a simple question - how to control width/height of given elements using compound widget in Grid? Currently this is not possible, but maybe it should be.

Modified example of link, so we can have everything in a grid. I hope I don't have to explain why grid... I want to easily manage this layout when adding, for example, another element in a new column to the middle row, it would get complicated with Vertical/Horizontal containers or not even possible.

from textual.app import App, ComposeResult
from textual.containers import Grid
from textual.widget import Widget
from textual.widgets import Input, Label, Select, Static


class InputWithLabel(Widget):
    """An input with a label."""

    DEFAULT_CSS = """
    InputWithLabel {
        layout: horizontal;
        height: auto;
    }
    InputWithLabel Label {
        padding: 1;
        text-align: right;
        background: red;
    }
    InputWithLabel Input {
        width: 1fr;
    }
    """

    def __init__(self, input_label: str, with_select: bool = False) -> None:
        self.input_label = input_label
        self.with_select = with_select
        super().__init__()

    def compose(self) -> ComposeResult:


        yield Label(self.input_label)
        yield Input()
        if self.with_select:
            yield Select([("a", "A"), ("b", "B"), ("c", "C")])


class CompoundApp(App):
    CSS = """
    InputWithLabel {
        width: 80%;
        margin: 1;
    }
    
    Grid {
        grid-size: 3;
        grid-columns: 12 1fr 1fr;
        grid-rows: 4;
        height: auto !important;
    }
    """

    def compose(self) -> ComposeResult:
        with Grid():
            yield from InputWithLabel("First Name").compose()
            yield Static()  # empty cell
            yield from InputWithLabel("Last Name", with_select=True).compose()
            yield from InputWithLabel("Email").compose()
            yield Static()  # empty cell


if __name__ == "__main__":
    app = CompoundApp()
    app.run()

@davep
Copy link
Contributor

davep commented Oct 10, 2023

Given the above example, this takes me back to my original suggestion. If I were to create the layout you were going for there, I'd do it like this:

from textual.app import App, ComposeResult
from textual.containers import Grid
from textual.widgets import Label, Input, Select

class StandardRow(Grid):
    """A standard row in the display.

    Other compound widgets inherit from this.
    """

    DEFAULT_CSS = """
    StandardRow {
        grid-size: 3;
        grid-columns: 12 1fr 1fr;
        height: auto;
        margin-bottom: 1;
    }
    """

class InputWithLabel(StandardRow):
    """A labeled input row with optional select."""

    def __init__(self, input_label: str, with_select: bool = False) -> None:
        self.input_label = input_label
        self.with_select = with_select
        super().__init__()

    def compose(self) -> ComposeResult:
        yield Label(self.input_label)
        yield Input()
        if self.with_select:
            yield Select([("a", "A"), ("b", "B"), ("c", "C")])

class Issue3483Example(App[None]):

    def compose(self) -> ComposeResult:
        yield InputWithLabel("First Name")
        yield InputWithLabel("Last Name", with_select=True)
        yield InputWithLabel("Email")

if __name__ == "__main__":
    Issue3483Example().run()

This retains the InputWithLabel compound widget, retains the layout you created, requires three fewer widgets, and introduces a standard base class for any other kind of row-based compound widget you wish to have in the same display; so if you want to tweak the proportions of the columns later on you only need to change in one place and every compound row-oriented input widget that's created in your application just adopts the new layout.

Here is the result of your code and mine side-by-side:

Screenshot 2023-10-10 at 08 25 48

davep added a commit to davep/textual-sandbox that referenced this issue Oct 10, 2023
@willmcgugan
Copy link
Collaborator

@mzebrak I am going to close this issue. We've tried very hard to help you, but we still don't know what you are asking for that Textual doesn't already do by default.

If you can think of an alternative way to express your problem, then feel free to open another issue. But please don't continue to use constructs that we have already explained won't work, like yield from compose. And please when you say you think something doesn't work, tells us why you think that.

@github-actions
Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

@mzebrak
Copy link
Author

mzebrak commented Oct 10, 2023

@davep Thanks for this example, it seems quite spot on. On 0.31.0, when running your example - I can see nothing on the screen, but on 0.38.1 it works fine. In fact, throwing CSS into the base class is a reasonable solution here, I didn't notice such an option in the previous example, and I was thinking about editing many places when there will be need to modify the width/height.

However I have the impression that this is not a perfect solution, because if there is an external grid container that covers it all, it is more difficult to make any unforeseen modifications to the layout. However, this is probably the best solution in a given situation, if we reject the idea of ​​something like yield from. I will try this way, thanks again.

@davep
Copy link
Contributor

davep commented Oct 10, 2023

Glad to hear it helped. Having a base class with the common styling would be one common idiom and one I use as much as possible as it makes it very easy to maintain code.

On 0.31.0, when running your example - I can see nothing on the screen, but on 0.38.1 it works fine.

It's always best to assume that all examples given are intended for the latest stable release of Textual as of the time of writing; even more the case when an issue is raised where the author has not included the output of textual diagnose as the template requests.

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