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

Playing with extended container support #5441

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

andydotxyz
Copy link
Member

This allows us to type-check an extended container as though it were a container...

However I also need to solve how it can access the Objects and Layout fields... Head scratching continues.

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@coveralls
Copy link

Coverage Status

coverage: 59.039% (+0.02%) from 59.024%
when pulling 76a025c on andydotxyz:fix/extendcontainer
into 2127941 on fyne-io:develop.

@dweymouth
Copy link
Contributor

What is the use case for an extended container? As far as I can remember it's never been something I have felt I would need in all of my time using Fyne.

@Jacalz
Copy link
Member

Jacalz commented Jan 26, 2025

I remember talking about this on a call a long time ago and we agreed that it wasn't necessary. Anyone that wants to extend something should use a widget as a container is just a container of objects and a layout. It doesn't feel like there is much point in extending it. If it is extendable then it is basically a widget and then suddenly everything is a widget?

I also think we concluded that it would be a better option to just move the widget wrappers from fyne-x into here. Maybe now's the time for that? https://github.com/fyne-io/fyne-x/tree/master/wrapper

@dweymouth
Copy link
Contributor

I also think we concluded that it would be a better option to just move the widget wrappers from fyne-x into here. Maybe now's the time for that? https://github.com/fyne-io/fyne-x/tree/master/wrapper

The thing that gives me pause about moving the wrappers as-is into the main library, is that they are not themselves extendable like widgets are, which IMO is a bit of a flaw. In other words, they're not composable, eg MakeMousable + MakeTappable can't be used to add both capabilities to one object.

@Jacalz
Copy link
Member

Jacalz commented Jan 26, 2025

The thing that gives me pause about moving the wrappers as-is into the main library, is that they are not themselves extendable like widgets are, which IMO is a bit of a flaw. In other words, they're not composable, eg MakeMousable + MakeTappable can't be used to add both capabilities to one object.

I suppose that might be solvable using generics but I don't know.

@andydotxyz
Copy link
Member Author

If it is extendable then it is basically a widget and then suddenly everything is a widget?

Pretty much that is the aim. People try to extend everything and if we can do it then why not?

I also think we concluded that it would be a better option to just move the widget wrappers from fyne-x into here. Maybe now's the time for that? https://github.com/fyne-io/fyne-x/tree/master/wrapper

I don't recall that. It feels like a workaround. This was an attempt to do it without requiring that.

@Jacalz
Copy link
Member

Jacalz commented Jan 26, 2025

It feels like any way we implement extendability will just be some sort of more or less ugly hack to get around the fact that Go doesn't have actual object orientation. We are already not quite happy with ExtendBaseWidget. It kind of feels like we are trying to write Java code in Go sometimes but I also don't quite have any better solutions

@Jacalz
Copy link
Member

Jacalz commented Jan 26, 2025

I'm just not a fan of adding another extendable type until we can figure out how to make extending widgets less clunky. And I really don't think making everything extendable is a good solution in the long run. Extending containers makes no sense to me. I'd rather have some other version of SimpleRenderer that makes it easy to lay out objects with a layout or similar (making it behave a bit like a slice of objects with a layout applied on them)

@andydotxyz
Copy link
Member Author

I'm just not a fan of adding another extendable type until we can figure out how to make extending widgets less clunky.

This approach would not suffer the same problems of requiring ExtendBaseWidget I am hopeful it could even lead to removing it.

Extending containers makes no sense to me.

You know we get folk saying "I want to handle a tap on this Xxx" or "My Yyy object, which is basically a Container, is invisible - help".

I'm not calling this step best practice but it is a path lots of people walk.

@andydotxyz
Copy link
Member Author

It feels like any way we implement extendability will just be some sort of more or less ugly hack to get around the fact that Go doesn't have actual object orientation. We are already not quite happy with ExtendBaseWidget. It kind of feels like we are trying to write Java code in Go sometimes but I also don't quite have any better solutions

Surely you can see that this does not require ExtendBaseWidget (for one thing it does not extend BaseWidget) - I opened this because I thought it would be good to explore alternatives.

@Jacalz
Copy link
Member

Jacalz commented Jan 27, 2025

Just an idea: would it not perhaps be better to have the renderer print an error if it gets a CanvasObject that isn't a container, canvas primitive or widget instead of adding extra complexity to satisfy developers doing something they shouldn't.

@andydotxyz
Copy link
Member Author

andydotxyz commented Jan 27, 2025

satisfy developers doing something they shouldn't.

Well this is a tricky one. I've always tried to take the path of - if the API provides something then it should work, and the Go compiler allows people to embed an object into another (exactly how we get extended widgets). Not supporting this on other types could be seen as our limitation not the developers mistake.

That is what I hoped to overcome - so that something that (for example) extends Image could be understood as an Image. This was a step in that direction.

@Jacalz
Copy link
Member

Jacalz commented Jan 27, 2025

Let's see where we can get to. If this allows us to also find a better way of extending widgets then we're golden :)

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.

4 participants