Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(160)

Issue 553750044: \compressFullBarRests should be renamed

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 5 days ago by Valentin Villenave
Modified:
2 weeks, 2 days ago
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

\compressFullBarRests should be renamed - Rename \compressFullBarRests to \compressEmptyMeasures as suggested by Trevor in #4375. - Document the new command (and explain its difference with \compressMMRests) by creating a new subsubsec in NR 1.6.3 "Writing parts". - Add index entries and links everywhere I can think of, obviously starting with NR 1.2.2.3 "Full measure rests". - Add convert rule and update syntax through the doc. - Clarify the (in)famous progerror "Multi_measure_rest::get_rods (): I am not spanned!" since a) the function it refers to has changed anyway b) its wording’s never been particularly helpful IMO. - This patch will require po-update and makelsr at some point (and snippets/new should be checked for duplicate stuff now that the LSR’s been updated).

Patch Set 1 #

Total comments: 13
Unified diffs Side-by-side diffs Delta from patch set Stats (+196 lines, -98 lines) Patch
M Documentation/changes.tely View 1 chunk +6 lines, -0 lines 3 comments Download
M Documentation/de/changes.tely View 1 chunk +2 lines, -2 lines 0 comments Download
M Documentation/de/notation/changing-defaults.itely View 3 chunks +3 lines, -3 lines 0 comments Download
M Documentation/de/notation/rhythms.itely View 8 chunks +15 lines, -15 lines 1 comment Download
M Documentation/de/notation/staff.itely View 1 chunk +1 line, -1 line 0 comments Download
M Documentation/fr/changes.tely View 1 chunk +2 lines, -2 lines 0 comments Download
M Documentation/it/changes.tely View 1 chunk +2 lines, -2 lines 0 comments Download
M Documentation/ja/changes.tely View 2 chunks +3 lines, -3 lines 0 comments Download
M Documentation/ja/notation/rhythms.itely View 2 chunks +3 lines, -3 lines 0 comments Download
M Documentation/notation/ancient.itely View 2 chunks +2 lines, -2 lines 0 comments Download
M Documentation/notation/changing-defaults.itely View 1 chunk +1 line, -0 lines 0 comments Download
M Documentation/notation/rhythms.itely View 7 chunks +17 lines, -43 lines 2 comments Download
M Documentation/notation/staff.itely View 2 chunks +119 lines, -3 lines 7 comments Download
M Documentation/snippets/multi-measure-rest-length-control.ly View 1 chunk +4 lines, -5 lines 0 comments Download
M Documentation/snippets/new/multi-measure-rest-length-control.ly View 2 chunks +2 lines, -2 lines 0 comments Download
A + Documentation/snippets/new/numbering-single-measure-rests.ly View 2 chunks +2 lines, -2 lines 0 comments Download
M Documentation/snippets/numbering-single-measure-rests.ly View 2 chunks +2 lines, -2 lines 0 comments Download
M input/regression/merge-rests-engraver.ly View 1 chunk +1 line, -1 line 0 comments Download
M input/regression/merge-rests-magnify-staff.ly View 1 chunk +1 line, -1 line 0 comments Download
M input/regression/multi-measure-rest-number-threshold.ly View 1 chunk +1 line, -1 line 0 comments Download
M input/regression/rest-hanging-breve.ly View 1 chunk +1 line, -1 line 0 comments Download
M lily/multi-measure-rest.cc View 1 chunk +1 line, -1 line 0 comments Download
M ly/property-init.ly View 1 chunk +3 lines, -3 lines 0 comments Download
M python/convertrules.py View 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 3
lemzwerg
LGTM, thanks! I have some nits here and there, though. https://codereview.appspot.com/553750044/diff/561590043/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/553750044/diff/561590043/Documentation/changes.tely#newcode66 ...
2 weeks, 5 days ago (2020-03-21 18:27:07 UTC) #1
Trevor Daniels
Thanks for picking this up and taking it over the finishing line, Valentin. LGTM apart ...
2 weeks, 4 days ago (2020-03-22 11:07:23 UTC) #2
Valentin Villenave
2 weeks, 2 days ago (2020-03-24 22:28:22 UTC) #3
Thanks guys, it means a lot! (Particularly from Trevor, who opened the tracker
page in the first place.)

Since I had both your LGTMs, I’ve pushed my patch onto staging as
http://git.savannah.gnu.org/cgit/lilypond.git/commit/?h=staging&id=83045b846a...
(then I realized the review window was technically still open until tomorrow.
Sorry about that.)

I’ve taken into account all your suggestions, with only a couple of additional
comments:

On 2020/03/21 18:27:07, lemzwerg wrote:
> I guess you refer to the `measure-length` property, right?  If this is
correct,
> then you should write `@code{measure-length}` instead.

Hm.  Measure-length was the wording used so far, but it’s really more of an
internal property than something we want to emphasize as user-exposed; it’s not
documented anywhere in the NR other than through grob-properties.scm (not even a
regtest).  So I’d rather rewrite the sentence as "the length of a measure",
which is wordier but a bit clearer I think.

> why @var?

’coz I keep misreading the CG :-/

On 2020/03/22 11:07:23, Trevor Daniels wrote:
> Thanks for picking this up and taking it over the finishing line, Valentin.

My pleasure.  Hopefully I’ll be able to contribute a bit more in the next few
weeks… 
I hope you’re well; take good care of yourself… and stay away from London!

Cheers,
V.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b