ExtendRealityLtd / Zinnia.Unity

A collection of design patterns for solving common problems.
http://vrtk.io
MIT License
321 stars 38 forks source link

ObservableList custom inspector Editor no longer reactive #400

Closed thestonefox closed 5 years ago

thestonefox commented 5 years ago

Steps to reproduce

Run the scene

Expected behavior

It should fire the Removed event when the collection size changes to 0 It should fire the Added event when the collection size changes to 1

Current behavior

No events are fired

thestonefox commented 5 years ago

Doing a git bisect shows a change in malimbe has caused the issue

https://github.com/ExtendRealityLtd/Malimbe/commit/e4cbe1093f40eccb4b7de86358e8f55e833daafe

merged into Zinnia at https://github.com/ExtendRealityLtd/Zinnia.Unity/commit/614f1bfa1066409283a79a3122aebb01d9cf5f0c

thestonefox commented 5 years ago

The line in Malimbe InspectorEditor.cs

if (changeCheckScope.changed
                        && Application.isPlaying
                        && (!(targetObject is Behaviour behaviour) || behaviour.isActiveAndEnabled)
                        && ChangeHandlerMethodInfos.Count > 0)

Removing && ChangeHandlerMethodInfos.Count > 0 fixes the issue.

I believe this was the only actual change in the Malimbe commit (everything else is just reversing the order of the check)

But I believe the Malimbe commit is specifically to add that offending check, so just removing it isn't the solution.

thestonefox commented 5 years ago

Ok so wrapping

ApplyModifiedProperty(property, ChangeHandlerMethodInfos.Count > 0);

like

else if (changeCheckScope.changed)
{
    BeforeChange(property);
    ApplyModifiedProperty(property, ChangeHandlerMethodInfos.Count > 0);
    AfterChange(property);
}

Seems to fix the issue.

From dumping out some info though, it seems that the ObsList when changed has a ChangeHandlerMethodInfos.Count of 0 which is why the main chunk of code doesn't actually run.

So I'm confused as to why the above does fix it as each of those methods simply does a foreach on the ChangeHandlerMethodInfos but as proved they are both 0 so wouldn't actually do anything anyway?

thestonefox commented 5 years ago

Could this actually be an issue in the Zinnia ObservableListEditor perhaps where it's not assigning change handlers so Malimbe generated code can't pick up on it?

bddckr commented 5 years ago

Could this actually be an issue in the Zinnia ObservableListEditor perhaps where it's not assigning change handlers so Malimbe generated code can't pick up on it?

The change handlers are looked up in the superclass, not in this Zinnia subclass of the editor. The subclass just overrides (and adds to) some of the superclass' methods. None of those is doing the actual handler lookup.

It could very well be the subclass, but probably not for the reason of change handler lookups.

bddckr commented 5 years ago
else if (changeCheckScope.changed)
{
    BeforeChange(property);
    ApplyModifiedProperty(property, ChangeHandlerMethodInfos.Count > 0);
    AfterChange(property);
}

Seems to fix the issue.

The subclass ObservableListEditor doesn't care about the ChangeHandlerMethodInfos, which is why it seems like this change doesn't actually change any logic.

It's the fact that the subclass' Before/AfterChange methods are called that is fixing it. I believe this is the fix we're looking for.

thestonefox commented 5 years ago

Fixed with https://github.com/ExtendRealityLtd/Zinnia.Unity/pull/403