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): https://coderevie
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/