-
Notifications
You must be signed in to change notification settings - Fork 59
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
data/binding/disableableBinding.go
Outdated
|
||
// 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) { |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Not really sure how, or if, I should address the failed test. |
… the disableable binding and in the information for it in README.md."
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. |
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. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry about that. Thought that this was in the main Fyne repository. Ignore my previous comment. |
|
Did not mean to close the PR. |
What about This is a great addition and it would be good to get that change in so we can land it. |
There was a problem hiding this 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 ;)
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. |
It would be better @Bluebugs if we could help contributors to iterate based on feedback rather than replace their work. 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. |
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 Of course the code here could also be adapted to take the reverse of the bind requirement order, @Solander is this making sense to you? Do you have thoughts based on your use-case? |
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. |
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.