Add independent control of thickness and offset for underline markup

Add independent control of thickness and offset for underline markup

[Neil Puttock, 2010-05-28 20:29:04]

Hi Kieren,

I don't think we can remove the link between 'line-thickness and underline
offset, since it should scale based on staff-size.  At small staff-sizes,
'line-thickness gets progressively larger, which matches the thicker underline
with a slightly bigger gap.

What you could do instead is leave out the default value for offset and keep the
existing behaviour unless the user sets offset (if it's unset, it will return
#f).

Cheers,
Neil

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

http://codereview.appspot.com/1347041/diff/1/2#newcode280
scm/define-markup-commands.scm:280: #:properties ((thickness 1) (offset 0.5))
I think this default for offset is too large.

http://codereview.appspot.com/1347041/diff/1/2#newcode301
scm/define-markup-commands.scm:301: (y (* -1 offset))
Simpler:

(- offset)

[c_sorensen, 2010-05-28 21:32:26]

On 5/28/10 2:29 PM, "n.puttock@gmail.com" <n.puttock@gmail.com> wrote:

> Hi Kieren,
> 
> I don't think we can remove the link between 'line-thickness and
> underline offset, since it should scale based on staff-size.  At small
> staff-sizes, 'line-thickness gets progressively larger, which matches
> the thicker underline with a slightly bigger gap.

Couldn't we define the offset as a multiple of 'line-thickness, instead of
an absolute value?  That's how fret diagrams are scaled.
> 
> http://codereview.appspot.com/1347041/diff/1/2#newcode301
> scm/define-markup-commands.scm:301: (y (* -1 offset))

(y (* thick (- offset)))


Thanks,

Carl

[kieren_macmillan_sympatico.ca, 2010-05-29 16:10:16]

Hi Neil,

Thanks for the comments!
[Should I be somehow commenting on codereview instead of here?]

> I don't think we can remove the link between 'line-thickness and
> underline offset, since it should scale based on staff-size.  At small
> staff-sizes, 'line-thickness gets progressively larger, which matches
> the thicker underline with a slightly bigger gap.

OK. But I want two things out of this patch:
    1. Control of offset independent of thickness, even if offset is defined as
a "multiple of 'line-thickness" (that I can adjust).
    2. An easy way to globally set the property (i.e., not just a direct
\override of the \underline command).

> What you could do instead is leave out the default value for offset and
> keep the existing behaviour unless the user sets offset (if it's unset,
> it will return #f).

I tried
  #:properties ((thickness 1))
and it failed.

> http://codereview.appspot.com/1347041/diff/1/2#newcode280
> scm/define-markup-commands.scm:280: #:properties ((thickness 1) (offset
> 0.5))
> I think this default for offset is too large.

This is precisely why we need the offset property...  ;)

> http://codereview.appspot.com/1347041/diff/1/2#newcode301
> scm/define-markup-commands.scm:301: (y (* -1 offset))
> Simpler:
> 
> (- offset)

Thanks!

Once these points are addressed, how do I submit a revision?

Cheers,
Kieren.

[kieren_macmillan_sympatico.ca, 2010-05-29 16:11:26]

Hi Carl,

> Couldn't we define the offset as a multiple of 'line-thickness, instead of
> an absolute value?  That's how fret diagrams are scaled.

Sounds good to me!
To be clear, 'line-thickness and the thickness property of \underline are
[potentially] two different independent values, right?

Cheers,
Kieren.

[c_sorensen, 2010-05-29 17:51:11]

On 5/29/10 10:10 AM, "Kieren MacMillan" <kieren_macmillan@sympatico.ca>
wrote:

> Hi Neil,
> 
> Thanks for the comments!
> [Should I be somehow commenting on codereview instead of here?]

Either way works.

> 
>> I don't think we can remove the link between 'line-thickness and
>> underline offset, since it should scale based on staff-size.  At small
>> staff-sizes, 'line-thickness gets progressively larger, which matches
>> the thicker underline with a slightly bigger gap.
> 
> OK. But I want two things out of this patch:
>     1. Control of offset independent of thickness, even if offset is defined
> as a "multiple of 'line-thickness" (that I can adjust).
>     2. An easy way to globally set the property (i.e., not just a direct
> \override of the \underline command).
> 
>> What you could do instead is leave out the default value for offset and
>> keep the existing behaviour unless the user sets offset (if it's unset,
>> it will return #f).
> 
> I tried
>   #:properties ((thickness 1))
> and it failed.
> 
>> http://codereview.appspot.com/1347041/diff/1/2#newcode280
>> scm/define-markup-commands.scm:280: #:properties ((thickness 1) (offset
>> 0.5))
>> I think this default for offset is too large.
> 
> This is precisely why we need the offset property...  ;)
> 
>> http://codereview.appspot.com/1347041/diff/1/2#newcode301
>> scm/define-markup-commands.scm:301: (y (* -1 offset))
>> Simpler:
>> 
>> (- offset)

>> Couldn't we define the offset as a multiple of 'line-thickness, instead of
>> an absolute value?  That's how fret diagrams are scaled.
> 
> Sounds good to me!
> To be clear, 'line-thickness and the thickness property of \underline are
> [potentially] two different independent values, right?

Yes.  I would suggest that both thickness and offset should be multipliers
of 'line-thickness (which shows up in your scheme code as thick.

> Once these points are addressed, how do I submit a revision?
> 
Attach (or inline) a diff patch on an email, and I'll apply and repost to
Rietveld.

Thanks,

Carl

[Neil Puttock, 2010-05-30 20:59:27]

On 2010/05/29 16:10:16, kieren_macmillan_sympatico.ca wrote:

> I tried
>   #:properties ((thickness 1))
> and it failed.

Sorry, I meant remove the default value, but keep `offset' in the properties
list, i.e.,

#:properties ((thickness 1) (offset))

This way it's bound to #f if the user hasn't overridden it.

Carl's suggestion is a more elegant solution though.

Cheers,
Neil

[Carl, 2010-07-06 13:22:06]

On 2010/05/29 17:51:11, c_sorensen_byu.edu wrote:
> On 5/29/10 10:10 AM, "Kieren MacMillan" <mailto:kieren_macmillan@sympatico.ca>
> wrote:
 > To be clear, 'line-thickness and the thickness property of \underline are
> > [potentially] two different independent values, right?
> 
> Yes.  I would suggest that both thickness and offset should be multipliers
> of 'line-thickness (which shows up in your scheme code as thick.
> 
> > Once these points are addressed, how do I submit a revision?
> > 
> Attach (or inline) a diff patch on an email, and I'll apply and repost to
> Rietveld.
> 

I haven't seen a follow-up patch for this issue.  Are you still planning on it? 
Or did I miss it?

Thanks,

Carl

[Carl, 2010-08-13 03:15:48]

Having not seen anything back from Kieren, I went ahead and made the changes
that were suggested.

Is the default OK now?

Thanks,

Carl

[Neil Puttock, 2010-08-13 22:08:50]

On 2010/08/13 03:15:48, Carl wrote:

> Is the default OK now?

Looks fine to me.

Cheers,
Neil