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

Implement ValueSource #3012

Open
mhsmith opened this issue Dec 3, 2024 · 11 comments
Open

Implement ValueSource #3012

mhsmith opened this issue Dec 3, 2024 · 11 comments
Labels
enhancement New features, or improvements to existing features.

Comments

@mhsmith
Copy link
Member

mhsmith commented Dec 3, 2024

ValueSource itself is already implemented, documented and tested, but there's no way to link it to any other component.

This would be required to support the Invent datastore. There's a sketch of how that might work here:

Any implementation of this feature is likely to involve replacing all the widget @properties with a custom class. Such a class could also be used to store metadata for an interactive GUI builder, such as:

  • A list of possible choices for the property, to generate a drop-down menu
  • An icon to represent the property

Related:

@mhsmith mhsmith added the enhancement New features, or improvements to existing features. label Dec 3, 2024
@HalfWhitt
Copy link
Contributor

An alternative would be to leave widget properties essentially as they are, but give a value source the ability to link to and modify any arbitrary attribute of any object, or at least of any widget.

Something along the lines of

a_source.link_to(a_widget, "attribute_name")

Or perhaps more like

link(a_source, a_widget, "attribute_name")

Would probably be most ergonomic to give widget a convenience method that then calls the above, like

a_widget.link_source("attribute_name", a_source)

However it's phrased, the source would keep a list of things it's connected to, and update them whenever it's changed.

@freakboy3742
Copy link
Member

The root of the issue is the source of truth.

In other source-based widgets (tree, table), the source is the source of truth, and the widget renders that source of truth. Change the source and the widget is informed of the need to re-render. If there are multiple widgets using that source, they're all notified.

However, in other widgets (TextInput is a good example), the widget's value is the source of truth. That's problematic for a source-based implementation, because you can't tie a second widget to the first widget's internal state (or, if you do, you risk notification cascades - changing one widget changes the source, which is an event that changes the second widget which is an event that changes the source, and so on).

The other key difference is that Toga currently differentiates between the "value" of a widget (i.e., the data actually being displayed) and all other properties of the widget. As I understand it, Invent doesn't make this distinction - all properties of the widget are essentially treated as being "source controlled". So, as an example, the "enabled" status of a widget is a property that could be controlled by the source of another widget. A switch widget could be used literally to control the enabled status of a text input with nothing more than "plugging the wires together", as it were.

The risk with these sorts of changes is that you end up with the possibility of diverged state - where the physical state of the widget doesn't reflect the value that the source thinks the widget has. This should be resolvable; we just need to be careful to get the details right. And most of the details will be exactly the same - the main difference is that any get_* property (e.g., get_enabled()) on a backend implementation of a widget needs to be replaced with a signal monitoring that property which notifies the source of the change, with some mechanism to prevent being "self-notified" of a change.

Ironically, Toga has been down this path in the past - one of the big changes in the 0.3 release was moving to "widget as source of truth" for most widget; with a handful of widgets (like NumberInput) retaining some core-side representation of value because of issues with round-tripping values in the backend.

@mhsmith
Copy link
Member Author

mhsmith commented Dec 4, 2024

you can't tie a second widget to the first widget's internal state (or, if you do, you risk notification cascades - changing one widget changes the source, which is an event that changes the second widget which is an event that changes the source, and so on).

Invent currently avoids this because JavaScript events are not fired on programmatic changes. But Toga consistently fires events on programmatic changes on all platforms, with the backend generating the event itself if the native layer doesn't already do so.

Toga currently differentiates between the "value" of a widget (i.e., the data actually being displayed) and all other properties of the widget.

I'm not sure what you mean by this: how is the "value" property different from the others?

the main difference is that any get_* property (e.g., get_enabled()) on a backend implementation of a widget needs to be replaced with a signal monitoring that property which notifies the source of the change, with some mechanism to prevent being "self-notified" of a change.

Since the native widget always exists but the ValueSource is optional, I'd prefer to keep the native widget as the source of truth for its own properties if we can, with the ValueSource being a layer on top. In that case, the "self-notification" could be prevented by making the ValueSource take no action if set to its existing value.

A couple of other notes:

  • The change notification mechanism can be implemented on top of Toga's existing event handlers, as long as we impose the restriction that any given property can either have an explicit event handler or a ValueSource binding, but not both. Since the ValueSource is effectively an event handler already, I think this is acceptable.

  • There are issues with binding properties of different types to the same ValueSource. For example, Invent already supports binding a Slider and a Label to the same source, and the Label will display the value of the Slider. But this is only possible because the Label doesn't generate any events of its own. If you instead linked a Slider to a TextInput, it would be unclear whether the type of the value was a number or a string. I guess this would require a custom ValueSource subclass to convert the types as desired.

