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

Access length and value of models from .60 #98

Closed
ogoffart opened this issue Nov 3, 2020 · 9 comments
Closed

Access length and value of models from .60 #98

ogoffart opened this issue Nov 3, 2020 · 9 comments
Labels
enhancement New feature or request rfc Request for comments: proposals for changes

Comments

@ogoffart
Copy link
Member

ogoffart commented Nov 3, 2020

Suggested syntax:

  property<[string]> model: ["set", "by", "the", "user"];
  property<string> current_value:  model.at(current_index);
  property<int> xxx:  model.length();

Alternative syntax

  property<string> current_value:  model[current_index];
  property<int> xxx:  model.length;

The problem is that we want to get binding updates when the model changes, which makes the implementation a bit tricky.

@ogoffart ogoffart added enhancement New feature or request rfc Request for comments: proposals for changes labels Nov 3, 2020
@ogoffart ogoffart added this to Needs triage in Issue priorities via automation Feb 2, 2021
@ogoffart ogoffart moved this from Needs triage to Medium priority in Issue priorities Feb 2, 2021
@jamesblacklock
Copy link
Contributor

I am working on model[current_index] to complement the array length ticket that I already created.
The question I have is, how do we handle out of bounds accesses? In particular, I don't know what the behavior should be for types that have no default value, as there is no null or anything like that.

@tronical
Copy link
Member

tronical commented Oct 19, 2021

PR #580 covers the API part and initial implementation of this ticket - thanks @jamesblacklock! What's left is binding updates then the model changes - I'll take a look at that.

@tronical
Copy link
Member

I am working on model[current_index] to complement the array length ticket that I already created.
The question I have is, how do we handle out of bounds accesses? In particular, I don't know what the behavior should be for types that have no default value, as there is no null or anything like that.

That's a good question and IMO it ties into a bigger topic: IMO the evaluation of bindings can fail, for different reasons. When that is the case I think we should make diagnostics available to the developer (this happens at run-time) and maintain the previous value of the property the binding is associated with.

@ogoffart
Copy link
Member Author

how do we handle out of bounds accesses?

Good point, right now it panicks, but that might not be a good idea. We should change the model implementation to return a default value.

In particular, I don't know what the behavior should be for types that have no default value, as there is no null or anything like that.

I think all types that can be in properties are bounded on the Default trait.

@tronical
Copy link
Member

Regarding the tracking of changes to the length, I see that the C++ and Rust implementation will have to differ (that's ok).

For C++ it might be possible to add a int tracked_row_count() const (or row_count_tracked()?) function to the Model base-class that could call row_count and implement the dirty-tracking commonly for all models.

For Rust, the use of ModelNotify is required by any Model implementation as that's only the way to implement fn attach_peer correctly for a model that is not constant. So ModelNotify lends itself as the place where to centrally know when the model size has changed for all types of models. The part that remains unclear to me is how this should look like on the caller side, so here are some ideas:

  1. We could add a fn model_notify(&self) -> Option<&ModelNotify> { None } function to Model and implement it in all our models. Then we could implement fn tracked_row_count(&self) -> usize in the Model trait as a default that uses ModelNotify and delegates to the implementation there. This is fully compatible but won't work for existing dynamically changing models unless the new function in the Model trait is implemented (one-liner though).

  2. We could try to encapsulate this in a separate type TrackedModelLength that is constructed with a ModelHandle, and (it's inner) implements ViewAbstraction. ViewAbstraction would be enhanced with a callback that receives a &ModelNotify and that in turn is called in the TrackedModelLength's new() function by calling attach_peer() on the model handle, which forwards to ModelNotify. That way ModelNotify could either create and maintain a shared Pin<Rc<Property<usize>>> or TrackedModelLength gets the size of two pointers - one for the ModelHandle to retrieve the actual row_count() and one pointer to some ModelNotify internals to register the property dependency.

(2) is nicely automatic but it's missing elegance. Perhaps there's a third way for Rust? I could also start with a variant of (2) and we see in the review if it can be simplified. What do you think?

@ogoffart
Copy link
Member Author

with (2) who will own and initialize the TrackedModelLength?

@tronical
Copy link
Member

The Rust generator would have to generate a Property<TrackingModelHandle<#inner>> instead of a Property<ModelHandle<#inner>> perhaps, and TrackingModelHandle as an internal type implements all the plumbing (forget TrackedModelLength :-). It's still very weird to go through attach_peer.

@tronical
Copy link
Member

I think I may have found a way to get hold of the ModelNotify without a temporary ViewAbstraction and a lot less plumbing: ModelPeer is public but can't be constructed. Attaching a lifetime to it and using a variant inside allows passing a callback reference. It's still abusing the term "Peer", but might be okay until we break compatibility.

tronical added a commit that referenced this issue Oct 20, 2021
This is done by exposing the ModelNotify to the caller via the new
ModelTracker trait, which has a function that allows "hooking" into the
dirty tracking of the size.

By extension, this also works in JavaScript.

cc #98
tronical added a commit that referenced this issue Oct 20, 2021
Similar to the parent commit, the model tracks changes to the rows and
marks an internal property dirty. Since we have a base class this is a
little less intrusive.

cc #98
tronical added a commit that referenced this issue Oct 20, 2021
This is done by exposing the ModelNotify to the caller via the new
ModelTracker trait, which has a function that allows "hooking" into the
dirty tracking of the size.

By extension, this also works in JavaScript.

cc #98
tronical added a commit that referenced this issue Oct 20, 2021
Similar to the parent commit, the model tracks changes to the rows and
marks an internal property dirty. Since we have a base class this is a
little less intrusive.

cc #98
ogoffart pushed a commit that referenced this issue Oct 25, 2021
This is done by exposing the ModelNotify to the caller via the new
ModelTracker trait, which has a function that allows "hooking" into the
dirty tracking of the size.

By extension, this also works in JavaScript.

cc #98
ogoffart pushed a commit that referenced this issue Oct 25, 2021
Similar to the parent commit, the model tracks changes to the rows and
marks an internal property dirty. Since we have a base class this is a
little less intrusive.

cc #98
Issue priorities automation moved this from Medium priority to Closed Jan 27, 2022
@ogoffart
Copy link
Member Author

(implemented in 0.1.6)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rfc Request for comments: proposals for changes
Projects
Development

No branches or pull requests

3 participants