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

Add remove attribute to mount and mount_all #4133

Closed
willmcgugan opened this issue Feb 7, 2024 · 7 comments · Fixed by #4183
Closed

Add remove attribute to mount and mount_all #4133

willmcgugan opened this issue Feb 7, 2024 · 7 comments · Fixed by #4183
Assignees
Labels
enhancement New feature or request Task

Comments

@willmcgugan
Copy link
Collaborator

It's common to remove some widgets and add some other widgets. To make this easier, I think we should add a remove attribute to the mount methods which accepts a selector and removes those widgets prior to mounting.

The remove + mount should be atomic, to avoid flicker (might need to be wrapped in batch_update).

API would look something like this:

# Replace all MyWidgets with a single MyWidget
await self.mount(MyWidget("Foo"), remove="MyWidget")

# Remove all children and replace with a label
await self.mount(Label("Goodbye"), remove="*")
@rodrigogiraoserrao
Copy link
Contributor

I think of the method mount as a method that “builds”, “adds”, “constructs”.
Adding a kwd argument that goes in the opposite direction feels a bit clunky.

Wouldn't it make more sense to grow a method replace(selector, new_widget)?

@willmcgugan
Copy link
Collaborator Author

The thing is that it is not replacing. It's not like we can guarantee the widgets will be mounted in the same place as the "remove" selector.

@rodrigogiraoserrao
Copy link
Contributor

In that case, call it remove_then_mount.
Or, better yet, let Widget.remove_children grow the selector parameter that is "*" by default but that you can change.
I think

self.remove_children(selector="...")
self.mount(widget)

looks much better than

self.mount(widget, remove="...")

The thing with remove inside mount is that people will think that the parameter remove is somehow related to the mounting when it's completely orthogonal.

@willmcgugan
Copy link
Collaborator Author

Two methods aren't atomic. Which means you would have to wrap both calls in an asyncio Lock, and await them both. You would also need to use batch_update to avoid flicker. So now your code will look like this:

async with self.lock:
    with self.app.batch_update():
        await self.remove_children("*")
        await self.mount(widget)

That's too much to ask of devs.

The remove attribute on mount is still my favorite. People misunderstanding the extra parameter can be alleviated by docs.

@rodrigogiraoserrao
Copy link
Contributor

If you prefer the mount(..., remove=selector) over remove_then_mount, sure.

@willmcgugan
Copy link
Collaborator Author

After a discussion in the office. We could combine the locking and batch update in to a single context manager.

async with self.batch():
    await self.remove_children("*")
    await self.mount(widget)

That feels a little less clumsy, but it has its own flaw: Namely, that it requires async. You wouldn't be able to have optional awaitables.

Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants