-
Notifications
You must be signed in to change notification settings - Fork 448
Add helper functions for keeping bindables of different types in sync #6604
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
base: master
Are you sure you want to change the base?
Add helper functions for keeping bindables of different types in sync #6604
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.
Not super sure on this in general. Yes there's a lot of this type of boilerplate in the game but that's because this stuff is very difficult to get completely right in the general case.
Also note that we're internally gotten a lot more bearish on extensive bindable use across the years.
source.BindDisabledChanged(disabled => | ||
{ | ||
dest.Disabled = disabled; | ||
}, true); |
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 guess I get why you're doing this but this is not going to cut it in the general case. Take RangeConstrainedBindable
, or BindableNumber
, for example - you're going to want to keep MinValue
and MaxValue
and Precision
synced as well to avoid catastrophic failure of this automagic helper.
/// <summary> | ||
/// Creates a readonly <see cref="IBindable{T}"/> with it's value automatically assigned from the source <see cref="IBindable{T}"/> and converted using the given transform function. | ||
/// </summary> | ||
public static IBindable<TDest> Map<TSource, TDest>(this IBindable<TSource> source, Func<TSource, TDest> transform) |
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 find this API design very dangerous. By having fluent / LINQ-like API of Map<>()
a naive consumer may be inclined to believe that this sort of function is chainable into other functions that take and return a bindable - but they are not chainable because the BindValueChanged()
machinery requires all intermediary bindables to be retained to fields, because otherwise your bindable instances get collected, and all your value change callbacks stop working. Bindables being weak ref-based means that you can't just write a fluent API like 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.
Doesn't BindValueChanged
prevent the intermediate bindable from being collected by the lambda holding a strong reference to it? From what I can see WeakRefs are only used for Bindable.BindTo
but not for OnChange
callbacks, so I'd actually be more worried about automatic collection not happening in some cases where it actually should.
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.
Okay I see what you're saying, I did confuse the failure mode here with binding directly. That said as you say there's a decent chance that what this will do instead is leak memory.
Needs general testing, but I'd be very careful with including this method.
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.
How about instead having a ComputedBindable<>
class that will make sure to clean everything up correctly? Would also solve the case of calling UnbindAll
on the destination bindable not removing the change listener on the source bindable, while also preventing the failure mode mentioned above due to the LINQ-like method.
I think the class should probably be internal
and be exposed as an IBindable<>
since it's value shouldn't be writable, the bindable class relies quite a bit on only interacting with other Bindable<>
s so I don't think it's possible to make a custom IBindable<>
implementation.
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 guess? We already have similar in AggregateBindable
. Would need to be sure one doesn't duplicate the other in purpose.
source.BindDisabledChanged(disabled => | ||
{ | ||
dest.Disabled = disabled; | ||
}, true); |
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.
Same argument as above wrt BindableNumber
et al. applies here too.
/// <summary> | ||
/// Bidirectionally syncs the value of two <see cref="Bindable{T}"/>s with the two given transform functions. | ||
/// </summary> | ||
public static void SyncWith<TSource, TDest>(this Bindable<TDest> dest, Bindable<TSource> source, Func<TSource, TDest> toDest, Func<TDest, TSource> toSource) |
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.
On one side this is talking about "bidirectional sync", and on the other it's talking about "sources" and "destinations" and "taking precedence". Which one is it? You can't have both simultaneously.
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.
The way I think about these terms is largely shaped by the reactivity system in vuejs, which has computed
values that can be either be readonly or also write back to wherever they source their data from.
const count = ref(0);
const doubled = computed(() => count.value * 2);
const tripled = computed({
get: () => count.value * 3;
set: (value: number) => count.value = value / 3;
})
in this case while the data is synced bidirectionally I think it is pretty clear what the source and destination would be here
That being said without that context I agree that this could be misleading, though I'm not 100% sure what the best way to name these things would be
/// Bidirectionally syncs the value of two <see cref="Bindable{T}"/>s with the two given transform functions, with the ability to | ||
/// reset the state based on the source <see cref="Bindable{T}"/> if the destination <see cref="Bindable{T}"/>'s value becomes invalid. | ||
/// </summary> | ||
public static void SyncWith<TSource, TDest>(this Bindable<TDest> dest, Bindable<TSource> source, Func<TSource, TDest> toDest, SafeMappingFunction<TDest, TSource> tryParse) |
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.
The issue pointed about with naming above is even more egregious here because here you've explicitly got a source of truth and a bindable that attempts to populate said source of truth.
Co-authored-by: Bartłomiej Dach <[email protected]>
Adds a couple extension functions to deal with a bunch of scenarios of having to keep 2 bindables of different types in sync that I encounter quite often.
Bindable.Map
Creates an
IBindable
which automatically gets it's value updated from the source bindable with the value converted using the given mapping function.Example usage
Bindable.ComputeFrom
Same as
Bindable.Map
but used when the destination bindable already existsBindable.SyncWith
Bidirectionally syncs 2 Bindables of different types with each other, with mapping functions in each direction.
Example usage
It also comes with a second variant that allows to reset to the source value if the destination value is invalid, expecting a mapping function with the same type as the
TryParse
methods commonly found in .net.String <-> Number synchronization methods
Specialized verions of the
SyncWith
method for keeping two bindables of string & number types in sync, with additional parameters to customize number formatting & parsing options.Common usage scenario here would be having text boxes that hold number values.
Example usage