Wrong loop boundaries when iterating over markup list

[Carl, 2011-02-27 13:59:26]

A couple of comments.

Also, there should be a regression test that demonstrates that these lines work
properly, so we'd know if they're broken.

Thanks,

Carl

http://codereview.appspot.com/4236047/diff/1/lily/paper-book.cc
File lily/paper-book.cc (right):

http://codereview.appspot.com/4236047/diff/1/lily/paper-book.cc#newcode530
lily/paper-book.cc:530: * The property 'first-markup-line is #t iff the given
line is
I believe that our multi-line comment practice is *not* to start each line with
a *.

http://codereview.appspot.com/4236047/diff/1/lily/paper-book.cc#newcode539
lily/paper-book.cc:539: ps->set_boolean_property ("first-markup-line",
I would prefer to see

ps->set_property ("first-markup-line",
      ((list == texts) && (scm_is_pair (scm_cdr(list))) ? SCM_BOOL_T :
SCM_BOOL_F)

instead of creating a new boolean property setting function.

[Patrick McCarty, 2011-02-27 19:08:56]

Hi Boris,

Just a code nitpick (in two places).

Please remember to Cc: lilypond-devel for Rietveld patches so that we know code
review is taking place.

Thanks,
Patrick

http://codereview.appspot.com/4236047/diff/1/lily/paper-book.cc
File lily/paper-book.cc (right):

http://codereview.appspot.com/4236047/diff/1/lily/paper-book.cc#newcode540
lily/paper-book.cc:540: (list == texts) && (scm_is_pair(scm_cdr(list)))); // I
am first && there is more stuff
There should be spaces before all open parens:

  (scm_is_pair (scm_cdr (list)))

http://codereview.appspot.com/4236047/diff/1/lily/paper-book.cc#newcode542
lily/paper-book.cc:542: (!scm_is_pair(scm_cdr(list))) && (list != texts)); // I
am last && not first
My previous comment applies here, too.

[hanwenn, 2011-02-28 00:08:39]

http://codereview.appspot.com/4236047/diff/1/lily/include/prob.hh
File lily/include/prob.hh (right):

http://codereview.appspot.com/4236047/diff/1/lily/include/prob.hh#newcode59
lily/include/prob.hh:59: void set_boolean_property (const char *sym, bool val);
can you use set_property() rather than adding this method?  set_property
memoizes the symbol lookup.

http://codereview.appspot.com/4236047/diff/1/lily/paper-book.cc
File lily/paper-book.cc (right):

http://codereview.appspot.com/4236047/diff/1/lily/paper-book.cc#newcode542
lily/paper-book.cc:542: (!scm_is_pair(scm_cdr(list))) && (list != texts)); // I
am last && not first
why not introduce a 

  Prob *first = 0; 
  Prob *last = 0;

and init those in the loop? That would make the logic more explicit than having
comments.

[Keith, 2011-02-28 02:41:58]

> Also, there should be a regression test that demonstrates that these lines
work

Boris,
  I tested this using the existing regression test 'markup-lines.ly' by
shrinking its page size to A7, increasing baseline-skip from 2.5 to 2.8,
inserting a one-line \paragraph {Se continue...} in the middle, and an empty
\markuplines{} at the end.
  The behavior looks good to me.

[Keith, 2011-03-07 02:12:49]

Boris,
  This is the last remaining critical bug preventing version 2.14.  
I applied the suggestions above to your patch, resulting in 
http://codereview.appspot.com/4253061/

Would you either comment there, or adopt the revisions, 
whichever is appropriate given the amount of time you can give to LilyPond.

--Keith