globaltcad / swing-tree

A small DSL library for building Swing UIs
MIT License
6 stars 0 forks source link

Unexpected property behavior #212

Open Mazi-S opened 1 day ago

Mazi-S commented 1 day ago

I noticed some strange behavior when using views. At some point the view suddle stopped updating. So I think something is being garbage collected that shouldn't be.

It was very hard to reproduce, but I think the following example shows it. (I hope this is the same cause and not a different bug)

public class ExampleView extends JPanel {

    public ExampleView(ExampleViewModel vm) {

        Val<String> result = vm.c()
            .viewAsString(s -> "Result: " + s);

        of(this).withLayout("wrap 2, fillx", "[][grow]")
            .withMinSize(200, 200)
            .add(label("a:"))
            .add(GROW_X, textField(vm.a()).isEditableIf(false))
            .add(label("b:"))
            .add(GROW_X, textField(vm.b()))
            .add(label("c:"))
            .add(GROW_X, label(result));
    }

    public static void main(String[] args) {
        UI.run(() -> UI.show(new ExampleView(new ExampleViewModel())));
    }

}
public class ExampleViewModel {

    private final Var<String> a;
    private final Var<String> b;
    private final Val<String> c;

    public ExampleViewModel() {
        a = Var.of("A");
        b = Var.of("B");
        c = Val.viewOf(a, b, (s1, s2) -> s1 + s2);
    }

    // getter ...

}

Here everything works until you use vm.a().get() for the disabled textField. If you use vm.a().get(), result will not be updated.

Maybe I'm missing something, but I wasn't expecting this.

Gleethos commented 17 hours ago

Ah! Interesting scenario. You are right, this triggers garbage collection. It actually causes the entire view model to be garbage collected!

If you keep a strong reference to the view model then it does not break:

public class ExampleView extends JPanel {

    private final ExampleViewModel vm;

    public ExampleView(ExampleViewModel vm) {
        this.vm = vm;
        Val<String> result = vm.c()
                .viewAsString(s -> "Result: " + s);

        of(this).withLayout("wrap 2, fillx", "[][grow]")
                .withMinSize(200, 200)
                .add(label("a:"))
                .add(GROW_X, textField(vm.a().get()).isEditableIf(false))
                .add(label("b:"))
                .add(GROW_X, textField(vm.b()))
                .add(label("c:"))
                .add(GROW_X, label(result));
    }

    public static void main(String[] args) {
        UI.run(() -> UI.show(new ExampleView(new ExampleViewModel())));
    }

}

The behavior is actually kind of by design. Although, this scenario makes me question if that was a good idea in the first place...

But either way, it is actually interesting that .get() for a makes a difference here when it comes to the view model being garbage collected...

Gleethos commented 16 hours ago

This is ultimately caused by the code in sprouts.impl.ParentRef, where a WeakReference is chosen in case of a property not being a view or lens:

interface ParentRef<V extends Val<?>> {

    static <T extends Val<?>> ParentRef<T> of( Reference<T> ref ) {
        return ref::get;
    }

    static <T extends Val<?>> ParentRef<T> of( T value ) {
        if ( value.isView() || value.isLens() )
            return ()->value;
        else
            return of(new WeakReference<>(value));
    }

    @Nullable
    V get();

}

The idea behind this is that the entire chain of views and lenses is always strongly referenced, up until the root property from where everything originates. (a strong tree with weak leaves) And the SwingTree binding mechanism honors this principle by also making this distinction between roots and views/lenses.

This is based on the assumption that if the root property is not used in some other part of the business side of the application, then there is no point in keeping it, as it will not have any side effects there (except for maybe static side effects, but we should avoid global/static side effects, andz so that is fine imho)

But!

As this scenario you created here points out elegantly, the purpose of a view model may also exist to just have the view talk back to itself, independent of the rest of the application.

Not sure how to approach this because besides loosing track of side effects, the other big drawback of event based architectures like MVVM we use here is that memory leaks happen very easily due to forgotten change listeners capturing all sorts of references to stuff which is otherwise no longer used...

And so this whole approach we take in this design ensures that when event chains no longer have a source that is strongly referenced elsewhere, the whole thing gets removed.

Mazi-S commented 8 hours ago

I understand the idea behind this, but I think it violates core Java principles. With this design, you have to know the implementation details and always keep memory management in mind. 🤔

Also, IntelliJ suggests removing the private final ExampleViewModel vm; because it is not used. But, removing it will break the code.

And what about VMs that just model the state of an animation? they do not need to be referenced from the business side.

One thing I still don't understand is why it doesn't work when I keep a reference to a property in the view? For example, instead of private final ExampleViewModel vm; -> public final Var<String> b;. Shouldn't this keep the VM alive? (I didn't look at the code)