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

Add ability to use data binding to drive disable state of a widget. #3890

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

Conversation

Bluebugs
Copy link
Contributor

Description:

This add the ability to link the Disable state of a widget to a data binding.Bool.

Checklist:

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

Where applicable:

  • Public APIs match existing style and have Since: line.

@Bluebugs Bluebugs requested a review from andydotxyz May 15, 2023 04:08
@coveralls
Copy link

Coverage Status

Coverage: 61.959% (-0.09%) from 62.044% when pulling 1ac2327 on Bluebugs:features/disable-binding into 01205ce on fyne-io:develop.

@andydotxyz
Copy link
Member

Isn't this a duplicate of fyne-io/fyne-x#46?

As per discussions before the built-in bindings are currently about input/user data. We should complete the implementation of that for all widgets before we expand the scope IMHO.

@Bluebugs
Copy link
Contributor Author

Isn't this a duplicate of fyne-io/fyne-x#46?

It does provide the same feature, but with an API that is more discoverable and inline with the rest of the data binding api, I think.

As per discussions before the built-in bindings are currently about input/user data. We should complete the implementation of that for all widgets before we expand the scope IMHO.

I have been heavily using data binding in a network oriented application and I have run into the limitation that we currently can't manage state information (button label, checkbox text, disable state, select options, ...). I have run into the limitation on the input/user data side only for table, but I do not have a good proposition for it yet as it is quite more complex.

I think it would make sense to continue to move forward the usability of data binding by adding what is useful right away to application and has a relatively simple API. In that regard, I think we should have some Reader interface (ex: BoolReader) that only have listener and get api on their interface for this kind of use case.

@andydotxyz
Copy link
Member

It does provide the same feature, but with an API that is more discoverable and inline with the rest of the data binding api, I think.

If that is the case we should be helping get the existing contributions to this level instead of replacing the work provided by contributors.

I have run into the limitation that we currently can't manage state information (button label, checkbox text, disable state, select options, ...)

All of this is possible, you can add custom binding in app code with binding.NewDataListener - there is an example at https://github.com/FyshOS/textedit/blob/0e758cc1932905824087a1390968f8ad5aaa1fe3/main.go#L24 (something similar should indeed somehow get incorporated into our docs).

@Bluebugs
Copy link
Contributor Author

It does provide the same feature, but with an API that is more discoverable and inline with the rest of the data binding api, I think.

If that is the case we should be helping get the existing contributions to this level instead of replacing the work provided by contributors.

I have run into the limitation that we currently can't manage state information (button label, checkbox text, disable state, select options, ...)

All of this is possible, you can add custom binding in app code with binding.NewDataListener - there is an example at https://github.com/FyshOS/textedit/blob/0e758cc1932905824087a1390968f8ad5aaa1fe3/main.go#L24 (something similar should indeed somehow get incorporated into our docs).

Both solution are pretty similar and have the same issue. If you want to bind an object that is inside a List or Table, you end up having to manipulate an additional data that somehow need to be undone when the object is recycled for another use. At the end, the only solution I could find that work is to extend the widget just to add the ability to bind the state. So yes, it can be done without a new API, but it is a lot more complex than it really should be without reason.

@andydotxyz
Copy link
Member

Perhaps you can work with @Solander so we do not have parallel work on this same topic?
It is pretty disappointing for new contributors if their work is superseded by a core developer implementing the same thing.

@andydotxyz andydotxyz removed their request for review June 3, 2023 16:24
@Bluebugs
Copy link
Contributor Author

Bluebugs commented Jun 4, 2023

Perhaps you can work with @Solander so we do not have parallel work on this same topic? It is pretty disappointing for new contributors if their work is superseded by a core developer implementing the same thing.

I agree. Still this doesn't give us any direction in addressing the main problem. If there is a no to get this in fyne, there is likely not much better solution that is any better than what is proposed in fyne-x. An agreement of where the code and API should land does create constraint on what can be done and how the API can look like.

@andydotxyz
Copy link
Member

If there is a no to get this in fyne, there is likely not much better solution that is any better than what is proposed in fyne-x. An agreement of where the code and API should land does create constraint on what can be done and how the API can look like.

I wasn't saying no, I was saying we should discuss with folk already working on this API.

@andydotxyz
Copy link
Member

When I suggested discussing I was rather expecting we could work with the contributor rather than simply pointing them to this PR to get their feedback on your work.
I have continued the conversation on the original PR as I would prefer we try to work with the original contribution as far as possible.

@Bluebugs
Copy link
Contributor Author

I simply don't think there is a way to implement this without major drawback outside of Fyne repository as I have pointed during our conversation here.

@andydotxyz
Copy link
Member

I guess I don't understand what the major drawback is, or where we had the discussion. What I was discussing here is working with open PRs from community contributors before replacing their work.

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

3 participants