Re: [frogs] Re: tablature: ties and harmonics (issue1669041)

[Marc Hohl <marc@xxxxxxxxxx>]

Carl.D.Sorensen@xxxxxxxxx schrieb:
> Looks pretty good.  I have a few comments about things that seem to be
> unnecessarily specific to harmonics.
>
> Thanks,
>
> Carl
>
>
>
> http://codereview.appspot.com/1669041/diff/26001/27003
> File scm/define-grob-interfaces.scm (right):
>
> http://codereview.appspot.com/1669041/diff/26001/27003#newcode91
> scm/define-grob-interfaces.scm:91: 'harmonic-parentheses-interface
> Calling this "parenthesis-interface" would allow its use for other
> applications of parentheses and would be a good idea, in my opinion.
Ok, done.
> http://codereview.appspot.com/1669041/diff/26001/27004
> File scm/define-grob-properties.scm (right):
>
> http://codereview.appspot.com/1669041/diff/26001/27004#newcode56
> scm/define-grob-properties.scm:56: (angularity ,number? "Angularity of a
> bracket.")
> "angle bracket" instead of "bracket"?
Done.
> http://codereview.appspot.com/1669041/diff/26001/27004#newcode130
> scm/define-grob-properties.scm:130: (bracket-width ,number? "Width of
> the harmonic angle bracket.")
> Why do we need bracket-width?  Why can't we just use the pre-existing
> width property?
Hm, Neil proposed to use a more descriptive name for this property, IIUC.
Done.
> http://codereview.appspot.com/1669041/diff/26001/27004#newcode850
> scm/define-grob-properties.scm:850: (white-padding ,number? "Amount of
> padding surrounding a harmonic
> Why make this specific for harmonics?  Why not just "Amount of white
> space boundary in a whiteout" or something similar?
Done.
> http://codereview.appspot.com/1669041/diff/26001/27007
> File scm/tablature.scm (right):
>
> http://codereview.appspot.com/1669041/diff/26001/27007#newcode155
> scm/tablature.scm:155: (define (draw-harmonic-stencil dir grob)
> Why not use parenthesize-stencil (see scm/stencil.scm)?  This seems like
> duplicated code, and we should probably avoid that if possible.
I just copied the code from scm/output-lib.scm similar to the parenthesizing
routine within scm/tablature.scm. I thought I could call the newly 
introduced
helper routine itself, but this is not possible due to the limitation 
that I can't
(at least easily?) get the current property values of the 
HarmonicParenthesesItem
grob within the tie callback.
parenthesize-stencil (in scm,/stencil.scm) does not take the whiteout 
into account,
and at the moment, the whiteout handling is not done properly throughout
scm/tablature.scm. Sometimes it is hardcoded, sometimes it takes the
'whiteout property into account. I think the brackets and parentheses
should follow the TabNoteHead #'whiteout more consequently in this respect.


I'll shut down my computer for now - today he seems to hate me ;-)
I had to redo my patch sets and did a partial revert of a former patch,
no idea why this happened ...

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


---
----
Join the Frogs!