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

Added custom binding 'DisableableBinding' and added it to README.md. #46

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Solander
Copy link

Added a bool binding which accepts targets that implements fyne.Disableable. These targets will have their Disable status reflected by the bool.
The behaviour can also be switched so that true disables and false enables.

@Solander Solander changed the title "Added custom binding 'DisableableBinding' and added it to README.md." Added custom binding 'DisableableBinding' and added it to README.md. Sep 20, 2022
Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice feature, just a note on naming.
One other thing came to mind - "Target" isn't really a word that Fyne has used - do you think people will know what it means? I wondered if "Widget" would fit OK, and is a known word


// Invert will switch the behavior of when the targets will be Enabled or Disabled.
// This will update the Disable status of the targets immediately.
func (b *disableableBinding) Invert(inverted bool) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be SetInverted? The name Invert applies action, which is to take something and make it the opposite state - which isn't really what the method does...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea with SetInverted. I had it as an action first, but then realized that it wasn't a good solution so I changed it. I'll change the name.
Isn't Widgets to generalized since it's only widgets that implements Disableable that are accepted? I agree that "targets" isn't a good name either. I guess "disableableWidgets" too long?

Not really sure what similar scenarios there are in Fyne and how they are named.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Go we don't typically put the type in the naming do we? For example Form.AddItem takes a widget.FormItem etc.
So the type ensures safety and the "Disablable" context is kindof provided by the object you are working with.
Happy to get others thoughts of course.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the name to Widget and also a SetInverted.
Do you feel that this issue is resolved or was there something more about name?
I've been thinking about maybe changing the name of the binding. Fells a bit to much to say binding.DisableableBinding.
Either change the name to Disableable or DisableableBool.
Any opinions on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you it could have a smoother name. binding.NewDisableable is a pretty good suggestion.

Apologies for the delay

@Solander
Copy link
Author

Not really sure how, or if, I should address the failed test.
Any suggestions?

… the disableable binding and in the information for it in README.md."
@Solander
Copy link
Author

Not really sure how, or if, I should address the failed test. Any suggestions?

Just realized that I really need to return an exported object. Not really sure if it's ok to make the struct exported, or if I should bind a way to follow the convention and export an interface.

@Solander
Copy link
Author

Solander commented Sep 28, 2022

Now I've added a test file and implemented an interface as the return type, but the more I work with it, the more it feels like it should have a new name.
binding.DisableableBinding doesn't feel right.
What do you think about 'DisableableBool"?

@Jacalz

This comment was marked as off-topic.

@Jacalz
Copy link
Member

Jacalz commented Sep 28, 2022

Sorry about that. Thought that this was in the main Fyne repository. Ignore my previous comment.

@Solander
Copy link
Author

Sorry about that. Thought that this was in the main Fyne repository. Ignore my previous comment.

@Solander Solander closed this Sep 28, 2022
@Solander
Copy link
Author

Solander commented Sep 28, 2022

Did not mean to close the PR.

@andydotxyz
Copy link
Member

Now I've added a test file and implemented an interface as the return type, but the more I work with it, the more it feels like it should have a new name. binding.DisableableBinding doesn't feel right. What do you think about 'DisableableBool"?

What about binding.NewDisablable and binding.Disablable? I don't think there is need to add 'bool' in there, as there are not multiple different types of disablable.

This is a great addition and it would be good to get that change in so we can land it.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in the conversation the naming should reduce the duplication.
I wonder also should AddWidget not be just Add? After all it doesn't have to be a widget ;)

@Bluebugs
Copy link
Contributor

Bluebugs commented Jun 11, 2023

I have tried this approach in my own program and it doesn't lead to a very usable API when inside list for example due to the need to have an object to keep track of the binding. I do believe now that this should be a core feature of the toolkit and not a side one in fyne-x. I have created a PR: fyne-io/fyne#3890 to that effect. Let me know what you think of it.

@andydotxyz
Copy link
Member

It would be better @Bluebugs if we could help contributors to iterate based on feedback rather than replace their work.
If the idea of tracking Bool internally is the cause of your concern, then I imagine it would be quite possible to move from NewDisableableBinding(widgets ...fyne.Disableable) to BindDisableable(widgets ...fyne.Disableable, bind binding.Bool) rather than dropping the whole PR.

Such an approach would probably lead to a lighter API as you don't need a new type, and may be useful later if we want to bring the feature in up-stream.

@Bluebugs
Copy link
Contributor

I don't think you can implement your suggested API without having memory leak as it will require a map and you have no control over the lifecycle of the widget you are binding to.

@andydotxyz
Copy link
Member

I don't think you can implement your suggested API without having memory leak as it will require a map and you have no control over the lifecycle of the widget you are binding to.

I'm at a loss here - what lifecycle is available in other approaches that we don't have here (or need?) I also don't understand the need for a map as BindDisableable could simply iterate on the input parameters and make the bind directly.

Of course the code here could also be adapted to take the reverse of the bind requirement order, fyne-x can extend widgets just like fyne can, unless I am mistaken?

@Solander is this making sense to you? Do you have thoughts based on your use-case?

@andydotxyz
Copy link
Member

Hi @Solander are you able to help see how to iterate the Api following the conversation above, and do you have opinions on this?

@Solander
Copy link
Author

Hi @Solander are you able to help see how to iterate the Api following the conversation above, and do you have opinions on this?

At the moment I'm away traveling and been so for a while so I'll do this when I get home in end of July.

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