Issue 659: alternate segno symbol

Issue 659: alternate segno symbol

An alternate segno symbol is included in mf/feta-scripts.mf.
New arguments to the \bar command are introduced for easier
use of the new sign.

[Reinhold, 2010-01-06 14:13:39]

I haven't tried this out, but some things simply jump out..

http://codereview.appspot.com/181144/diff/1/3
File lily/bar-line.cc (right):

http://codereview.appspot.com/181144/diff/1/3#newcode96
lily/bar-line.cc:96: Stencil segno = Font_interface::get_default_font
(me)->find_by_name ("scripts.varsegno");
You need this stencil only in that one if clause below... No need to allocate it
for each and every compound bar line. I would move it directly before the
m.add_at_edge below.

http://codereview.appspot.com/181144/diff/1/3#newcode121
lily/bar-line.cc:121: str = "$";
Are you sure you want $ and not §?

http://codereview.appspot.com/181144/diff/1/3#newcode385
lily/bar-line.cc:385: " @code{|.|}, @code{:}, @code{dashed}, code{§} and
@code{'}.\n"
How easy is it to produce a § on an US keyboard? Here in Europe it's easy, but I
don't think that key is on an English (non-international) keyboard by default.

[marc, 2010-01-06 15:27:18]

reinhold.kainhofer@gmail.com schrieb:
> I haven't tried this out, but some things simply jump out..
>
>
> http://codereview.appspot.com/181144/diff/1/3
> File lily/bar-line.cc (right):
>
> http://codereview.appspot.com/181144/diff/1/3#newcode96
> lily/bar-line.cc:96: Stencil segno = Font_interface::get_default_font
> (me)->find_by_name ("scripts.varsegno");
> You need this stencil only in that one if clause below... No need to
> allocate it for each and every compound bar line. I would move it
> directly before the m.add_at_edge below.
Done.
>
> http://codereview.appspot.com/181144/diff/1/3#newcode121
> lily/bar-line.cc:121: str = "$";
> Are you sure you want $ and not §?
>
No, I wasn't :-) Just a type which remained undetected during the 
compilation
of the regression test file.
Done.

> http://codereview.appspot.com/181144/diff/1/3#newcode385
> lily/bar-line.cc:385: " @code{|.|}, @code{:}, @code{dashed}, code{§} and
> @code{'}.\n"
> How easy is it to produce a § on an US keyboard? Here in Europe it's
> easy, but I don't think that key is on an English (non-international)
> keyboard by default.
After consulting wikipedia for several keyboard layouts, you have 
convinced me.
I changed the § sign to an uppercase S, which should be accessible from any
latin keyboard.
>
> http://codereview.appspot.com/181144
Thanks!

Marc

[Neil Puttock, 2010-01-07 22:55:40]

http://codereview.appspot.com/181144/diff/1002/1003
File input/regression/bar-line-segno.ly (right):

http://codereview.appspot.com/181144/diff/1002/1003#newcode9
input/regression/bar-line-segno.ly:9: \relative \new StaffGroup <<
\relative c'

http://codereview.appspot.com/181144/diff/1002/1004
File lily/bar-line.cc (right):

http://codereview.appspot.com/181144/diff/1002/1004#newcode382
lily/bar-line.cc:382: " @code{|.|}, @code{:}, @code{dashed}, code{S} and
@code{'}.\n"
@code{S}

[marc, 2010-01-08 08:39:25]

n.puttock@gmail.com schrieb:
>
> http://codereview.appspot.com/181144/diff/1002/1003
> File input/regression/bar-line-segno.ly (right):
>
> http://codereview.appspot.com/181144/diff/1002/1003#newcode9
> input/regression/bar-line-segno.ly:9: \relative \new StaffGroup <<
> \relative c'
Ok, done. I copied this just from input/regression/bar-lines-tick.ly
where this error also appears.
>
> http://codereview.appspot.com/181144/diff/1002/1004
> File lily/bar-line.cc (right):
>
> http://codereview.appspot.com/181144/diff/1002/1004#newcode382
> lily/bar-line.cc:382: " @code{|.|}, @code{:}, @code{dashed}, code{S} and
> @code{'}.\n"
> @code{S}
Done.
>
> http://codereview.appspot.com/181144
>

[Neil Puttock, 2010-01-10 20:50:38]

http://codereview.appspot.com/181144/diff/1009/29
File lily/span-bar.cc (right):

http://codereview.appspot.com/181144/diff/1009/29#newcode204
lily/span-bar.cc:204: else if (type == "S")
You also need to pick up "S."/".S" here, otherwise you'll get a nasty surprise
between staves ;)

else if (type.find ("S") != NPOS)

[Reinhold, 2010-01-11 01:05:07]

http://www.codereview.appspot.com/181144/diff/1009/29
File lily/span-bar.cc (right):

http://www.codereview.appspot.com/181144/diff/1009/29#newcode204
lily/span-bar.cc:204: else if (type == "S")
On 2010/01/10 20:50:38, Neil Puttock wrote:
> You also need to pick up "S."/".S" here, otherwise you'll get a nasty surprise
> between staves ;)
> 
> else if (type.find ("S") != NPOS)

AFAICS, Bar_line::compound_barline resets both "S." and ".S" to "S", so this
should be fine. Or am I missing something?

At least the regtest shows the span_bar working correctly for both "S." and ".S"

[Neil Puttock, 2010-01-12 00:10:12]

On 2010/01/11 01:05:07, Reinhold wrote:

> AFAICS, Bar_line::compound_barline resets both "S." and ".S" to "S", so this
> should be fine. Or am I missing something?

If "S." or ".S" is (incorrectly) set in the middle of a line, the glyph
calculation won't be reset, resulting in a segno glyph pasted between staves.

[marc, 2010-01-12 08:36:39]

n.puttock@gmail.com schrieb:
>
> http://codereview.appspot.com/181144/diff/1009/29
> File lily/span-bar.cc (right):
>
> http://codereview.appspot.com/181144/diff/1009/29#newcode204
> lily/span-bar.cc:204: else if (type == "S")
> You also need to pick up "S."/".S" here, otherwise you'll get a nasty
> surprise between staves ;)
>
> else if (type.find ("S") != NPOS)
Ah, I see. Done. Thank you!
>
> http://codereview.appspot.com/181144
>

[Carl, 2010-02-15 20:21:33]

Looks good to me, with the exception of the hardcoded value stuff.

Thanks,

Carl

http://codereview.appspot.com/181144/diff/4008/5018
File lily/bar-line.cc (right):

http://codereview.appspot.com/181144/diff/4008/5018#newcode242
lily/bar-line.cc:242: m.add_at_edge (X_AXIS, RIGHT, thick, 2.5 * staff_space + 2
* thinkern); // Urg, hardcoded value
If you are going to use the hardcoded value (which I think is not a good idea; 
you can call ly_stencil_extent to get the width of the segno stencil), then I
think it would be better to define a local variable for your hardcoded value,
i.e. 
float segno_width = 2.5 * staff_space // Urg, hardcoded value

so that it's clear what the 2.5 is for.

[marc, 2010-02-16 19:34:46]

Carl.D.Sorensen@gmail.com schrieb:
> Looks good to me, with the exception of the hardcoded value stuff.
>
> Thanks,
>
> Carl
>
>
>
> http://codereview.appspot.com/181144/diff/4008/5018
> File lily/bar-line.cc (right):
>
> http://codereview.appspot.com/181144/diff/4008/5018#newcode242
> lily/bar-line.cc:242: m.add_at_edge (X_AXIS, RIGHT, thick, 2.5 *
> staff_space + 2 * thinkern); // Urg, hardcoded value
> If you are going to use the hardcoded value (which I think is not a good
> idea;  you can call ly_stencil_extent to get the width of the segno
> stencil), then I think it would be better to define a local variable for
> your hardcoded value, i.e.
> float segno_width = 2.5 * staff_space // Urg, hardcoded value
>
> so that it's clear what the 2.5 is for.
I used Patrick's proposal and it seems to work fine (see the uploaded 
patch set).