@HalfWhitt
Copy link
Contributor

HalfWhitt commented Dec 4, 2024

If you instead linked a Slider to a TextInput, it would be unclear whether the type of the value was a number or a string. I guess this would require a custom ValueSource subclass to convert the types as desired.

One of my (not terribly common) fond memories of using QML in Qt was the ability to set the value of basically anything as dynamic. Like something as simple as width = 2 * height would mean that width would remain twice the height, updating automatically when height was changed. Of course, it had the advantage of being an entire DSL.

Anyway, I doubt Toga could feasibly do anything quite so magical (especially not that example, since I don't think actual, calculated sizes are even exposed), but my point in this case is that the ability to modify the received value when the source updates is useful, and if there were a way to supply a callable when setting up a binding, one could supply something like lambda x: 2 * x. Or, in the situation you mention, str or int. Or even lambda x: f"It's {x}º outside" for a label, say.

@freakboy3742
Copy link
Member

you can't tie a second widget to the first widget's internal state (or, if you do, you risk notification cascades - changing one widget changes the source, which is an event that changes the second widget which is an event that changes the source, and so on).

Invent currently avoids this because JavaScript events are not fired on programmatic changes. But Toga consistently fires events on programmatic changes on all platforms, with the backend generating the event itself if the native layer doesn't already do so.

Agreed - I suspect we may need to add something like a "notification source" to the notification mechanism, with notifications not being reflected back to the widget that created them.

Toga currently differentiates between the "value" of a widget (i.e., the data actually being displayed) and all other properties of the widget.

I'm not sure what you mean by this: how is the "value" property different from the others?

I mean in the sense of things like places where source can be used. For example, you can use a source for the value of a table, but readonly is a property of the widget.

I guess you could argue that distinction is mostly in my head, and that this is really a manifestation of "not using ValueSource everywhere it could be".

Since the native widget always exists but the ValueSource is optional, I'd prefer to keep the native widget as the source of truth for its own properties if we can, with the ValueSource being a layer on top. In that case, the "self-notification" could be prevented by making the ValueSource take no action if set to its existing value.

I agree that this would minimise the number of changes required; my concern is that it will be prone to "Widget has a different value to the source"-style bugs, because the "source of truth" becomes a concept, rather than literally the only place you can get the value. I guess we can protect against this with testing, but it still makes me nervous.

A couple of other notes:

  • The change notification mechanism can be implemented on top of Toga's existing event handlers, as long as we impose the restriction that any given property can either have an explicit event handler or a ValueSource binding, but not both. Since the ValueSource is effectively an event handler already, I think this is acceptable.

I think I can see what you're describing here; from a functionality perspective, it concerns me a little, as it seems like the sort of thing that could be easy to misunderstand/misconfigure. From an external API perspective, it won't necessarily be obvious that because you've added a source, custom event handlers will no longer work (or vice versa).

This is possibly ratholing on the implementation, but I suspect that in order to manage this, we'll need to put the tools in place so that every property is implicitly a source under the hood; that will also mean that we can rely on a source existing for every data value, so we should be in a position to tie user-defined event handling and data-source handling into the same wrapper.

  • There are issues with binding properties of different types to the same ValueSource. For example, Invent already supports binding a Slider and a Label to the same source, and the Label will display the value of the Slider. But this is only possible because the Label doesn't generate any events of its own. If you instead linked a Slider to a TextInput, it would be unclear whether the type of the value was a number or a string. I guess this would require a custom ValueSource subclass to convert the types as desired.

Agreed - I suspect "value" will need to have some basic typed variants that also do validation.

@mhsmith
Copy link
Member Author

mhsmith commented Jan 30, 2025

OK, here's an initial spec for discussion.

There are 3 kinds of properties in Toga:

  • Read-only properties, such as Slider.tick_step, would remain unchanged.

  • Read-write properties which can only be programmatically changed, such as Slider.max, would have their @property decorator replaced with @ValueProperty, which is described below.

  • Read-write properties which can be both programmatically and interactively changed, such as Slider.value, would also need to specify the associated event handler, e.g. @ValueProperty(event="on_change").

The ValueProperty decorator would have no effect on the property's getter, which would continue to go straight through to the widget.

However, ValueProperty would wrap the property's setter, and add the following behavior when the property is set to a ValueSource:

  • Make an initial call to the underlying setter with the current value of the source.
  • Add a listener to the source which will call the underlying setter again whenever the source's value changes.
  • If the property has an associated event handler, assign a listener to it which will update the source from the property.

When the setter wrapper receives anything other than a ValueSource, any existing ValueSource link would be broken by removing its listeners. The value would then be passed straight through to the underlying setter.

To prevent infinite recursion, the ValueSource would only generate a change notification when the new value is actually different from the previous one.

There are issues with binding properties of different types to the same ValueSource. For example, Invent already supports binding a Slider and a Label to the same source, and the Label will display the value of the Slider. But this is only possible because the Label doesn't generate any events of its own. If you instead linked a Slider to a TextInput, it would be unclear whether the type of the value was a number or a string.

How to resolve this situation is very context-dependent, so for now I suggest we require the user to resolve it themselves by providing a custom ValueSource subclass. For example, Invent can resolve it because it has prior knowledge of the type of each datastore element – which in this example, would be a number.

@freakboy3742
Copy link
Member

OK, here's an initial spec for discussion.

The broad shape of this makes sense; however, the devil is in the details.

One immediate detail that stood out: the current Listener definition is an interface; in practice, the Impl of a widget like Table is registered as a listener on the Source. Does this design require that the interface be the listener?

What happens in the case where there are multiple ValueSource properties (e.g., on Slider, where there is a max, min, and value)? How do we differentiate the callbacks? I'm not sure I see a solution other than three separate listener objects on the impl. That works, but it's a bit convoluted. However, I'm not sure I see an alternative that doesn't involve changing the prototype of listeners to include "context" of some kind, or the prototype for registering listeners to allow for different methods.

When the setter wrapper receives anything other than a ValueSource, any existing ValueSource link would be broken by removing its listeners. The value would then be passed straight through to the underlying setter.

To prevent infinite recursion, the ValueSource would only generate a change notification when the new value is actually different from the previous one.

Different as determined where? Does the widget check if there's been a change in value before notifying the source, or does the source only propagate a change notification if the value changes?

The complications I'm expecting to see here are:

  1. the ability to differentiate programatic from user-initiated input changes, and
  2. the possibility of bi- or multi-stable comparison states. A string comparison is fairly straightforward; but float comparisons could easily end up in a state where 2 widgets have the "same" value, but they don't evaluate as equal, so they bounce a change between themselves. This is especially important if the ValueSources are typed, as the coercion process will increase the likelihood of "differences" being detected.

There are issues with binding properties of different types to the same ValueSource. For example, Invent already supports binding a Slider and a Label to the same source, and the Label will display the value of the Slider. But this is only possible because the Label doesn't generate any events of its own. If you instead linked a Slider to a TextInput, it would be unclear whether the type of the value was a number or a string.

How to resolve this situation is very context-dependent, so for now I suggest we require the user to resolve it themselves by providing a custom ValueSource subclass. For example, Invent can resolve it because it has prior knowledge of the type of each datastore element – which in this example, would be a number.

Some related issues:

  • If I provide a ValueSource as the data for a TextInput, and the value of the ValueSource is 3 - whose responsibility is it to convert that into "3"? When is that conversion reflected in the ValueSource itself? Is it an immediate change when the value is assigned? Or does the change only propagate when the value is modified?

  • What happens with validators? They're currently defined on the widget, and change notifications are limited to post-validation. Will this change require moving validation to the ValueSource? If not - what happens if two different widgets have different (possibly incompatible) validators?

@mhsmith
Copy link
Member Author

mhsmith commented Jan 31, 2025

Does this design require that the interface be the listener?

I'm imagining this to be done entirely at the interface layer, so none of the backends would need to change. But the interface object doesn't have to be the listener; it can create a helper object that implements the Listener interface.

I'm not sure I see a solution other than three separate listener objects on the impl.

Yes, one of these Listener objects would exist for each binding between a ValueSource and a property.

However, I'm not sure I see an alternative that doesn't involve changing the prototype of listeners to include "context" of some kind

The context would be passed to the constructor of the Listener subclass; the Listener interface wouldn't need to change.

the ValueSource would only generate a change notification when the new value is actually different from the previous one.

Different as determined where? Does the widget check if there's been a change in value before notifying the source, or does the source only propagate a change notification if the value changes?

I think it would be easiest to implement it in the ValueSource base class, like this:

--- a/core/src/toga/sources/value_source.py
+++ b/core/src/toga/sources/value_source.py
@@ -13,6 +13,11 @@ class ValueSource(Source):
         return str(getattr(self, self.accessor, None))
 
     def __setattr__(self, attr: str, value: object) -> None:
-        super().__setattr__(attr, value)
-        if attr == getattr(self, "accessor", None):
-            self.notify("change", item=value)
+        if attr == self.accessor:
+            old_value = getattr(self, self.accessor)
+            super().__setattr__(attr, value)
+            new_value = getattr(self, self.accessor)
+            if new_value != old_value:
+                self.notify("change", item=value)
+        else:
+            super().__setattr__(attr, value)

The complications I'm expecting to see here are:

the ability to differentiate programatic from user-initiated input changes

  • Programmatic change – assigning the value directly to the property would break the binding. So the change would have to be initiated on the source, which would then notify the property. If it has an associated event, the property would then notify the source, but this would be ignored because of the code above.

  • Interactive change – the property would notify the source, then the source would notify the property. I believe we've left it undefined whether setting a property to its existing value causes an event, but if it does, it will be ignored as above.

the possibility of bi- or multi-stable comparison states. A string comparison is fairly straightforward; but float comparisons could easily end up in a state where 2 widgets have the "same" value, but they don't evaluate as equal, so they bounce a change between themselves.

I think we've been pretty consistent about making sure properties are perfectly round-trippable as long as they're set with the correct type. If they aren't, then we should fix that, e.g. #3136, but even in that case the value will only "bounce" once before stabilizing.

If I provide a ValueSource as the data for a TextInput, and the value of the ValueSource is 3 - whose responsibility is it to convert that into "3"? When is that conversion reflected in the ValueSource itself?

The int to string conversion is done by the underlying TextInput.value setter, as already documented. The property would then notify the source of the new string value, and I can see two reasonable ways for the source to react:

  • If the ValueSource base class is used unchanged, then the accessor is a simple attribute, which will be set to the string. Since "3" != 3", this will cause an additional cycle of updates and notifications before stabilizing.

  • Or the app could implement a typed ValueSource subclass, where the accessor converts the string back into an integer. In this case the source's value would be unchanged, and the code above would prevent any further updates.

    However, I don't think Toga should provide any such subclass until we have more experience of how this feature is used in practice.

Is it an immediate change when the value is assigned? Or does the change only propagate when the value is modified?

When a property is set to a ValueSource, it would immediately call the underlying setter with the initial value of the source.

What happens with validators? They're currently defined on the widget, and change notifications are limited to post-validation. Will this change require moving validation to the ValueSource? If not - what happens if two different widgets have different (possibly incompatible) validators?

As I understand it, validation is completely orthogonal to this – it can display an error message, but it can't interfere with either the value or the change notification.

@mhsmith
Copy link
Member Author

mhsmith commented Jan 31, 2025

A couple more details I didn't spell out above:

  • The properties which already use the Source protocol, such as Table.data, would be left unchanged.

  • Assigning a ValueSource to an interactively-modifiable property would replace any existing event handler associated with that property. However, the app would be free to replace or clear that event handler later, if it wanted the data flow to be strictly one way from source to property. This would be another way to resolve the type-conversion issue.

@freakboy3742
Copy link
Member

I'm not sure I see a solution other than three separate listener objects on the impl.

Yes, one of these Listener objects would exist for each binding between a ValueSource and a property.

However, I'm not sure I see an alternative that doesn't involve changing the prototype of listeners to include "context" of some kind

The context would be passed to the constructor of the Listener subclass; the Listener interface wouldn't need to change.

I was referring to a new concept on the listener callback - essentially being able to tell the listener more than just "this is a new value", but adding "this is a new value for the minimum".

However, it occurs me to that there's another way - modify the implementation of add_listener. If we modify the prototype so that it is:

def add_listener(self, listener=None, *, on_insert=None, on_change=None, on_remove=None, on_clear=None) -> ListenerID:

We can then modify the internal handling of listener so that the current listener argument is effectively a registration of listener.insert, listener.change, etc. add_listener() returns a token that can be used as the placeholder for removing the listener later. The current listener object can be used as it's own token; we can either create a placeholder ID string, or just use the tuple of (on_insert, on_change, on_remove, on_clear) as the ID token for the new-style usage.

This would allow existing Listener usage to remain unchanged; but the Slider case where we have the need for multiple value listeners, we can differentiate between them by registering min_change and max_change, and registering them explicitly.

It also means it's possible to just listen to a specific event (e.g., clear) without needing to define an interface that implements all 4 methods. I'm not sure I have a use case in mind for this, but I can see how that might be useful.

Different as determined where? Does the widget check if there's been a change in value before notifying the source, or does the source only propagate a change notification if the value changes?

I think it would be easiest to implement it in the ValueSource base class,

Agreed. That's also closer to the "reactive" model of having the widget rendering responding to changes in the source of truth.

The complications I'm expecting to see here are:
the ability to differentiate programatic from user-initiated input changes

  • Programmatic change – assigning the value directly to the property would break the binding.

That's a good point - I hadn't considered that assigning directly to the property would break the binding.

  • Interactive change – the property would notify the source, then the source would notify the property. I believe we've left it undefined whether setting a property to its existing value causes an event, but if it does, it will be ignored as above.

the possibility of bi- or multi-stable comparison states. A string comparison is fairly straightforward; but float comparisons could easily end up in a state where 2 widgets have the "same" value, but they don't evaluate as equal, so they bounce a change between themselves.

I think we've been pretty consistent about making sure properties are perfectly round-trippable as long as they're set with the correct type. If they aren't, then we should fix that, e.g. #3136, but even in that case the value will only "bounce" once before stabilizing.

The difference is that in the existing uses, round-tripping only needs to be consistent between 2 values - the user provided value and the widget's value. Once ValueSource exists, it becomes N+1, where N is the number of ValueSources attached.

Consider the case of a 2 Sliders attached to the same integer-based ValueSource. Slider A is defined as 0-20, step 2; Slider B is defined as 0-20, step 3; and you then set the ValueSource to the value 5. Slider A is notified of the change, normalises the value to 4, and notifies the source. This is a value change, so it notifies both sliders. Slider B is notified of the change, and normalises the value to 3. And so on.

If I provide a ValueSource as the data for a TextInput, and the value of the ValueSource is 3 - whose responsibility is it to convert that into "3"? When is that conversion reflected in the ValueSource itself?

The int to string conversion is done by the underlying TextInput.value setter, as already documented. The property would then notify the source of the new string value, and I can see two reasonable ways for the source to react:

  • If the ValueSource base class is used unchanged, then the accessor is a simple attribute, which will be set to the string. Since "3" != 3", this will cause an additional cycle of updates and notifications before stabilizing.
  • Or the app could implement a typed ValueSource subclass, where the accessor converts the string back into an integer. In this case the source's value would be unchanged, and the code above would prevent any further updates.

My concern here is unstable comparisons, especially in the case of floats.

However, I don't think Toga should provide any such subclass until we have more experience of how this feature is used in practice.

I get the intention of minimising scope here; my concern is rolling out an API and then discovering that it can't be used as intended. I feel like we need at least a proof of concept implementation so that we know the API is fit for purpose.

What happens with validators? They're currently defined on the widget, and change notifications are limited to post-validation. Will this change require moving validation to the ValueSource? If not - what happens if two different widgets have different (possibly incompatible) validators?

As I understand it, validation is completely orthogonal to this – it can display an error message, but it can't interfere with either the value or the change notification.

Yes and no. As currently implemented, validation is defined on the widget, and the widget won't propagate a change until the value is legal - so yes, it's (currently) a widget concern.

However, consider the case of 2 text inputs attached to a value source - one which has a validator that says "string must contains no vowels" and one that has a validator that says "string must only contain vowels". If I enter "xxx" into the first input, it validates, and notifies the ValueSource; the ValueSource then notifies the second input - but it's an invalid value, so it can't be assigned. How does Toga/the app recover from this?

It seems to me that make validation a concern of the Source provides a resolution for this. Moving responsibility to Source might also provide a way to resolve the "slider normalisation" example from before; treating value normalisation as something that engages validation logic, you'd be able to prevent an invalid value because invalid step values would be rejected. It also potentially solves "typed Source", issues, because it requires an explicit definition of what "valid" means for a specific source.

The alternative would be to allow a widget to hold "invalid" values that don't match the value in the ValueSource... at which point we start hitting serious "source of truth" problems.

@mhsmith
Copy link
Member Author

mhsmith commented Feb 11, 2025

As currently implemented, validation is defined on the widget, and the widget won't propagate a change until the value is legal

the ValueSource then notifies the second input - but it's an invalid value, so it can't be assigned.

I don't think either of those behaviors have been implemented – for example, see test_textinput.py::test_validation_order.

Slider A is notified of the change, normalises the value to 4, and notifies the source. This is a value change, so it notifies both sliders. Slider B is notified of the change, and normalises the value to 3. And so on.

my concern is rolling out an API and then discovering that it can't be used as intended. I feel like we need at least a proof of concept implementation so that we know the API is fit for purpose.

I agree there are still some unanswered questions. The original intention of this design was to support the Invent datastore. Now that we're not pursuing that, I suggest we park this discussion. If we reopen it, it would be a good idea to first review how this kind of thing is done in other frameworks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features, or improvements to existing features.
Projects
None yet
Development

No branches or pull requests

3 participants