Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
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/
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
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/
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
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
On 2019/11/16 18:07:37, Dan Eble wrote:
When you add a new file, I think you're supposed to use (C)
year>
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 (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):
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
Hello, On Mon, 18 Nov 2019 12:56:54 +1100, Andrew Bernard 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
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
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, 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.
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
Is it nestable? (Can you put one of these spanners inside of another?) --- Christopher Heckman https://codereview.appspot.com/571180043/
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
On 11/16/19, 7:49 AM, "Kieren MacMillan" wrote: >> I'd vote for MeasureSpanner. > +1 +1 Kieren. MeasureSpanner works for me. Carl
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
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/
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
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 When you add a new file, I think you're supposed to use (C) . 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 (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/
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
>> I'd vote for MeasureSpanner. > +1 +1 Kieren.
Re[2]: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
-- Original Message -- From: thomasmorle...@gmail.com To: david.nales...@gmail.com; lemzw...@googlemail.com; carl.d.soren...@gmail.com; nine.fierce.ball...@gmail.com Cc: re...@codereview-hr.appspotmail.com; lilypond-devel@gnu.org Sent: 16/11/2019 14:30:43 Subject: Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com) 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
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
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/
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
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/
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
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/
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
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/
Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
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/
