fyne-io / fyne

Cross platform GUI toolkit in Go inspired by Material Design
https://fyne.io/
Other
25.25k stars 1.4k forks source link

Add data binding support to Select #2836

Open Bluebugs opened 2 years ago

Bluebugs commented 2 years ago

Is your feature request related to a problem? Please describe:

Currently widget.Select doesn't have support for data binding. Would be nice to have.

Is it possible to construct a solution with the existing API?

No API available for this.

Describe the solution you'd like to see:

I can see the following:

func NewSelectWithData(options []string, data binding.String) *Select
func NewSelectWithDataOption(options binding.StringList, data binding.String) *Select

func (s *Select) Bind(data binding.String)
func (s *Select) Unbind()
func (s *Select) BindOptions(options binding.StringList)
func (s *Select) UnbindOptions()
andydotxyz commented 2 years ago

Yes, adding the String bind for the main data matches our current APIs. Binding the options, however, is a step beyond what we have done elsewhere.

Bluebugs commented 2 years ago

Binding options would be a very useful/expected use case when connecting to databases. It would be good to figure out the right pattern for it.

andydotxyz commented 2 years ago

As mentioned in another issue it's outside the current scope of the binding implementation. If it is something to enable database features we probably need to work through the use-case and confirm that it (and the wider "bind everything" discussion) is worth including in core. Probably worth considering that, as it can be done in user-space, it could be done in fyne-x to keep the main APIs lean.

mbnoimi commented 1 year ago

As mentioned in another issue it's outside the current scope of the binding implementation. it could be done in fyne-x to keep the main APIs lean.

Why this is out of the scope?! binding.StringList already exists

what's making it -unlean- by implementing widget.NewSelectWithData ?!

andydotxyz commented 1 year ago

Because the data of a Select is it's selected String. Therefore to match our usage for entry etc the NewSelectWithData would bind to binding.String not the options item as binding.StringList.

jimorc commented 5 months ago

I have attempted to create a BoundSelect widget (in fyne-x) which binds both the Selected and Options fields by basing it on the Select widget (copying and modifying functionality). There are a number of properties and functions used in the SelectRenderer Refresh function that are internal to fyne and therefore unavailable to any widget in fyne-x. These are:

To continue on this development, it appears that the most direct way would be to externalize each of these (i.e. make them part of the external API). A lot of work, but could possibly make it a lot easier to develop future widgets in fyne-x.

Is this the right approach, or am I completely off base?

andydotxyz commented 5 months ago

The propertyLock is something we are working on. We are not going to expose it because there is a better solution coming in 2.6. You should not need to access super() as that is used only in core widgets for internal purposes - do you know why you need it?

For alignment - why would a bind select have a different alignment to a select?

I suspect that what you are reporting is due to extracting core code and modifying it instead of extending an existing widget in the way that the API is designed for?

jimorc commented 5 months ago

As I mentioned, I did a lot of copying of Select widget code, which appears to be entirely the wrong approach. I will try a different method. I tried this because go does not directly support inheritance, so I thought it would be necessary to duplicate the Select widget code.

andydotxyz commented 5 months ago

Go's embedding of structs is essentially inheritance - just not with all of the OOP implications.

https://docs.fyne.io/extend/extending-widgets

jimorc commented 2 months ago

BoundSelect Widget

I have created a BoundSelect widget with the following functionality:

func NewBoundSelectWithData(options *binding.StringList, selection *binding.String) *BoundSelect
func (b *BoundSelect) BindOptions(opts *binding.StringList)
func (b *BoundSelect) BindSelected(selection *binding.String)
func (b *BoundSelect) GetPlaceHolder() string
func (b *BoundSelect) SetPlaceHolder(placeholder string)
  1. There is no direct access to the encapsulated Select widget.
    • Access to the Select widget is only available through the functions listed above.
    • To be informed of selection changes, you must add a DataListener to the bound selection string.
  2. There are no Unbind... methods. To unbind, simply call the Bind... methods with nil parameters.
    • BindSelected(nil) unbinds the selected string and sets the Select.Selected value to "".
    • BindOptions(nil) unbinds the options string list and sets the Selected.Options value to []string{}.
  3. To view the code, see the boundselect branch at https://github.com/jimorc/fyne-x

Design Decisions

  1. Providing direct access to the fields of the underlying Select widget would be extremely difficult if not impossible. For example, if it were possible to directly access Select.Options, then this would override and invalidate the options binding. It would be possible to provide an AppendOption() method which would also change the bound options list, but this duplicates the Append method for the bound options list.
  2. Providing multiple New... functions for every combination of options, bound options, selection and bound selection is similarly complicated. If you want unbound options and selected, use the Select widget; if you want bound options and bound selected, then use the BoundSelect widget. The only design I came up with to provide this functionality would be to implement the unbound properties internally in the BoundSelect widget as bound values, but then I would have to provide extra methods to allow the user to add callbacks. This ends up duplicating much of the functionality of binding.DataItems. I chose separation of concerns instead.

Comments and Suggestions

Have I messed up the design? Have I missed needed functionality? Comments and suggestions are appreciated. If there are none, I will send a PR to the fyne-x repository.

andydotxyz commented 2 months ago

I'm not sure if it was covered before, but the core binding is to support the main data item for a widget, using "...WithData" - i.e. NewSelectWithData.

If you follow the model from other components then they don't return a wrapped version of the component, instead offering the same widget returned from the constructor. Accessing data inside the widget that is also bound elsewhere should not invalidate any binding as they are two way by default.

Essentially your proposal doesn't match our standard setup and it's not totally clear why?

jimorc commented 2 months ago

I'm not sure if it was covered before, but the core binding is to support the main data item for a widget, using "...WithData" - i.e. NewSelectWithData.

If you follow the model from other components then they don't return a wrapped version of the component, instead offering the same widget returned from the constructor. Accessing data inside the widget that is also bound elsewhere should not invalidate any binding as they are two way by default.

Essentially your proposal doesn't match our standard setup and it's not totally clear why?

I have tried to provide the functionality requested by the OP. As pointed out above regarding binding the the StringList in addition to the String:

As mentioned in another issue it's outside the current scope of the binding implementation. If it is something to enable database features we probably need to work through the use-case and confirm that it (and the wider "bind everything" discussion) is worth including in core. Probably worth considering that, as it can be done in user-space, it could be done in fyne-x to keep the main APIs lean.

So, should I modify the Select widget to add binding to the select string, and then a different widget in fyne-x for also binding the options string list?

andydotxyz commented 2 months ago

So, should I modify the Select widget to add binding to the select string, and then a different widget in fyne-x for also binding the options string list?

I know it seems peculiar but that is the quickest way to get it accepted because it's matching the current usage patterns.

You could also champion an extended "standard usage" for BindXxx where Xxx is the name of a property on the widget... but that would require a bit more discussion with contributors to see if we like that route forward.

I hope that helps. Feel free to jump onto one of our social channels to discuss if that is easier.