Re: Issue 5251/1: set default restNumberThreshold = 1 (issue 353850043 by address@hidden)

[lilypond]

Reviewers: thomasmorley651,


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 to test your patch, but 'make' failed with
> lilypond-book.py: error: file not found:
numbering-single-measure-rests.ly
> I had to exclude this lines to finish 'make'

> Is there the _need_ to import lsr first?

Seems so.

> If so, I think it should be part of the current patch in an own
commit. Imho,
> it's good practise to have every issue's patch-set being able to stand
alone.

(See my next comment)

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 12:56:57, thomasmorley651 wrote:
> no need for \relative here

Oh, of course you’re right, I’ll change that.

> Meanwhile I've approved this snippet in LSR (deleting \relative). So I
don't
> think there is any need to put it in Documentation/snippets/new as
well.
> It will be available after next lsr-import anyway.

I can’t find the snippet using the search function; why?

I followed James’s directions
(https://lists.gnu.org/archive/html/lilypond-devel/2018-12/msg00153.html),
that’s why I didn’t do the makelsr.py run.

https://codereview.appspot.com/353850043/diff/1/lily/multi-measure-rest-engraver.cc
File lily/multi-measure-rest-engraver.cc (right):

https://codereview.appspot.com/353850043/diff/1/lily/multi-measure-rest-engraver.cc#newcode186
lily/multi-measure-rest-engraver.cc:186: int t = scm_to_int (thres);
On 2018/12/27 12:56:57, thomasmorley651 wrote:
> I don't think this is the correct way.

You’re right, I hadn’t considered \unset.

> We want a fall-back-value if restNumberThreshold is unset. Only
setting it in
> engraver-init.ly is not sufficient, imho, because setting
context-properties
> will not create a reversible stack. Thus this example fails:

I didn’t know that, thanks for explanation.

> Not sure if the old coding is the best way o provide such a fall-back,
my
> C++-knowledge is close to zero, so I can't come up with a better
proposal.

It isn’t, we have robust_scm2int for that; I’ll use it in the next patch
set.

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

https://codereview.appspot.com/353850043/diff/1/ly/engraver-init.ly#newcode272
ly/engraver-init.ly:272: restNumberThreshold = 1
Should we keep this even if we have a default value in
multi-measure-rest-engraver.cc? IMHO: yes, because then it will be
listed in the IR.

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

Please review this at https://codereview.appspot.com/353850043/

Affected files (+27, -3 lines):
  M Documentation/notation/rhythms.itely
  A Documentation/snippets/new/numbering-single-measure-rests.ly
  M lily/multi-measure-rest-engraver.cc
  M ly/engraver-init.ly


Index: Documentation/notation/rhythms.itely
diff --git a/Documentation/notation/rhythms.itely  
b/Documentation/notation/rhythms.itely
index  
f0d2954d28916f33ca729a3c055ae10377894c60..68706c4b9e71addf2c0fe47f8a90318d6040cdab  
100644
--- a/Documentation/notation/rhythms.itely
+++ b/Documentation/notation/rhythms.itely
@@ -962,6 +962,9 @@ setting, resulting bar-check warnings may not be  
displayed.
 @lilypondfile[verbatim,quote,ragged-right,texidoc,doctitle]
 {multi-measure-rest-length-control.ly}

address@hidden,quote,ragged-right,texidoc,doctitle]
+{numbering-single-measure-rests.ly}
+
 @lilypondfile[verbatim,quote,ragged-right,texidoc,doctitle]
 {changing-form-of-multi-measure-rests.ly}

Index: Documentation/snippets/new/numbering-single-measure-rests.ly
diff --git a/Documentation/snippets/new/numbering-single-measure-rests.ly  
b/Documentation/snippets/new/numbering-single-measure-rests.ly
new file mode 100644
index  
0000000000000000000000000000000000000000..7db6b0f9513ae9aac60be2a400937979e020fbcc
--- /dev/null
+++ b/Documentation/snippets/new/numbering-single-measure-rests.ly
@@ -0,0 +1,21 @@
+\version "2.18.2"
+
+\header {
+  lsrtags = "rhythms, preparing-parts"
+
+  texidoc = "
+Multi measure rests show their length by a number except for single
+measures. This can be changed by setting @code{restNumberThreshold}.
+
+"
+  doctitle = "Numbering single measure rests"
+}
+
+\relative {
+  \compressFullBarRests
+  R1 R1*10 R1*11 \bar "||"
+  \set restNumberThreshold = 0
+  R1 R1*10 R1*11 \bar "||"
+  \set restNumberThreshold = 10
+  R1 R1*10 R1*11
+}
Index: lily/multi-measure-rest-engraver.cc
diff --git a/lily/multi-measure-rest-engraver.cc  
b/lily/multi-measure-rest-engraver.cc
index  
ff1e155abc1eaef3d4e8e0fc794425a22d32aa4c..e3f59e96552985101955dcc5bb4c69d59b49a700  
100644
--- a/lily/multi-measure-rest-engraver.cc
+++ b/lily/multi-measure-rest-engraver.cc
@@ -183,9 +183,7 @@ Multi_measure_rest_engraver::set_measure_count (int n)
   if (scm_is_null (g->get_property ("text")))
     {
       SCM thres = get_property ("restNumberThreshold");
-      int t = 1;
-      if (scm_is_number (thres))
-        t = scm_to_int (thres);
+      int t = scm_to_int (thres);

       if (n <= t)
         g->suicide ();
Index: ly/engraver-init.ly
diff --git a/ly/engraver-init.ly b/ly/engraver-init.ly
index  
325622a15c8a2b07e0d1fa84c707a3308c320809..a3f34c429562013b1b05880a1c802f4ada455a3e  
100644
--- a/ly/engraver-init.ly
+++ b/ly/engraver-init.ly
@@ -268,6 +268,8 @@ multiple voices on the same staff."
   \consists "Tie_engraver"
   \consists "Tuplet_engraver"
   \consists "Instrument_switch_engraver"
+
+  restNumberThreshold = 1
 }

 \context{