Reviewers: Keith, Graham Percival,
http://codereview.appspot.com/6109046/diff/1/flower/include/direction.hh
File flower/include/direction.hh (right):
http://codereview.appspot.com/6109046/diff/1/flower/include/direction.hh#newcode63
flower/include/direction.hh:63: // huh?
On 2012/04/24 03:32:28, Keith wrote:
If you replace *all* the while(flip()) loops, you can remove the flip
function,
which is merely object-oriented obfuscation for the unary minus
operator.
Well, there will be one more place: spacing-spanner.cc:291
There is:
while (flip (&d) != LEFT && rb);
By now I can't tell what the whole code is for. Could you give me a tip?
http://codereview.appspot.com/6109046/diff/1/flower/include/direction.hh#newcode78
flower/include/direction.hh:78: #define DOWN_and_UP(d) \
On 2012/04/24 04:00:02, Graham Percival wrote:
I see that our code uses both versions, and I fully support this patch
making a
simple substitution for these macros.
After it's accepted, it might be nice to investigate this further: do
we ever
depend on the order of DOWN_and_UP, and if not, could we standardize
on
UP_and_DOWN (or vice versa).
It might also be nice to standardize on either (d) or (dir); I support
the
latter. However, this is again something which should happen after
this patch
is pushed.
Yes - first let's push THAT patch and then discuss on other changes
linked to my macros :)
Btw as for (d) or (dir) almost always (d) was used in the code. (dir)
occured only sometimes. In some single cases there were other names used
such as: downup, event_dir, which or updowndir. But that's a topic for a
next patch :)
http://codereview.appspot.com/6109046/diff/1/lily/ledger-line-spanner.cc
File lily/ledger-line-spanner.cc (right):
http://codereview.appspot.com/6109046/diff/1/lily/ledger-line-spanner.cc#newcode52
lily/ledger-line-spanner.cc:52: + current_extents[d].length ();
On 2012/04/24 04:10:36, Keith wrote:
On 2012/04/24 03:32:28, Keith wrote:
> but it looks okay to me.
I take that back. There is exactly the problem that HanWen predicted
in the
email chain. Chords in seconds get a bit too much space, if they have
ledgers.
{ \time 4/1
\override Score.SpacingSpanner #'packed-spacing = ##t
c'1 a <b'' c'''> <g''' a'''> }
The cause seems to be the decision to scale the ledger from the
notecolumn
width, so it seems to fix it would require a bit of a redesign.
Certainly not related to the rest of this patch.
To be honest, by now I don't try to understand what Han Wenn and you are
trying to say about this bug - first I would like to finish the
for_UP_and_DOWN patch. Is that ok? :)
As I guess, this bug should be filed as a separate issue, right? Should
I ask such questions or just do what I think is proper?
http://codereview.appspot.com/6109046/diff/1/lily/ledger-line-spanner.cc#newcode68
lily/ledger-line-spanner.cc:68: while (flip (&d) != DOWN);
On 2012/04/24 03:32:28, Keith wrote:
Possibly you have found the cause for
<http://code.google.com/p/lilypond/issues/detail?id=2493>
One nice thing about your macro (or a loop with both conditions in one
place) is
that it helps to avoid this type of mistake.
Nice although that was of course unintentional :)
On 22 April 2012 13:28, Han-Wen Nienhuys <hanw...@gmail.com> wrote:
Looks like a bug. Maybe you could work on a test/repro case?
Well, my understanding of this code is really, really vague, so it would
take me a lot of time to: understand this, learn how to write test cases
and then write one.
Therefore I will just do what Keith suggested - I'll correct that in
this issue, but as a separate commit.
Description:
Macro for(UP_and_DOWN) and 3 similar.
Replaces do{ ... } while(flip (&d) != DOWN/UP/... with a macro
for(DOWN_and_UP/UP_and_DOWN/....) { }
http://code.google.com/p/lilypond/issues/detail?id=2491
Please review this at http://codereview.appspot.com/6109046/
Affected files:
M flower/include/direction.hh
M lily/ambitus-engraver.cc
M lily/axis-group-interface.cc
M lily/beam-quanting.cc
M lily/beam.cc
M lily/bezier.cc
M lily/break-substitution.cc
M lily/dynamic-align-engraver.cc
M lily/figured-bass-continuation.cc
M lily/hairpin.cc
M lily/horizontal-bracket.cc
M lily/interval-minefield.cc
M lily/item.cc
M lily/ledger-line-spanner.cc
M lily/line-spanner.cc
M lily/lookup.cc
M lily/lyric-hyphen.cc
M lily/multi-measure-rest-engraver.cc
M lily/multi-measure-rest.cc
M lily/new-fingering-engraver.cc
M lily/note-collision.cc
M lily/note-spacing.cc
M lily/ottava-bracket.cc
M lily/ottava-engraver.cc
M lily/paper-column.cc
M lily/piano-pedal-bracket.cc
M lily/rest-collision.cc
M lily/rod.cc
M lily/script-column.cc
M lily/slur-configuration.cc
M lily/slur-scoring.cc
M lily/slur.cc
M lily/spacing-determine-loose-columns.cc
M lily/spacing-interface.cc
M lily/spanner.cc
M lily/staff-symbol.cc
M lily/stem.cc
M lily/system-start-delimiter.cc
M lily/system.cc
M lily/tie-column.cc
M lily/tie-engraver.cc
M lily/tie-formatting-problem.cc
M lily/tie.cc
M lily/tuplet-bracket.cc
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
