Issue 3072: Nested overrides get confused with multiple contexts in play

Issue 3072: Nested overrides get confused with multiple contexts in play

The main problem with nested overrides lies not as much with the
override (which can be done reasonably well) but rather with the
corresponding reverts.  Detecting and undoing a previously established
nested override by its effects on an alist, particularly when nested
overrides may be present at several different levels in several
different contexts and/or stack depths and may be undone in different
order from being established, is a complex problem.  It is complex
enough that it is not clear that a satisfactory solution does even
exist: at least LilyPond's implementation breaks down for a number of
test cases.

The approach of this implementation is to not even try reverting nested
overrides from looking at their effects on the final result.  Instead
nested overrides like

\override Staff.TextSpanner.bound-details.left.text = #"foo"

are not done by creating an assembled alist element for TextSpanner
looking like

(bound-details . ((left . ((text . "foo") ...)) ...))

with ... filled in from other parts of the alist (which may change
independently at a later point of time) but rather by using
(bound-details left text) itself as a key and thus storing

((bound-details left text) . "foo")

as the representation of the override.  Consequently, reverting an
override with the same nested property path is straightforward.  Neither
the action of overriding nor of reverting now require referring to or
updating any part of the property stack outside of the current context.

The downside is that an actual reference to the resulting grob alist
requires expanding the nested overrides at the time of creating a grob
or otherwise asking for ly:context-grob-properties.

Since issue 2507 encapsulated grob property access into the
Grob_property_info algorithm container structure relying on data stored
in context properties of elements of type Grob_properties, changing the
respective algorithms can be facilitated completely in those classes.

In order to provide similarly efficient access and reasonable caching
and just-in-time reevaluation of previously computed values,
Grob_properties needed to be significantly extended in size.

[dak, 2014-08-16 15:48:03]

Actual changes on top of issue 2507

[dak, 2014-08-16 16:22:16]

Actual changes on top of issue 2507

[lemzwerg, 2014-08-16 18:36:50]

I don't actually understand the code, but I like the encapsulation, so: LGTM.

[dak, 2014-08-16 18:55:57]

On 2014/08/16 18:36:50, lemzwerg wrote:
> I don't actually understand the code, but I like the encapsulation, so: LGTM.

Well, the encapsulation is actually issue 2507 (just presented here as Patch Set
1 so that patchy will be able to test the patch on top).  This issue itself is
Patch Set 2, so it's basically just lily/context-property.cc,
lily/include/context.hh and lily/nested-property.cc.  Which is nicely
well-confined, but that's because of issue 2507.

If you like the encapsulation, you like the patch for issue 2507.  If you like
that nested properties will just work without handwaving and ifs and buts, you
like this issue.

The encapsulation means that it should be reasonably straightforward to swap out
the internals for something different again at some other point of time if
that's desirable.

[Carl, 2014-08-23 20:40:21]

On 2014/08/16 18:55:57, dak wrote:
> On 2014/08/16 18:36:50, lemzwerg wrote:
> 
> If you like the encapsulation, you like the patch for issue 2507.  If you like
> that nested properties will just work without handwaving and ifs and buts, you
> like this issue.

I really like this solution.  I spent some time trying to figure out how to get
reverts
right during my work on beaming; I never found a good algorithm.  This makes it
nice.

> 
> The encapsulation means that it should be reasonably straightforward to swap
out
> the internals for something different again at some other point of time if
> that's desirable.

I love the encapsulation.  Separating implementation from function is always
good.

Thanks,

Carl

[dak, 2014-08-23 21:41:43]

On 2014/08/23 20:40:21, Carl wrote:
> On 2014/08/16 18:55:57, dak wrote:
> > On 2014/08/16 18:36:50, lemzwerg wrote:
> > 
> > If you like the encapsulation, you like the patch for issue 2507.  If you
like
> > that nested properties will just work without handwaving and ifs and buts,
you
> > like this issue.
> 
> I really like this solution.  I spent some time trying to figure out how to
get
> reverts
> right during my work on beaming; I never found a good algorithm.

Neither did I, and I spent a non-trivial amount of time over the last years
(there are some rather old "I will do this" kinds of comment on the respective
issues that were actually bolstered by sketches, code, planning, pseudocode
etc).  So this does not even try.  It just reapplies all remaining nested
overrides from scratch when a result is needed, with some caching/memoization to
avoid the effort for some trivial but frequent situations (like not using nested
overrides at all).  I am still not 100% sure that it would not have been doable
in the original spirit.  But I'm pretty sure that it would have been very hard
to convincingly prove correct.

I spent a lot less time on writing this version that I spent on trying to get
the problems from the original approach under control.  And I spent no time at
all now on drawing diagrams of data structures and brooding over their
implications depending on the order in which several modifications were applied.
 The remaining bugs after getting the stuff to compile were of the "stupid typo"
rather than "weird special case" type.  All that is a good sign.

> I love the encapsulation.  Separating implementation from function is always
good.

Well, the bookkeeping of the internal data structures was scattered all over the
place.  Keeping it in that manner would not have been prudent given the
additional complexity.  I probably spent more time on the encapsulation than
actually replacing the internals.