Issue 7029045: Introducing two new markup-commands: draw-dashed-line and draw-dotted-line. (Closed)
|
Created:
11 years, 3 months ago by thomasmorley65 Modified:
11 years, 3 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionIntroducing two new markup-commands: draw-dashed-line and draw-dotted-line.
Adding a reg-test for them.
Patch Set 1 #
Total comments: 10
Patch Set 2 : David Nalesnik's proposals included. #Patch Set 3 : fix of oversight #
MessagesTotal messages: 10
Please review
Sign in to reply to this message.
Excuse my ignorance but is the \xxx-xxx-xxx ok in terms of consistency of name of command and/or is 'draw' redundant?
Sign in to reply to this message.
On 2012/12/30 16:30:45, J_lowe wrote: > Excuse my ignorance but is the \xxx-xxx-xxx ok in terms of consistency of name > of command and/or is 'draw' redundant? \line arranges a text line, \draw-line draws an actual line, so the name choice seems ok. Another possibility would be to use properties to specify dotting/dashing and just use \draw-line (or \draw-circle and so on). After all, lines are not the only things that might be desirable to do with a different line style.
Sign in to reply to this message.
On 2012/12/30 16:42:56, dak wrote:
> On 2012/12/30 16:30:45, J_lowe wrote:
> > Excuse my ignorance but is the \xxx-xxx-xxx ok in terms of consistency of
name
> > of command and/or is 'draw' redundant?
>
> \line arranges a text line, \draw-line draws an actual line, so the name
choice
> seems ok. Another possibility would be to use properties to specify
> dotting/dashing and just use \draw-line (or \draw-circle and so on).
I thought about extending \draw-line with a 'style-property.
But in extreme cases you would have to write:
\markup {
\override #'(style . dashed)
\override #'(on . 0.5)
\override #'(off . 0.2)
\override #'(thickness . 2)
\draw-line #'(4 . 1)
}
This seems a bit overlaoded to me. Therefore I decided to define separate
markup-commands.
Ok, this will reduce the needed \overrides only by one, but I think any
\override less would be nice.
And I'm thinking about writing additional commands for zigzag- and trill-style
lines...
> After all,
> lines are not the only things that might be desirable to do with a different
> line style.
Do you have anything particular in mind?
Sign in to reply to this message.
On 2012/12/31 00:07:12, thomasmorley65 wrote:
>
> I thought about extending \draw-line with a 'style-property.
> But in extreme cases you would have to write:
>
> \markup {
> \override #'(style . dashed)
> \override #'(on . 0.5)
> \override #'(off . 0.2)
> \override #'(thickness . 2)
> \draw-line #'(4 . 1)
> }
>
> This seems a bit overlaoded to me.
I don't consider
\markup {
\override #'(on . 0.5)
\override #'(off . 0.2)
\override #'(thickness . 2)
\draw-dashed-line #'(4 . 1)
}
considerably less overloaded.
> Ok, this will reduce the needed \overrides only by one, but I think
> any \override less would be nice.
Hm.
> And I'm thinking about writing additional commands for zigzag- and
> trill-style lines...
> > After all,
> > lines are not the only things that might be desirable to do with a
> > different line style.
>
> Do you have anything particular in mind?
Boxes, circles, underlines, ... No, nothing particular, but it's not
uncommon.
Sign in to reply to this message.
Harm-- This looks great! Thank you for the 'full-length option. I can't be alone in hating lines ending with incomplete dashes. All I have is a suggestion or two, and some quibbles. Besides what I've pointed out inline, I should mention that I got a number of whitespace errors when I applied your patch. There are a number of spots where you should trim the ends of lines (mostly in the .scm file). https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... scm/define-markup-commands.scm:155: If @code{full-length} is set @code{#t} (default) the dashed-line extends to the set to #t https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... scm/define-markup-commands.scm:156: whole length given by @var{dest}, without white space at begin/end. beginning or end. https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... scm/define-markup-commands.scm:169: (let* (;; Get the line-thickness. I think your variable name is sufficient--no comment needed. https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... scm/define-markup-commands.scm:181: (begin Branches of if expression should be aligned with test (here and elsewhere). https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... scm/define-markup-commands.scm:195: (new-off (/ (- line-length corr (* (+ 1 guess) on)) guess)) Why not use (1+ guess) https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... scm/define-markup-commands.scm:231: #f) Could omit the boolean--value will be unspecified. https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... scm/define-markup-commands.scm:233: ;; If `on´ or `off´ is negative, or both, `on´ and `off´ equals zero a Possibly "...or the sum of `on' and `off' equals [is] zero..." https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... scm/define-markup-commands.scm:244: #f) Here again, you could omit the alternate. https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... scm/define-markup-commands.scm:248: (interval-widen (ordered-cons 0 x) half-thick) You do such a thorough job of commenting everything, but you don't explain why you need `half-thick' here. https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... scm/define-markup-commands.scm:264: Manual settings for @code{off} is possible to get larger or smaller space "are possible"
Sign in to reply to this message.
David Nalesnik's proposals included.
Sign in to reply to this message.
On 2012/12/31 14:59:45, david.nalesnik wrote: > Harm-- > > This looks great! Thank you for the 'full-length option. I can't be alone in > hating lines ending with incomplete dashes. Hi David, thanks for reviewing! > All I have is a suggestion or two, and some quibbles. > > Besides what I've pointed out inline, I should mention that I got a number of > whitespace errors when I applied your patch. There are a number of spots where > you should trim the ends of lines (mostly in the .scm file). Done. (Hope so) > https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm > File scm/define-markup-commands.scm (right): > > https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... > scm/define-markup-commands.scm:155: If @code{full-length} is set @code{#t} > (default) the dashed-line extends to the > set to #t Done > https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... > scm/define-markup-commands.scm:156: whole length given by @var{dest}, without > white space at begin/end. > beginning or end. Done > https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... > scm/define-markup-commands.scm:169: (let* (;; Get the line-thickness. > I think your variable name is sufficient--no comment needed. Ok. Deleted. > https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... > scm/define-markup-commands.scm:181: (begin > Branches of if expression should be aligned with test (here and elsewhere). Done > https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... > scm/define-markup-commands.scm:195: (new-off (/ (- line-length corr (* (+ 1 > guess) on)) guess)) > Why not use (1+ guess) Done > https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... > scm/define-markup-commands.scm:231: #f) > Could omit the boolean--value will be unspecified. Done > https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... > scm/define-markup-commands.scm:233: ;; If `on´ or `off´ is negative, or both, > `on´ and `off´ equals zero a > Possibly "...or the sum of `on' and `off' equals [is] zero..." Changed > https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... > scm/define-markup-commands.scm:244: #f) > Here again, you could omit the alternate. Done > > https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... > scm/define-markup-commands.scm:248: (interval-widen (ordered-cons 0 x) > half-thick) > You do such a thorough job of commenting everything, but you don't explain why > you need `half-thick' here. Comment added. > https://codereview.appspot.com/7029045/diff/1/scm/define-markup-commands.scm#... > scm/define-markup-commands.scm:264: Manual settings for @code{off} is possible > to get larger or smaller space > "are possible" Done.
Sign in to reply to this message.
fix of oversight
Sign in to reply to this message.
author Thomas Morley <thomasmorley65@googlemail.com> Mon, 7 Jan 2013 15:37:33 +0000 (15:37 +0000) committer James Lowe <pkx166h@gmail.com> Mon, 7 Jan 2013 15:39:54 +0000 (15:39 +0000) commitfa9c6713e581994ed2882a0d447f40452caa713f I assumed Thomas didn't have push privileges based on last few commits by other devs. So have pushed this myself. Please close Rietveld Issue. Thank you. James
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
