🐛 Fixes PropagateStop and PropagateOver

[ekwoka]

Objective

Fixes #21620

Ensures PropagateStop works as documented. At it stands, PropagateStop<C> is described as only preventing it's Children from inheriting C but in practice that entity itself would not inherit C.

Makes PropagateOver work. It just plain didn't work at all.

Solution

Adjusted the flow to prevent recursively inheriting Inherited when PropagateStop present, as opposed to not accessing PropagateStop entities entirely.

Skips applying C when Inherited<C> and PropagateOver<C> are on the entity.

Testing

I updated the tests first to verify the test cases. They were incomplete and didn't pass once checking for the proper end state.

After verifying those were not functioning, trimmed and adjusted code until it functioned.

Fuller Explanation

To my understanding of the goals (and my implementation) the idea is that the result of these components would be like

(Propagate<C>, Inherited<C>, C)
  (Inherited<C>, C)
    (PropagateOver<C>, Inherited<C>)
      (Inherited<C>, C)
        (PropagateStop<C>, Inherited<C>, C)

While the previous (incorrect by my measure) appeared to be

(Propagate<C>, Inherited<C>, C)
  (Inherited<C>, C)
    (PropagateOver<C>, Inherited<C>, C)
      (Inherited<C>, C)
        (PropagateStop<C>)

So of note: The PropagateOver<C> is still getting Inherited<C> in this current adjustment. I believe it was doing that before, but I didn't fully understand the reasoning for the Inherited<C> at all? I guess it's to properly mark that the C is an inherited one, so maybe I do need to remove it on PropagateOver<C> as well, to ensure cleanup doesn't also remove any separately applied C?

I didn't look closely at what all the other tests are attempting to test (or if they are correctly testing those) to evaluate if other changes need to be made.

if PropagateOver<C> doesn't have Inherited<C> and is reparented, would it be simple to ensure that its children have their Inherited<C> removed/updated properly?

Looking at the code more, chances are this can be simplified a lot with the use of AncestorIter and DescendentIter, but I am not sure if jumping into a fuller refactor makes sense at this moment as opposed to attempting to fix this immediate issue and following up with a bigger refactor?

thoughts?

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[robtfm]

I actually made propagatestop include the target since sometimes you want to take it on the stopped entity, and you can always add a propagateover as well if you don’t want it on that entity. It’s hard to include the entity otherwise (you’d have to add stop to all your children).

I’ll check the propagateover behaviour tomorrow.

Edit to add: if we choose to keep propagating onto the stop entity I definitely agree the docs should be more explicit

[ekwoka]

Yes, I figured that was the intent, since that also makes sense to me. Unfortunately that isn't what the code was doing, which gave me some headache where I couldn't tell why this wasn't working in my project.

So this fixes that behavior.

[ekwoka]

I added more explanation to the main post to show the expected and intended behavior (as I understand it) as well as some additional comments and questions.

[robtfm]

apologies, in my previous comment i didn't have the code in front of me. i see now that the behaviour i described is not what it was actually doing. i fully agree that pushing C onto the PropagateStop::<C>-holder is better behaviour, and confirm the implementation does that with your changes.

to ensure cleanup doesn't also remove any separately applied C?

i think you're right. it looks like an entity with (PropagateOver::<C>, C) will have the C removed if the ancestor's Propagate::<C> is removed, as the removal is done in propagate_inherited without checking if there's a PropagateOver::<C>. we should probably process removals separately in propagate_output.

[ekwoka]

Updates tests with more cases and pushed some code to solve those.

It still doesn't handle some other edge cases I can think of (like multiple PropagateOver in a row when one is removed)