globaltcad / sprouts

A property MVVM API for the Swing-Tree library
MIT License
1 stars 0 forks source link

List view #109

Open Mazi-S opened 3 weeks ago

Mazi-S commented 3 weeks ago

Would it be possible to support views of Vars in the same way as views of Var? I think it would be consistent and very useful.

Vars<Integer> numbers = Vars.of(1,2,3);
Vals<String> strings = numbers.viewAs(String.class, n -> "n:" + n);

// strings: ["n:1", "n:2", "n:3"]

numbers.removeAt(1);
// strings: ["n:1", "n:3"]

numbers.add(4);
// strings: ["n:1", "n:3", "n:4"]

numbers.setAt(2, 5);
// strings: ["n:1", "n:3", "n:5"]

numbers.at(2).set(6);
// strings: ["n:1", "n:3", "n:6"]
Gleethos commented 3 weeks ago

I think it is both possible and also a good idea to make the API more consistent. So I am all for it!

Gleethos commented 3 weeks ago

One important thing I want to add to this is that we should be careful not to fall into the trap of implementing this naively by simply mapping the source properties to views and call it a day. So take the following derivative of your code snippet:

Vars<Integer> numbers = Vars.of(1,2,3);
Vals<String> strings = numbers.viewAs(String.class, n -> "n:" + n);

Val<String> singleView = strings.at(1);

Here we are viewing the string at index 1, but what happens when the source property list shrinks and the item at index 1 is gone?

Is that not an error?

Mazi-S commented 2 weeks ago

I would say that's not an error.

Consider the following without list views:

Vars<Integer> numbers = Vars.of(1,2,3);

Var<Integer> singleNumber = numbers.at(1);
Val<Integer> singleNumberView = numbers.at(1);

// singleNumber == 2
// singleNumberView == 2

numbers.removeAt(1);

// singleNumber == 2
// singleNumberView == 2

singleNumber.set(22);

// singleNumber == 22
// singleNumberView == 22

I would expect the same here:

Vars<Integer> numbers = Vars.of(1,2,3);
Vals<String> strings = numbers.viewAs(String.class, n -> "n:" + n);

Var<Integer> singleNumber = numbers.at(1);
Val<String> singleStringView = strings.at(1);

// singleNumber == 2
// singleStringView == "n:2"

numbers.removeAt(1);

// singleNumber == 2
// singleStringView == "n:2"

singleNumber.set(22);

// singleNumber == 22
// singleStringView == "n:22"
Mazi-S commented 2 weeks ago

I did a draft implementation and ran into some problems.

We always end up adding views of properties to a Vars list. Views are of type Val and can change as the viewed Var change. When adding a Val to a Vars list, we treat Vals as values and not as views (We wrap the value in a new property). This results in changes not being reflected.

String nullObject = "";
String errorObject = "error";
Function<Integer, String> mapper = (i) -> "n:" + i;

Vars<Integer> integers = Vars.of(Integer.class);
Vars<String> _strings = Vars.of(String.class);
integers.onChange(delegate -> {
    if (delegate.changeType() == Change.ADD) {

        Vals<Integer> newValues = delegate.newValues();
        List<Val<String>> newViews = new ArrayList<>();

        for (int i = 0; i < newValues.size(); i++) {
            newViews.add(newValues.at(i).view(nullObject, errorObject, mapper));
        }

        for (int i = 0; i < newViews.size(); i++) {
            // todo: here is the problem
            _strings.addAt(delegate.index() + i, newViews.get(i));
        }
    }

    // ...
});

Vals<String> strings = _strings;

// integers: Vars<Integer>[]
// strings:  Vars<String>[]

integers.add(1);
integers.add(2);
integers.add(3);

// integers: Vars<Integer>[1, 2, 3]
// strings:  Vars<String>[n:1, n:2, n:3]

integers.at(1).set(42);

System.out.println(integers);
System.out.println(strings);

// integers: Vars<Integer>[1, 42, 3]
// strings:  Vars<String>[n:1, n:2, n:3]

There is a workaround:

