Menu

#4973 Group contexts in the Keep_alive_together_engraver

Started
mkdev
None
needs_work
Enhancement
2016-09-26
2016-09-16
mkdev
No

Group contexts in the Keep_alive_together_engraver

This introduces a VerticalAxisGroup.keep-alive-group' property which can be set to a symbol to associate a subset of contexts under the control of the same Keep_alive_together_engraver. This group operates only up to the highestremove-layer` level of its
members.

http://codereview.appspot.com/310230043

Discussion

  • Anonymous

    Anonymous - 2016-09-17
    • Description has changed:

    Diff:

    
    
    • Needs: -->
    • Patch: new --> review
     
  • Anonymous

    Anonymous - 2016-09-17

    Passes make, make check and a full make doc.

     
  • Anonymous

    Anonymous - 2016-09-18
    • Patch: review --> countdown
     
  • Anonymous

    Anonymous - 2016-09-18

    Patch on countdown for Sept 21st

     
  • Anonymous

    Anonymous - 2016-09-25
    • Patch: countdown --> push
     
  • Anonymous

    Anonymous - 2016-09-25

    Patch counted down - please push - although does this new feature (if that is what it is) need some documentation, and an entry in changes.tely?

     
  • David Kastrup

    David Kastrup - 2016-09-25

    Can I suggest holding off? I've not found the time and focus to review but this patch seems to me like it is going down a rabbithole of complexity that is unmanageable at the user level and unnecessarily contorted at the implementation level. I don't see that it accomplishes more (actually less) than the low-level proposal based on lives-with/dies-with previously, and that latter level would be a better basis for creating functionality at a higher level.

    Basically, you rejected that proposal at the time based on the contention that what you implemented instead would be sufficient, and this patch here is based on the realization that it isn't. And it might be followed by further such iterations.

    So I'd really suggest reconsidering this yet-another-bolt-on approach (or the original issue) to see whether we are not better off, after all, with a less contorted and more universally engine on the inside and then just creating some nicer interface on top in Scheme only.

     
  • mkdev

    mkdev - 2016-09-25

    I have no intention of pushing this - it was always intended as a starting point for review and discussion.

    Basically, you rejected that proposal at the time based on the
    contention that what you implemented instead would be
    sufficient, and this patch here is based on the realization that
    it isn't. And it might be followed by further such iterations.

    That is not my recollection of the discussion.

     
  • David Kastrup

    David Kastrup - 2016-09-25

    That is not my recollection of the discussion.

    Obviously, since our approaches have diverged a bit more. And it's not always easy for me to tell just what amount of the discussion happened only in my head: sometimes I come to conclusions after arguing back and forth with myself and after a while it feels like the argument had been made in public when all that appeared there was the conclusion without much of a rationale.

    Now here is the essence of what I was brooding over: my original remove-layer feature was coded more or less on-demand, implemented by providing context properties, and those properties and the underlying mechanism were documented and explained.

    Half a year later, several power users including those requesting the feature were totally baffled that those properties could be used to do just what they needed.

    That was the "simple" request and feature not supporting nested divisi structures.

    So my basic conclusion about this feature as implemented by myself is that it is a copout feature: its only usefulness is to myself since I can then claim "but I already solved that problem". It's not user accessible.

    Now obviously a scheme-accessible mechanism for implementing a user-accessible feature is better than nothing.

    But I think the next thing we need to do is creating an actual user interface for the use cases this mechanism has been created for. One where the user tells LilyPond in musical rather than in programming terms what his music is. This user interface needs not cover every use case the underlying mechanism can be made to cater for: if we forgot an application, one can always add it afterwards, per LSR or in LilyPond proper.

    Your patches here try casting the terms of the mechanism and the properties in the language of the musical application in order to make them more accessible. But when that did not even work convincingly for the simple original remove-layer, I don't think it will fly for the complex ones.

    So I think we should separate what we want to do in musical terms (and create scheme functions and/or container contexts providing all of the requisite folderol behind the scenes) and how it is implemented, and make the former work in straightforward music terms and the latter work in straightforward programming terms.

    I don't think that we can expect to make one interface that's both easy to use and easy to understand (regarding its innermost workings), given the previous experience with the simple version. And this is, as far as my gut feeling goes, exactly what this patch is aiming to do, and I don't see that it makes the ends meet. So I prefer getting the ends done well separately, one end for the user, one for the programmers.

    So I'd like us to figure out how to specify the simple case in a simple manner best (without worrying about the implementation), and then think about how to best generalize this in a manner for multiple divisi situations, if we can in a manner that is not any more complicate but just a generalization of the simple way that can be nested. And only then look for the simplest set of properties and engravers that we would need for implementing this in a reasonably straightforward manner (without reverting to trick programming).

     
  • mkdev

    mkdev - 2016-09-25

    Thanks David. You're probably right, although I'm not sure that this is all solveable in scheme, at least from my understanding of the problem. I won't have time to look at this again for a little while, which may at least have the advantage of a fresh perspective when I return to it.

     
  • Anonymous

    Anonymous - 2016-09-26
    • Patch: push --> needs_work
     

Log in to post a comment.