jwijenbergh / puregotk

Autogenerated GTK4+Adwaita bindings for Go leveraging ebitengine/purego
MIT License
13 stars 3 forks source link

Proposal: All class constructors should return their constructed types #4

Closed pdf closed 10 months ago

pdf commented 10 months ago

Currently working with the lib is a bit awkward due to the dance required by constructors returning unexpected types, in particular widget constructors returning *gtk.Widget instead of the type the constructor is named for.

For example in the current API:

// Assume we've already created a *gtk.Box as parentBox, and some other *gtk.Widget as childWidget
// We wish to create a new *gtk.Viewport, set its child, and append the viewport to the parent

viewportWidget := gtk.NewViewport() // NewViewport returns *gtk.Widget, rather than the expected *gtk.Viewport
viewport := &gtk.Viewport{}
viewportWidget.Cast(viewport) // We need to cast to the correct type to use its methods
viewport.SetChild(childWidget)
parentBox.Append(viewportWidget) // But we still typically need the underlying widget, so we need to keep both around

If instead we return the expected type, because it already embeds gtk.Widget, this is much cleaner:

viewport := gtk.NewViewport() // NewViewport returns *gtk.Viewport, which embeds gtk.Widget
viewport.SetChild(childWidget)
parentBox.Append(&viewport.Widget)

Drastically friendlier to work with, and because class types already embed their ancestors, the returned object provides access to all methods from the composed types on one object, and the ancestor types can be passed to any function that needs them by virtue of the pointer being set on the earliest ancestor (nice work btw, makes things very neat).

Obviously this would be a breaking change, but I think the usability benefits are well worth it.

As a quick hack to test I made the change in this PR, which unless I'm missing something ought be valid for all class constructors, and have been testing locally with no issues after migrating my consumer code to use the new API. With the change to make this work seeming so trivial though, I wonder if there was a reaon I'm not grokking that the current API was left standing.

jwijenbergh commented 10 months ago

Thanks! The reason I didn't do this is because I wasn't sure that this would break things. But it seems like gotk4 also does this so I have merged this. I converted my own codebase to this too and I saw no issues.

nice work btw, makes things very neat

Thank you. It wasn't really trivial to implement tbh (especially with all the interfaces and stuff), but it's a neat design I would say. Note that casting is a bit awkward right now, ideally we want to do (like gotk4):

value.Cast().(type)

This is possible to implement but will take some effort

jwijenbergh commented 10 months ago

See the current readme example, we have to do

    window.SetChild(&label.Widget)

which is okay and exactly what you stated, what gotk4 allows is pretty neat though:

https://github.com/diamondburned/gotk4-examples/blob/0d54e47813240c045e118b6b0aafa050f3587fab/gtk4/simple/main.go#L22C14-L22C14

we will also have to look at this maybe