Vals<Integer> newValues = delegate.newValues();
List<Var<String>> newViews = new ArrayList<>();

for (int i = 0; i < newValues.size(); i++) {
    Val<Integer> source = newValues.at(i);
    // todo: create mapper that maps null / error to the nullObject / errorObject
    Var<String> view = Var.of(mapper.apply(source.orElseNull()));
    source.onChange(From.ALL, d -> view.set(From.ALL, mapper.apply(d.orElseNull())));
    newViews.add(view);
}

But with this workaround, we can't use Val#view and the code gets pretty messy. (There is another problem. The change delegate does not contain the changed properties. It contains the values wrapped in new properties).

But long story short. I think we should consider splitting Val into two types. Currently we use Val for values and for views, which leads to confusion and many problems. I think what we need is a view type as a supertype and Var and Val as two subtypes of the view type. Val mutable and Val immutable. Same for lists.

Gleethos commented 2 weeks ago

Yeah, so all of this is getting very very complicated and so I have been struggling to formulate a insightful and thorough answer to this problem.

First of all, let me answer to this more generally. I suggest to make the implementation of this not dependent on the existing API in the sense that it should not be a quick and easy solution that is nice to read. I believe the problem here is inherently complex due to a wide range of requirements. So for example, one thing I believe is important here is performance and memory safety. Approaching this from a straight forward point of view will most likely lead to a memory leaking ball of event listeners.

So what I mean by this is that we should not just create property views for every property in the source list and throw them into a new list! Instead we should carefully craft a facade on a new dedicated class implementing the ´Vals´ API which only creates property views and event listeners when the user explicitly requests access to a Val on the ´Vals´ view (of a mutable source ´Vars´ object).

