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

Make button work with a widget as its child #446

Closed

Conversation

futurepaul
Copy link
Collaborator

@futurepaul futurepaul commented Jan 6, 2020

We've been talking about this for a while in zulip, though I'm just now realizing there's no issue for it. This is a first step toward doing some fancy buttons like a control strip and the "button" aspect of a drop-down menu, but also as-is it should be helpful for many scenarios where something needs to be "clickable".

Two issues with my implementation:

  1. to recreate the existing styling, I made a ButtonBackground widget, but for some reason that widget doesn't get is_active updates from its parent, only is_hot, so I have to do extra event handling. (It's very possible I'm doing a very dumb error but I couldn't find it).
  2. to make lifetimes work with WidgetExt I had to make WidgetExt T: Data + `static which I'm guessing is not ideal? It's unclear to me what I'm doing weird here, because WidgetExt is working with other widgetpod-as-child widgets as-is.

Also all the names for things are up for debate, but I'm pretty happy with it right now. I guess there's a larger question if this is appropriate for WidgetExt, but it felt right to me.

@mendelt
Copy link
Contributor

mendelt commented Jan 6, 2020

Cool! I've been playing with this too.
I had the same issue with WidgetExt needing to be T: Data + 'static over here #445 when I moved sized from Button to WidgetExt.
I got a bit stuck with making the "template" as dynamic as the current button is. I see you moved that problem to a ButtonBackground widget which makes sense. I thought of two other ways of solving the problem.

  • The quick way would be to have Button take a function pointer from data and env to a widget so it can dynamically repaint every time
  • Eventually I would like to see a system where every property of all widgets can be dynamic like the LabelText . That way child widgets can dynamically change properties in a very natural way.

Another thing I was experimenting with was to make the child widget take a ButtonState struct as its T. The child widget does not really need access to the application data. It needs to be a template that can draw a button, so it needs to know about the button state. That way fn event can pass in the button state, the child widget can then do its own event handling and set clicked when the button needs to be clicked.
I'll see if I can create a pull request so we can compare notes.

@futurepaul
Copy link
Collaborator Author

Issue #1 is solved now.

@cmyr
Copy link
Member

cmyr commented Jan 9, 2020

(The reason you need the 'static bound is that button stores a boxed closure, which has to be static. So I think that's fine.)

Cool, I think this is definitely moving in the right direction.

I would like to see if we can really simplify this. As I see it, at its most minimal a button is simply a widget that keeps track of active state, and fires an event when appropriate.

Maybe we can just call this a Click widget? Or a Responder widget?

This widget can have any child; and the commonly used children will know to draw themselves differently based on hot/active status.

In this world I could imagine a ButtonDecoration widget that is basically your ButtonBackground; and we could continue to ship a Button widget that was just Click + ButtonDecoration + some arbitrary widget?

@cmyr
Copy link
Member

cmyr commented Jan 9, 2020

One possible idea: we could experiment with this new stuff as new API, not touching button, and then clean it all up at the end?

@futurepaul
Copy link
Collaborator Author

Hmmm maybe I'm doing it wrong but it doesn't seem like extracting Click is simplifying things much. Only difference is in the constructors and that layer of indirection is causing problems (see the failing test).

@mendelt
Copy link
Contributor

mendelt commented Jan 9, 2020

Just a thought related to the click widget;
We could generalize this and make it a way to expose all events to our users. You can have event-handler widgets that can be inserted in the widget stack to allow users to attach event handlers to the widgets it wraps. We can have a mouse-down, mouse-up, mouse-move widget etc. Things like a click widget and toggle widget are a bit more complex, they need to do a bit more processing on incoming events, but they also need to expose their state (clicked, toggled etc) to underlying widgets.
Another thing I've been thinking about that would make this even more powerful is if we make more widget properties behave like LabelText where the property can be dynamic. This way the state of a click or toggle or slider widget can determine the look of underlying widgets dynamically. I think this would be a really nice way of providing the composability I was looking for earlier.

@futurepaul
Copy link
Collaborator Author

Closing this in favor of #672 or something more like that.

@futurepaul futurepaul closed this Mar 16, 2020
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

Successfully merging this pull request may close these issues.

3 participants