Reviewers: lemzwerg, carl.d.sorensen_gmail.com, Dan Eble, thomasmorley651,
t.daniels_treda.co.uk, kieren_kierenmacmillan.info, c_sorensen, checkma,
Message:
Name has been changed to MeasureSpanner, and all references (including
file names) have been adjusted. Thanks for the reviews!
https://codereview.appspot.com/571180043/diff/565230043/input/regression/measure-spanner.ly
File input/regression/measure-spanner.ly (right):
https://codereview.appspot.com/571180043/diff/565230043/input/regression/measure-spanner.ly#newcode5
input/regression/measure-spanner.ly:5: Measure attached spanners can
span single and multiple
On 2019/11/15 17:49:24, lemzwerg wrote:
Shouldn't this be rather
Measure-attached spanners ...
?
Changed to reflect new name
https://codereview.appspot.com/571180043/diff/565230043/lily/include/measure-attached-spanner.hh
File lily/include/measure-attached-spanner.hh (right):
https://codereview.appspot.com/571180043/diff/565230043/lily/include/measure-attached-spanner.hh#newcode4
lily/include/measure-attached-spanner.hh:4: Copyright (C) 1997--2015 Jan
Nieuwenhuizen <jann...@gnu.org>
On 2019/11/16 18:07:37, Dan Eble wrote:
When you add a new file, I think you're supposed to use (C) <this
year> <your
name>. At least, that's what I was once told.
Done.
https://codereview.appspot.com/571180043/diff/565230043/lily/include/measure-attached-spanner.hh#newcode20
lily/include/measure-attached-spanner.hh:20: #ifndef
Measure_attached_spanner_HH
On 2019/11/16 18:07:37, Dan Eble wrote:
all caps, please
Done.
https://codereview.appspot.com/571180043/diff/565230043/lily/include/measure-attached-spanner.hh#newcode24
lily/include/measure-attached-spanner.hh:24: #include "std-vector.hh"
On 2019/11/16 18:07:37, Dan Eble wrote:
I don't see anything in this header that uses a vector. Unless I'm
wrong, this
should be moved to whichever cc files require it. Same goes for any
other
unnecessary headers.
Done. (Unnecessary includes pruned from lily/measure-spanner.cc as
well.)
https://codereview.appspot.com/571180043/diff/565230043/lily/measure-attached-spanner.cc
File lily/measure-attached-spanner.cc (right):
https://codereview.appspot.com/571180043/diff/565230043/lily/measure-attached-spanner.cc#newcode45
lily/measure-attached-spanner.cc:45: //Direction dir =
get_grob_direction (me);
On 2019/11/16 18:07:37, Dan Eble wrote:
remove
Done.
https://codereview.appspot.com/571180043/diff/565230043/lily/measure-attached-spanner.cc#newcode53
lily/measure-attached-spanner.cc:53: Spanner *orig_spanner =
dynamic_cast<Spanner *> (me->original ());
On 2019/11/16 18:07:37, Dan Eble wrote:
If I understand correctly, me->original () can only ever be either
null or
another instance of the same type as me; therefore, use static_cast
here.
Also, if it's logically possible for me->original () to be null in
this case,
check for it before dereferencing below.
Done. (Updated according to your recently pushed patch.)
https://codereview.appspot.com/571180043/diff/565230043/lily/measure-attached-spanner.cc#newcode93
lily/measure-attached-spanner.cc:93: : ly_symbol2scm ("staff-bar"));
On 2019/11/16 18:41:00, thomasmorley651 wrote:
If I understand correctly (and I may be wrong, because my knowledge
about c++ is
more or less zero), then "staff-bar" is a fall-back.
I'd create an entry for 'spacing-pair' in define-grobs.scm, too.
Similar to
MeasureCounter, MultiMeasure Rest and PercentRepeat.
Done.
https://codereview.appspot.com/571180043/diff/565230043/lily/measure-attached-spanner.cc#newcode96
lily/measure-attached-spanner.cc:96: }
On 2019/11/15 17:49:24, lemzwerg wrote:
The `}' is not aligned with `{'. Maybe incorrect use of tabs?
Done.
https://codereview.appspot.com/571180043/diff/565230043/lily/measure-attached-spanner.cc#newcode141
lily/measure-attached-spanner.cc:141: "break-overshoot "
On 2019/11/16 18:41:00, thomasmorley651 wrote:
Probably add a regtest for break-overshoot.
Or extent input/regression/spanner-break-overshoot.ly
break-overshoot is not a supported property; removed from interface list
https://codereview.appspot.com/571180043/diff/565230043/ly/spanners-init.ly
File ly/spanners-init.ly (right):
https://codereview.appspot.com/571180043/diff/565230043/ly/spanners-init.ly#newcode25
ly/spanners-init.ly:25:
On 2019/11/16 14:42:02, thomasmorley651 wrote:
"View side-by-side diff with in-line comments" is broken for this
file.
Yeah, this happened to me in the past. Not sure what to do about it.
https://codereview.appspot.com/571180043/diff/565230043/scm/define-grobs.scm
File scm/define-grobs.scm (right):
https://codereview.appspot.com/571180043/diff/565230043/scm/define-grobs.scm#newcode1457
scm/define-grobs.scm:1457: ;(font-size . -2)
On 2019/11/16 14:42:02, thomasmorley651 wrote:
Mmh, this is commented. Why?
Same below.
Just some test code. Removed.
https://codereview.appspot.com/571180043/diff/565230043/scm/define-music-types.scm
File scm/define-music-types.scm (right):
https://codereview.appspot.com/571180043/diff/565230043/scm/define-music-types.scm#newcode311
scm/define-music-types.scm:311:
On 2019/11/16 14:42:02, thomasmorley651 wrote:
"View side-by-side diff with in-line comments" broken here as well
See other comment.
https://codereview.appspot.com/571180043/diff/565230043/scm/define-music-types.scm#newcode313
scm/define-music-types.scm:313:
On 2019/11/15 17:49:24, lemzwerg wrote:
In case this a hard line break between `measure-' and `attached',
please avoid
it (and do the line break before `measure-').
Done.
https://codereview.appspot.com/571180043/diff/565230043/scm/scheme-engravers.scm
File scm/scheme-engravers.scm (right):
https://codereview.appspot.com/571180043/diff/565230043/scm/scheme-engravers.scm#newcode98
scm/scheme-engravers.scm:98: (define-public
(Measure_attached_spanner_engraver context)
On 2019/11/16 14:42:02, thomasmorley651 wrote:
Not related to the current patch:
Meanwhile I've seen several scheme-spanners-engravers for new
spanner-grobs (and
wrote some for my own work) or to customize existing spanner-grobs.
All are more or less the same, ofcourse they need to be so.
I'm musing whether it would be possible to create some specialized
macro. We
already have `make-engraver´, maybe something like
`make-spanner-engraver´.
Thoughts?
I think so. I can imagine lots of similar spanners which would involve
lots of redundant code.
https://codereview.appspot.com/571180043/diff/565230043/scm/scheme-engravers.scm#newcode172
scm/scheme-engravers.scm:172: This engraver creates spanners bounded by
the columns which start and
On 2019/11/15 17:49:24, lemzwerg wrote:
s/which/that/
Done.
Description:
Implement MeasureAttachedSpanner
This patch creates a spanner whose ends are oriented to measure
boundaries, a frequent request from users. The ends of the
spanner may be aligned in various ways to prefatory material.
The spanner may hold text or markup in a centered gap.
The spanner is begun with the command '\startMeasureAttachedSpanner'
and ended with '\stopMeasureAttachedSpanner'.
Provide two regression tests.
Please review this at https://codereview.appspot.com/571180043/
Affected files (+317, -16 lines):
A input/regression/measure-spanner.ly
A input/regression/measure-spanner-spacing-pair.ly
M lily/bracket.cc
M lily/enclosing-bracket.cc
M lily/include/bracket.hh
lily/include/measure-spanner.hh
A lily/measure-spanner.cc
M ly/spanners-init.ly
M scm/define-event-classes.scm
M scm/define-grobs.scm
scm/define-music-types.scm
M scm/scheme-engravers.scm