I would then suggest to keep this Val view in a ´WeakReference´ or `WeakHashmap´ so that these views can ge garbage collected. We should also use weak action listeners on the source property list which ensures that the view list can be garbage collected.

The list view knows when and at which index a user accesses a ´Val´ using the ´at(int index)´ method. So we should use this as a way to optimize the list view so that it becomes merely a list of eagerly computed properties. So when the user accesses an item at a particular index I suggest running it eagerly through the mapper function of the list view.

Note that a weak reference in the property list view on the source list is the preferred goal here for better memory safety, but this can obviously lead to problems when computing the viewed values eagerly... So maybe we need a most recently viewed cache here. Not sure about that.


I agree that a super-type is a good idea. I have been eyeing at the idea of a Maybe type for a long time now. This is especially important for the Result type, where event registrations make no sense. So this might be helpful here:

https://github.com/globaltcad/sprouts/pull/116

I am currently a little unsure how you would like to use such a generic supertype in this issue specifically. So I would appreciate if you could post some example/pseudo code.

Looking at the code snippet you posted:

integers.onChange(delegate -> {
    if (delegate.changeType() == Change.ADD) {

        Vals<Integer> newValues = delegate.newValues();
        List<Val<String>> newViews = new ArrayList<>();

        for (int i = 0; i < newValues.size(); i++) {
            newViews.add(newValues.at(i).view(nullObject, errorObject, mapper));
        }

        for (int i = 0; i < newViews.size(); i++) {
            // todo: here is the problem
            _strings.addAt(delegate.index() + i, newViews.get(i));
        }
    }

    // ...
});

I want to note here that ´newViews.add(newValues.at(i).view(nullObject, errorObject, mapper));´ should not work as the way the API makes it seem like. So this is also where I would like to use ´Maybe´, because the data reported by the delegate should be just an immutable snapshot of what has changed. The reason for this is because **creating a view creates an event listener internally, and so when you create a view inside an onChange event action you are esentially creating a an event listener in an executed event handler. And this sort of stuff cn get out of hand really quickly... So this is why I want it to be impossible to register listeners or create views on the things exposed by a delegate object.

I understand that a user might want to do custom event forwarding themselves... But other libraries like RxJava have kinda shown how easy this sort of stuff it makes to create event listener hellholes. I believe we should be careful here to ensure that Sprouts resides more on the stupid and simple kind of side of things. Reactive programming is useful and all, but also full of complexity pitfalls if you know what I mean.

Mazi-S commented 1 week ago

I want to note here that ´newViews.add(newValues.at(i).view(nullObject, errorObject, mapper));´ should not work as the way the API makes it seem like.

I know, and in the draft on the branch I solved this problem by accessing the actual source list. But I think this is a problem in general. The API gives the impression that we can do this. I think if we decide not to provide the actual properties in the delegate, we should make that clear.

My suggestion would be to just have the value of the properties in a normal List<T> in the delegate.

Alternatively, I would say we provide the view of the properties. So users should not be allowed to change the value. But users can access the current value and also create a new view (.viewAs ...) or even register an event listener. But as you pointed out, this is problematic and we should not do this. ( By view at this point I mean the same objects, but the delegate should only have Vals / Valss; so values can't be set).

I'm not sure I understand the need for a Maybe type. If you don't allow changes and don't allow listening for changes, why not use the value itself? (Or Optional<T>)


The super type I had in mind would allow event registration. (Let's call it View<T> for now). The View<T> type would provide basic functionality to access the value and listen for changes. Var<T> would implement it and add setter functionality. Val<T> would also implement View<T> but can ignore the change listener because the value never changes. Then we could have a Views<T> list containing both.

Mazi-S commented 1 week ago

Instead we should carefully craft a facade on a new dedicated class implementing the ´Vals´ API which only creates property views and event listeners when the user explicitly requests access to a Val on the ´Vals´ view (of a mutable source ´Vars´ object).

I like this idea.

Gleethos commented 1 week ago

The API gives the impression that we can do this. I think if we decide not to provide the actual properties in the delegate, we should make that clear.

Yeah and I agree.


The super type I had in mind would allow event registration. (Let's call it View for now). The View type would provide basic functionality to access the value and listen for changes. Var would implement it and add setter functionality. Val would also implement View but can ignore the change listener because the value never changes. Then we could have a Views list containing both.

Ok, I see. So you are proposing the following hierarchy: (sorry, not quite UML, the arrows should go the other way)

flowchart TD
    O(Observable) --> A(View)
    A --> B(Val)
    A --> C(Var)
    B --> D(Result)
    B --> E(ValDelegate)

... instead of the current one:

flowchart TD
    O(Observable) --> A(Val)
    A --> B(Var)
    A --> C(Result)
    A --> D(ValDelegate)

I see where you are coming from, although I am not yet sure how it improves the property list view situation... Currently, it certainly feels like the ´Val´ kinda wants to be an ´Optional´ of sorts. At least when using its factory methods, like for example ´Val.of("test")´, it is basically a monad more than a view (because registering change listeners does nothing...). But I would argue that despite that, when it comes to how it is used in practice, the ´Val´ type is a view more than anything else. I guess the name is not as fitting anymore... But its usage is that of a view most of the time.

So what I am getting here at is that I don't see how the introduction of a ´View´ would distinguish itself from the ´Val´ type in a meaningful way. I am not even sure what methods the ´Val´ type have over the ´View´ to be honest. Please feel free to elaborate.

So I would argue, ´Val´ is already ´View´, it just evolved into having the wrong name :/ .... I guess we could rename that ... but, uff ... that would require some fancy migration scripts ...


I'm not sure I understand the need for a Maybe type. If you don't allow changes and don't allow listening for changes, why not use the value itself? (Or Optional)

So to be clear here, I am proposing the following architecture:

flowchart TD
    O(Observable) --> A(Maybe)
    A --> B(Val)
    B --> C(Var)
    A --> D(Result)
    A --> E(ValDelegate)

EDIT FOR CORRECTING THE ABOVE:

flowchart TD
    A(Maybe) --> B(Val)
    O(Observable) --> B
    B --> C(Var)
    A --> D(Result)
    A --> E(ValDelegate)

"Maybe" should of course not be observable

Currently, the generic ´Result´ type extends ´Val´. The initial intend behind this was to allow it to be passed to SwingTree as a quasi ´Optional´. So that it is possible to pass it to let's say ´withBackground(Val)´. If the ´Result´ is empty, nothing happens, and the color is not applied...

This has not proven to be terribly useful so far, so I am willing to drop this "feature" and instead would like to let the ´Result´ type to have an unambiguous API. Down the road, we could then consider widening the SwingTree API so that methods like ´withBackground(Val)´ could become ´withBackground(Maybe)´. Under the hood, SwingTree would perform a check to see if it is also a ´Val´, in which case it would then register a listener.

So in this hypothetical new SwingTree API, one such ´Maybe´ based method would accept ´Result.of(..)´, ´Maybe.of(..)´, ´Val.of(..)´ and ´Var.of(..)´. Each with their own unique purpose.

´Maybe´ -> A basic monad (basically an extendable Optional). ´Result´ -> A monad with support for control flow safe error handling. ´Val´ -> A view on a thing which can actively be observed. `Var´ -> A mutable thing which can actively be observed.

