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

SimpleWidget implementation #8

Closed
wants to merge 6 commits into from

Conversation

tehsphinx
Copy link

@tehsphinx tehsphinx commented Feb 16, 2021

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:

  • widget struct must be based on SimpleWidgetBase
  • ExtendBaseWidget must be called on widget creation
  • Build function must be implemented (overwritten)

No renderer needs to be implemented, as it is part of SimpleWidgetBase implementation.

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.
  • Public APIs match existing style.

widget/simple_renderer.go Outdated Show resolved Hide resolved
widget/simple_renderer.go Outdated Show resolved Hide resolved
widget/simple_renderer.go Outdated Show resolved Hide resolved
@tehsphinx
Copy link
Author

Could potentially solve fyne-io/fyne#709

@tehsphinx tehsphinx changed the title simple widget interface, base implementation incl. simple renderer SimpleWidget implementation Feb 16, 2021
@AlbinoGeek
Copy link
Contributor

Looks like an interesting idea, will be curious to see where this goes.

@tehsphinx
Copy link
Author

tehsphinx commented Feb 20, 2021

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 fyne repository (not fyne-x).

@tehsphinx tehsphinx marked this pull request as ready for review February 20, 2021 14:49
@andydotxyz
Copy link
Member

It seems like there are two separate things here, a simple widget wrapper, and a safe state management API.
The latter is more likely to be moved upstream than the former.

@tehsphinx
Copy link
Author

tehsphinx commented Feb 21, 2021

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 BaseWidget on the fyne repo?

@andydotxyz
Copy link
Member

Thinking more about this today it seems that this SimpleWidget is actually not simplifying widgets, but widget renderers. In fact because you’ve pushed the MinSize code back into widget this could be seen as more complicated than before.

I wonder if instead this belongs as a SimpleWidgetRenderer which is where its value seems to lie. Then any widget could return this from CreateRenderer and we don’t need to document different types of base widget?...

@tehsphinx
Copy link
Author

tehsphinx commented Feb 22, 2021

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:

  • create a struct implementing Widget interface
  • base it on BaseWidget
  • create a CreateRenderer function returning another struct
  • create a renderer struct implementing WidgetRenderer
  • implement 5 (!) functions on the renderer

As a newcomer there are the additional hurdles of:

  • understanding that some of the renderer code actually goes into the widgets CreateRenderer method (creation of objects).
  • understanding why there is Layout and Refresh and what code should go where.
  • understanding that Destroy can be left empty in most cases
  • understanding what to return from MinSize as it needs some return value
  • understanding what to return from Objects.

With SimpleWidget

  • one struct needs to be created based on SimpleWidgetBase
  • one function needs to be implemented with a clear description of what to do
  • (optional) more functions can be overwritten as needed, but don't get in the way for a newcomer as they default to a working solution for most cases

@tehsphinx
Copy link
Author

tehsphinx commented Feb 22, 2021

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.

@andydotxyz
Copy link
Member

I understand your and appreciate the summary. However (if we omit render details) extending BaseWidget is almost exactly the same as this SimpleWidget - you make a new struct, extend a thing and implement a single required method to return the render details.

So this is what I mean by the indication that this is largely easing rendering - if CreateRenderer could return NewSimpleWidgetRenderer(...) we can avoid needing new widget types... Is that not true?

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.

@tehsphinx
Copy link
Author

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.

@andydotxyz
Copy link
Member

andydotxyz commented Feb 22, 2021

Why is there Widget and WidgetRenderer?

Because Widget API should be focused on behaviour and logical state - not rendering.
The rendering of a widget is separate from it's API which is why they are separate types. Any visual information that is cached should be in the renderer instead of the widget. Renderers have a different lifecycle than widgets as they are only needed when displayed.

But from a users point of view, there is no reason to split the two.

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.

@andydotxyz
Copy link
Member

because the widget part would still be mostly boilerplate.

I don't agree that the following is too much boilerplate:

type myWidget struct {
	widget.BaseWidget
}

func (m *myWidget) CreateRenderer() fyne.WidgetRenderer {
	return NewSimpleWidgetRenderer(...)
}

@tehsphinx
Copy link
Author

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:

Because Widget API should be focused on behaviour and logical state - not rendering.

All the widgets I have seen so far put rendering details in the CreateRenderer function which is part of the widget.

@andydotxyz
Copy link
Member

All the widgets I have seen so far put rendering details in the CreateRenderer function which is part of the widget.

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.
If they were in two separate files the CreateRenderer method would exist with the renderer rather than the widget for better grouping of code.

@tehsphinx
Copy link
Author

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).

@AlbinoGeek
Copy link
Contributor

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 Go1.12 is because some version of Mac OS X which still exists for some reason only ships this version, and it's not immediately obvious to the user of that OS version as to how they would get a newer version.

E.g.: fyne seems to wish to support any LTS OS's package manager's version of Golang.

@AlbinoGeek
Copy link
Contributor

AlbinoGeek commented Feb 23, 2021

I don't agree that the following is too much boilerplate:

type myWidget struct {
	widget.BaseWidget
}

func (m *myWidget) CreateRenderer() fyne.WidgetRenderer {
	return NewSimpleWidgetRenderer(...)
}

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 go test -race in the sense that it's testing an unusual condition of the application, and could be a build tag that caused the application to perform much slower, etc, as long as it would help developers understand the need for proper state separation.)

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 Widget, and only create a dummy renderer, it may be "incorrect form", but it's not like it doesn't work. In fact, it works just fine, and I haven't noticed a performance or stability difference between a widget which manages its state within the renderer vs the widget itself.

If this difference was more pronounced, we would better understand the need for separation.


Another thought, is that whenever state serialization gets implemented in the Framework, it's going to be very obvious which widgets don't have their state being saved/loaded properly, but until then, it's not currently obvious when a renderer disappeared, nor the implications of it doing so.

This is what makes Renderers currently feel like a "dangling implementation detail" of Widgets.

@andydotxyz
Copy link
Member

The split between widget and renderer only creates problems and extra hurdles for the implementer.

I disagree. The split is essential for separation of these concerns, cleaner testing, and better maintainability over time.
It is also key to avoiding accidentally having render state cached in widget objects which can never be cleared from memory if there is a reference, for example, to hidden elements.

Because in the renderer you'd want access to the state and the properties of the widget, a reference to the widget is needed.

Yes, a renderer can reference the widget for state, that is intended. However a widget should not reference a renderer.

@andydotxyz
Copy link
Member

This is what makes Renderers currently feel like a "dangling implementation detail" of Widgets.

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.

If there was a method that a developer could call to crash/rebuild the Driver for example..

This is an interesting thought, something that we could indeed consider adding.

@andydotxyz
Copy link
Member

andydotxyz commented Feb 23, 2021

I am struggling with this ticket a lot, apologies. I have outline our design and why it is the way that it is.
I don't think it likely that we will merge in widgets or utilities that do not match our design (API or UI).

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

@Jacalz
Copy link
Member

Jacalz commented Oct 10, 2021

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.

@Jacalz Jacalz closed this Oct 10, 2021
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.

None yet

4 participants