[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Allow a markup to replace the default LyricHyphen (issue 325470043 b
From: |
dak |
Subject: |
Re: Allow a markup to replace the default LyricHyphen (issue 325470043 by address@hidden) |
Date: |
Sat, 16 Sep 2017 12:53:56 -0700 |
Rats, forgot to publish my code comments.
Indepent of those: I don't actually see this addressing issue 1255, so
maybe create a new issue in the Sourceforge issue tracker for it and
attach this Rietveld issue? It seems at best loosely related to issue
#1255 but seems to be desirable nevertheless.
https://codereview.appspot.com/325470043/diff/20001/lily/lyric-hyphen.cc
File lily/lyric-hyphen.cc (right):
https://codereview.appspot.com/325470043/diff/20001/lily/lyric-hyphen.cc#newcode125
lily/lyric-hyphen.cc:125: Box b (Interval (0, dash_length), Interval (h,
h + th));
It seems wasteful to put these in the loop, particularly since dash_mol
is then additionally copied before shifting. Maybe pull if
(Text_interface::is_markup ...) outside of the loop and have a separate
loop for each branch? There is no shared loop code outside of if/else
in the current implementation anyway.
https://codereview.appspot.com/325470043/diff/20001/scm/define-grobs.scm
File scm/define-grobs.scm (right):
https://codereview.appspot.com/325470043/diff/20001/scm/define-grobs.scm#newcode1403
scm/define-grobs.scm:1403: (text . '())
That's the default when unset. For properties that may optionally be
set, in particular when this default formally does not match the
predicate, I think we tend not to explicitly specify it.
https://codereview.appspot.com/325470043/