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

Issue 353850043: Issue 5251/1: set default restNumberThreshold = 1

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 3 weeks ago by Malte Meyn
Modified:
5 months, 2 weeks ago
Reviewers:
thomasmorley651
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Issue 5251/1: set default restNumberThreshold = 1 Issue 5251/2: add snippet for doc Issue 5251/3: add snippet to NR

Patch Set 1 #

Total comments: 10

Patch Set 2 : fix bug mentioned at SF, add regtest #

Patch Set 3 : run makelsr #

Patch Set 4 : clean patchset without makelsr based on makelsr-ed master #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -6 lines) Patch
M Documentation/notation/rhythms.itely View 1 1 chunk +3 lines, -0 lines 0 comments Download
A input/regression/multi-measure-rest-number-threshold.ly View 1 1 chunk +18 lines, -0 lines 1 comment Download
M lily/multi-measure-rest-engraver.cc View 1 3 chunks +3 lines, -6 lines 0 comments Download
M ly/engraver-init.ly View 1 1 chunk +2 lines, -0 lines 1 comment Download

Messages

Total messages: 9
thomasmorley651
Hi Malte, some concerns: https://codereview.appspot.com/353850043/diff/1/Documentation/notation/rhythms.itely File Documentation/notation/rhythms.itely (right): https://codereview.appspot.com/353850043/diff/1/Documentation/notation/rhythms.itely#newcode966 Documentation/notation/rhythms.itely:966: {numbering-single-measure-rests.ly} I tried to test ...
5 months, 3 weeks ago (2018-12-27 12:56:57 UTC) #1
Malte Meyn
https://codereview.appspot.com/353850043/diff/1/Documentation/notation/rhythms.itely File Documentation/notation/rhythms.itely (right): https://codereview.appspot.com/353850043/diff/1/Documentation/notation/rhythms.itely#newcode966 Documentation/notation/rhythms.itely:966: {numbering-single-measure-rests.ly} On 2018/12/27 12:56:57, thomasmorley651 wrote: > I tried ...
5 months, 3 weeks ago (2018-12-27 14:35:18 UTC) #2
thomasmorley651
https://codereview.appspot.com/353850043/diff/1/Documentation/snippets/new/numbering-single-measure-rests.ly File Documentation/snippets/new/numbering-single-measure-rests.ly (right): https://codereview.appspot.com/353850043/diff/1/Documentation/snippets/new/numbering-single-measure-rests.ly#newcode14 Documentation/snippets/new/numbering-single-measure-rests.ly:14: \relative { On 2018/12/27 14:35:18, Malte Meyn wrote: > ...
5 months, 3 weeks ago (2018-12-27 20:58:55 UTC) #3
Malte Meyn
https://codereview.appspot.com/353850043/diff/1/Documentation/snippets/new/numbering-single-measure-rests.ly File Documentation/snippets/new/numbering-single-measure-rests.ly (right): https://codereview.appspot.com/353850043/diff/1/Documentation/snippets/new/numbering-single-measure-rests.ly#newcode14 Documentation/snippets/new/numbering-single-measure-rests.ly:14: \relative { On 2018/12/27 20:58:55, thomasmorley651 wrote: > On ...
5 months, 3 weeks ago (2018-12-27 21:05:58 UTC) #4
thomasmorley651
While you're on it, how about creating a reg-test? Currently 'restNumberThreshold' is only used in ...
5 months, 3 weeks ago (2018-12-27 22:05:21 UTC) #5
Malte Meyn
fix bug mentioned at SF, add regtest
5 months, 3 weeks ago (2018-12-28 10:57:45 UTC) #6
Malte Meyn
run makelsr
5 months, 3 weeks ago (2018-12-28 10:59:22 UTC) #7
Malte Meyn
clean patchset without makelsr based on makelsr-ed master
5 months, 3 weeks ago (2018-12-29 17:28:12 UTC) #8
thomasmorley651
5 months, 2 weeks ago (2019-01-05 10:52:26 UTC) #9
Sorry to come back to this that late.
I can't comment on the C++ part of it. Some other remarks, though:

https://codereview.appspot.com/353850043/diff/60001/input/regression/multi-me...
File input/regression/multi-measure-rest-number-threshold.ly (right):

https://codereview.appspot.com/353850043/diff/60001/input/regression/multi-me...
input/regression/multi-measure-rest-number-threshold.ly:14: \set
restNumberThreshold = 0
Iiuc, this tests whether \unset works but not whether "only future multi measure
rests" are affected. To do so some code default-code should be done first, imho.
Probably:
{
  \compressFullBarRests
  R1*2 R1
  \set restNumberThreshold = 0
  R1*2 R1
  \unset restNumberThreshold
  R1*2 R1
}

https://codereview.appspot.com/353850043/diff/60001/ly/engraver-init.ly
File ly/engraver-init.ly (right):

https://codereview.appspot.com/353850043/diff/60001/ly/engraver-init.ly#newco...
ly/engraver-init.ly:272: restNumberThreshold = 1
Inserting the deafult here in Voice, is not sufficient, see:
{
  \applyContext 
    #(lambda (ctx) 
      (write (ly:context-property ctx 'restNumberThreshold)))
  R1
}
displaying '() 

I'd do it in Score. Settings for Chords and Tabs are done in Score as well,
probably for the same reason.
Sign in to reply to this message.

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