Moreover, I compiled lilypond a dozen times yesterday, and today, make 
failed
because I "forgot" the closing >"< on several lines, but these chars 
definitely
were already missing yesterday.

Marc
>
> http://codereview.appspot.com/181144/show
>

[Neil Puttock, 2010-02-17 21:59:43]

Hi Marc,

LGTM, though I still think this is a lot of effort for a very obscure and
little-used symbol.

Cheers,
Neil

http://codereview.appspot.com/181144/diff/5028/4010
File input/regression/bar-line-segno.ly (right):

http://codereview.appspot.com/181144/diff/5028/4010#newcode2
input/regression/bar-line-segno.ly:2: \header { texidoc = "Segno bar lines can
be used to mark
newline for texidoc

http://codereview.appspot.com/181144/diff/5028/4010#newcode7
input/regression/bar-line-segno.ly:7: \paper {  ragged-right = ##t }
{ ragged-right

http://codereview.appspot.com/181144/diff/5028/4011
File lily/bar-line.cc (right):

http://codereview.appspot.com/181144/diff/5028/4011#newcode97
lily/bar-line.cc:97: Stencil segno;
I'd move this back to a separate if { } block (like Reinhold suggested for your
original patch)

(see below)

http://codereview.appspot.com/181144/diff/5028/4011#newcode210
lily/bar-line.cc:210: else if (str == "S")
(as mentioned above)

else if (str.find ("S") != NPOS || str = "|._.|")
  {
    Stencil segno
    ...

    if (str == "S")
      {
        ...

[marc, 2010-02-18 19:42:06]

n.puttock@gmail.com schrieb:
> Hi Marc,
>
> LGTM, though I still think this is a lot of effort for a very obscure
> and little-used symbol.
Perhaps it is little-used because it was missing in lilypond? ;-)

Or do you think this is just too much and we should forget about it all?
(I don't hope so, because I spent hours on it, but anyway ...)
>
> Cheers,
> Neil
>
>
> http://codereview.appspot.com/181144/diff/5028/4010
> File input/regression/bar-line-segno.ly (right):
>
> http://codereview.appspot.com/181144/diff/5028/4010#newcode2
> input/regression/bar-line-segno.ly:2: \header { texidoc = "Segno bar
> lines can be used to mark
> newline for texidoc
Done.
>
> http://codereview.appspot.com/181144/diff/5028/4010#newcode7
> input/regression/bar-line-segno.ly:7: \paper {  ragged-right = ##t }
> { ragged-right
Done.
>
> http://codereview.appspot.com/181144/diff/5028/4011
> File lily/bar-line.cc (right):
>
> http://codereview.appspot.com/181144/diff/5028/4011#newcode97
> lily/bar-line.cc:97: Stencil segno;
> I'd move this back to a separate if { } block (like Reinhold suggested
> for your original patch)
>
> (see below)
Done.
>
> http://codereview.appspot.com/181144/diff/5028/4011#newcode210
> lily/bar-line.cc:210: else if (str == "S")
> (as mentioned above)
>
> else if (str.find ("S") != NPOS || str = "|._.|")
>   {
>     Stencil segno
>     ...
>
>     if (str == "S")
>       {
>         ...
Thanks for your comments!

Marc

http://codereview.appspot.com/181144/show

[marc, 2010-03-01 09:30:26]

Marc Hohl schrieb:
> n.puttock@gmail.com schrieb:
>> Hi Marc,
>>
>> LGTM, though I still think this is a lot of effort for a very obscure
>> and little-used symbol.
> [...]
Should the new segno sign be included in the documentation?
If yes, where?

Apart from that, are there any further objections to this patch set?

Greetings,

Marc

http://codereview.appspot.com/181144/show

[marc, 2010-03-02 11:29:15]

Graham Percival schrieb:
> On Mon, Mar 1, 2010 at 9:30 AM, Marc Hohl <marc@hohlart.de> wrote:
>   
>> Should the new segno sign be included in the documentation?
>> If yes, where?
>>     
>
> In the feta font (I think that's included/font-table.ly), and in NR
> 1.2.5 rehearsal marks, in the example wi ththe other segno signs.
>   
I didn't touch included/font-table.ly, because the varsegno is part
of the scripts, so it is included anyway, IIUC.

I added some explanations in rhythms.itely, I hope it is correct.

> (if I understand the matter at hand... I'm sick and not thinking
> clearly, so if this seems like totally off-base advice, ignore it)
>
>   
All the best for you!

Marc

http://codereview.appspot.com/181144/show

[Graham Percival, 2010-03-02 12:36:31]

On Tue, Mar 02, 2010 at 12:29:09PM +0100, Marc Hohl wrote:
> Graham Percival schrieb:
>> In the feta font (I think that's included/font-table.ly), and in NR
>> 1.2.5 rehearsal marks, in the example wi ththe other segno signs.
>>   
> I didn't touch included/font-table.ly, because the varsegno is part
> of the scripts, so it is included anyway, IIUC.

Ok.

> I added some explanations in rhythms.itely, I hope it is correct.
>
> http://codereview.appspot.com/181144/show

The changes to rhythm.itely look good; I didn't try to examine the
code changes.

Cheers,
- Graham

[marc, 2010-03-14 10:09:55]

Graham Percival schrieb:
> On Tue, Mar 02, 2010 at 12:29:09PM +0100, Marc Hohl wrote:
>   
>> Graham Percival schrieb:
>>     
>>> In the feta font (I think that's included/font-table.ly), and in NR
>>> 1.2.5 rehearsal marks, in the example wi ththe other segno signs.
>>>   
>>>       
>> I didn't touch included/font-table.ly, because the varsegno is part
>> of the scripts, so it is included anyway, IIUC.
>>     
>
> Ok.
>
>   
>> I added some explanations in rhythms.itely, I hope it is correct.
>>
>> http://codereview.appspot.com/181144/show
>>     
>
> The changes to rhythm.itely look good; I didn't try to examine the
> code changes.
>   
Is the segno patch ready to be applied, or did I miss something?

Marc

[marc, 2010-03-30 07:50:58]

Marc Hohl schrieb:
> [...]
> Is the segno patch ready to be applied, or did I miss something?
I don't want to annoy you, but if there is some more work I should
do to improve the patch, let me know ... on the other hand,
if it is ready now, don't hesitate to push it ;-)
>
> Marc
>
>
>
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/lilypond-devel
>