Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

Dev mailing list
LGTM from reading the code without testing.  Thanks!


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
Shouldn't this be rather

   Measure-attached spanners ...

?

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#newcode96
lily/measure-attached-spanner.cc:96: }
The `}' is not aligned with `{'.  Maybe incorrect use of tabs?

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#newcode313
scm/define-music-types.scm:313:
In case this a hard line break between `measure-' and `attached', please
avoid it (and do the line break before `measure-').

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#newcode172
scm/scheme-engravers.scm:172: This engraver creates spanners bounded by
the columns which start and
s/which/that/

https://codereview.appspot.com/571180043/

Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

Carl Sorensen
I have not reviewed the code, but this looks like a worthwhile addition.
  Thanks for doing it!

I think the name should be changed from MeasureAttachedSpanner to
BarAttachedSpanner.  A measure is the interval between bar lines.  The
spanner is attached to the bar line.  While it requires some work, I
think it's worth making the change to be more clear in our terminology.

Thanks,

Carl


https://codereview.appspot.com/571180043/

Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

nine.fierce.ballads
In reply to this post by Dev mailing list
On 2019/11/15 19:21:09, Carl wrote:
> I think the name should be changed from MeasureAttachedSpanner to
> BarAttachedSpanner.

Calling it just MeasureSpanner would also address the specific problem
you raised.  Is it more important for the name to state where it is
attached or what it spans?

https://codereview.appspot.com/571180043/

Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

Thomas Morley-2
In reply to this post by Dev mailing list
On 2019/11/16 14:27:25, Dan Eble wrote:
> On 2019/11/15 19:21:09, Carl wrote:
> > I think the name should be changed from MeasureAttachedSpanner to
> > BarAttachedSpanner.

> Calling it just MeasureSpanner would also address the specific problem
you
> raised.  Is it more important for the name to state where it is
attached or what
> it spans?

I'd vote for MeasureSpanner.

https://codereview.appspot.com/571180043/

Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

Thomas Morley-2
In reply to this post by Dev mailing 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:
"View side-by-side diff with in-line comments" is broken for this file.

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)
Mmh, this is commented. Why?
Same below.

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:
"View side-by-side diff with in-line comments" broken here as well

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)
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?

https://codereview.appspot.com/571180043/
Reply | Threaded
Open this post in threaded view
|

Re[2]: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

Trevor Daniels
In reply to this post by Thomas Morley-2

------ Original Message ------
From: [hidden email]
To: [hidden email]; [hidden email];
[hidden email]; [hidden email]
Cc: [hidden email]; [hidden email]
Sent: 16/11/2019 14:30:43
Subject: Re: Implement MeasureAttachedSpanner (issue 571180043 by
[hidden email])

>On 2019/11/16 14:27:25, Dan Eble wrote:
>>On 2019/11/15 19:21:09, Carl wrote:
>> > I think the name should be changed from MeasureAttachedSpanner to
>> > BarAttachedSpanner.
>
>>Calling it just MeasureSpanner would also address the specific problem you
>>raised.  Is it more important for the name to state where it is attached or what
>>it spans?
>
>I'd vote for MeasureSpanner.
+1

Trevor

https://codereview.appspot.com/571180043


Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

Kieren MacMillan-4
>> I'd vote for MeasureSpanner.
> +1

+1
Kieren.


Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

nine.fierce.ballads
In reply to this post by Dev mailing list
I haven't reviewed the ly or scm.


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 <[hidden email]>
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.

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
all caps, please

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"
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.

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);
remove

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 ());
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.

https://codereview.appspot.com/571180043/

Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

Thomas Morley-2
In reply to this post by Dev mailing list
Thanks for working on it !!

Some other nits:


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#newcode93
lily/measure-attached-spanner.cc:93: : ly_symbol2scm ("staff-bar"));
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.

https://codereview.appspot.com/571180043/diff/565230043/lily/measure-attached-spanner.cc#newcode141
lily/measure-attached-spanner.cc:141: "break-overshoot "
Probably add a regtest for break-overshoot.
Or extent input/regression/spanner-break-overshoot.ly

https://codereview.appspot.com/571180043/

Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

Carl Sorensen-3
In reply to this post by Kieren MacMillan-4


On 11/16/19, 7:49 AM, "Kieren MacMillan" <[hidden email]> wrote:

    >> I'd vote for MeasureSpanner.
    > +1
   
    +1
    Kieren.

MeasureSpanner works for me.

Carl

   
   

Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

checkma
In reply to this post by Dev mailing list

Is it nestable? (Can you put one of these spanners inside of another?)

--- Christopher Heckman



https://codereview.appspot.com/571180043/

Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

Andrew Bernard
In reply to this post by Carl Sorensen
MeasureAttachedSpanner is better. I often use span bars with no
barlines. It's the measure I am concerned about.

And don't forget that we Aussies and Brits call a measure a bar!

Andrew

On Sat, 16 Nov 2019 at 06:21, <[hidden email]> wrote:

> I think the name should be changed from MeasureAttachedSpanner to
> BarAttachedSpanner.  A measure is the interval between bar lines.  The
> spanner is attached to the bar line.  While it requires some work, I
> think it's worth making the change to be more clear in our terminology.

Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

James Lowe-3
Hello,

On Mon, 18 Nov 2019 12:56:54 +1100, Andrew Bernard <[hidden email]> wrote:

> MeasureAttachedSpanner is better. I often use span bars with no
> barlines. It's the measure I am concerned about.
>
> And don't forget that we Aussies and Brits call a measure a bar!

Yes, there are many different terms for the many of the same musical 'objects' in different cultures.

So we try our best to standardize:

http://lilypond.org/doc/v2.19/Documentation/contributor-big-page#general-writing

Regards

James
Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

David Nalesnik
In reply to this post by Dev mailing list
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 <[hidden email]>
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


Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

sphema72
In reply to this post by Dev mailing list
how to wipe my dept & how to put money on y bank account ,pay pall .
useing development tool . because What I have lean while im seching
things ,is the is no thing such as money .It just a number123... we work
hard to get that number


https://codereview.appspot.com/571180043/

Reply | Threaded
Open this post in threaded view
|

Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nalesnik@gmail.com)

sphema72
In reply to this post by Dev mailing list
Dear Sir/Madam . I want to know something, how to get nulled to
someone's business system and do whatever I like to do.

https://codereview.appspot.com/571180043/