Besides the more flexible SwingTree integration, we can then also use the ´Maybe´ type instead of ´Val´ to clearly indicate that something can not be actively listened to but only accessed actively.

So instead of the ´ValDelegate´ extending ´Val´, it could then extend ´Maybe´. This makes it clear that the delegate can not be observed actively.

I hope that clearly conveys my thoughts for all of this.

Mazi-S commented 1 week ago

I think I got it 😅. But why does Result need to implement Observable?

flowchart TD
    O(Observable) --> B(Val)
    B --> C(Var)
    A(Maybe) --> B
    A --> D(Result)
    A --> E(ValDelegate)

And what exactly do you mean by "actively observed"?

Gleethos commented 1 week ago

But why does Result need to implement Observable?

Wooops! That was a mistake. It shouldn't extend Observable! The diagram I posted was wrong. The one you posted in your response is exactly what I have in mind.

And what exactly do you mean by "actively observed"?

By "to observe actively" I explicitly refer to the ability to register callback listeners which react back to the subscriber on changes. In a sense you can also observe something by just accessing it through a getter or something, so I wanted to be explicit about event listeners being involved. Maybe a better wording would to observe "re-actively".

Mazi-S commented 1 week ago

Got it and I like it :)

Under the hood, SwingTree would perform a check to see if it is also a ´Val´, in which case it would then register a listener.

But I think we should avoid doing a check under the hood. Maybe can't be observed. So when a user provides a Maybe to SwingTree, I think registering a listener is not what is expected. We don't even know if the user knows the runtype. Instead I would overload the API to take Result and Val.

Mazi-S commented 1 week ago

The View type I suggested above would then be located like this:

flowchart TD
    O(Observable) --> B(View)
    B --> C(Var)
    B --> F(Val)
    A(Maybe) --> B
    A --> D(Result)
    A --> E(ValDelegate)

Maybe → A basic monad (basically an extendable Optional). Result → A monad with support for control flow safe error handling. View → A view of a thing that can be actively observed. Var → A mutable thing that can be actively observed. Val → An immutable thing that can be actively observed.

Val allows a lot of optimizations (e.g. observing = mapping the value and returning a Val; no registration of listeners)

But you are right, the new type View would basically be the current type Val. And the new type Val is basically a very simple version of Var. It would not add much functionality, but in my opinion it would make things clearer and more readable.

But you are probably right, and it is not worth it.

Gleethos commented 1 week ago

Val allows a lot of optimizations (e.g. observing = mapping the value and returning a Val; no registration of listeners)

Yeah, so I think I know how you feel about Val and I feel with you. Val is kind of a terrible name for what it is used now. I totally agree with that sentiment. The name ´Val´ feels more like it is a Value Type, which is an immutable and transparent data carrier, usually not at all something that hides behind an interface and delegates changing state.

So Val is a bit of a lie. And definitely confusing on first sight.

If I had a magic wand, I would totally replace it with the View type you suggested and made Val a simple monad. In fact, I guess we would not even need Val anymore. I believe we could even use the Maybe class to fill that same role much much more neatly and without an API making false promises (registering change listeners on a monad is a lie).

