[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Issue 5251/1: set default restNumberThreshold = 1 (issue 353850043 by ad
From: |
thomasmorley65 |
Subject: |
Issue 5251/1: set default restNumberThreshold = 1 (issue 353850043 by address@hidden) |
Date: |
Thu, 27 Dec 2018 04:56:57 -0800 |
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 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?
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.
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 {
no need for \relative here
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.
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);
I don't think this is the correct way.
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:
\relative {
\compressFullBarRests
\set restNumberThreshold = 0
R1 R1*2
\unset restNumberThreshold
R1 R1*2
}
with:
ERROR: Wrong type (expecting exact integer): ()
Leaving the user perplexed what he might have done.
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.
https://codereview.appspot.com/353850043/
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Issue 5251/1: set default restNumberThreshold = 1 (issue 353850043 by address@hidden),
thomasmorley65 <=