[prev in list] [next in list] [prev in thread] [next in thread] 

List:       lilypond-devel
Subject:    Re: [GSoC] Implement cross-voice dynamic spanners (issue 304160043 by starrynte@gmail.com)
From:       starrynte () gmail ! com
Date:       2016-07-25 10:47:33
Message-ID: 001a113451ce0176080538738305 () google ! com
[Download RAW message or body]

Thank you very much for taking the time to review this!

On 2016/07/25 10:01:52, dak wrote:
> The main problem I see with the current code is that it is ad-hoc and
basically
> unmaintainable.  Spanner_engraver is a class that really serves no
clear purpose
> apart from carrying a few helper functions.  The collection of helper
functions
> does not create a coherent design.  All the actual management of
multiple
> spanners still happens in the spanner classes themselves.
...
> So the question is how would we _want_ multiple-spanner management to
be
> reflected in the code?  It's complex and mostly orthogonal to the
actual work
> (except when multiple spanners or likely rather their grobs have to
notice each
> other in order to avoid collisions), so the answer is "as little as
possible".
> The ideal implementation would be to change ASSIGN_EVENT_ONCE to
> ASSIGN_EVENT_SPANNER_ID and not change anything else.  However, the
amount of
> trickery that would need to get done then would be considerable.

> However, the principle underlying this somewhat unrealistic example is
still
> valid: the bulk of the code should only cater for a single spanner.
If we can
> arrive there, we will also be in a good position to let users create
Scheme
> engravers that can deal with multiple spanners without having to delve
into
> multiple-spanner management themselves.

I agree. Handling multiple spanners in Spanner_engraver seems like a
good idea; I'll try to revise this patch to do so (with the help of your
suggestions).


https://codereview.appspot.com/304160043/diff/20001/input/regression/dynamics-alignment-autobreak.ly#newcode19

> input/regression/dynamics-alignment-autobreak.ly:19: <<
> This code is not really seminal to the topic of this regtest.  You
should rather
> create one or more separate regtests for the new functionality and
have a
> matching description.

> The same for the other regtests.

Will do.

https://codereview.appspot.com/304160043/

_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel


[prev in list] [next in list] [prev in thread] [next in thread]