Code review - Issue 3705042: Doc: More NR and LM additions based on user emailshttps://codereview.appspot.com/2010-12-22T23:43:19+00:00rietveld
Message from unknown
2010-12-16T18:25:31+00:00pkx166hurn:md5:e41375b4d859996bc1a88270c9cac548
Message from pkx166h@gmail.com
2010-12-16T18:28:21+00:00pkx166hurn:md5:c8b00cc426d6aa9397f5b892fa7906dc
Hello,
This is another patch following on from
http://codereview.appspot.com/3581041/
James
Message from percival.music.ca@gmail.com
2010-12-17T14:05:27+00:00Graham Percival (old account)urn:md5:52a19d95170542294057400fb7cee3cb
http://codereview.appspot.com/3705042/diff/1/Documentation/learning/fundamental.itely
File Documentation/learning/fundamental.itely (right):
http://codereview.appspot.com/3705042/diff/1/Documentation/learning/fundamental.itely#newcode2114
Documentation/learning/fundamental.itely:2114: objects to note heads, if you remove the @code{Note_heads_engraver}
I'm not convinced that the comma is grammatical. How do you feel about semicolons?
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely
File Documentation/notation/repeats.itely (right):
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely#newcode150
Documentation/notation/repeats.itely:150: @rprogram{An extra staff appears}. Do not place bar checks outside
Could this be in a separate @warning? I'd rather not have two totally distinct warnings in the same box.
I also think it's a bit wordier than it needs to be -- I really want to keep warnings short, otherwise they lose their power. How about
"Bar checks must be placed @emph{inside} the braces of each alternate block." ? ok, that's not quite the same thing (it says that you *must* use bar checks)... but please try to think of a reformulation along those lines.
Message from Carl.D.Sorensen@gmail.com
2010-12-17T14:21:03+00:00Carlurn:md5:42cdb30ede32823b9b2bcb7bc99c528a
Thanks, James!
Carl
http://codereview.appspot.com/3705042/diff/1/Documentation/learning/fundamental.itely
File Documentation/learning/fundamental.itely (right):
http://codereview.appspot.com/3705042/diff/1/Documentation/learning/fundamental.itely#newcode2114
Documentation/learning/fundamental.itely:2114: objects to note heads, if you remove the @code{Note_heads_engraver}
I agree with Graham. You either need to change the comma to a semicolon, or add a conjunction like "so". I think it would actually be best to put in a period and make it two sentences.
I also think it would be better to reword it as
"If the @code{Note_heads_engraver} is removed"
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely
File Documentation/notation/repeats.itely (right):
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely#newcode151
Documentation/notation/repeats.itely:151: of the braces of the grouped notes within an @code{@bs{}alternative} block, otherwise you will not get the expected number of endings. Any bar checks used inside @code{@bs{}alternative} block must be placed in inside the braces of each group of notes.}
I think that with the addition of the bar checks to the examples, this warning can be eliminated.
The only reason it was needed was because the example we had previously had no bar checks, so a user guessed on where to put them and guessed wrong.
Message from pkx166h@gmail.com
2010-12-17T19:07:51+00:00pkx166hurn:md5:dfcd90db2a9bfa2ed7789b18dde8d2f3
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely
File Documentation/notation/repeats.itely (right):
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely#newcode150
Documentation/notation/repeats.itely:150: @rprogram{An extra staff appears}. Do not place bar checks outside
Hopefully this is better.
I have also moved this warning because there was another place it could go, (the first warning couldn't move anywhere really) and I thought that having two warnings one after another looked odd.
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely#newcode151
Documentation/notation/repeats.itely:151: of the braces of the grouped notes within an @code{@bs{}alternative} block, otherwise you will not get the expected number of endings. Any bar checks used inside @code{@bs{}alternative} block must be placed in inside the braces of each group of notes.}
No I think it is a good warning and would rather this is left in, I have modified the wording as per Graham's suggestion.
Message from unknown
2010-12-17T19:09:28+00:00pkx166hurn:md5:6c544522ce66c8b49f998173599dcf68
Message from Carl.D.Sorensen@gmail.com
2010-12-17T19:13:45+00:00Carlurn:md5:ba82427546b71f9e25baa3715bf52c36
Fine to push, but one last try on the barcheck warning.
Thanks,
Carl
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely
File Documentation/notation/repeats.itely (right):
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely#newcode151
Documentation/notation/repeats.itely:151: of the braces of the grouped notes within an @code{@bs{}alternative} block, otherwise you will not get the expected number of endings. Any bar checks used inside @code{@bs{}alternative} block must be placed in inside the braces of each group of notes.}
Why a warning for barchecks, and not for markups or individual notes?
I still think this is logically inconsistent. But I will approve of pushing it in the revised state if you disagree with me.
Message from tdanielsmusic@googlemail.com
2010-12-17T21:02:21+00:00Trevor Danielsurn:md5:65f0195a5add531bb855b75209a4a33f
Like Carl, I'm happy to approve this as it is, but I'd prefer to see a comprehensive and correct explanation of the effect of inserting code between alternatives.
http://codereview.appspot.com/3705042/diff/1/Documentation/learning/fundamental.itely
File Documentation/learning/fundamental.itely (right):
http://codereview.appspot.com/3705042/diff/1/Documentation/learning/fundamental.itely#newcode2114
Documentation/learning/fundamental.itely:2114: objects to note heads, if you remove the @code{Note_heads_engraver}
Like Carl, I prefer two sentences.
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely
File Documentation/notation/repeats.itely (right):
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely#newcode151
Documentation/notation/repeats.itely:151: of the braces of the grouped notes within an @code{@bs{}alternative} block, otherwise you will not get the expected number of endings. Any bar checks used inside @code{@bs{}alternative} block must be placed in inside the braces of each group of notes.}
Why is this all on one line?
I agree with Graham - it's too long. Use his words with "should" instead of "must".
http://codereview.appspot.com/3705042/diff/8001/Documentation/notation/repeats.itely#newcode239
Documentation/notation/repeats.itely:239: @cindex repeats with ties
I agree with Carl. If there is nothing special about bar checks would it be correct to say, "if there are two or more alternatives nothing should appear between the closing brace of one and the opening brace of the next, or ..."
Message from pkx166h@gmail.com
2010-12-19T11:27:33+00:00pkx166hurn:md5:e45255b8800515247b4f894593c4a8cf
Thanks for the input.
Corrections are done.
http://codereview.appspot.com/3705042/diff/1/Documentation/learning/fundamental.itely
File Documentation/learning/fundamental.itely (right):
http://codereview.appspot.com/3705042/diff/1/Documentation/learning/fundamental.itely#newcode2114
Documentation/learning/fundamental.itely:2114: objects to note heads, if you remove the @code{Note_heads_engraver}
On 2010/12/17 14:21:03, Carl wrote:
> I agree with Graham. You either need to change the comma to a semicolon, or add
> a conjunction like "so". I think it would actually be best to put in a period
> and make it two sentences.
>
> I also think it would be better to reword it as
>
> "If the @code{Note_heads_engraver} is removed"
Done.
http://codereview.appspot.com/3705042/diff/1/Documentation/learning/fundamental.itely#newcode2114
Documentation/learning/fundamental.itely:2114: objects to note heads, if you remove the @code{Note_heads_engraver}
On 2010/12/17 14:21:03, Carl wrote:
> I agree with Graham. You either need to change the comma to a semicolon, or add
> a conjunction like "so". I think it would actually be best to put in a period
> and make it two sentences.
>
> I also think it would be better to reword it as
>
> "If the @code{Note_heads_engraver} is removed"
Done.
http://codereview.appspot.com/3705042/diff/1/Documentation/learning/fundamental.itely#newcode2114
Documentation/learning/fundamental.itely:2114: objects to note heads, if you remove the @code{Note_heads_engraver}
On 2010/12/17 21:02:21, Trevor Daniels wrote:
> Like Carl, I prefer two sentences.
Done.
http://codereview.appspot.com/3705042/diff/1/Documentation/learning/fundamental.itely#newcode2114
Documentation/learning/fundamental.itely:2114: objects to note heads, if you remove the @code{Note_heads_engraver}
On 2010/12/17 14:21:03, Carl wrote:
> I agree with Graham. You either need to change the comma to a semicolon, or add
> a conjunction like "so". I think it would actually be best to put in a period
> and make it two sentences.
>
> I also think it would be better to reword it as
>
> "If the @code{Note_heads_engraver} is removed"
Done.
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely
File Documentation/notation/repeats.itely (right):
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely#newcode150
Documentation/notation/repeats.itely:150: @rprogram{An extra staff appears}. Do not place bar checks outside
On 2010/12/17 14:05:27, Graham Percival wrote:
> Could this be in a separate @warning? I'd rather not have two totally distinct
> warnings in the same box.
>
> I also think it's a bit wordier than it needs to be -- I really want to keep
> warnings short, otherwise they lose their power. How about
> "Bar checks must be placed @emph{inside} the braces of each alternate block." ?
> ok, that's not quite the same thing (it says that you *must* use bar checks)...
> but please try to think of a reformulation along those lines.
Done.
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely#newcode151
Documentation/notation/repeats.itely:151: of the braces of the grouped notes within an @code{@bs{}alternative} block, otherwise you will not get the expected number of endings. Any bar checks used inside @code{@bs{}alternative} block must be placed in inside the braces of each group of notes.}
On 2010/12/17 21:02:21, Trevor Daniels wrote:
> Why is this all on one line?
>
> I agree with Graham - it's too long. Use his words with "should" instead of
> "must".
Done.
http://codereview.appspot.com/3705042/diff/1/Documentation/notation/repeats.itely#newcode151
Documentation/notation/repeats.itely:151: of the braces of the grouped notes within an @code{@bs{}alternative} block, otherwise you will not get the expected number of endings. Any bar checks used inside @code{@bs{}alternative} block must be placed in inside the braces of each group of notes.}
See my comment below for second patch set to Trevor.
http://codereview.appspot.com/3705042/diff/8001/Documentation/notation/repeats.itely#newcode239
Documentation/notation/repeats.itely:239: @cindex repeats with ties
OK, but that was not clearly stated for me in the original user thread nor implied in the last set of comments of the patch. I assumed this problem was only for bar checks, so it's only 'logically inconsistent' (as per Carl;s comment) because what was asked for was a doc entry about a common problem that Carl ran into with bar checks in alternatives. It may be obvious to some that other items outside of the braces would cause a problem, but not to me evidently. Thanks for the clarification.
Message from unknown
2010-12-19T11:28:43+00:00pkx166hurn:md5:3d4f34fc87181a63bd60562693b53eb2
Message from Carl.D.Sorensen@gmail.com
2010-12-19T14:45:14+00:00Carlurn:md5:b956729bc9d19f23228b6f3261998ffd
I like it, and think it's ready for pushing.
Just for purposes of clarification, let me summarize the original problem, since I didn't do a good job before.
The syntax of alternative is:
\alternative {
ending_one
ending_two
ending_three
}
NOT
\alternative {
{ ending_one }
{ ending_two }
{ ending_three }
}
So when somebody enters
\alternative {
{ c4 c c c} |
{ d4 d d d}
}
they are getting three endings:
{c4 c c c} for the first ending
| for the second ending
{d4 d d d} for the third ending
If this follows \repeat volta 2 { }, the third ending (intended second ending) will never be printed.
So if somebody has the problem above, they're confused in one of two ways, as far as I can tell:
1) They don't understand that a bar check is a music expression
2) They think the braces are part of the basic alternative construct, rather than part of the alternative music expressions
What's the best way to clarify? I'm not sure. But the current patch works, because it's always fine to have each alternative enclosed in braces, even if it's only a single note.
So I'm happy with the current patch.
Thanks,
Carl
Message from graham@percival-music.ca
2010-12-20T18:36:31+00:00Graham Percivalurn:md5:fa54c9f409de0303fa26c74e83ab7faa
On Sun, Dec 19, 2010 at 02:45:14PM +0000, Carl.D.Sorensen@gmail.com wrote:
> So if somebody has the problem above, they're confused in one of two
> ways, as far as I can tell:
>
> 1) They don't understand that a bar check is a music expression
> 2) They think the braces are part of the basic alternative construct,
> rather than part of the alternative music expressions
Interesting, I didn't know that!
> What's the best way to clarify? I'm not sure. But the current patch
> works, because it's always fine to have each alternative enclosed in
> braces, even if it's only a single note.
Sounds fine, then. We've gone with "always add {}, even if
they're not strictly necessary" in the past for our docs.
Cheers,
- Graham
Message from pkx166h@gmail.com
2010-12-21T13:30:38+00:00pkx166hurn:md5:cb7dbfde48a97968d1d878d105b84dc3
Hello,
Is this OK to give someone a patch to push?
James
Message from tdanielsmusic@googlemail.com
2010-12-21T15:21:29+00:00Trevor Danielsurn:md5:870922a5be3d8931e998806d5e16ac63
Looks fine to me, but then you've used the wording I suggested, so it would, wouldn't it :) Best wait for someone else to approve.
Trevor
Message from Carl.D.Sorensen@gmail.com
2010-12-21T15:29:28+00:00Carlurn:md5:21a8df35fbc1819238fc8fafbc99706c
On 2010/12/21 15:21:29, Trevor Daniels wrote:
> Looks fine to me, but then you've used the wording I suggested, so it would,
> wouldn't it :) Best wait for someone else to approve.
>
I believe that Graham and I have both approved it.
Carl
Message from tdanielsmusic@googlemail.com
2010-12-22T11:09:52+00:00Trevor Danielsurn:md5:19e2fc7420aceb4fb8738b317664a6d7
Hi James
I think a further change is needed to position the warning in repeats correctly.
Trevor
http://codereview.appspot.com/3705042/diff/12001/Documentation/notation/repeats.itely
File Documentation/notation/repeats.itely (right):
http://codereview.appspot.com/3705042/diff/12001/Documentation/notation/repeats.itely#newcode237
Documentation/notation/repeats.itely:237: @warning{If there are two or more alternatives, nothing should appear
I think this warning would be better placed immediately after the discussion of \alternative, i.e. before the warning about \relative. (Sorry to have missed this earlier - it wasn't obvious until I recompiled the manual with the patch applied.)
Message from pkx166h@gmail.com
2010-12-22T12:37:41+00:00pkx166hurn:md5:8a1819ee0417b426cbdefd0fdede8412
Done.
However I think that the two 'warning' boxes will look a bit awkward, but I understand why Graham doesn't want them all in the same @warning {}.
Message from unknown
2010-12-22T12:39:17+00:00pkx166hurn:md5:4aef10e3d7226c9ed2192fcba0dcaca1
Message from tdanielsmusic@googlemail.com
2010-12-22T23:43:19+00:00Trevor Danielsurn:md5:98d9465ce870b8e04a3284f025f0b387
On 2010/12/22 12:37:41, pkx166h wrote:
> I think that the two 'warning' boxes will look a bit awkward
Maybe, but the placement makes the warning much more relevant. Let's leave for a couple of days, and if there are no further comments send me a patch.