include lines in breve X-extent (issue 1814)

include lines in breve X-extent (issue 1814)

char box of breve glyphs is widened to include
the lines, not only notehead.  This will allow
Lily to calculate collisions with breves
correctly.  It shouldn't change how breves
are aligned in note columns.

[Bertrand Bordage, 2011-08-25 15:02:33]

Just a quick review.

Bertrand

[Bertrand Bordage, 2011-08-25 15:03:04]

Sorry, I forgot to send the comments.

http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf
File mf/feta-noteheads.mf (right):

http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf#newcode168
mf/feta-noteheads.mf:168: gap# := (0.95 - 0.008 * design_size) * stemthick#;
You should save gap, line 161.

http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf#newcode169
mf/feta-noteheads.mf:169: define_pixels (gap);
This should be moved after set_char_box.

http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf#newcode213
mf/feta-noteheads.mf:213: draw_gridline (z1 - (i * (gap + stemthick), 0),
Hmmm... Don't you think (i * (gap + stemthick), 0) has to be written one time
instead of four ?

[pkx166h, 2011-08-27 08:10:31]

passes make and reg tests

[Ian Hulin (gmail), 2011-08-27 23:22:30]

LGTM
Patch applied and it fixes documents originally showing collisions between
double-lined breves, bar-lines and accidentals.

Cheers,
Ian

[Ian Hulin (gmail), 2011-08-30 10:29:42]

Janek,
Bertrand posted some review comments here.  

I think it would be polite in the case of a newer contributor like Bertrand to
post some responses one way or another (either "don't worry about it, because. .
." or "nice catch, I'll upload an updated patch set".)

Cheers,
Ian

[janek, 2011-09-06 08:54:40]

Ian,

On 2011/08/30 10:29:42, Ian Hulin (gmail) wrote:
> Janek,
> Bertrand posted some review comments here.  
> 
> I think it would be polite in the case of
> a newer contributor like Bertrand to
> post some responses one way or another
> (either "don't worry about it, because. .
> ." or "nice catch, I'll upload an updated patch set".)

You are absolutely right!  I had limited access to my e-mail and missed this. 
Sorry, Bertrand!

I have new patch set ready, but i have trouble logging as another user in git-cl
(this issue was created by my previous e-mail account).  Can you give me some
clue?

http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf
File mf/feta-noteheads.mf (right):

http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf#newcode168
mf/feta-noteheads.mf:168: gap# := (0.95 - 0.008 * design_size) * stemthick#;
On 2011/08/25 15:03:04, Bertrand Bordage wrote:
> You should save gap, line 161.

Done.

http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf#newcode169
mf/feta-noteheads.mf:169: define_pixels (gap);
On 2011/08/25 15:03:04, Bertrand Bordage wrote:
> This should be moved after set_char_box.

Done.

http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf#newcode213
mf/feta-noteheads.mf:213: draw_gridline (z1 - (i * (gap + stemthick), 0),
On 2011/08/25 15:03:04, Bertrand Bordage wrote:
> Hmmm... Don't you think (i * (gap + stemthick), 0) has to be written one time
> instead of four ?

I'm not sure what you mean.  Are you saying that i should assign (i * (gap +
stemthick), 0) to a variable in the for loop?  I.e. sth like

	for i := 0 step 1 until linecount - 1:
		foobar := (i * (gap + stemthick), 0);
		draw_gridline (z1 - foobar,
		               z2 - foobar,
		               stemthick);
		draw_gridline (z3 + foobar,
		               z4 + foobar,
		               stemthick);
	endfor;
?

[Graham Percival, 2011-09-06 10:28:08]

On Tue, Sep 06, 2011 at 08:54:40AM +0000, janek.lilypond@gmail.com wrote:
> I have new patch set ready, but i have trouble logging as another user
> in git-cl (this issue was created by my previous e-mail account).  Can
> you give me some clue?

Just make a new issue.  Follow the instructions in the CG to
"reset git-cl".

Cheers,
- Graham

[Bertrand Bordage, 2011-09-06 16:26:40]

On 2011/09/06 08:54:40, janek wrote:
> I'm not sure what you mean.  Are you saying that i should assign (i * (gap +
> stemthick), 0) to a variable in the for loop?  I.e. sth like
> 
> 	for i := 0 step 1 until linecount - 1:
> 		foobar := (i * (gap + stemthick), 0);
> 		draw_gridline (z1 - foobar,
> 		               z2 - foobar,
> 		               stemthick);
> 		draw_gridline (z3 + foobar,
> 		               z4 + foobar,
> 		               stemthick);
> 	endfor;
> ?

Yes, that would be cleaner and easier to understand.

[Janek Warchol, 2011-09-06 17:15:52]

On 2011/09/06 16:26:40, Bertrand Bordage wrote:
> On 2011/09/06 08:54:40, janek wrote:
> > I'm not sure what you mean.  Are you saying that i should assign (i * (gap +
> > stemthick), 0) to a variable in the for loop?  I.e. sth like
> > 
> > 	for i := 0 step 1 until linecount - 1:
> > 		foobar := (i * (gap + stemthick), 0);
> > 		draw_gridline (z1 - foobar,
> > 		               z2 - foobar,
> > 		               stemthick);
> > 		draw_gridline (z3 + foobar,
> > 		               z4 + foobar,
> > 		               stemthick);
> > 	endfor;
> > ?
> 
> Yes, that would be cleaner and easier to understand.

Done (http://codereview.appspot.com/4986042/)

[Keith, 2012-05-09 06:03:39]

http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf
File mf/feta-noteheads.mf (right):

http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf#newcode183
mf/feta-noteheads.mf:183: set_char_box (stemthick# * linecount + gap# *
(linecount - 1),
Jan, the breves seem to be the only note-heads that have an extent to the left
of the reference point.  Presumably this is to make them line up properly with
whole-notes in note-columns.  
It complicates chord-collisions a bit, so take a look at the issue 1713 patch if
you can.

[janek, 2012-05-09 15:01:43]

On Wed, May 9, 2012 at 8:03 AM,  <k-ohara5a5a@oco.net> wrote:
>
> http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf
> File mf/feta-noteheads.mf (right):
>
> http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf#newcode183
> mf/feta-noteheads.mf:183: set_char_box (stemthick# * linecount + gap# *
> (linecount - 1),
> Jan, the breves seem to be the only note-heads that have an extent to
> the left of the reference point.  Presumably this is to make them line
> up properly with whole-notes in note-columns.
> It complicates chord-collisions a bit, so take a look at the issue 1713
> patch if you can.

I'll try - hopefully when i come back home today evening.

cheers,
Janek

[janek, 2012-05-09 21:52:12]

On Wed, May 9, 2012 at 5:01 PM, Janek Warchoł <janek.lilypond@gmail.com> wrote:
> On Wed, May 9, 2012 at 8:03 AM,  <k-ohara5a5a@oco.net> wrote:
>>
>> http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf
>> File mf/feta-noteheads.mf (right):
>>
>> http://codereview.appspot.com/4931043/diff/1/mf/feta-noteheads.mf#newcode183
>> mf/feta-noteheads.mf:183: set_char_box (stemthick# * linecount + gap# *
>> (linecount - 1),
>> Jan, the breves seem to be the only note-heads that have an extent to
>> the left of the reference point.  Presumably this is to make them line
>> up properly with whole-notes in note-columns.
>> It complicates chord-collisions a bit, so take a look at the issue 1713
>> patch if you can.
>
> I'll try - hopefully when i come back home today evening.

I've compiled it and checked the output of a test file; i see the
problem.  Unfortunately i'm too tired to continue testing today :(

thanks,
Janek