I am happy to admit that this was not the best design decision. I remember when I started building this, I simply wanted a to created a super type to Var which is exactly the same thing with the simple simple difference that its setter is not present. That's all! The idea was to allow MVVM based view models to have the same kind of encapsulation as regular Java classes. So for example:

class User {
    public void setName(..) {...}
    public void getName() {...}
    public void getAge() {...}
}

Would then be expressed like so in MVVM:

class User {
    public Var<String> name() {...}
    public Val<Integer> age() {...}
}

Initially there was no way to use them as views! They were just a similar sounding thing which encapsulates mutable state. I remember fondly how I like how get/set mirror Val/Var.


I no longer think that MVVM, is the ideal pattern to pursue in all cases, mainly because functional patterns are usually superior in modern software development. So yeah, I agree with your assessments and that the current design has aged a bit. But unfortunately I feel like changing that would be a super massive undertaking with very little gain.

And I say that with a big big sigh.

Gleethos commented 1 week ago

I have a little idea to simplify some things.

We could use Maybe instead of Val on the Vals interface to ensure that it is completely impossible to create listeners on individual properties in a Vals list.

Consider the following method in Vals:


    /**
     *  The property at the given index.
     * @param index The index of the property.
     * @return The property at the given index.
     */
    Val<T> at( int index );

We could refactor this to:


    /**
     *  The potential item at the given index.
     * @param index The index of the potential item wrapped in a {@link Maybe}.
     * @return The {@link Maybe} at the given index.
     */
    Maybe<T> at( int index );

And in the Vars type, the above method continues to be overridden to return Var. I believe this way we can massively simplify how the PropertyListView is implemented, simply because we do no longer have to think about individual property views. Only the entire Vals can be observed, instead of the ability for views in individual properties.

In theory, this removes a feature, but in practice this gives us another feature which has much more value in my opinion: less danger to end up in event listener and memory leak hell.

An you you could still filer out changes to individual items through the list change listeners with a little bit more boilerplate code on the user side of things. I'm okay with that.

What do you think?

Mazi-S commented 1 week ago

What exactly do you mean by wrapped in a Maybe? Is the actual value wrapped in a new Maybe or the property. I am thinking of how changes are reflected in the maybe 🤔

Even if you cannot register a listener to Maybe is the value always the current value or just a snapshot?

Vars<Integer> vars = Vars.of(0, 1, 2, 3);
Vals<Integer> vals = vars;

Maybe<Integer> maybe = vals.at(1);
// maybe.get() == 1

vars.at(1).set(2);
// maybe.get() == 2 or still 1?

vars.setAt(1, 3);
// maybe.get() == 2 or 3?
Gleethos commented 1 week ago

Hmmm, good question. Although this crossed my mind for a short moment. I haven't given the two ways of implementing this that much though, and so I do not at all have a strong opinion on this!

Yeah so I guess the main question here is, should the Maybe of a Vals be a passive view of the position in the Vals, or should it be a snapshot...

My developer intuition tells me that without a listener, it being a view makes much less sense, as you can not react to changes. And so changes are only propagated to the client code when the client code does a re-read. And so if the user of the API, realizes that it is a view and not a snapshot, they might want to always have the latest value delegated by the Maybe. And so I can see how this leads users to writing some kind of busy re-reading... which not good.

On the other hand, if a user just want to read out an item in the property list, they understandably don't want to suffer from null pointer exceptions, and so instead of unpacking the item from the Maybe, they may decide to keep the Maybe in the application state instead of using null. This usage implies that the value of the Maybe does not change, just like a nullable variable does not change at some hidden place.

So my first assessment of this would be that a live view Maybe instead of a snapshot Maybe is just a useless view with unexpected side effect. And the side effects are especially problematic because the API of Maybe heavily implies that it is just a monad, which are known for their immutability...

Gleethos commented 1 week ago

I think making it snapshot based also has the benefit of making it even easier to implement, than some kind of eager thing.