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
SimpleWidget implementation #8
Conversation
Could potentially solve fyne-io/fyne#709 |
Looks like an interesting idea, will be curious to see where this goes. |
From my side this suggestion is ready to be discussed. I would add tests after we've agreed on moving forward with this. Note: I would actually love to see this added to the |
It seems like there are two separate things here, a simple widget wrapper, and a safe state management API. |
I can remove the state management API from this PR if that pulls the focus away from the simple widget wrapper, which this PR should be about mainly. What do you think? 🤔 I can also make an extra PR for the state management if you'd let me know where that would fit. In the |
Thinking more about this today it seems that this I wonder if instead this belongs as a |
It does not simplify widget or renderer. It simplifies widget creation. Before, ...Before, someone that wanted to create a new widget (which I want to do like multiple times a day -- at least I do when writing flutter code, React code or Vue code) had to:
As a newcomer there are the additional hurdles of:
With SimpleWidget
|
I think this is a matter of "outside perspective" vs "inside perspective". As someone very new to fyne I knew within the hour that I wanted a simpler way to create widgets. I was sure there was already something and when I didn't find it, I went ahead and started to create my own. Creating widgets just needed too much boiler plate code and too much looking at existing widgets to even understand what to do. The documentation on widget creation helped but there are too many missing pieces. Also I think it will help adoption a lot if there is a way to do things that is similar to other modern GUI frameworks out there. That's why I'm always mentioning flutter, react and vue, as there are many people out there that know how it's done there. In case this is not clear: I don't want to replace anything. I also don't mean the existing code to change in any way. This is helper code building ON TOP of the existing framework making it a lot easier for people to create new widgets. And again, not widgets that replace the existing ones, but custom combinations of existing widgets that build up to higher level widgets for the very specific use cases the users have. |
I understand your and appreciate the summary. However (if we omit render details) extending So this is what I mean by the indication that this is largely easing rendering - if You are right that we should be inspired by other GUI frameworks but I don't think we should mimic their APIs. We can document how they differ if that will help - but our plan was to use a fresh start to find a better solution - we will not be well-served by trying to match other ways unless we think it matches our design. |
From a users (developer who uses fyne) point of view: Why is there Widget and WidgetRenderer? If I want to create a new widget, why do I need to create 2 structs? (I think), I know why, from the frameworks point of view. The widget contains the definition with the properties and the renderer is created by the framework as needed and "renders" the widget to the canvas. It can be thrown away and recreated. But from a users point of view, there is no reason to split the two. So, I would not be happy with a SimpleWidgetRenderer, because the widget part would still be mostly boilerplate. |
Because Widget API should be focused on behaviour and logical state - not rendering.
If developers are going to mix widget state with rendering components we think their code will be less robust and a lot harder to test adequately. |
I don't agree that the following is too much boilerplate: type myWidget struct {
widget.BaseWidget
}
func (m *myWidget) CreateRenderer() fyne.WidgetRenderer {
return NewSimpleWidgetRenderer(...)
} |
Ok, so going with your reasoning: Where would you create the child widgets and canvas elements that this widget is made off? Because as is, this doesn't hold:
All the widgets I have seen so far put rendering details in the |
Yes we got lazy. Really it should be in a render constructor function. This is a shorthand we adopted that avoids adding a new method. |
The split between widget and renderer only creates problems and extra hurdles for the implementer. I tried that approach already before. I have now added a sample implementation. Because in the renderer you'd want access to the state and the properties of the widget, a reference to the widget is needed. It can't be abstracted (at least not without generics) as you'd not want to have to type cast the widget. This leaves the CreateRenderer function (although pretty much predefined) to the developer to be implemented. While I like the idea of keeping the state and properties separate from the renderer, it just doesn't turn out to be practical enough for the user. This could be revisited in 1 year when generics are out, but since you seem to care about some backwards compatibility, it is more like 3 years (static check indicates compatibility with go1.12 which was released 2 years ago). |
AFAIK the only reason they support E.g.: |
If only a real-world usage for widget wrapping were every this simple though (as mentioned above with the reference passing.) I didn't even know for the longest time that I wasn't supposed to keep state in my Widget, simply because there was no obvious breakage for doing so, e.g.: The framework didn't stop me. If there was a method that a developer could call to crash/rebuild the Driver for example... then we could actually see what happens when our state gets messed up, and start to care about that separation as much as the core developers do. (This would be equiv. to Don't get me wrong (please don't take offense by my wording), when I say "we don't care", I mean "it doesn't seem like we should, because we don't understand the internal reasoning for it, and it's not made evidently apparent through the API or documentation." E.g.: There is very little penalty for "doing it wrong" atm. E.g: If I keep 100% of my state and logic in the If this difference was more pronounced, we would better understand the need for separation. Another thought, is that whenever This is what makes Renderers currently feel like a "dangling implementation detail" of Widgets. |
I disagree. The split is essential for separation of these concerns, cleaner testing, and better maintainability over time.
Yes, a renderer can reference the widget for state, that is intended. However a widget should not reference a renderer. |
Yes I can see that. As we progress towards Aberlour release we will see further disconnection of renderers, to enable better thread safety in rendering. We will also start to see more renderers being garbage collected at runtime.
This is an interesting thought, something that we could indeed consider adding. |
I am struggling with this ticket a lot, apologies. I have outline our design and why it is the way that it is. We can have a separate discussion about the shortcomings of our design and how to improve them, but a PR is probably not the best place to do that. We have a proposals process that allows us to spend time weighing the benefits and drawbacks of larger changes. https://github.com/fyne-io/fyne/wiki/Contributing%3A-Proposals |
Given the activity on the PR and with https://github.com/fyne-io/fyne/blob/master/internal/widget/simple_renderer.go in 2.1.0, I think that this can be closed. Feel free to reopen if you want to continue working on it. |
My first draft of the SimpleWidget implementation. I will create a few comments on the code where I have open questions and probably still have to adjust / extend things.
Any improvement suggestions are very welcome.
Basic idea: Be able to easily create new widgets without having to write a lot of boiler code.
With SimpleWidget (only) the following 3 things are necessary in a widget implementing it:
SimpleWidgetBase
ExtendBaseWidget
must be called on widget creationBuild
function must be implemented (overwritten)No renderer needs to be implemented, as it is part of SimpleWidgetBase implementation.
Checklist: