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

mount should have additional options to insert new widgets #778

Closed
willmcgugan opened this issue Sep 15, 2022 · 5 comments · Fixed by #1095
Closed

mount should have additional options to insert new widgets #778

willmcgugan opened this issue Sep 15, 2022 · 5 comments · Fixed by #1095
Assignees

Comments

@willmcgugan
Copy link
Collaborator

willmcgugan commented Sep 15, 2022

Currently, mount() inserts new widgets at the end of the list.

We should provide some way of inserting widgets at the beginning and arbitrary point.

I'm thinking an before and after parameter which would accept an integer or a CSS query. And insert the new widget at a matching index or query. Indexes can be negative, so after=-1 would insert at the end of the list.

The default for before and after should be None, which would insert at the end of the list.

@davep davep self-assigned this Oct 26, 2022
@davep
Copy link
Contributor

davep commented Oct 26, 2022

First thing to do, before tacking this, is to remove support for adding "anonymous" widgets vs "named" widgets. This will mean dropping support in the App and Widget mount methods, and also looking through the code, examples and documentation to remove any use and mention of this.

davep added a commit to davep/textual that referenced this issue Oct 26, 2022
Before now it was possible to do something like:

  self.mount(hello=Static("Hello!"))

and the Static widget would be created with the ID of 'hello'. Pretty much
the same as doing:

  self.mount(Static("Hello!", id="hello"))

The former wasn't very well advertised in the documentation, wasn't used in
any example code and wasn't used in internally. Meanwhile it also meant that
it was going to be tricky to add extra parameters to the mount
method (something we want to do for Textualize#778).

This commit removes that support.
davep added a commit to davep/textual that referenced this issue Oct 26, 2022
Remove some duplication of effort in mount_all, simply calling on App.mount
to do the work.

Related to Textualize#778.
@davep
Copy link
Contributor

davep commented Oct 26, 2022

Having tidied up the parameters for the two mount methods (removing support for "named" widgets), the next thing to do is to look at NodeList with a view to adding other methods of getting a widget into the list, that isn't just a simple append.

@davep
Copy link
Contributor

davep commented Oct 26, 2022

From what I can see there are no unit tests for NodeList -- this might be the perfect moment to start to add some.

davep added a commit to davep/textual that referenced this issue Oct 26, 2022
As part of work on Textualize#778 I'll be making various changes to NodeList, so
having some tests to make sure that the existing code carries on working,
and to make sure that any new code works as expected, is a good idea.
@davep
Copy link
Contributor

davep commented Oct 26, 2022

Reviewing the higher-level requirement, and thinking about how it could be used, I'd like to just recheck the approach suggested. As I understand it, right now, we have mount, which (if we can forgive relating this to web development for a moment) would seem to be equivalent to appendChild. If that's the case, I can't help but feel that an analogous interface (which I suspect folk would be looking for), would be something akin to insertBefore (although of course we may want both before and after versions). What I'm not sure I'm getting is when someone would want to work with numeric indexes.

Taking our stopwatch tutorial, with this in mind:

def action_add_stopwatch(self) -> None:
    """An action to add a timer."""
    new_stopwatch = Stopwatch()
    self.query_one("#timers").mount(new_stopwatch)
    new_stopwatch.scroll_visible()

I can imagine wanting to turn this into "add after the focused stopwatch or at the end if one isn't focused", with some query to find the relevant Stopwatch within the #timers container resulting in something akin to:

self.query_one("#timers").mount_after(focused_stopwatch, new_stopwatch)

or if one isn't focused, the original code happening.

Likewise, an "insert at the focused position" resulting in something like:

self.query_one("#timers").mount_before(focused_stopwatch, new_stopwatch)

What I can't quite see is when someone would want to do this sort of thing with a numeric index, and what the code would look like. Even if someone wanted to use an index with the stopwatches, would it make more sense to do something like:

self.query_one("#timers").mount_before(self.query(Stopwatch)[3], new_stopwatch)

It could be that I'm not seeing what the use case is for the initial suggested approach, in which case I think an example or two to illustrate how that would work would be helpful.

@github-actions
Copy link

github-actions bot commented Nov 3, 2022

Did we solve your problem?

Glad we could help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants