markup-commands rest-by-number and rest

markup-commands rest-by-number and rest

Introduces two new markup-commands:
rest-by-number and rest
similiar to the existing note-by-number and note.
Two regression-tests for them are added.

[thomasmorley65, 2012-11-18 23:54:37]

Please review

[thomasmorley65, 2012-11-19 00:02:23]

2012/11/19  <thomasmorley65@googlemail.com>:
> Reviewers: ,
>
> Message:
> Please review
>
> Description:
> markup-commands rest-by-number and rest
>
> Introduces two new markup-commands:
> rest-by-number and rest
> similiar to the existing note-by-number and note.
> Two regression-tests for them are added.
>
> Please review this at http://codereview.appspot.com/6850073/
>
> Affected files:
>   A input/regression/markup-rest-styles.ly
>   A input/regression/markup-rest.ly
>   M scm/define-markup-commands.scm
>
>

BTW, four month ago I uploaded my last patch. Obviously I forgot a lot
about Rietveld during that period: I tried more than an hour to upload
the current patch and I've _no_ idea how I managed it finally.

I suggest to add a chapter: Rietveld for dummies to the CG.

Seems I need a 2x4 cluestick to refresh my memory from time to time.


Cheers,
  Harm

[lemzwerg, 2012-11-19 05:11:38]

http://codereview.appspot.com/6850073/diff/1/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/6850073/diff/1/scm/define-markup-commands.scm#n...
scm/define-markup-commands.scm:3341: breve, longa and maxima are valid
input-strings
This should be rather

  @code{breve}, @code{longa} and @code{maxima} ...

Ditto for the next lines, which are missing proper texinfo markup.  And please
make sure that at least the documentation stays within the 80 character line
length limit.

[thomasmorley65, 2012-11-19 21:46:10]

Hi Werner,

thanks for reviewing.

New patch set uploded.

[thomasmorley65, 2012-11-19 21:48:23]

On 2012/11/19 05:11:38, lemzwerg wrote:
> http://codereview.appspot.com/6850073/diff/1/scm/define-markup-commands.scm
> File scm/define-markup-commands.scm (right):
> 
>
http://codereview.appspot.com/6850073/diff/1/scm/define-markup-commands.scm#n...
> scm/define-markup-commands.scm:3341: breve, longa and maxima are valid
> input-strings
> This should be rather
> 
>   @code{breve}, @code{longa} and @code{maxima} ...
> 
> Ditto for the next lines, which are missing proper texinfo markup.  And please
> make sure that at least the documentation stays within the 80 character line
> length limit.

Done.
(At least I hope so)

[benko.pal, 2012-11-21 08:05:09]

http://codereview.appspot.com/6850073/diff/9001/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/6850073/diff/9001/scm/define-markup-commands.sc...
scm/define-markup-commands.scm:3247: "M"
there are no separate glyphs for rests and multi measure rests: the M in glyph
names stands not for MultiMeasure, but for Minus.  that makes for rewriting the
cond below, I'm afraid, but it will be simpler.

[thomasmorley65, 2012-11-25 23:03:43]

On 2012/11/21 08:05:09, benko.pal wrote:
[...]
> there are no separate glyphs for rests and multi measure rests: the M in glyph
> names stands not for MultiMeasure, but for Minus.  

Didn't know that.

> that makes for rewriting the
> cond below, I'm afraid, but it will be simpler.

Well, I want the markup-commands to produce different output depending on the
used style. I can't see an other way, than to write detailed conditions. 
OTOH, I tried to consider your hint and to write them concise.

Thanks for reviewing!

[benko.pal, 2012-11-26 11:37:26]

On 2012/11/25 23:03:43, thomasmorley65 wrote:
> On 2012/11/21 08:05:09, benko.pal wrote:
> [...]
> > there are no separate glyphs for rests and multi measure rests: the M in
glyph
> > names stands not for MultiMeasure, but for Minus.  
> 
> Didn't know that.
> 
> > that makes for rewriting the
> > cond below, I'm afraid, but it will be simpler.
> 
> Well, I want the markup-commands to produce different output depending on the
> used style. I can't see an other way, than to write detailed conditions. 
> OTOH, I tried to consider your hint and to write them concise.

I just meant that instead of checking several times whether dealing with
multi-measure rest or not, you may convert duration log at a single place (with
the caveat of turning a negative sign to 'M' instead of '-').

[dak, 2012-11-26 11:51:03]

On 2012/11/26 11:37:26, benko.pal wrote:
 
> I just meant that instead of checking several times whether dealing with
> multi-measure rest or not, you may convert duration log at a single place
(with
> the caveat of turning a negative sign to 'M' instead of '-').

Probably even without the caveat.  I think that some code fairly close to the
font backend does this conversion.  I remember distinctly going nuts over those
character names at some point of time because I could not figure out why a minus
did not cause problems.

[dak, 2012-11-26 12:55:42]

On 2012/11/26 11:51:03, dak wrote:
> On 2012/11/26 11:37:26, benko.pal wrote:
>  
> > I just meant that instead of checking several times whether dealing with
> > multi-measure rest or not, you may convert duration log at a single place
> (with
> > the caveat of turning a negative sign to 'M' instead of '-').
> 
> Probably even without the caveat.  I think that some code fairly close to the
> font backend does this conversion.  I remember distinctly going nuts over
those
> character names at some point of time because I could not figure out why a
minus
> did not cause problems.

lily/font-metric.cc:

Stencil
Font_metric::find_by_name (string s) const
{
  replace_all (&s, '-', 'M');

So I'd suggest not treating the '-' at all (to make things not more confusing
than they already are) and letting the font backend sort things out.

[benko.pal, 2012-11-26 15:09:58]

> So I'd suggest not treating the '-' at all (to make things not more
> confusing than they already are) and letting the font backend sort
> things out.

I concur.

http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.sc...
scm/define-markup-commands.scm:3251: (or multi-measure-rest (< log 1)))
there are no Petrucci rest glyphs at all; use mensural without any further
condition.  or don't even allow petrucci as style for these markups.

http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.sc...
scm/define-markup-commands.scm:3299: ;; If there is a ledger, move the dots in
X-direction to avoid collision.
is this comment true?  I can't see how the condition below checks for ledger.

http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.sc...
scm/define-markup-commands.scm:3305: (and (= log 1) (equal? style 'petrucci))))
this check looks fishy, as there are no Petrucci-style rest glyphs.

[thomasmorley65, 2012-11-26 21:11:53]

On 2012/11/26 15:09:58, benko.pal wrote:
> > So I'd suggest not treating the '-' at all (to make things not more
> > confusing than they already are) and letting the font backend sort
> > things out.
> 
> I concur.

Ok, I'll try.

> http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.scm
> File scm/define-markup-commands.scm (right):
> 
>
http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.sc...
> scm/define-markup-commands.scm:3251: (or multi-measure-rest (< log 1)))
> there are no Petrucci rest glyphs at all; use mensural without any further
> condition.  or don't even allow petrucci as style for these markups.

Well, writing this conditions I tried to orientate myself on `select-head-glyph´
from output-lib.scm
`select-head-glyph´ returns note-head-glyphs for some styles without defined
note-head-glyphs as well. Mostly if (< log 1)
So I tried to transfer this behaviour to rests.

Did you had a look on the compiled output of the new reg-tests?
This output is what I tried to achieve. 
You'll see that for some styles mensural- or neomensural-glyphes are taken,
others are defaulting.

The main question is: 
If there are no defined glyphs for a specific style, 
should I try to select others (currently tried)
or
should I return default-glyphs (I suspect this would be much easier)?

Opinions?

>
http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.sc...
> scm/define-markup-commands.scm:3299: ;; If there is a ledger, move the dots in
> X-direction to avoid collision.
> is this comment true?  I can't see how the condition below checks for ledger.

You're right, the comment is misleading.
I selected ledgered glyphs for whole and half rests for all styles, apart from
the listed ones. 
Depending on the decision which glyphs are taken (see above), I'll change the
comment and/or the code.

>
http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.sc...
> scm/define-markup-commands.scm:3305: (and (= log 1) (equal? style
'petrucci))))
> this check looks fishy, as there are no Petrucci-style rest glyphs.

Same problem as above:
Which glyph to take, if there's none for the specified style.

[benko.pal, 2012-11-27 20:03:06]

On 2012/11/26 21:11:53, thomasmorley65 wrote:
> On 2012/11/26 15:09:58, benko.pal wrote:
> >
http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.scm
> > File scm/define-markup-commands.scm (right):
> > 
> >
>
http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.sc...
> > scm/define-markup-commands.scm:3251: (or multi-measure-rest (< log 1)))
> > there are no Petrucci rest glyphs at all; use mensural without any further
> > condition.  or don't even allow petrucci as style for these markups.
> 
> Well, writing this conditions I tried to orientate myself on
`select-head-glyph´
> from output-lib.scm
> `select-head-glyph´ returns note-head-glyphs for some styles without defined
> note-head-glyphs as well. Mostly if (< log 1)
> So I tried to transfer this behaviour to rests.

actually if (< log 0).  all this is driven by the actual layout of the Feta font
(see NR A.8): petrucci has its own notehead glyphs if (> log -1), but it has no
rest glyphs at all.

> Did you had a look on the compiled output of the new reg-tests?

no.  could you push to a dev/ branch?

> This output is what I tried to achieve. 
> You'll see that for some styles mensural- or neomensural-glyphs are taken,
> others are defaulting.
> 
> The main question is: 
> If there are no defined glyphs for a specific style, 
> should I try to select others (currently tried)
> or
> should I return default-glyphs (I suspect this would be much easier)?
> 
> Opinions?

I don't exactly know what is style and how is it set.  I thought it's a local
property of your new command and must be set explicitly by the user; in this
case you can either declare that petrucci is an invalid style or make sure that
setting petrucci is read as setting mensural.  in other words, translate
petrucci to mensural once and for all in the beginning.


>
http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.sc...
> > scm/define-markup-commands.scm:3299: ;; If there is a ledger, move the dots
in
> > X-direction to avoid collision.
> > is this comment true?  I can't see how the condition below checks for
ledger.
> 
> You're right, the comment is misleading.
> I selected ledgered glyphs for whole and half rests for all styles, apart from
> the listed ones. 
> Depending on the decision which glyphs are taken (see above), I'll change the
> comment and/or the code.

I don't know whether it's relevant or not, but (in the default style) there's a
ledgered variant for breve rest too (though not for longa).

[thomasmorley65, 2012-11-29 00:41:14]

On 2012/11/27 20:03:06, benko.pal wrote:

> > Did you had a look on the compiled output of the new reg-tests?
> 
> no.  could you push to a dev/ branch?

Sorry. I'm still a newbie in devel tasks.
Especially with the tools needed for it.
Imagine that I tried to upload my new patch this evening. It lasted over 4 hours
before I managed to _upload_ it.
And it was wrong. I had to do it again.
A hundred times (wasn't, but it feels like) I had to start again. Most of the 
mistakes were typos in git, which I couldn't correct. Two times I corrupted my
build-directory, so I had to start from scratch.
Or I produced a [PATCH 1/2] and I have no clue why, or why not the next time I
tried.
etc., etc

I remember trying to checkout a dev/ branch. It resulted in a messed up git.
So no, I don't know how to push a dev/branch and currently I feel not motivated
to learn it. 

But in case this will change, do you have a hint where to look to learn not
alone about dev/branch but to deal with git? Something like git for dummies
would be suitable.

> > The main question is: 
> > If there are no defined glyphs for a specific style, 
> > should I try to select others (currently tried)
> > or
> > should I return default-glyphs (I suspect this would be much easier)?
> > 
> > Opinions?
> 
> I don't exactly know what is style and how is it set.  I thought it's a local
> property of your new command and must be set explicitly by the user; in this
> case you can either declare that petrucci is an invalid style or make sure
that
> setting petrucci is read as setting mensural.  in other words, translate
> petrucci to mensural once and for all in the beginning.

style _is_ a local property, but I tried to make it consistent with the
style-property from \note-by-number, \note and the usage in \override NoteHead
#'style ... and \override Rest #'style ...

petrucci is now translated to mensural.

> I don't know whether it's relevant or not, but (in the default style) there's
a
> ledgered variant for breve rest too (though not for longa).

Yep.
I decided not to use it, _because_ there are no ledgered variants for longa and
maxima.
If wanted, this could be changed quite easily.

[benko.pal, 2012-11-29 20:42:21]

On 2012/11/29 00:41:14, thomasmorley65 wrote:
> On 2012/11/27 20:03:06, benko.pal wrote:
> 
> > > Did you had a look on the compiled output of the new reg-tests?

I did now, it's OK with me.

https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest-st...
File input/regression/markup-rest-styles.ly (right):

https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest-st...
input/regression/markup-rest-styles.ly:25: '(-3 -2 -1 0 1 2 3 4 5 6 7))))))))
please fix indentation, this and the next list are at quite different levels. 
similarly in the other .ly file.

https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest.ly
File input/regression/markup-rest.ly (right):

https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest.ly...
input/regression/markup-rest.ly:27: (if (>= duration 0) dots "")))))
this if looks superfluous

[thomasmorley65, 2012-11-30 01:33:08]

On 2012/11/29 20:42:21, benko.pal wrote:
> On 2012/11/29 00:41:14, thomasmorley65 wrote:
> > On 2012/11/27 20:03:06, benko.pal wrote:
> > 
> > > > Did you had a look on the compiled output of the new reg-tests?
> 
> I did now, 

Thanks for doing this.

> it's OK with me.
> 
>
https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest-st...
> File input/regression/markup-rest-styles.ly (right):
> 
>
https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest-st...
> input/regression/markup-rest-styles.ly:25: '(-3 -2 -1 0 1 2 3 4 5 6 7))))))))
> please fix indentation, this and the next list are at quite different levels. 
> similarly in the other .ly file.

Couldn't find a guideline to do correct indentation here.
I ask on -devel

> https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest.ly
> File input/regression/markup-rest.ly (right):
> 
>
https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest.ly...
> input/regression/markup-rest.ly:27: (if (>= duration 0) dots "")))))
> this if looks superfluous

Yep.
Will delete it.

[dak, 2012-12-02 15:41:01]

thomasmorley65@googlemail.com writes:

> On 2012/11/27 20:03:06, benko.pal wrote:
>
>> > Did you had a look on the compiled output of the new reg-tests?
>
>> no.  could you push to a dev/ branch?
>
> Sorry. I'm still a newbie in devel tasks.

If you have a branch where you checked in your changes, you can push the
current HEAD with

git push origin HEAD:refs/heads/dev/thomas

or similar.  Don't ever use a fourth argument without explicit source
and target separated via colon: git's idea of putting defaults in there
is rather absurd and can cause major headaches.

To delete a branch again, do

git push origin :refs/heads/dev/thomas

namely nothing before the colon.  That's all.

> of the mistakes were typos in git, which I couldn't correct. Two times
> I corrupted my build-directory, so I had to start from scratch.  Or I
> produced a [PATCH 1/2] and I have no clue why, or why not the next
> time I tried.  etc., etc

Huh.  The CG gives no useful information here?

> But in case this will change, do you have a hint where to look to
> learn not alone about dev/branch but to deal with git? Something like
> git for dummies would be suitable.

man git has a number of references at the bottom, both other manual
pages (under SEE ALSO) as well as further links.

[thomasmorley65, 2012-12-03 02:03:37]

Better indentation, minor changes

[thomasmorley65, 2012-12-03 20:22:09]

On 2012/11/30 01:33:08, thomasmorley65 wrote:
> On 2012/11/29 20:42:21, benko.pal wrote:
>
https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest-st...
> > File input/regression/markup-rest-styles.ly (right):
> > 
> >
>
https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest-st...
> > input/regression/markup-rest-styles.ly:25: '(-3 -2 -1 0 1 2 3 4 5 6
7))))))))
> > please fix indentation, this and the next list are at quite different
levels. 
> > similarly in the other .ly file.
> 
> Couldn't find a guideline to do correct indentation here.
> I ask on -devel

Done.
(Hope so)
> 
> >
https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest.ly
> > File input/regression/markup-rest.ly (right):
> > 
> >
>
https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest.ly...
> > input/regression/markup-rest.ly:27: (if (>= duration 0) dots "")))))
> > this if looks superfluous
> 
> Yep.
> Will delete it.

Done.

[thomasmorley65, 2012-12-03 20:30:34]

On 2012/12/02 15:41:01, dak wrote:
> mailto:thomasmorley65@googlemail.com writes:
> 
> > On 2012/11/27 20:03:06, benko.pal wrote:
> >
> >> > Did you had a look on the compiled output of the new reg-tests?
> >
> >> no.  could you push to a dev/ branch?
> >
> > Sorry. I'm still a newbie in devel tasks.
> 
> If you have a branch where you checked in your changes, you can push the
> current HEAD with
> 
> git push origin HEAD:refs/heads/dev/thomas
> 
> or similar.  Don't ever use a fourth argument without explicit source
> and target separated via colon: git's idea of putting defaults in there
> is rather absurd and can cause major headaches.
> 
> To delete a branch again, do
> 
> git push origin :refs/heads/dev/thomas
> 
> namely nothing before the colon.  That's all.

Thanks for the hints.

> 
> > of the mistakes were typos in git, which I couldn't correct. Two times
> > I corrupted my build-directory, so I had to start from scratch.  Or I
> > produced a [PATCH 1/2] and I have no clue why, or why not the next
> > time I tried.  etc., etc
> 
> Huh.  The CG gives no useful information here?

The CG is quite nice to give the information you need, _if you understand them
(the CG is never translated) and if you don't make a mistake while following
them.
But there is no help-chapter if things are going wrong.

> 
> > But in case this will change, do you have a hint where to look to
> > learn not alone about dev/branch but to deal with git? Something like
> > git for dummies would be suitable.
> 
> man git has a number of references at the bottom, both other manual
> pages (under SEE ALSO) as well as further links.

Thanks again.
Both, Pál and Marc pointed me, privately, to a publication explaining git. 
Be sure, I will study it.

[benko.pal, 2012-12-04 19:49:00]

LGTM

https://codereview.appspot.com/6850073/diff/17001/input/regression/markup-res...
File input/regression/markup-rest-styles.ly (right):

https://codereview.appspot.com/6850073/diff/17001/input/regression/markup-res...
input/regression/markup-rest-styles.ly:18: (symbol->string style))
sorry for nitpicking, but please don't use tabs.  if nothing else is to be done,
I'll be happy to do the formatting.

https://codereview.appspot.com/6850073/diff/17001/input/regression/markup-res...
File input/regression/markup-rest.ly (right):

https://codereview.appspot.com/6850073/diff/17001/input/regression/markup-res...
input/regression/markup-rest.ly:30: (number->string (expt 2 duration))
more formatting nitpicking: do we have a line length limit?

[thomasmorley65, 2012-12-05 01:00:11]

Better indentation, next try.

[thomasmorley65, 2012-12-05 01:15:56]

On 2012/12/04 19:49:00, benko.pal wrote:
> LGTM
> 
>
https://codereview.appspot.com/6850073/diff/17001/input/regression/markup-res...
> File input/regression/markup-rest-styles.ly (right):
> 
>
https://codereview.appspot.com/6850073/diff/17001/input/regression/markup-res...
> input/regression/markup-rest-styles.ly:18: (symbol->string style))
> sorry for nitpicking, but please don't use tabs.  

Thought I had eliminated them. 
Done now. 
Also, in scm/define-markup-commands.scm 

> if nothing else is to be done,
> I'll be happy to do the formatting.

Thanks for the offer. But if you do it for me, I'll never learn how to do
correct. 

> 
>
https://codereview.appspot.com/6850073/diff/17001/input/regression/markup-res...
> File input/regression/markup-rest.ly (right):
> 
>
https://codereview.appspot.com/6850073/diff/17001/input/regression/markup-res...
> input/regression/markup-rest.ly:30: (number->string (expt 2 duration))
> more formatting nitpicking: do we have a line length limit?

I feel the guide-lines for indentation are contradictory.
How to do a good, readable indentation with leveled expressions on a limited
range?
Sometimes there mhas to be a reasonable compromise.

Hope I did it better this time.

[benko.pal, 2012-12-05 07:51:17]

LGTM, thanks!