Re: Implement MeasureAttachedSpanner (issue 571180043 by address@hidden)

[david . nalesnik]

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 <address@hidden>
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