Re: Improve markup->string (issue 347000043 by thomasmorle...@gmail.com)
Hi Harm, I took a quick look and it LGTM at a quick read. I have a couple nit-level suggestions. -Paul https://codereview.appspot.com/34743/diff/40001/scm/markup.scm File scm/markup.scm (right): https://codereview.appspot.com/34743/diff/40001/scm/markup.scm#newcode141 scm/markup.scm:141: ;; The string is split at line-breaks, emty strings removed and finally typo: empty https://codereview.appspot.com/34743/diff/40001/scm/markup.scm#newcode148 scm/markup.scm:148: simple-markup))) Indent 'list' expression so it is clearer that it is the second argument to 'member' and not another 'and' expression. https://codereview.appspot.com/34743/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by chazwi...@gmail.com)
Hi Charles, Today I built and ran 'make check' with your patch applied to current master. I was able to get it to pass 'make check' by making the following two changes. 1. In `chord-entry.scm` line 267, remove `(write-me "base3: " bass)`. 2. In that same file, line 100, remove the parens to change `(chord-semantics)` to just `chord-semantics`. The first change fixed this error (but note the type check warning): input/regression/chord-names-languages.ly' ~~~ Parsing... Renaming input to: `/home/james/lilypond-git/input/regression/chord-names-languages.ly' warning: type check for `bass' failed; value `(#t . #t)' must be of type `boolean' /home/james/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:267:59: In procedure memoization in expression (if ba ss (write-me "base3: " bass) ...): /home/james/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:267:59: In file "/home/james/lilypond-git/build/out/s hare/lilypond/current/scm/chord-entry.scm", line 266: Missing or extra expression in (if bass (write-me "base3: " bass) (list (make -note-ev bass (quote bass) #t)) (quote ())). ~~~ And the second change fixed this error: input/regression/chord-name-entry.ly ~~~ $ /home/dev/lilypond-git/build/out/bin/lilypond input/regression/chord-name-entry.ly GNU LilyPond 2.21.0 Processing `input/regression/chord-name-entry.ly' Parsing.../home/dev/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:100:35: In expression (chord-semantics): /home/dev/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:100:35: Wrong type to apply: ((modifier . #f) (root . #) (extension . 7) (additions) (removals) (bass . #f)) ~~~ So if you have a chance to upload a new patch set with those two changes, that should get things moving forward with the code review process. Cheers, -Paul https://codereview.appspot.com/337870043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by chazwi...@gmail.com)
Hi Charles, I was thinking about working on MusicXML export for chord symbols, and wondered about the status of your GSOC project. (It would, of course, make exporting chord symbols much simpler and more robust.) I was really glad to find your code up on Reitveld, and was curious, so I gave it a review. I haven't had a chance to try building with your patch set, but let me offer my congratulations on what you've done here! Quite a nice bit of work. I've left a number of comments, some nitpicky, but hopefully all helpful. Let me know if you have questions or if I can clarify anything. If I find time I might try building and looking into the test failures. Would be great to get this into LilyPond. Cheers, -Paul https://codereview.appspot.com/337870043/diff/40001/Documentation/notation/chords.itely File Documentation/notation/chords.itely (right): https://codereview.appspot.com/337870043/diff/40001/Documentation/notation/chords.itely#newcode679 Documentation/notation/chords.itely:679: represent the structure of the chord. When entered in node mode, typo: "note mode" https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-name-exceptions.ly File input/regression/chord-name-exceptions.ly (right): https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-name-exceptions.ly#newcode4 input/regression/chord-name-exceptions.ly:4: texidoc = "The property @code{chordNameExceptions} can used 'can be used' (This was carried over, but might as well fix while we're here.) https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-name-exceptions.ly#newcode29 input/regression/chord-name-exceptions.ly:29: chExceptions = #(append (chordmode->exception-entry chordVar markupVar) chExceptions) Hmm, chExceptions is used in its own definition here? Does that work? This is not making sense to me. https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-basic.ly File input/regression/chord-semantics-basic.ly (right): https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-basic.ly#newcode10 input/regression/chord-semantics-basic.ly:10: Whitespace on extra unneeded line. https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-bass.ly File input/regression/chord-semantics-bass.ly (right): https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-bass.ly#newcode10 input/regression/chord-semantics-bass.ly:10: Whitespace on unneeded empty line. https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-lowercase-root.ly File input/regression/chord-semantics-lowercase-root.ly (right): https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-lowercase-root.ly#newcode14 input/regression/chord-semantics-lowercase-root.ly:14: Whitespace on unnecessary blank line. https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-name-exceptions.ly File input/regression/chord-semantics-name-exceptions.ly (right): https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-name-exceptions.ly#newcode5 input/regression/chord-semantics-name-exceptions.ly:5: texidoc = "The property @code{chordSemanticsNameExceptions} can used can be used https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-power-chord.ly File input/regression/chord-semantics-power-chord.ly (right): https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-power-chord.ly#newcode12 input/regression/chord-semantics-power-chord.ly:12: Whitespace on unneeded blank line. https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm File scm/chord-entry.scm (right): https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode23 scm/chord-entry.scm:23: Unnecessary new line. https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode75 scm/chord-entry.scm:75: chord-semantics)) It's hard to read this code because of the way it's formatted. Would be better with more line-breaks to keep the lines from being too long and the indentation from going so far to the right and wrapping around to the next line (in narrow views like this code review one). Similar comment for other places in this file like this. https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode238 scm/chord-entry.scm:238: 'chord-semantics chord-semantics)) Hmm, so there's a single 'chord-semantics property under a 'ChordSemanticsEvent ? Seems like we could avoid that extra step of nesting and just have the subproperties of 'chord-semantics under 'ChordSemanticsEvent ? And that would be more like NoteEvent and other events. Or maybe I'm missing something? https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode243 scm/chord-entry.scm:243: (ly:pitch-octave (entry-pitch
Re: Doc: document grob metadata in SVG output in Notation Reference (issue 357720044 by paulwmor...@gmail.com)
On 2018/06/22 07:18:20, lilypond-pkx wrote: https://codereview.appspot.com/357720044/diff/1/Documentation/notation/input.itely File Documentation/notation/input.itely (right): https://codereview.appspot.com/357720044/diff/1/Documentation/notation/input.itely#newcode2780 Documentation/notation/input.itely:2780: @node SVG Output I think you will need to add this 'node' to the @menu section after line #2665. Thanks James. I took another look and since I'm adding an @unnumberedsubsubsec underneath the '@subsection Alternative output formats' I needed to add a new menu for that subsection and add this new node to it. That's what I've done in patch set 2. -Paul https://codereview.appspot.com/357720044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Doc: document grob metadata in SVG output in Notation Reference (issue 357720044 by paulwmor...@gmail.com)
Reviewers: , Message: Please review, thanks, -Paul Description: Doc: document grob metadata in SVG output in Notation Reference I.e. using the 'output-attributes' grob property to add id, class, and other attributes to the SVG output for a given grob. Please review this at https://codereview.appspot.com/357720044/ Affected files (+35, -1 lines): M Documentation/notation/input.itely Index: Documentation/notation/input.itely diff --git a/Documentation/notation/input.itely b/Documentation/notation/input.itely index f31d23086fda0aa31dbe72b32cbfe88a5f302a63..247cb0dba592228aec0ff5d99e3946cce122a2d8 100644 --- a/Documentation/notation/input.itely +++ b/Documentation/notation/input.itely @@ -2774,9 +2774,43 @@ voices and staves, saving even more time. The default output formats for the printed score are Portable Document Format (PDF) and PostScript (PS). Portable Network Graphics (PNG), Scalable Vector Graphics (SVG) and Encapsulated -PostScript (EPS) output is available through the command line option, +PostScript (EPS) output is available through command line options, see @rprogram{Basic command line options for LilyPond}. +@node SVG Output +@unnumberedsubsubsec SVG Output + +SVG output can optionally contain metadata for graphical objects (grobs) like +note heads, rests, etc. This metadata can be standard SVG attributes like +@code{id} and @code{class}, or non-standard custom attributes. Specify the +attributes and their values by overriding a grob's @code{output-attributes} +property with a Scheme association list (alist). The values can be numbers, +strings, or symbols. For example: + +@example +@{ + \once \override NoteHead.output-attributes = + #'((id . 123) + (class . "this that") + (data-whatever . something)) + c +@} +@end example + +@noindent +The input above will produce the following @code{} (group) tag in the SVG +file: + +@example + + ...NoteHead grob SVG elements... + +@end example + +@noindent +The @code{} tag contains all of the SVG elements for a given grob. (Some +grobs generate multiple SVG elements.) In SVG syntax the @code{data-} prefix +is used for non-standard custom metadata attributes. @node Replacing the notation font @subsection Replacing the notation font ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc: CG: Add guideline for use of 'CSS' prefix in commit messages (issue 325970043 by paulwmor...@gmail.com)
Round two. -Paul https://codereview.appspot.com/325970043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc: CG: Add guideline for use of 'CSS' prefix in commit messages (issue 325970043 by paulwmor...@gmail.com)
On second thought... https://codereview.appspot.com/325970043/diff/1/Documentation/contributor/source-code.itexi File Documentation/contributor/source-code.itexi (right): https://codereview.appspot.com/325970043/diff/1/Documentation/contributor/source-code.itexi#newcode1257 Documentation/contributor/source-code.itexi:1257: affect the website, the documentation/manuals, or both. I'm now thinking it would be best to leave out the both case like so: CSS: Commits that change CSS files should use @qq{Web:@tie{}CSS:@tie{}} or @qq{Doc:@tie{}CSS:@tie{}} depending on whether they affect the website or the documentation/manuals. ...since a 'both' commit should probably be split into two commits. (Alternatively, if we did want to specify a both case, it should probably include all three prefixes like "Doc/Web: CSS: " or similar.) https://codereview.appspot.com/325970043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Doc: CG: Add guideline for use of 'CSS' prefix in commit messages (issue 325970043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review. Working on css changes for docs, I realized it wasn't clear to me, from reading the CG, whether the commit prefix should be 'Doc:' or 'Web:' or something else. So this clarifies things. -Paul Description: Doc: CG: Add guideline for use of 'CSS' prefix in commit messages Please review this at https://codereview.appspot.com/325970043/ Affected files (+5, -0 lines): M Documentation/contributor/source-code.itexi Index: Documentation/contributor/source-code.itexi diff --git a/Documentation/contributor/source-code.itexi b/Documentation/contributor/source-code.itexi index bfebb9a0c8cf0850a58b62d88d1528af5f0b9eaf..74071217f8c5bcc06dd869c75fb509c35680fd75 100644 --- a/Documentation/contributor/source-code.itexi +++ b/Documentation/contributor/source-code.itexi @@ -1252,6 +1252,11 @@ website should use @qq{Web:@tie{}} for English, and @qq{Web-@var{**}:@tie{}} for other languages. @item +CSS: Commits that change CSS files should use @qq{Doc:@tie{}CSS:@tie{}}, +@qq{Web:@tie{}CSS:@tie{}}, or @qq{CSS:@tie{}} depending on whether they +affect the website, the documentation/manuals, or both. + +@item Changes to a single file are often prefixed with the name of the file involved. @end itemize ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
lilypond-manuals.css: Add a maximum width for manuals sidebar (issue 328740043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review, a few follow-ups to the previous manuals css patch. The following demo site reflects the changes in this patch (remove spaces after the domain). http://clairnote.org /lilypond-web-demo/Documentation/ (I'll upload a screenshot showing the former brown color on the issue tracker.) Thanks, -Paul Description: lilypond-manuals.css: Add a maximum width for manuals sidebar When the browser window is 1280px wide or more, the sidebar will be 346px wide, rather than a percentage of the window width. Also contains these two commits: lilypond-manuals.css: A better brown for the usage manual I think this shade goes better with the other colors, giving us a more coherent color scheme across manuals. lilypond-manuals.css: Add space between top-levels in sidebar (This one was originally in my last patch for the manuals css, but I found a better way to do it, so removed it from the previous patch and including it here.) Please review this at https://codereview.appspot.com/328740043/ Affected files (+19, -1 lines): M Documentation/css/lilypond-manuals.css Index: Documentation/css/lilypond-manuals.css diff --git a/Documentation/css/lilypond-manuals.css b/Documentation/css/lilypond-manuals.css index d444792955d54edb875b65b5f6b153716863174e..f7b88a1ca27e495bb5d9f640f8109baa15b1d73b 100644 --- a/Documentation/css/lilypond-manuals.css +++ b/Documentation/css/lilypond-manuals.css @@ -257,7 +257,7 @@ div#tocframe { body.learning#tocframe { background-color: #407f40; } body.notation#tocframe { background-color: #40657f; } -body.usage #tocframe { background-color: #7d765a; } +body.usage #tocframe { background-color: #81613e; } body.extending #tocframe { background-color: #7f4040; } body.internals #tocframe { background-color: #6a407f; } body.contributor #tocframe { background-color: #33; } @@ -323,6 +323,10 @@ div#tocframe h4 { font-size: 1em; } +#tocframe .contents > ul.toc > li { + margin-top: 0.5em; +} + #tocframe ul.toc li li { padding-left: 1em; } @@ -495,3 +499,17 @@ div#search p, div#search form { border-radius: 5px; margin: 0.5em 0.5em 2em 3em; } + +/***/ +/* RESPONSIVE DESIGN */ +/***/ + +@media (min-width: 1280px) { + div#main { +left: 346px + } + div#tocframe { +width: 346px; +right: 0; + } +} ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add on-page-greater-than, -less-than (on-the-fly) (issue 74540044)
On 2017/06/09 13:36:53, dak wrote: Ok, I've taken another look at something that should help with this amount of fine-grained definitions. LGTM. Although I'm still new to understanding scheme macros, I think I understand the gist of what's going on. Do you think that the following macro markup-when would be fine-grained enough to forego these kind of definition? You mean so the user would use this markup-when macro to define their own 'on-page-greater-than' and/or 'on-page-less-than' (like you've shown), rather than adding them to LilyPond? If so, that seems fine to me. They seem like fairly rare use cases. Would make a good snippet for the LSR or docs, so it can be found and used when needed. I demonstrate it for implementing on-page-greater-than but of course one may use it directly. Directly like this I assume? (Which works.) \paper { #(set-paper-size "a7landscape") oddFooterMarkup = \markup \on-the-fly #(markup-when ((page:page-number -1)) (> page:page-number 3)) "This is long!" } Its first argument is of the same style as the #:properties keyword arg for define-markup-command . That's a nice consistency. I tried it out and the following works too: #(define (onpage proc nmbr) (markup-when ((page:page-number -1)) (proc page:page-number nmbr))) \paper { #(set-paper-size "a7landscape") oddFooterMarkup = \markup \on-the-fly #(onpage > 3) "This is long!" } -Paul https://codereview.appspot.com/74540044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-manuals.css: edit color scheme and some spacing (issue 322070043 by paulwmor...@gmail.com)
On 2017/06/10 17:58:43, pwm wrote: Footers - simplify color scheme, no need to draw attention to footers with color - language text size matches rest of footer (I'll provide a screenshot for language section which isn't in the demo site) Languages / footer screenshot posted to the sourceforge issue: https://sourceforge.net/p/testlilyissues/issues/5144/ -Paul https://codereview.appspot.com/322070043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
lilypond-manuals.css: edit color scheme and some spacing (issue 322070043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review these css edits for the manuals. Here is a demo site for easier review (remove spaces after the domain to reassemble the url): http://clairnote.org /lilypond-web-demo/Documentation/web/manuals.html The biggest difference is the color scheme. There are a few minor spacing and border edits as well. I realize this is more changes in one review than is preferred, but I wanted to expedite things given the release plans for 2.20. Things to look at: Table of contents sidebars - color scheme, colors are same as the previous 2.19 footers, except for usage (now brown) and the CG is a dark grey rather than full black. - a little vertical space between top level sections Code blocks - more subtle border color to avoid distracting from their content - see Notation 1.1.1, for example "Note" boxes - color scheme, borders, see Notation 1.1.1 under accidentals "Advanced" boxes - color scheme, borders, see CG 1.2 Main/top page for a manual - simpler color scheme, some vertical spacing around the title Grey navigation bars/rows/strips - color scheme is more subtle and spacing Footers - simplify color scheme, no need to draw attention to footers with color - language text size matches rest of footer (I'll provide a screenshot for language section which isn't in the demo site) I think that covers it. -Paul Description: lilypond-manuals.css: edit color scheme and some spacing This patch simplifies and (IMHO) improves the color scheme and spacing of the manuals on the web. I will include a link to screenshots and a demo site, with more details on what's different. Please review this at https://codereview.appspot.com/322070043/ Affected files (+56, -98 lines): M Documentation/css/lilypond-manuals.css ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
abc2ly: Support R (rhythm / meter) and Z (transcription) fields (issue 324890043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review, -Paul Description: abc2ly: Support R (rhythm / meter) and Z (transcription) fields Please review this at https://codereview.appspot.com/324890043/ Affected files (+5, -1 lines): M scripts/abc2ly.py Index: scripts/abc2ly.py diff --git a/scripts/abc2ly.py b/scripts/abc2ly.py index 752f97f3be6a27bc785a97e6411b6c2d3b72e739..e6e0d12407e4ed53006ee529da85f55f7ac7dbab 100644 --- a/scripts/abc2ly.py +++ b/scripts/abc2ly.py @@ -761,8 +761,12 @@ def try_parse_header_line (ln, state): lyrics_append(a) if g == 'w':# vocals slyrics_append (a) -if g == 'Q':#tempo +if g == 'Q':# tempo try_parse_q (a) +if g == 'R':# Rhythm (e.g. jig, reel, hornpipe) +header['meter'] = a +if g == 'Z':# Transcription (e.g. Steve Mansfield 1/2/2000) +header['transcription'] = a return '' return ln ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Web: nest GSOC project ideas under subsection/h3 (issue 314530043 by paulwmor...@gmail.com)
On 2017/03/15 02:44:17, pwm wrote: Only change appearance of GSOC project ideas I've rebased this now that issue 5085 landed (which changed some text on the GSOC page). Also, I looked at the other pages that were affected by the CSS changes in patch set 1, and did not like the results. So patch set 2 only changes the appearance of the GSOC project ideas, leaving the rest of the site unchanged. -Paul https://codereview.appspot.com/314530043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Web-GSoC: Update MusicXML project description (issue 314620043 by g...@ursliska.de)
LGTM https://codereview.appspot.com/314620043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: blah (issue 320290043 by pkx1...@gmail.com)
On 2017/03/05 13:44:00, pkx166h wrote: more fixes to Hungarian to make this bloody thing compile. LGTM. Thank you James for working this out and making the necessary changes. -Paul https://codereview.appspot.com/320290043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Web: Move older news to the attic page (issue 318630043 by paulwmor...@gmail.com)
On 2017/03/04 20:52:49, pkx166h wrote: I'll have a go at fixing this now. Thanks, I haven't had a chance to work on this yet. Sorry I didn't try a 'make doc' before uploading for review. Since 'make website' succeeded I thought it was good to go... but that's not the case. One simple solution would be: Instead of renaming those two files (which breaks the translations), effectively create copies of them and rename the copies. Then later after the translations have been updated to use the new files, delete the old copies with the old names. (Of course building on patch set 1, one would copy the new versions and give them the old names...) -Paul https://codereview.appspot.com/318630043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Web: Move older news to the attic page (issue 318630043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review. I realized after changing the file names that it might not have been the best approach. I'm not sure how it will affect translations of the site. Not hard to change them back in a revised patch, but in the long run I think these newer names are clearer. Two screenshots attached to the issue tracker: https://sourceforge.net/p/testlilyissues/issues/5078/ -Paul Description: Web: Move older news to the attic page Please review this at https://codereview.appspot.com/318630043/ Affected files (+24, -6549 lines): M Documentation/common-macros.itexi M Documentation/contributor/release-work.itexi M Documentation/web/community.itexi D Documentation/web/news.itexi D Documentation/web/news-front.itexi A + Documentation/web/news-new.itexi A + Documentation/web/news-old.itexi ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: SCM/bool confusion in One_page_breaking::solve (issue 314490043 by d...@gnu.org)
On 2017/02/18 20:20:27, dak wrote: Actually I didn't. I just put the finishing touches on a task mainly done by Valentin that made it possible to enable a debugging option in Guile, and the compiler then dug this one up in consequence as well as a few others. Ah, cool, nice to have these checks. I gave it a try with the new change to the code and it still works as intended. I think the reason this did not show up as a problem before is that this part of the code is a defensive measure against a rare edge case where the last musical system (or top-level-markup) in the data structure does not produce the lowest visual point of the 'music' on the page (excluding footers, etc.). I looked back at the test file I was using locally while working on this, and found an example that seems to cover this edge case. However, it appears to work fine with or without the newest change to the code... So it's possible that this code is more defensive than it needs to be, but it's also possible that removing the defensive measure might lead to bugs in other edge cases, and it doesn't hurt to have it in there. So I think it's better to leave it as is and err on the defensive side. I can upload a regtest for that edge case in another patch. https://codereview.appspot.com/314490043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
SCM/bool confusion in One_page_breaking::solve (issue 314490043 by d...@gnu.org)
https://codereview.appspot.com/314490043/diff/1/lily/one-page-breaking.cc File lily/one-page-breaking.cc (right): https://codereview.appspot.com/314490043/diff/1/lily/one-page-breaking.cc#newcode97 lily/one-page-breaking.cc:97: if (scm_is_true (scm_gr_p (this_pos, lowest_line_pos))) Thanks for catching this. I'll try to take a closer look in the next day or so. https://codereview.appspot.com/314490043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Web: nest GSOC project ideas under subsection/h3 (issue 314530043 by paulwmor...@gmail.com)
Reviewers: , Message: I'll upload screenshots to the issue tracker. Description: Web: nest GSOC project ideas under subsection/h3 Please review this at https://codereview.appspot.com/314530043/ Affected files (+77, -72 lines): M Documentation/css/lilypond-website.css M Documentation/web/community.itexi ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add lilypond version predicates/operators (issue 317270043 by g...@ursliska.de)
https://codereview.appspot.com/317270043/diff/40001/scm/lily-library.scm File scm/lily-library.scm (right): https://codereview.appspot.com/317270043/diff/40001/scm/lily-library.scm#newcode895 scm/lily-library.scm:895: "Lexicographically compare to lists @var{a} and @var{b} using I like David's suggestion for a more general function name, something like "lexicographic-list-compare?", "list-compare?", "lexicographic-compare?", or someting similar? Typo: compare to lists -> compare two lists https://codereview.appspot.com/317270043/diff/40001/scm/lily-library.scm#newcode911 scm/lily-library.scm:911: sequence or a list of numbers with up to three elements." To me it would be simpler and clearer to only accept a list for the version. That also makes it easy to use a variable: (ly:version? > '(2 18 2)) (ly:version? > my-version) I can't think of advantages to also accepting the other form (aside from saving a few keystrokes). Am I missing something? (ly:version? > 2 18 2) https://codereview.appspot.com/317270043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Web: examples page: move more common use cases higher in the list (issue 318560043 by paulwmor...@gmail.com)
Reviewers: , Message: Finally got around to this. -Paul Description: Web: examples page: move more common use cases higher in the list Makes the order match the list on the home page. Also adds an additional line between the code for each example for better readability. Please review this at https://codereview.appspot.com/318560043/ Affected files (+39, -28 lines): M Documentation/web/introduction.itexi Index: Documentation/web/introduction.itexi diff --git a/Documentation/web/introduction.itexi b/Documentation/web/introduction.itexi index a3bde1bf941c20acda44a2edf049ab14c27f001b..4327edde1e3257bd2959345e917d387e1bba4849 100644 --- a/Documentation/web/introduction.itexi +++ b/Documentation/web/introduction.itexi @@ -302,7 +302,6 @@ already decided to try LilyPond, first read about our @unnumberedsec Examples @divClass{column-center-top} - @subheading Beautiful Examples LilyPond is a powerful and flexible tool for engraving tasks of @@ -310,6 +309,7 @@ all kinds. Please browse our gallery of examples and be inspired! @divEnd + @divClass{column-center-middle-color2} @subheading Classical Music @@ -319,6 +319,7 @@ in LilyPond. @exampleImage{bach-bwv610} @divEnd + @divClass{column-center-middle-color2} @subheading Complex Notation @@ -329,6 +330,7 @@ beams, cross-staff stems, and voice-follow lines. @exampleImage{granados} @divEnd + @divClass{column-center-middle-color2} @subheading Early Music @@ -338,6 +340,7 @@ as this passage of Gregorian chant. @exampleImage{ancient-headword} @divEnd + @divClass{column-center-middle-color2} @subheading Modern Music @@ -365,6 +368,7 @@ full score, piano-vocal reduction, and a violin part. @divEnd + @divClass{column-center-middle-color2} @subheading Tablature @@ -376,25 +380,6 @@ staff. @exampleImage{tab-example} @divEnd -@divClass{column-center-middle-color2} -@subheading Schenker Graphs - -Standard output can be modified heavily. Here is an impressive -Schenkerian analysis, created by Kris Schaffer, for an article -in @uref{http://www.linuxjournal.com/article/8364 , Linux Journal}. -The colors have been added for better visibility. - -@exampleImage{bach-schenker} -@divEnd - -@divClass{column-center-middle-color2} -@subheading Customized Output - -A short excerpt from Stockhausen's Klavierstück II to demonstrate -LilyPond's ability to provide customised output. - -@exampleImage{Stockhausen_Klavierstueck2} -@divEnd @divClass{column-center-middle-color2} @subheading Vocal Music @@ -410,14 +395,6 @@ and the ligature braces above certain groups of notes. @exampleImage{aucun-snippet} @divEnd -@divClass{column-center-middle-color2} -@subheading Educational Applications - -LilyPond is perfectly suited for educational purposes as well. -Here is an example of a simple counterpoint exercise. - -@exampleImage{theory} -@divEnd @divClass{column-center-middle-color2} @subheading Lead Sheets @@ -430,6 +407,17 @@ to suit nearly any situation. @exampleImage{chart} @divEnd + +@divClass{column-center-middle-color2} +@subheading Educational Applications + +LilyPond is perfectly suited for educational purposes as well. +Here is an example of a simple counterpoint exercise. + +@exampleImage{theory} +@divEnd + + @divClass{column-center-middle-color2} @subheading Large Projects @@ -441,6 +429,29 @@ contributed by Hu Haipeng, a blind composer. @exampleImage{orchestra} @divEnd + +@divClass{column-center-middle-color2} +@subheading Customized Output + +A short excerpt from Stockhausen's Klavierstück II to demonstrate +LilyPond's ability to provide customised output. + +@exampleImage{Stockhausen_Klavierstueck2} +@divEnd + + +@divClass{column-center-middle-color2} +@subheading Schenker Graphs + +Standard output can be modified heavily. Here is an impressive +Schenkerian analysis, created by Kris Schaffer, for an article +in @uref{http://www.linuxjournal.com/article/8364 , Linux Journal}. +The colors have been added for better visibility. + +@exampleImage{bach-schenker} +@divEnd + + @divClass{column-center-bottom} @subheading Where now? ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add lilypond version predicates/operators (issue 317270043 by g...@ursliska.de)
On 2017/02/14 15:15:07, git wrote: I like this more because it's more of a typical procedure invocation. I think I'll create a new patch with this and a simplified calculate-version (that doesn't accept string lists) Well, having the procedure as the first argument might be more scheme-ish? Maybe best would be to have a "version-compare" function that compares two version lists using a procedure. Then define "ly:version?" so it calls "version-compare" with the current LilyPond version (to handle the primary use case). The "version-compare" function could be David's looping function, but modified so that it took two version lists and a procedure as arguments. https://codereview.appspot.com/317270043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add lilypond version predicates/operators (issue 317270043 by g...@ursliska.de)
https://codereview.appspot.com/317270043/diff/1/scm/lily-library.scm File scm/lily-library.scm (right): https://codereview.appspot.com/317270043/diff/1/scm/lily-library.scm#newcode909 scm/lily-library.scm:909: (define-public (lilypond>? ref-version) Instead of a separate procedure for each comparison, what about making it generic: (define-public (version-compare v1 proc v2) "Compare two versions @var{v1} and @var{v2} with the procedure @var{proc}. The versions are lists of three numbers like @code{'(2 18 0)}. The procedure would typically be <, >, <=, >=, =, etc." (proc (calculate-version v1) (calculate-version v2))) Usage looks like: (version-compare (ly:version) >= '(2 18 0)) Then it can be used to compare any two versions, not necessarily LilyPond (might also be useful for packages some day?) and the user learns ly:version. (Having done this, I see David's suggestion... haven't looked at it closely yet.) https://codereview.appspot.com/317270043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add `-dgs-neverembed-fonts` option (issue 312970043 by truer...@gmail.com)
For consistency and readability should there be a dash between "never" and "embed"? For example: -dgs-never-embed-fonts rather than -dgs-neverembed-fonts https://codereview.appspot.com/312970043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Don't merge non-overlapping ledger lines (issue 308560043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review, fixes issue 4979. -Paul Description: Don't merge non-overlapping ledger lines Issue 4979/2: Add regression test for non-merging ledger lines Issue 4979/1: Don't merge non-overlapping ledger lines Please review this at https://codereview.appspot.com/308560043/ Affected files (+71, -26 lines): A input/regression/ledger-lines-non-merging.ly M lily/ledger-line-spanner.cc Index: input/regression/ledger-lines-non-merging.ly diff --git a/input/regression/ledger-lines-non-merging.ly b/input/regression/ledger-lines-non-merging.ly new file mode 100644 index ..ac3bbed2cc4f8c9622e060ef333eebe369dd7e5c --- /dev/null +++ b/input/regression/ledger-lines-non-merging.ly @@ -0,0 +1,32 @@ +\version "2.19.49" + +\header { + texidoc = "In some rare cases like these the +extents of two ledger lines at the same vertical +position in the same note column do not overlap +horizontally, and they should not be merged into +a single ledger line. +See LSR 505: Displaying complex chords +http://lsr.di.unimi.it/LSR/Item?id=505 +" +} + +fixA = { + \once \override Stem.length = #11 +} + +fixB = { + \once \override NoteHead.X-offset = #1.7 + \once \override Stem.length = #7 + \once \override Stem.rotation = #'(45 0 0) + \once \override Stem.extra-offset = #'(-0.1 . -0.2) + \once \override Flag.style = #'no-flag + \once \override Accidental.extra-offset = #'(4 . -.1) +} + +\relative c' { + % case 1 + \pitchedTrill a'' \startTrillSpan a + % case 2 + << { \fixA 8 } \\ { \voiceThree \fixB dis } >> s +} Index: lily/ledger-line-spanner.cc diff --git a/lily/ledger-line-spanner.cc b/lily/ledger-line-spanner.cc index 2fe2d7d9c3896e0b627dccc8f78ab51444007006..8aeb71aab2004dc661f4cac41278c2993299fb2d 100644 --- a/lily/ledger-line-spanner.cc +++ b/lily/ledger-line-spanner.cc @@ -161,7 +161,9 @@ struct Ledger_request Interval max_head_extent_; int max_position_; vector heads_; - mapledger_extents_; + // The map's keys are vertical ledger line positions. The values are + // vectors of the x-extents of ledger lines. + map ledger_extents_; Ledger_request () { max_ledger_extent_.set_empty (); @@ -294,9 +296,7 @@ Ledger_line_spanner::print (SCM smob) // Iterate through ledger requests and the data they have about each // note head to generate the final extents for all ledger lines. - // Note heads that are different widths produce different ledger - // extents and these are merged so the widest extent prevails - // (the union of the intervals) for each ledger line. + // Note heads of different widths produce different ledger extents. for (Ledger_requests::iterator i (reqs.begin ()); i != reqs.end (); i++) { @@ -337,10 +337,25 @@ Ledger_line_spanner::print (SCM smob) natural + downstem. */ } + // When the extents of two ledgers at the same + // vertical position overlap horizontally, we merge + // them together to produce a single stencil. In rare + // cases they do not overlap and we do not merge them. + if (lr.ledger_extents_.find (lpos) == lr.ledger_extents_.end ()) -lr.ledger_extents_[lpos] = x_extent; +// Found nothing for this lpos. +lr.ledger_extents_[lpos].push_back(x_extent); else -lr.ledger_extents_[lpos].unite (x_extent); +{ + vector = lr.ledger_extents_.find (lpos)->second; + for (vsize e = 0; e < extents.size (); e++) +{ + if (intersection (extents[e], x_extent).is_empty ()) +extents.push_back (x_extent); + else +extents[e].unite (x_extent); +} +} } } } @@ -349,36 +364,34 @@ Ledger_line_spanner::print (SCM smob) // Create the stencil for the ledger line spanner by iterating // through the ledger requests and their data on ledger extents. Stencil ledgers; - Real ledgerlinethickness -= Staff_symbol::get_ledger_line_thickness (staff); + Real thickness = Staff_symbol::get_ledger_line_thickness (staff); + Real half_thickness = thickness * 0.5; + Interval y_extent = Interval (-half_thickness, half_thickness); - for (Ledger_requests::iterator i (reqs.begin ()); - i != reqs.end (); i++) + for (Ledger_requests::iterator i (reqs.begin ()); i != reqs.end (); i++) { for (DOWN_and_UP (d)) { - map = i->second[d].ledger_extents_; - for (map ::iterator k = lex.begin (); + map =
Re: Add output-attributes grob property to replace id (issue 308430043 by paulwmor...@gmail.com)
Patch Set 2: Improve convert rule, keep id property. https://codereview.appspot.com/308430043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add output-attributes grob property to replace id (issue 308430043 by paulwmor...@gmail.com)
On 2016/09/27 10:06:26, dak wrote: https://codereview.appspot.com/308430043/diff/1/python/convertrules.py File python/convertrules.py (right): https://codereview.appspot.com/308430043/diff/1/python/convertrules.py#newcode3906 python/convertrules.py:3906: @rule ((2, 19, 49), r"""\override ... .id = "a-string" -> Any reason we just don't keep the id? I considered this, but it's simpler for both code and user if all these svg output attributes are in the same alist property rather than split across two. Currently the id property is not used for anything else, so no need to keep it around. Later if another (e.g. internal) use for a grob id property arises, we could add it back then. And if not, why override all of output-attributes instead of just output-attributes.id ? Good call, that would be better (not to mention easier to do with the convert rule). And what's with tweaks? And/or overrideProperty? Oops, that's an oversight. I'll do another patch set. Thanks for the review, -Paul https://codereview.appspot.com/308430043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Doc: update toplevel-score-handler definition reference (issue 308490043 by paulwmor...@gmail.com)
Reviewers: , Message: Just a small doc edit. -Paul Description: Doc: update toplevel-score-handler definition reference The file where the default handler is defined has changed. Please review this at https://codereview.appspot.com/308490043/ Affected files (+3, -2 lines): M Documentation/notation/input.itely Index: Documentation/notation/input.itely diff --git a/Documentation/notation/input.itely b/Documentation/notation/input.itely index 83d18c5c06cc7fb2bc02f2f33721b406eeea59eb..2c8df57890e245772a2abb30b41dd2e3fdd891fa 100644 --- a/Documentation/notation/input.itely +++ b/Documentation/notation/input.itely @@ -374,8 +374,9 @@ books within the file (see @ref{Titles explained}). A @code{\score} block. This score will be collected with other toplevel scores, and combined as a single @code{\book}. This behavior can be changed by setting the variable -@code{toplevel-score-handler} at toplevel. The default handler is -defined in the init file @file{../scm/lily.scm}. +@code{toplevel-score-handler} at toplevel. (The default handler is +defined in the file @file{../scm/lily-library.scm} and set in the file +@file{../ly/declarations-init.ly}.) @item A @code{\book} block logically combines multiple movements ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Add output-attributes grob property to replace id (issue 308430043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review, thanks, -Paul Description: Add output-attributes grob property to replace id Allows setting multiple attributes on group nodes ( in SVG output), specified as an alist. Please review this at https://codereview.appspot.com/308430043/ Affected files (+84, -35 lines): M Documentation/changes.tely D input/regression/id.ly A input/regression/output-attributes.ly M lily/grob.cc M lily/stencil-integral.cc M lily/stencil-interpret.cc M python/convertrules.py M scm/define-grob-properties.scm M scm/define-stencil-commands.scm M scm/output-ps.scm M scm/output-svg.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Web: home page: add examples/images, reduce news to headlines (issue 306350043 by paulwmor...@gmail.com)
Now using the Bach BWV861 example from the essay. Screenshot: https://drive.google.com/open?id=0ByNTIEA63_a_ZGZrZzlkSUhJeU0 I like the cleaner look since this image doesn't have any title, text, etc. like the previous one. If you click on the image you can see a larger version. I will upload the pngs to gperciva/lilypond-extra -Paul https://codereview.appspot.com/306350043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Web: home page: add examples/images, reduce news to headlines (issue 306350043 by paulwmor...@gmail.com)
Thanks for the feedback everyone! On 2016/09/06 16:39:22, Carl wrote: I think the only thing remaining is to get the best possible example for the front page. Any suggestions anyone? I'm fine with the Bach one or whatever people think is best. One of the existing ones would be simplest of course. Simon, I'm fine with your suggestion of "tasks of all kinds, for example" and will make that change before this lands. (I'll forego another patch review for it though, unless anyone objects.) -Paul https://codereview.appspot.com/306350043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Web: home page: add examples/images, reduce news to headlines (issue 306350043 by paulwmor...@gmail.com)
Patch set 3 uploaded, with just one example image on the home page and a list of links to the other examples on the examples page. I like how this gets an image of sheet music onto the home page and helps communicate the wide range of LilyPond's use cases. Screenshot: https://drive.google.com/open?id=0ByNTIEA63_a_ZzMtNTFiZHFqN2c The news links and examples links now link directly to their individual news item or example. I realized that the anchor tags () already exist in the generated HTML for headings, so it's just a matter of using them. (I figured out how to use them for the website. I consulted the texinfo manuals but could not find a way to make these kinds of links work for other output formats. I assume that when we have support for @anchor it would work.) (While an interactive accordion or slide-show would be nice, it would be more work than I can take on at this point.) -Paul https://codereview.appspot.com/306350043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Web: home page: add examples/images, reduce news to headlines (issue 306350043 by paulwmor...@gmail.com)
On 2016/08/30 15:07:47, fedelogy wrote: Paul, thanks for tackling this issue! Thanks, glad to help. It's been on my wish list for awhile. I'd prefer a slider of images to save vertical space and scrolling. Some time ago I made a quick test of a pure CSS image slider of lilypond examples, but never tried to apply it on texinfo and lilypond source. Hmmm, I don't mind the scrolling or use of vertical space, and I like how the basic page of images fits the simple aesthetic of the rest of the site. Would a slider work well with the various heights of the images? Maybe it's something we could consider for a future revision? I _guess_ that the problem with the @anchor tag is due to lilypond-texi2html.init Probably so. Maybe @anchor is a newer feature in texinfo? I've uploaded a patch set 2 that succeeds at a full 'make doc' and adds a second commit: Web: Rearrange examples/images on home page To raise the prominence of some common use cases, the 4 marked with an asterisk (*) below. NEW ORDER: Classical Music Complex Notation Early Music Modern Music * Tablature * Vocal Music * Popular Music Efficient, flexible creation of performance materials * Educational Applications Schenker Graphs Customized Output Large Projects OLD ORDER: Classical Music Complex Notation Early Music Modern Music Efficient, flexible creation of performance materials Tablature Schenker Graphs Customized Output Vocal Music Educational Applications Popular Music Large Projects https://codereview.appspot.com/306350043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Web: home page: add examples/images, reduce news to headlines (issue 306350043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review and let me know what you think about these changes. They are inspired by recent and not so recent discussions on the lists. Here's a screenshot of the home page with these changes: https://drive.google.com/open?id=0ByNTIEA63_a_bkxrWlpKcW43cHc The images and other content from the 'Examples' page is moved to the home page, and the 'Examples' page is removed from the 'Introduction' section. The news entries on the home page are reduced to just headline links in the sidebar. They link to the 'Old News' page where the full entries live (under 'Community'). And that page is renamed to just 'News'. (I tried to make the headline links point to individual entries on the News page, but the texinfo @anchor tag is unrecognized by our build scripts. So an upgrade is needed before we can use this feature.) If these changes are to land as they are (pending review and discussion) then we may want to add redirects to the htaccess file for two changed urls before landing them: old-news.html -> news.html examples.html -> home page -Paul Description: Web: home page: add examples/images, reduce news to headlines Examples/images is moved from the Introduction section to the home page. Please review this at https://codereview.appspot.com/306350043/ Affected files (+182, -181 lines): M Documentation/css/lilypond-website.css M Documentation/web.texi M Documentation/web/community.itexi M Documentation/web/introduction.itexi M Documentation/web/news-front.itexi A Documentation/web/news-headlines.itexi ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allow override of NoteHead.ledger-positions (issue 295970043 by paulwmor...@gmail.com)
Patch set 4 with some minor cleanups. https://codereview.appspot.com/295970043/diff/40001/input/regression/ledger-positions-customization.ly File input/regression/ledger-positions-customization.ly (right): https://codereview.appspot.com/295970043/diff/40001/input/regression/ledger-positions-customization.ly#newcode19 input/regression/ledger-positions-customization.ly:19: #'(lambda (staff-symbol-grob pos) (list pos)) On 2016/04/24 03:05:12, lemzwerg wrote: I would indent this line... Thanks for catching this. Done, along with a few other other cleanups and making things more concise in staff-symbol.cc, in patch set 4. https://codereview.appspot.com/295970043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allow override of NoteHead.ledger-positions (issue 295970043 by paulwmor...@gmail.com)
On 2016/04/23 20:33:21, pwm wrote: Add StaffSymbol.ledger-positions-function I realized that calls to StaffSymbol::on_line would not be aware of any customizations made via NoteHead.ledger-positions, but they will be with the StaffSymbol.ledger-positions-function from patch set 3. I considered removing the NoteHead.ledger-positions option, but it's the only way to customize ledger positions for a single note, so I left it in. Regtest is updated. https://codereview.appspot.com/295970043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allow override of NoteHead.ledger-positions (issue 295970043 by paulwmor...@gmail.com)
Patch set 2, based on David's Feedback. -Paul https://codereview.appspot.com/295970043/diff/1/lily/ledger-line-spanner.cc File lily/ledger-line-spanner.cc (right): https://codereview.appspot.com/295970043/diff/1/lily/ledger-line-spanner.cc#newcode221 lily/ledger-line-spanner.cc:221: int len = scm_to_int (scm_length (posns)); On 2016/04/22 08:17:09, dak wrote: scm_ilength already does scm_to_int. Ah, thanks for the tip. But if you are getting the size of the vector in advance anyway (rather than just do {...} while (scm_is_pair (posns));), why don't you dimension the vector in advance? Ok, thanks, I changed it to set the vector size and switched to using do/while. But then I thought there must be a helper function for this, and sure enough there is ly_scm2floatvector in lily-guile.cc, so I've used that in patch set 2. But at any rate, wouldn't it be better to just pass an optional grob argument to Staff_symbol::ledger_positions so that this function will extract the ledger-positions preferably from this extra argument? That way the extraction code is all in one place. That makes sense to me. I finally got the argument passing working (as an optional const pointer) ...after a few maddening rounds wrestling with C++. Please let me know if there's a better way than what I did. https://codereview.appspot.com/295970043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Allow override of NoteHead.ledger-positions (issue 295970043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review. I've posted an implementation question in the issue tracker. https://sourceforge.net/p/testlilyissues/issues/4828/ Thanks, -Paul Description: Allow override of NoteHead.ledger-positions Can be a literal list of positions: \once \override NoteHead.ledger-positions = #'(...) Or a scheme procedure that returns such a list: \override NoteHead.ledger-positions = #(lambda (grob) ...) Please review this at https://codereview.appspot.com/295970043/ Affected files (+38, -5 lines): A input/regression/ledger-positions-override-via-note-head.ly M lily/ledger-line-spanner.cc M lily/note-head.cc M scm/define-grob-properties.scm Index: input/regression/ledger-positions-override-via-note-head.ly diff --git a/input/regression/ledger-positions-override-via-note-head.ly b/input/regression/ledger-positions-override-via-note-head.ly new file mode 100644 index ..0894373ca31884481fa973c7892722905649d69a --- /dev/null +++ b/input/regression/ledger-positions-override-via-note-head.ly @@ -0,0 +1,18 @@ + +\header { + +texidoc = "Ledger line positions can be overridden +via NoteHead grobs." + +} + +\version "2.19.41" +\paper { ragged-right = ##t } +\relative { + c'''4 + \once \override NoteHead.ledger-positions = #'(8 10 12 14) + c + \override NoteHead.ledger-positions = + #(lambda (grob) (list (ly:grob-staff-position grob))) + d e +} Index: lily/ledger-line-spanner.cc diff --git a/lily/ledger-line-spanner.cc b/lily/ledger-line-spanner.cc index 42d32b496af7bb5ec7a15f1b52cdbc4ea84959c8..9246cd0f62a710cafb89f3d484a72112b71fa360 100644 --- a/lily/ledger-line-spanner.cc +++ b/lily/ledger-line-spanner.cc @@ -211,10 +211,22 @@ Ledger_line_spanner::print (SCM smob) for (vsize i = heads.size (); i--;) { Item *h = dynamic_cast (heads[i]); - int pos = Staff_symbol_referencer::get_rounded_position (h); - vector ledger_positions = -Staff_symbol::ledger_positions (staff, pos); + vector ledger_positions; + + // allow user to override ledger positions by note head grob + SCM posns = h->get_property ("ledger-positions"); + if (scm_is_pair (posns)) +{ + int len = scm_to_int (scm_length (posns)); + for (int p = 0; p < len; p++) +{ + ledger_positions.push_back +(scm_to_double (scm_list_ref (posns, scm_from_int (p; +} +} + else +ledger_positions = Staff_symbol::ledger_positions (staff, pos); // We work with all notes that produce ledgers and any notes that // fall outside the staff that do not produce ledgers, such as Index: lily/note-head.cc diff --git a/lily/note-head.cc b/lily/note-head.cc index 33bbf339cafd9b431b83d2955522c2c8f91c1379..73a2255d9040a91199e994cf05c2fe0e6f9a74b5 100644 --- a/lily/note-head.cc +++ b/lily/note-head.cc @@ -211,5 +211,6 @@ ADD_INTERFACE (Note_head, "glyph-name " "stem-attachment " "style " + "ledger-positions " ); Index: scm/define-grob-properties.scm diff --git a/scm/define-grob-properties.scm b/scm/define-grob-properties.scm index e54bdaf18386363be5afb59c7016051dedf88af2..dcf9705998236a2a78da8ae062fb62d003930731 100644 --- a/scm/define-grob-properties.scm +++ b/scm/define-grob-properties.scm @@ -575,8 +575,10 @@ lines for.") lines. It is the sum of 2@tie{}numbers: The first is the factor for line thickness, and the second for staff space. Both contributions are added.") - (ledger-positions ,list? "Repeating pattern for the vertical positions -of ledger lines. Bracketed groups are always shown together.") + (ledger-positions ,list? "Vertical positions of ledger lines. +When set on a @code{StaffSymbol} grob it defines a repeating +pattern of ledger lines and any parenthesized groups will always be +shown together.") (left-bound-info ,list? "An alist of properties for determining attachments of spanners to edges.") (left-padding ,ly:dimension? "The amount of space that is put ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
ledger-line-spanner.cc: refactor Ledger_line_spanner::print (issue 297820043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review. I was working on a scheme version of the ledger line print routine (for some custom ledger line patterns) and having figuring out how it worked, I thought I would make some improvements. Since I'm new to C++ ...review is especially appreciated. Thanks, -Paul Description: ledger-line-spanner.cc: refactor Ledger_line_spanner::print Optimizations and removal of code duplication. Only iterate through all note heads once, generating all relevant data for just note heads involved in ledger lines. Then work with that data. Merge overlapping ledger data before producing ledger stencils so we only produce one stencil for each ledger line. Please review this at https://codereview.appspot.com/297820043/ Affected files (+142, -81 lines): M lily/ledger-line-spanner.cc ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc: CG: push to staging with rebase not merge (issue 292370043 by paulwmor...@gmail.com)
https://codereview.appspot.com/292370043/diff/1/Documentation/contributor/source-code.itexi File Documentation/contributor/source-code.itexi (right): https://codereview.appspot.com/292370043/diff/1/Documentation/contributor/source-code.itexi#newcode2296 Documentation/contributor/source-code.itexi:2296: on savannah. On 2016/04/13 10:19:17, benko.pal wrote: or by running $ gitk after $ git fetch or $ git pull -r as suggested later, but then check for origin/master being the same as origin/staging (actually there's no need for a local master or staging branch). or simply whether $ git log origin/master..origin/staging lists any commit. Thanks, I'll add a version of this to the next patch set. https://codereview.appspot.com/292370043/diff/1/Documentation/contributor/source-code.itexi#newcode2325 Documentation/contributor/source-code.itexi:2325: git pull -r origin staging On 2016/04/13 10:19:17, benko.pal wrote: $ git pull -r suffices if staging is not ahead (and branch_name is based on origin/master) Yes, although I think it's a good idea to show a way that always works, regardless of other factors, to keep it simple and foolproof. Those who know git well will know the shortcuts. https://codereview.appspot.com/292370043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Doc: CG: push to staging with rebase not merge (issue 292370043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review, thanks. -Paul Description: Doc: CG: push to staging with rebase not merge Use 'git pull -r' for consistency with other parts of the CG. Please review this at https://codereview.appspot.com/292370043/ Affected files (+21, -8 lines): M Documentation/contributor/source-code.itexi Index: Documentation/contributor/source-code.itexi diff --git a/Documentation/contributor/source-code.itexi b/Documentation/contributor/source-code.itexi index b961a82cf6c65897090fd5736731974ac19096b4..a96a408fdd169a743ea9c23542d02589fe17f263 100644 --- a/Documentation/contributor/source-code.itexi +++ b/Documentation/contributor/source-code.itexi @@ -2288,11 +2288,18 @@ end up in master after all, defeating the purpose of the system. The proper fix usually involves rewriting the staging branch and is best left to core developers after discussion on the developer list. +Before pushing to staging it is a good practice to check whether +staging is ahead of master, and if so, wait until master has caught up +with staging before pushing. This simplifies things if changes to +staging have to be backed out for some reason. You can check whether +master has caught up with staging by looking at the git web interface +on savannah. + @subsubheading If your work is in a patch file Assuming that your patch is in a file called -@file{0001-my-patch.patch}, and you are currently on git master, -do: +@file{0001-my-patch.patch} (see @ref{Patches}), and you are currently +on git master, do: @example git checkout staging @@ -2310,20 +2317,26 @@ commit ahead of @code{origin/staging}.} @subsubheading If your work is in a branch -If you are working on branches and your work in is +If you are working on branches and your work is in @code{my_branch_name}, then do: @example -git checkout staging -git pull -r -git merge my_branch_name +git checkout my_branch_name +git pull -r origin staging +@end example + +This will rebase your branch on @code{origin/staging}. At this point +git will let you know if there are any conflicts. If so, resolve them +before continuing: + +@example gitk -git push origin staging +git push origin HEAD:staging @end example @warning{Do not skip the @command{gitk} step; a quick 5-second check of the visual history can save a great deal of frustration -later on. You should see that @code{staging} is only ahead of +later on. You should see that @code{my_branch_name} is only ahead of @code{origin/staging} by the commits from your branch.} ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Doc: CG: itemize section on commit message prefixes (issue 296810043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review, thanks, -Paul Description: Doc: CG: itemize section on commit message prefixes Please review this at https://codereview.appspot.com/296810043/ Affected files (+20, -8 lines): M Documentation/contributor/source-code.itexi Index: Documentation/contributor/source-code.itexi diff --git a/Documentation/contributor/source-code.itexi b/Documentation/contributor/source-code.itexi index 19b835a0ef959caac542cedae29d10b82cfa88cb..b961a82cf6c65897090fd5736731974ac19096b4 100644 --- a/Documentation/contributor/source-code.itexi +++ b/Documentation/contributor/source-code.itexi @@ -1235,17 +1235,29 @@ high-res images, fixed cropping on Finale example. @end example Commit messages often start with a short prefix describing the -general location of the changes. If a commit affects the +general location of the changes. + +@itemize +@item +Doc: and Doc-@var{**}: If a commit affects the documentation in English (or in several languages simultaneously) -the commit message should be prefixed with @qq{Doc:@tie{}}. If -the commit affects only one of the translations, the commit +the commit message should be prefixed with @qq{Doc:@tie{}}. If the +commit affects only one of the translations, the commit message should be prefixed with @qq{Doc-@var{**}:@tie{}}, where -@var{**} is the two-letter language code. Commits that affect the +@var{**} is the two-letter language code. + +@item +Web: and Web-@var{**}: Commits that affect the website should use @qq{Web:@tie{}} for English, and -@qq{Web-@var{**}:@tie{}} for the other languages. Also, changes -to a single file are often prefixed with the name of the file -involved. Visit the links listed in @ref{Understanding commits} -for examples. +@qq{Web-@var{**}:@tie{}} for other languages. + +@item +Changes to a single file are often prefixed with the name of the file +involved. +@end itemize + +Visit the links listed in @ref{Understanding commits} for examples. + @node Patches ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
engraver-init.ly: guarantee no ledger lines in NullVoice (issue 297730043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review. See issue 4818 for examples that show this obscure edge case. Luckily it was an easy fix once I figured out what was going on. Thanks, -Paul Description: engraver-init.ly: guarantee no ledger lines in NullVoice for staves with custom line positions and ledger lines that appear inside the staff. Please review this at https://codereview.appspot.com/297730043/ Affected files (+4, -0 lines): M ly/engraver-init.ly Index: ly/engraver-init.ly diff --git a/ly/engraver-init.ly b/ly/engraver-init.ly index 4d35598887d5c39db36d29f05e14aa718050e415..6421d9efa1f3fa2aa990743062a3dbb1a5c13521 100644 --- a/ly/engraver-init.ly +++ b/ly/engraver-init.ly @@ -828,6 +828,10 @@ context." \consists "Pitch_squash_engraver" squashedPosition = 0 + %% generate no ledger lines, needed for staves with custom + %% line positions and ledger lines that appear inside the staff + \override NoteHead.no-ledgers = ##t + %% the engravers that control the 'busy' flags for note-onsets and melismata \consists "Grob_pq_engraver" \consists "Tie_engraver" ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc: CG - Re-organize information about 'Patches' (issue 291160043 by pkx1...@gmail.com)
LGTM. On 2016/03/17 18:15:29, pkx166h wrote: I've opted for this * The Git contributor's cycle:: * Pulling and rebasing:: * Using local branches:: * Commits:: * Patches:: * Uploading a patch for review:: * The patch review cycle:: That way those more expert devs that already know how to create a patch but may not know our review process can find this section more easily. Good call. Having separate sections for each of these topics will make it easy to jump directly to the one that covers what you are trying to do. -Paul https://codereview.appspot.com/291160043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Doc: NR A.2: Fix typos in chord modifiers table (issue 293900043 by paulwmor...@gmail.com)
Reviewers: , Message: Found a couple of typos. -Paul Description: Doc: NR A.2: Fix typos in chord modifiers table Please review this at https://codereview.appspot.com/293900043/ Affected files (+2, -2 lines): M Documentation/notation/notation-appendices.itely Index: Documentation/notation/notation-appendices.itely diff --git a/Documentation/notation/notation-appendices.itely b/Documentation/notation/notation-appendices.itely index 241daf752fafcd6a0ec6e8aaa068190a9d077f49..3d8bf79f43efacfbd0afb4d72cd8345ee2debaed 100644 --- a/Documentation/notation/notation-appendices.itely +++ b/Documentation/notation/notation-appendices.itely @@ -204,7 +204,7 @@ Augmented triad, @*minor seventh @tab @code{aug7} @tab -@code{c1:aug} +@code{c1:aug7} @tab @lilypond[line-width=2.1\cm,noragged-right,notime] << @@ -236,7 +236,7 @@ Minor triad, @*major seventh @tab @code{m7+} @tab -@code{m7+} +@code{c1:m7+} @tab @lilypond[line-width=2.1\cm,noragged-right,notime] << ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: output-svg.scm: better defaults for font-family (issue 290850043 by paulwmor...@gmail.com)
On 2016/02/20 08:20:12, trueroad wrote: LGTM But here is another method. Thank you, this is better, and I've used it in patch set 2. Patch set 2 also includes some edits to the docs (as a separate commit). -Paul https://codereview.appspot.com/290850043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
output-svg.scm: better defaults for font-family (issue 290850043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review. See the discussion in this thread on the bug list: http://lists.gnu.org/archive/html/bug-lilypond/2016-02/msg00079.html Thanks, -Paul Description: output-svg.scm: better defaults for font-family if the user has not set the font, use a meaningful svg default ('serif' 'sans-serif' or 'monospace') for the font-family property Please review this at https://codereview.appspot.com/290850043/ Affected files (+7, -2 lines): M scm/output-svg.scm Index: scm/output-svg.scm diff --git a/scm/output-svg.scm b/scm/output-svg.scm index ca086318d7810d1f4769d59e6d2e88d4fc28668d..2f2ab1ff2395cf92977f0bde24903a6c3ecbfa32 100644 --- a/scm/output-svg.scm +++ b/scm/output-svg.scm @@ -132,8 +132,13 @@ (match (if match-1 match-1 match-2))) (if (regexp-match? match) -(begin - (set-attribute 'font-family (match:prefix match)) +;; use meaningful svg defaults if font has not been set by user +(let ((font-fam (match:prefix match)) + (lookup '(("LilyPond Serif" . "serif") +("LilyPond Sans Serif" . "sans-serif") +("LilyPond Monospace" . "monospace" + (set-attribute 'font-family (or (assoc-ref lookup font-fam) + font-fam)) (if (string? (match:substring match 1)) (set-attribute 'font-weight "bold")) (if (string? (match:substring match 2)) ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Web: CSS: improve heading styles (issue 290790043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review. I've put up a demo copy of the full LilyPond website and documentation with these CSS changes in effect, to make it easy to see what they do. (The changes only affect the various headings in the manuals.) Just remove the spaces in the following URL (which have been added here to help thwart web crawlers/indexers (along with a robots.txt entry)). http://clairnote.org /lilypond-web-demo/ Description: Web: CSS: improve heading styles - Remove dashed bottom borders - Adjust sizes and spacing - Restore previous dark blue heading color from 2.18 web manuals Please review this at https://codereview.appspot.com/290790043/ Affected files (+18, -16 lines): M Documentation/css/lilypond-manuals.css Index: Documentation/css/lilypond-manuals.css diff --git a/Documentation/css/lilypond-manuals.css b/Documentation/css/lilypond-manuals.css index 04a6877d8dbd833ba2568d7d5834ec3b6757ba29..cdbd6b76472ea8eeda00ee477ade4e2c44c69d25 100644 --- a/Documentation/css/lilypond-manuals.css +++ b/Documentation/css/lilypond-manuals.css @@ -48,11 +48,10 @@ body { .appendix, .appendixsec, .appendixsubsec, .unnumbered, .unnumberedsec, .unnumberedsubsec, .unnumberedsubsubsec, .subheading, .subsubheading { - color: #black; - border-bottom: 1px dashed black; + color: rgb(32, 74, 135); padding-bottom: 0.15em; margin-top: 0.6em; - margin-bottom: 1em; + margin-bottom: 0.6em; } .settitle { @@ -73,25 +72,28 @@ body.extending .settitle { background-color: #7f4040; } body.internals .settitle { background-color: #6a407f; } body.contributor .settitle { background-color: #00; } -.chapter, .appendix, .unnumbered { - font-size: 1.8em; -} - -.section, .appendixsec, .unnumberedsec { - font-size: 1.6em; +.chapter, .appendix, .unnumbered, +.section, .appendixsec, .unnumberedsec, +.subsection, .appendixsubsec, .unnumberedsubsec, +.subsubsection { + font-size: 1.7em; + margin-top: 0.9em; } -.subsection, .appendixsubsec, .unnumberedsubsec { - font-size: 1.4em; +.subheading, .unnumberedsubsubsec { + font-size: 1.5em; + margin-top: 0.9em; } -.subheading, .subsubsection, .unnumberedsubsubsec { - font-size: 1.25em; +.subheading { + border-top: 1px solid rgb(200, 200, 200); + padding-top: 0.8em; } .subsubheading { - font-size: 1em; + font-size: 1.15em; font-weight: bold; + margin-top: 1.6em; } .chapheading { @@ -214,8 +216,8 @@ div#main ul { } h1, h2, h3, h4, p, table, address, dt { - padding-left: 1em; - padding-right: 1em; + padding-left: 18px; + padding-right: 18px; } p { ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc: CG 3.3 separate commits and patches subsections (issue 285480043 by paulwmor...@gmail.com)
On 2016/02/12 12:49:06, benko.pal wrote: do we really need to talk about patches at all in the preferred (i.e. Rietveld-based) workflow? (do we really need any more about non preferred ways than "ask on devel"?) Good question... maybe, maybe not, but I think that deserves a separate issue/review and maybe a discussion on the dev list. (Currently anyone contributing who does not have push access (like myself) has to create patches as described in the "Making Patches" section, after a review is over, and email them to James or someone with push access who can push them. So that section currently documents part of our standard workflow, and just removing it is not so simple. I think revising the wording would help, and/or maybe changing the order of the sections so that the git-cl/Rietveld section comes first. But I think that deserves its own issue/review, if not a dev list discussion.) https://codereview.appspot.com/285480043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Doc: CG 3.3 separate commits and patches subsections (issue 285480043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review. Having separate subsections for Commits and Patches will make it easier to quickly navigate to one or the other from the table of contents. And since they are in fact two different things, separate subsections makes conceptual sense, perhaps especially for those learning git. Thanks, -Paul Description: Doc: CG 3.3 separate commits and patches subsections Please review this at https://codereview.appspot.com/285480043/ Affected files (+18, -9 lines): M Documentation/contributor/administration.itexi M Documentation/contributor/issues.itexi M Documentation/contributor/quick-start.itexi M Documentation/contributor/source-code.itexi Index: Documentation/contributor/administration.itexi diff --git a/Documentation/contributor/administration.itexi b/Documentation/contributor/administration.itexi index a392f7d660f8e62cf5518c6678acba472ffa3b58..510a799d9170a18b0b09693cf6c354e677ff32f4 100644 --- a/Documentation/contributor/administration.itexi +++ b/Documentation/contributor/administration.itexi @@ -167,7 +167,7 @@ The Patch Meister's responsibilities are: To keep track of all patches submitted for testing and review. This includes scanning the bug and dev email lists looking for any patches submitted by @q{random} contributors and advising them on how to submit -a patch for testing and review. See @ref{Commits and patches}. +a patch for testing and review. See @ref{Patches}. @item To makes sure that any patch submitted has a corresponding Issue Tracker Index: Documentation/contributor/issues.itexi diff --git a/Documentation/contributor/issues.itexi b/Documentation/contributor/issues.itexi index 883328f85a2345a76760986b8e45fff1def3eda8..09925d4208c80e084f080aad5aa1c42226e7bdb0 100644 --- a/Documentation/contributor/issues.itexi +++ b/Documentation/contributor/issues.itexi @@ -825,7 +825,7 @@ email should contain a link to the issue you just added. separate person handling this task.} For contributors/developers: follow the steps in -@ref{Commits and patches}, and @ref{Pushing to staging}. +@ref{Commits}, @ref{Patches}, and @ref{Pushing to staging}. @ignore For people doing maintenance tasks: git-cl is adding issues, James Index: Documentation/contributor/quick-start.itexi diff --git a/Documentation/contributor/quick-start.itexi b/Documentation/contributor/quick-start.itexi index 9a5b12a7ec36d597e6eef5875588c2d65ec6c4d5..b5ccb3b0b8212abee2f492de84d895003b9cd042 100644 --- a/Documentation/contributor/quick-start.itexi +++ b/Documentation/contributor/quick-start.itexi @@ -362,7 +362,7 @@ After entering a commit message, click @qq{OK} to finalize the commit. @advanced{for more information regarding commits and commit -messages, see @ref{Commits and patches}.} +messages, see @ref{Commits}.} @subsubheading 2b. Amend previous commit Index: Documentation/contributor/source-code.itexi diff --git a/Documentation/contributor/source-code.itexi b/Documentation/contributor/source-code.itexi index 719c85e041ba80021059c338d3c0b1047f76cc13..7c4dc63778584703f17823f636e9f928483aef82 100644 --- a/Documentation/contributor/source-code.itexi +++ b/Documentation/contributor/source-code.itexi @@ -848,7 +848,8 @@ The branches are kept for archival reasons. * The Git contributor's cycle:: * Pulling and rebasing:: * Using local branches:: -* Commits and patches:: +* Commits:: +* Patches:: @end menu @@ -940,7 +941,7 @@ refusing to pull with rebase: your working tree is not up-to-date @noindent it means that you have modified some files in you working tree -without committing changes (see @ref{Commits and patches}); you +without committing changes (see @ref{Commits}); you can use the @command{git@tie{}stash} command to work around this: @example @@ -1088,16 +1089,14 @@ to merge @code{translation} into @code{staging} whenever he has checked that @code{translation} builds successfully. -@node Commits and patches -@subsection Commits and patches +@node Commits +@subsection Commits @menu * Understanding commits:: * Making commits:: * Commit messages:: -* Making patches:: -* Uploading a patch for review:: @end menu @@ -1248,6 +1247,16 @@ involved. Visit the links listed in @ref{Understanding commits} for examples. +@node Patches +@subsection Patches + + +@menu +* Making patches:: +* Uploading a patch for review:: +@end menu + + @node Making patches @unnumberedsubsubsec Making patches ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Web: Review GSoC page (issue 285400043 by g...@ursliska.de)
LGTM, with one minor suggestion. Thanks Urs, I think this really improves this page. -Paul https://codereview.appspot.com/285400043/diff/1/Documentation/web/community.itexi File Documentation/web/community.itexi (right): https://codereview.appspot.com/285400043/diff/1/Documentation/web/community.itexi#newcode1107 Documentation/web/community.itexi:1107: achievements with regard to LilyPond's default output quality. On 2016/02/07 23:07:27, git wrote: On 2016/02/06 20:53:30, pwm wrote: > I don't think this is needed. It breaks out of the pattern of just describing > the project and gets into editorializing about its comparative worth relative to > other projects, or even how rewarding it would be. Maybe someone would find > another project more rewarding? Seems like this is better suited for a > conversation with a student. Hm, my intention was to draw the attention to the fact that this project relates to actual engraving quality. But I've taken your suggestion. Hmm... that's a good idea. Maybe add this to the title? "Improve engraving quality of slurs and ties" or (if not the title) maybe in the first sentence: "The engraving quality of the default curves of slurs and ties is often unsatisfactory." https://codereview.appspot.com/285400043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Web: Review GSoC page (issue 285400043 by g...@ursliska.de)
Thanks for working on this Urs. Here are my strong opinions and suggested revisions. Make of them what you will! -Paul https://codereview.appspot.com/285400043/diff/1/Documentation/web/community.itexi File Documentation/web/community.itexi (right): https://codereview.appspot.com/285400043/diff/1/Documentation/web/community.itexi#newcode909 Documentation/web/community.itexi:909: anyone who is interested in developing LilyPond. Why don't we just shorten this paragraph to: Below is a list of suggested projects for GSoC or for anyone who is interested in developing LilyPond. (Last updated: February 2016) https://codereview.appspot.com/285400043/diff/1/Documentation/web/community.itexi#newcode916 Documentation/web/community.itexi:916: you're welcome to suggest it. How about simplifying this paragraph to: If you have ideas for a GSoC project that is not listed below, please send us an email on our developer mailing list (see @ref{Contact}). There are a number of areas where LilyPond could be improved, and our development team is always willing to help those who would like to tackle a project like those listed below. https://codereview.appspot.com/285400043/diff/1/Documentation/web/community.itexi#newcode925 Documentation/web/community.itexi:925: case. This should come before the text about the sourceforge list, otherwise which list does it refer to? It would probably go best before the text about other projects that are not on the project list. I would suggest rewording it as follows: Mentor availability varies from project to project and from year to year. Send us an email on our developer mailing list (see @ref{Contact}), and we will help you find a mentor for a project that fits your interests and skills. I see no problem in changing the sort order, but I'd rather not specify the sort order, as it seems like a distraction and a future maintenance burden. The list isn't that long and we want students to read the whole thing and contact us if there's any project they're interested in. https://codereview.appspot.com/285400043/diff/1/Documentation/web/community.itexi#newcode1088 Documentation/web/community.itexi:1088: project from 2015. I would shorten to: There are several possibilities for this project, including building upon the MusicXML export project from GSoC 2015. https://codereview.appspot.com/285400043/diff/1/Documentation/web/community.itexi#newcode1091 Documentation/web/community.itexi:1091: @strong{Requirements:} MusicXML, Python, (Scheme), basic LilyPond knowledge (Scheme) --> Scheme https://codereview.appspot.com/285400043/diff/1/Documentation/web/community.itexi#newcode1092 Documentation/web/community.itexi:1092: @strong{Potential Mentors:} None confirmed for 2016 I would keep the names of potential mentors (partly for easy reference in future years or in the "off-season"), but just list their status as well. For example: @strong{Potential Mentors:} Reinhold Kainhofer, Mike Solomon (neither are available for GSoC 2016) Same suggestion for all listings like this. https://codereview.appspot.com/285400043/diff/1/Documentation/web/community.itexi#newcode1107 Documentation/web/community.itexi:1107: achievements with regard to LilyPond's default output quality. I don't think this is needed. It breaks out of the pattern of just describing the project and gets into editorializing about its comparative worth relative to other projects, or even how rewarding it would be. Maybe someone would find another project more rewarding? Seems like this is better suited for a conversation with a student. https://codereview.appspot.com/285400043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Doc: CG: Merge and remove redundant git-cl info (issue 285350043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review, thanks, -Paul Description: Doc: CG: Merge and remove redundant git-cl info Please review this at https://codereview.appspot.com/285350043/ Affected files (+43, -114 lines): M Documentation/contributor/quick-start.itexi M Documentation/contributor/source-code.itexi ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Web: Add GSoC entry for ScholarLY (issue 282290043 by g...@ursliska.de)
LGTM with an edit and a broader question about the mentor listings. -Paul https://codereview.appspot.com/282290043/diff/1/Documentation/web/community.itexi File Documentation/web/community.itexi (right): https://codereview.appspot.com/282290043/diff/1/Documentation/web/community.itexi#newcode936 Documentation/web/community.itexi:936: @strong{Mentor(s):} Urs Liska Just use "Mentor:" if there's only one listed. (See issue 4750 which drops the use of "(s)".) Maybe use "Potential Mentor:" for consistency with the other listings? Do we want to make a distinction between "Mentor" and "Potential Mentor" here? Using "Potential Mentor" for all listings means less to update from one year to the next. Then, if and when a student expresses interest, at that point we would confirm whether the potential mentors listed are willing and available to be mentors. https://codereview.appspot.com/282290043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Web: edits to Google Summer of Code page (issue 282260043 by paulwmor...@gmail.com)
Add "free software". -Paul https://codereview.appspot.com/282260043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add ly:one-page-breaking (issue 288910043 by paulwmor...@gmail.com)
Now that ly:one-line-auto-height is in master, this patch set updates the docs so it and ly:one-page-breaking are both present and accounted for. -Paul https://codereview.appspot.com/288910043/diff/1/Documentation/notation/spacing.itely File Documentation/notation/spacing.itely (right): https://codereview.appspot.com/288910043/diff/1/Documentation/notation/spacing.itely#newcode895 Documentation/notation/spacing.itely:895: @code{ly:one-page-breaking}, @code{ly:one-line-breaking} and On 2016/01/24 18:32:37, simon.albrecht wrote: @code{ly:one-line-auto-height-breaking} is missing here and below. Done. https://codereview.appspot.com/288910043/diff/1/Documentation/notation/spacing.itely#newcode895 Documentation/notation/spacing.itely:895: @code{ly:one-page-breaking}, @code{ly:one-line-breaking} and On 2016/01/24 20:53:39, simon.albrecht wrote: On 2016/01/24 18:32:37, simon.albrecht wrote: > @code{ly:one-line-auto-height-breaking} is missing here and below. Sorry for the noise, just saw that they were included in the other patch set. Done. https://codereview.appspot.com/288910043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add ly:one-page-breaking (issue 288910043 by paulwmor...@gmail.com)
Please review, thanks, -Paul https://codereview.appspot.com/288910043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Web: edits to Google Summer of Code page (issue 282260043 by paulwmor...@gmail.com)
Please review. Thanks, -Paul https://codereview.appspot.com/282260043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add ly:one-line-auto-height-breaking (issue 284240043 by paulwmor...@gmail.com)
A few doc edits. -Paul https://codereview.appspot.com/284240043/diff/1/Documentation/notation/spacing.itely File Documentation/notation/spacing.itely (right): https://codereview.appspot.com/284240043/diff/1/Documentation/notation/spacing.itely#newcode895 Documentation/notation/spacing.itely:895: @code{ly:one-line-breaking}, @code{ly:one-line-auto-height-page-breaking}, On 2016/01/18 12:43:54, git wrote: I think this should read "ly:one-line-auto-height-breaking" Done. https://codereview.appspot.com/284240043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Add ly:one-line-auto-height-breaking (issue 284240043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review, thanks. -Paul Description: Add ly:one-line-auto-height-breaking Includes 4 commits: Changes entry for ly:one-line-auto-height-breaking Doc: NR 4.3.2 add ly:one-line-auto-height-breaking Includes a few related edits to NR 4.3.2 mainly for ly:one-line-breaking Add regtest for ly:one-line-auto-height-breaking Add ly:one-line-auto-height-breaking Please review this at https://codereview.appspot.com/284240043/ Affected files (+90, -33 lines): M Documentation/changes.tely M Documentation/notation/spacing.itely A input/regression/one-line-auto-height-breaking.ly A + lily/include/one-line-auto-height-breaking.hh A + lily/one-line-auto-height-breaking.cc M lily/one-line-page-breaking.cc M lily/page-breaking-scheme.cc ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: stencil.scm: make args optional in stencil-whiteout (issue 284980043 by paulwmor...@gmail.com)
Slightly simpler docstring revision. -Paul https://codereview.appspot.com/284980043/diff/20001/scm/stencil.scm File scm/stencil.scm (right): https://codereview.appspot.com/284980043/diff/20001/scm/stencil.scm#newcode809 scm/stencil.scm:809: and @code{'box}. Given @code{'outline} the white background is produced On 2016/01/06 18:24:15, thomasmorley651 wrote: maybe change to: other arguments or unspecified will default to @code{'box}. Ok, I've uploaded a simpler version based on these suggestions. Let me know what you think. https://codereview.appspot.com/284980043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
stencil.scm: make args optional in stencil-whiteout (issue 284980043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review, thanks. -Paul Description: stencil.scm: make args optional in stencil-whiteout Please review this at https://codereview.appspot.com/284980043/ Affected files (+2, -1 lines): M scm/stencil.scm Index: scm/stencil.scm diff --git a/scm/stencil.scm b/scm/stencil.scm index bec03016505e4cc5317061f08d2b80f5e416e53f..950d99f9cb8e2b1e61baf87638e81b558105766a 100644 --- a/scm/stencil.scm +++ b/scm/stencil.scm @@ -801,7 +801,8 @@ the white outline extends past the extents of stencil @var{stil}." (stencil-with-color (ly:round-filled-box x-ext y-ext blot) color) stil))) -(define-public (stencil-whiteout stil style thickness line-thickness) +(define*-public (stencil-whiteout stil #:optional (style 'box) + (thickness '()) (line-thickness 0.1)) "@var{style} is a symbol that determines the shape of the white background. @var{thickness} is how far, as a multiple of @var{line-thickness}, the white background extends past the extents ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: stencil.scm: make args optional in stencil-whiteout (issue 284980043 by paulwmor...@gmail.com)
Addressing Harm's suggestions in patch set 2. https://codereview.appspot.com/284980043/diff/1/scm/stencil.scm File scm/stencil.scm (right): https://codereview.appspot.com/284980043/diff/1/scm/stencil.scm#newcode804 scm/stencil.scm:804: (define*-public (stencil-whiteout stil #:optional (style 'box) On 2016/01/03 23:14:22, thomasmorley651 wrote: I like it optional. But why do you give default-values for style and thickness? If you'd do: #:optional style thickness (line-thickness 0.1) then style and thickness default to #f, would be sufficient. Good point! I'll make that change. Though the doc-string needs to be changed then, maybe something at the lines of: [...] Please check spelling, grammar and wording, though. I'm not a native speaker Will do, and thanks for the text suggestions. See how you like the version in patch set 2. https://codereview.appspot.com/284980043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
scm/markup.scm comment out debugging code (issue 282890044 by paulwmor...@gmail.com)
Reviewers: , Message: The snippet below prints a list of markup commands to the log. It seems that a line of debugging code was not commented out in issue 4685: http://sourceforge.net/p/testlilyissues/issues/4685/ -Paul \version "2.19.34" \header { title = "anything" } { c } Description: scm/markup.scm comment out debugging code No need to display a list of markup commands when not debugging. Please review this at https://codereview.appspot.com/282890044/ Affected files (+1, -1 lines): M scm/markup.scm Index: scm/markup.scm diff --git a/scm/markup.scm b/scm/markup.scm index 5c38ae9ea503015b9217b994ee1cc72d8fa5dace..14a007a95bb27f8a89da2aeb51a607695af40068 100644 --- a/scm/markup.scm +++ b/scm/markup.scm @@ -113,7 +113,7 @@ following stencil. Stencils with empty Y extent are not given (string-join (list (car c) (string-cons-join (cdr c))) ""))) ;; We let the following line in for future debugging -(display-scheme-music (sort all-relevant-markup-commands symbol Remark: below only works, if markup?- or markup-list? arguments are the ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Add scheme procedure that gets paper column rank (issue 281020043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review. There is already a Paper_column::get_rank for C++ use, so I created a Scheme version alongside it, but I'm not sure if this is the best approach. I named the Scheme one "get_rank_number" to differentiate the two. Any suggestions are welcome. Background: I was working on a Scheme rewrite of ly:ledger-line-spanner::print, to allow for some local customizations, but it uses the rank of paper columns and currently there seems to be no way to access rank from scheme. Description: Add scheme procedure that gets paper column rank Please review this at https://codereview.appspot.com/281020043/ Affected files (+12, -0 lines): M lily/include/paper-column.hh M lily/paper-column.cc Index: lily/include/paper-column.hh diff --git a/lily/include/paper-column.hh b/lily/include/paper-column.hh index f1cb01e740685bcf11f9463e82a42466501d01f0..d4e68edc656438953c30633689aecab14bed41c4 100644 --- a/lily/include/paper-column.hh +++ b/lily/include/paper-column.hh @@ -49,6 +49,7 @@ public: DECLARE_SCHEME_CALLBACK (print, (SCM)); DECLARE_SCHEME_CALLBACK (before_line_breaking, (SCM)); + DECLARE_SCHEME_CALLBACK (get_rank_number, (SCM)); static int get_rank (Grob const *); static bool is_musical (Grob *); Index: lily/paper-column.cc diff --git a/lily/paper-column.cc b/lily/paper-column.cc index da3807c31d6f4ddb33fb75edf69c24ec3baf8175..f0b991780f81f1d5934642de1ab123f092f0bbdd 100644 --- a/lily/paper-column.cc +++ b/lily/paper-column.cc @@ -57,6 +57,17 @@ Paper_column::get_rank (Grob const *me) return dynamic_cast (me)->rank_; } +MAKE_DOCUMENTED_SCHEME_CALLBACK (Paper_column, get_rank_number, 1, + "Get the @code{rank number} of a" + " @code{PaperColumn} or" + " @code{NonMusicalPaperColumn}."); +SCM +Paper_column::get_rank_number (SCM grob) +{ + Grob *me = unsmob (grob); + return scm_from_int (Paper_column::get_rank (me)); +} + void Paper_column::set_rank (int rank) { ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add scheme procedure that gets paper column rank (issue 281020043 by paulwmor...@gmail.com)
Some digging turned up: Grob::spanned_rank_interval in grob.cc Spanner::spanned_rank_interval in spanner.cc Item::spanned_rank_interval in item.cc with the latter two being more illuminating. So this would indeed work and be much more generic. Now to figure out how and where to implement a Scheme version (or versions?)... https://codereview.appspot.com/281020043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add scheme procedure that gets paper column rank (issue 281020043 by paulwmor...@gmail.com)
On 2015/12/15 22:26:01, dak wrote: Not sure what you need it for. I was working on a Scheme rewrite of ly:ledger-line-spanner::print, to allow for some local customizations. It uses the rank of paper columns, namely, around line 210 in ledger-line-spanner.cc Direction vdir = Direction (sign (pos)); int rank = h->get_column ()->get_rank (); reqs[rank][vdir].ledger_extent_.unite (ledger_extent); reqs[rank][vdir].head_extent_.unite (head_extent); reqs[rank][vdir].position_ = vdir * max (vdir * reqs[rank][vdir].position_, vdir * pos); For each note head the code gets the rank of the note head's paper column as a way to effectively associate the note heads that belong to the same column... and thus calculate, for each column, the furthest note head position, how wide the ledger lines need to be, etc. Would it be more generic to provide spanned_rank_interval ()? That should be defined for every grob. Probably also for paper columns? I don't know enough about spanned_rank_interval to say for sure. (I looked it up in grob.cc) If it could be used to group note heads by paper column (as with paper column rank) then it should work and be more generic. https://codereview.appspot.com/281020043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add and use note-collision-threshold grob property (issue 271680043 by paulwmor...@gmail.com)
Please review, thanks. When StaffSymbol.staff-space is overridden to a value less than 1, collisions can occur between simultaneous notes that are more than one staff space apart. This patch introduces a property "note-collision-threshold" that makes it possible to prevent this. By setting this property to 2 or more, LilyPond will identify notes 2 or more staff positions apart as collisions and move the notes accordingly to prevent them. Basically, this patch un-hard-codes the assumption that collisions only occur when simultaneous notes are 1 staff space apart and allows the user to override this threshold with a different value when that is needed. See regtest for an example of use. -Paul https://codereview.appspot.com/271680043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc: CG - minor edits in CG 2.1 and CG 3.3 (issue 280810043 by paulwmor...@gmail.com)
Please review. I finally got around to some of these edits: http://lists.gnu.org/archive/html/lilypond-devel/2015-05/msg00065.html -Paul https://codereview.appspot.com/280810043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Implement rounded-box whiteout style (issue 274590043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review, thanks, -Paul Description: Implement rounded-box whiteout style As suggested in issue 4504. Includes edits to regression tests and changes entry. Please review this at https://codereview.appspot.com/274590043/ Affected files (+65, -25 lines): M Documentation/changes.tely M input/regression/whiteout.ly M input/regression/whiteout-lower-layers.ly M scm/define-grob-properties.scm M scm/define-markup-commands.scm M scm/stencil.scm Index: Documentation/changes.tely diff --git a/Documentation/changes.tely b/Documentation/changes.tely index bc0807624b36f11ceefc38ec9f3f99a3f94f8bc8..24e973e4fd4c89d5ac438acf377c8376593eb5c0 100644 --- a/Documentation/changes.tely +++ b/Documentation/changes.tely @@ -96,21 +96,30 @@ mus = \relative { c'4 cih d dih } @end lilypond @item -A new style of whiteout that approximates the contours of a glyph's -outline is now available using @code{whiteout-style}. The shape of the -white background is produced from multiple displaced copies of the -glyph. The @code{thickness} of both the new @code{outline} style and -the default @code{box} style, as a multiple of staff-line thickness, can -be customized. +Two new styles of whiteout are now available. The @code{outline} style +approximates the contours of a glyph's outline, and its shape is +produced from multiple displaced copies of the glyph. The +@code{rounded-box} style produces a rounded rectangle shape. For all +three styles, including the default @code{box} style, the whiteout +shape's @code{thickness}, as a multiple of staff-line thickness, can be +customized. + @lilypond[verbatim,quote] \markup { \combine \filled-box #'(-1 . 15) #'(-3 . 4) #1 -\override #'(thickness . 2) +\override #'(thickness . 3) \whiteout whiteout-box } \markup { \combine +\filled-box #'(-1 . 24) #'(-3 . 4) #1 +\override #'(style . rounded-box) +\override #'(thickness . 3) +\whiteout whiteout-rounded-box +} +\markup { + \combine \filled-box #'(-1 . 18) #'(-3 . 4) #1 \override #'(style . outline) \override #'(thickness . 3) Index: input/regression/whiteout-lower-layers.ly diff --git a/input/regression/whiteout-lower-layers.ly b/input/regression/whiteout-lower-layers.ly index 8549cbc783cde5c2ded54ea5550cc04749ca04ab..15c59e5770b04eee6afea06de97cfcbfec19078b 100644 --- a/input/regression/whiteout-lower-layers.ly +++ b/input/regression/whiteout-lower-layers.ly @@ -10,23 +10,35 @@ TimeSignature whites out the Tie but not the StaffSymbol. \relative { \time 3/4 - \override Staff.StaffSymbol.layer = #4 - \once \override Tie.layer = #2 + \override Staff.StaffSymbol.layer = 4 + \once \override Tie.layer = 2 b'2.~ \once \override Staff.TimeSignature.whiteout = ##t - \once \override Staff.TimeSignature.layer = #3 + \once \override Staff.TimeSignature.layer = 3 \time 5/4 b4 } \relative c' { \time 3/4 - \override Staff.StaffSymbol.layer = #4 - \once \override Tie.layer = #2 + \override Staff.StaffSymbol.layer = 4 + \once \override Tie.layer = 2 + b'2.~ + \once \override Staff.TimeSignature.whiteout-style = #'rounded-box + \once \override Staff.TimeSignature.whiteout = 3 + \once \override Staff.TimeSignature.layer = 3 + \time 5/4 + b4 +} + +\relative c' { + \time 3/4 + \override Staff.StaffSymbol.layer = 4 + \once \override Tie.layer = 2 b'2.~ \once \override Staff.TimeSignature.whiteout-style = #'outline - \once \override Staff.TimeSignature.whiteout = #3 - \once \override Staff.TimeSignature.layer = #3 + \once \override Staff.TimeSignature.whiteout = 3 + \once \override Staff.TimeSignature.layer = 3 \time 5/4 b4 } Index: input/regression/whiteout.ly diff --git a/input/regression/whiteout.ly b/input/regression/whiteout.ly index 7e10de9727be00498a693859cd563d77fe025808..543d94f369da04536a8fc13c3a88a0466871fac4 100644 --- a/input/regression/whiteout.ly +++ b/input/regression/whiteout.ly @@ -2,8 +2,9 @@ texidoc = "The whiteout command underlays a white background under a markup. The shape is determined by @code{whiteout-style}. The default -is @code{box} which produces a white rectangle. @code{outline} -approximates the outline of the markup." +is @code{box} which produces a rectangle. @code{rounded-box} produces +a rounded rectangle. @code{outline} approximates the outline of the +markup." } \version "2.19.32" @@ -25,7 +26,13 @@ approximates the outline of the markup." c c-\markup { \override #'(thickness . 3) -\override #'(whiteout-style . outline) +\override #'(style . rounded-box) +\whiteout foo + } + c + c-\markup { +\override #'(thickness . 3) +\override #'(style . outline) \whiteout foo } c Index: scm/define-grob-properties.scm diff --git a/scm/define-grob-properties.scm b/scm/define-grob-properties.scm index b387da206ea8bfeb304950075fe779f009b7e395..c50260d6c9672a29501338e777087e597ae3453a 100644 ---
Re: Issue 4504/5 update changes.tely (issue 275770043 by paulwmor...@gmail.com)
Thanks for the feedback. All fixed in patch set 2. -Paul https://codereview.appspot.com/275770043/diff/1/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/275770043/diff/1/Documentation/changes.tely#newcode72 Documentation/changes.tely:72: \\markup { Ah, thanks, and thanks for those links. I got tripped up by copying the example from define-markup-commands.scm where the double '\' is needed. Fixed in patch set 2. https://codereview.appspot.com/275770043/diff/1/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): https://codereview.appspot.com/275770043/diff/1/scm/define-grob-properties.scm#newcode1143 scm/define-grob-properties.scm:1143: (whiteout ,boolean-or-symbol? "If a number or true, the grob is On 2015/11/03 11:52:28, Trevor Daniels wrote: Shouldn't this be boolean-or-number> Yes, good catch, thanks. Fixed in patch set 2. (I had started to change to the "whiteout and whiteout-thickness" approach, but then changed it back, and in the process forgot to change these predicates back as well.) https://codereview.appspot.com/275770043/diff/1/scm/define-grob-properties.scm#newcode1149 scm/define-grob-properties.scm:1149: (whiteout-style ,number? "Determines the shape of the Yes, good catch, thanks. Fixed in patch set 2. https://codereview.appspot.com/275770043/diff/1/scm/define-grob-properties.scm#newcode1150 scm/define-grob-properties.scm:1150: @code{whiteout} background. Available are @code{outline} and the Fixed in patch set 2. https://codereview.appspot.com/275770043/diff/1/scm/define-grob-properties.scm#newcode1151 scm/define-grob-properties.scm:1151: default @code{box}.") Fixed in patch set 2. https://codereview.appspot.com/275770043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 4504/5 update changes.tely (issue 275770043 by paulwmor...@gmail.com)
Patch set 3 to removes the double quotes after '@end lilypond' in changes.tely. Should be all good to go now. -Paul https://codereview.appspot.com/275770043/diff/20001/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/275770043/diff/20001/Documentation/changes.tely#newcode90 Documentation/changes.tely:90: @end lilypond" On 2015/11/03 15:33:19, pkx166h wrote: Paul, sorry - I did comment on this previously but it didn't seem to get saved - there are unnecessary double quote marks here ('@end lilypond"') that need removing to allow make doc to succeed. Done, thanks for catching this. See patch set 3. https://codereview.appspot.com/275770043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 4504/5 update changes.tely (issue 275770043 by paulwmor...@gmail.com)
Thanks for your help. I made a test file and got it to compile without errors. https://codereview.appspot.com/275770043/diff/40001/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/275770043/diff/40001/Documentation/changes.tely#newcode88 Documentation/changes.tely:88: g1' On 2015/11/03 22:21:55, dak wrote: This needs to be g'1 instead of g1' . Yikes. How did I mess that up... https://codereview.appspot.com/275770043/diff/40001/Documentation/changes.tely#newcode91 Documentation/changes.tely:91: On 2015/11/03 21:45:03, pkx166h wrote: This snippet doesn't compile. I cannot work out why. Remember that this is all built using lilypond-book during doc compilation, so you can test any snippet you add by using a basic texinfo file layout and adding the TexInfo code: Thank you for showing me that; it's very helpful. I made a test file, corrected the g1' -> g'1, and fixed another problem. This: \override #'(whiteout-style . outline) should actually be this: \override #'(style . outline) (Originally I was using 'whiteout-style' for the markup command property, but changed it to just 'style' because other markup commands use 'style', but forgot to update it here. (groan...)) The good news is that with these fixes, and running it with the compiled version, everything went fine. Log output below. So I will submit another patch set with these changes. [build (whiteout-pad)]$ out/bin/lilypond-book --pdf mytest/test.tely lilypond-book (GNU LilyPond) 2.19.31 Reading mytest/test.tely... Running texi2pdf on file /tmp/tmpuL_Y2D.texi to detect default page settings. Dissecting... Writing snippets... Processing... Running lilypond... GNU LilyPond 2.19.31 Processing `./snippet-map-513811304.ly' Parsing... Processing `./63/lily-ffa13262.ly' Parsing... Interpreting music... Preprocessing graphical objects... Calculating line breaks... Drawing systems... Layout output to `./63/lily-ffa13262.eps'... Converting to `./63/lily-ffa13262.pdf'... Converting to PNG... Layout output to `./63/lily-ffa13262-1.eps'... Layout output to `./63/lily-ffa13262-2.eps'... Layout output to `./63/lily-ffa13262-3.eps'... Converting to `./63/lily-ffa13262-1.pdf'... Converting to `./63/lily-ffa13262-2.pdf'... Converting to `./63/lily-ffa13262-3.pdf'... Writing ./63/lily-ffa13262-systems.texi... Writing ./63/lily-ffa13262-systems.tex... Writing ./63/lily-ffa13262-systems.count... Success: compilation successfully completed Linking files... Compiling /home/paul/lilypond-git/build/test.texi... Writing `/home/paul/lilypond-git/build/test.texi'... [build (whiteout-pad)]$ https://codereview.appspot.com/275770043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 4504/5 update changes.tely (issue 275770043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review. This patch set implements the "whiteout and whiteout-style" grob property approach. Another approach ("whiteout and whiteout-thickness") has also been proposed. See issue tracker: https://sourceforge.net/p/testlilyissues/issues/4504/ Thanks, -Paul Description: Issue 4504/5 update changes.tely Issue 4504/4 edits to tests and docs Issue 4504/3 run scripts/auxiliar/update-with-convert-ly.sh Issue 4504/2 convert-ly rule: whiteout-box --> whiteout Issue 4504/1 add whiteout-style, drop whiteout-box Introduce whiteout-style grob property, with options of 'box and 'outline, to be used with the whiteout grob property, and an equivalent style property for the whiteout markup command. Remove the whiteout-box grob property and markup command, and use the new properties instead. Make the box style of whiteout the default style again, as it was before issue 4418. Please review this at https://codereview.appspot.com/275770043/ Affected files (+127, -100 lines): M Documentation/changes.tely M Documentation/fr/notation/rhythms.itely M Documentation/notation/rhythms.itely M Documentation/snippets/blanking-staff-lines-using-the--whiteout-command.ly M Documentation/snippets/new/using-the-whiteout-property.ly M Documentation/snippets/using-the-whiteout-property.ly M input/regression/markup-syntax.ly M input/regression/whiteout.ly M input/regression/whiteout-lower-layers.ly M lily/grob.cc M lily/lily-imports.cc M python/convertrules.py M scm/define-grob-properties.scm M scm/define-grobs.scm M scm/define-markup-commands.scm M scm/stencil.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc: Usage - Updated 'Running LilyPond' intros (issue 261240043 by pkx1...@gmail.com)
A couple of suggestions, but LGTM. https://codereview.appspot.com/261240043/diff/20001/Documentation/usage/running.itely File Documentation/usage/running.itely (right): https://codereview.appspot.com/261240043/diff/20001/Documentation/usage/running.itely#newcode43 Documentation/usage/running.itely:43: configurable options to be applied to the output. LilyPond also comes Since Frescobaldi has an "Engrave (custom)" function which allows the use of these command-line options, basically a command-line embedded in a GUI, maybe reword this as: "allows configurable options to be applied to the output." https://codereview.appspot.com/261240043/diff/20001/Documentation/usage/running.itely#newcode46 Documentation/usage/running.itely:46: available via the command-line. Since these helper programs are also available in Frescobaldi, maybe reword this last bit to something like: "which are available via the command-line." https://codereview.appspot.com/261240043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 4504: Let whiteout-box take a number argument (issue 264810043 by paulwmor...@gmail.com)
Pushed: authorPaul Morris paulwmor...@gmail.com Sun, 23 Aug 2015 02:32:42 + (22:32 -0400) committerJames Lowe pkx1...@gmail.com Sun, 30 Aug 2015 07:33:48 + (08:33 +0100) commita3fc862086a720201e4a4764c7beb952fab5f106 https://codereview.appspot.com/264810043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 4504: Let whiteout-box take a number argument (issue 264810043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review. This patch allows whiteout-box to take a number argument. A boolean argument is still possible as well. This brings it into parity with whiteout for both grobs and markups. This is a preliminary step needed for the further changes described in issue 4504. Description: Issue 4504: Let whiteout-box take a number argument This allows the size of the box whiteout style to be customized, for grobs and markups. Boolean arguments are still possible as well. Please review this at https://codereview.appspot.com/264810043/ Affected files (+28, -16 lines): M lily/grob.cc M scm/define-grob-properties.scm M scm/define-markup-commands.scm M scm/stencil.scm Index: lily/grob.cc diff --git a/lily/grob.cc b/lily/grob.cc index a9c9532c7181bbdadc461527dcd8b140a7000f07..369135d2b26a49446e692b4446e4cda3b7202bbf 100644 --- a/lily/grob.cc +++ b/lily/grob.cc @@ -160,10 +160,14 @@ Grob::get_print_stencil () const } /* Calls the scheme procedure stencil-whiteout-box in scm/stencils.scm */ - if (!transparent to_boolean (get_property (whiteout-box))) + if (!transparent (scm_is_number (get_property(whiteout-box)) + || to_boolean (get_property (whiteout-box { + Real thickness = robust_scm2double (get_property(whiteout-box), 0.0) + * layout ()-get_dimension (ly_symbol2scm (line-thickness)); retval = *unsmobStencil -(Lily::stencil_whiteout_box (retval.smobbed_copy ())); +(Lily::stencil_whiteout_box (retval.smobbed_copy (), + scm_from_double (thickness))); } if (transparent) Index: scm/define-grob-properties.scm diff --git a/scm/define-grob-properties.scm b/scm/define-grob-properties.scm index 8649a67117190ed03523651b06dd0002d5e16859..fc8c86c5a3241095e2eaf408abccd3130442561a 100644 --- a/scm/define-grob-properties.scm +++ b/scm/define-grob-properties.scm @@ -1146,9 +1146,11 @@ if the grob is visible. A number sets the thickness of the outline as a multiple of the staff-line thickness. For compatibility with former behavior (now available with @code{whiteout-box}) the value @code{#t} is treated as @code{3.0}. Usually @code{#f} by default.) - (whiteout-box ,boolean? If true, the grob is printed over a -rounded rectangular white background to white-out underlying material, -if the grob is visible. Usually @code{#f} by default.) + (whiteout-box ,boolean-or-number? If a number or true, the grob +is printed over a rectangular white background to white-out underlying +material, if the grob is visible. A number indicates how far the +outline extends beyond the bounding box of the grob as a multiple of +the staff-line thickness. Usually @code{#f} by default.) (width ,ly:dimension? The width of a grob measured in staff space.) (word-space ,ly:dimension? Space to insert between words in Index: scm/define-markup-commands.scm diff --git a/scm/define-markup-commands.scm b/scm/define-markup-commands.scm index 363977da9be00b291770bfaddf504749751bebfe..561481cbab9ea941286479134a630b2b5fc5e370 100644 --- a/scm/define-markup-commands.scm +++ b/scm/define-markup-commands.scm @@ -734,19 +734,23 @@ Provide a white background for @var{arg}. (define-markup-command (whiteout-box layout props arg) (markup?) #:category other + #:properties ((thickness 0)) -@cindex adding a rounded rectangular white background to text +@cindex adding a rectangular white background to text -Provide a rounded rectangular white background for @var{arg}. +Provide a rectangular white background for @var{arg}. @lilypond[verbatim,quote] \\markup { \\combine \\filled-box #'(-1 . 10) #'(-3 . 4) #1 -\\whiteout-box whiteout-box +\\override #'(thickness . 1.5) \\whiteout-box whiteout-box } @end lilypond - (stencil-whiteout-box (interpret-markup layout props arg))) + (stencil-whiteout-box +(interpret-markup layout props arg) + (* thickness +(ly:output-def-lookup layout 'line-thickness (define-markup-command (pad-markup layout props amount arg) (number? markup?) Index: scm/stencil.scm diff --git a/scm/stencil.scm b/scm/stencil.scm index f73b08163052b0567d0f2628887bbd8bd552dd19..fb2809e9fd2fee40c1d98660f7f10ac5ccf4711d 100644 --- a/scm/stencil.scm +++ b/scm/stencil.scm @@ -737,15 +737,17 @@ we make between 0 and 2*pi. `(delay-stencil-evaluation ,(delay whiteout-expr))) stil) -(define-public (stencil-whiteout-box stencil) +(define*-public (stencil-whiteout-box stencil + #:optional (thickness 0) (blot 0) (color white)) + @var{thickness} is how far in staff-spaces the white outline +extends past the extents of @var{stencil}. (let* - ((x-ext (ly:stencil-extent stencil X)) - (y-ext (ly:stencil-extent stencil Y))) + ((x-ext (interval-widen (ly:stencil-extent stencil X) thickness)) +
Re: \acceptance for letting one context-def share accepts for another (issue 260800043 by d...@gnu.org)
On 2015/08/18 14:46:02, dak wrote: Well, at least \acceptability would seem nicer than \acceptances. \inherit-acceptability would be somewhat descriptive but it is awkward in the sorting order and rather verbose. Hmmm... \inherit-acceptability MyStaff Staff is pretty good, if you ask me. Easy to understand what it's used for and the order of arguments is fairly clear. I could see \copy-acceptability Staff MyStaff working as well. One thing that helps is this command likely won't be typed all that frequently, so arguably concision/verbosity is less important. https://codereview.appspot.com/260800043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: \acceptance for letting one context-def share accepts for another (issue 260800043 by d...@gnu.org)
I brainstormed some ideas: \copy-acceptability \copy-acceptance \copy-acceptances \acceptable-where \accepts-alias \acceptability-alias ...but I'm not sure any of those are better than the ones mentioned already. FWIW, I like how it is easier for me to understand what the command does with \accept-equally or \accept-like, than with \acceptances. Seems there are trade-offs with any option. https://codereview.appspot.com/260800043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: \acceptance for letting one context-def share accepts for another (issue 260800043 by d...@gnu.org)
LGTM. This will make using custom modified contexts much simpler, thanks! My only suggestion is that the plural form of acceptances sounds odd if there are more than one: \layout { % ... \acceptances MyStaff Staff \acceptances MyLyrics Lyrics } What about \accept-like instead? \layout { % ... \accept-like Staff MyStaff \accept-like Lyrics MyLyrics } This is the name you suggested (for a different approach) here: http://lists.gnu.org/archive/html/bug-lilypond/2015-03/msg00020.html I find it clearer and easier to understand than \acceptances. https://codereview.appspot.com/260800043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)
Please review. Per feedback I narrowed and tested the convert-ly rule and edited the changes entry. (I also redid the commits for better commit history.) -Paul https://codereview.appspot.com/236480043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)
https://codereview.appspot.com/236480043/diff/60001/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/236480043/diff/60001/Documentation/changes.tely#newcode66 Documentation/changes.tely:66: now create a white background that follows the outline of the On 2015/06/07 20:43:54, Keith wrote: Better to be honest that follows the outline of - built from multiple displaced copies of how about built from multiple displaced copies of the glyph in order to approximate the contours of its outline. That indicates both what's better about the new behavior and what's actually happening to achieve it. https://codereview.appspot.com/236480043/diff/60001/python/convertrules.py File python/convertrules.py (right): https://codereview.appspot.com/236480043/diff/60001/python/convertrules.py#newcode3766 python/convertrules.py:3766: str = re.sub (r\\whiteout(?!-box), r\\whiteout-box, str) On 2015/06/07 20:43:54, Keith wrote: Somebody has probably defined \whiteout-custom or similar so you should match more narrowly \\whiteout(?![a-z_-]) Ok, sounds good. https://codereview.appspot.com/236480043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)
Just uploaded patch set 4. It includes improved convert-ly rules. I ran scripts/auxiliar/update-with-convert-ly.sh with these rules on both a clean branch and on the branch where I'm working on this functionality. It worked as expected for both. This patch set also includes a changes entry and addresses Keith's comments from the last set. I had to update one snippet manually (Using the whiteout property) to update the doc string to whiteout-box. I then copied it to the snippets/new directory and ran the snippet update script, all as described in the CG. It seems like that went as it should. Thanks for reviewing, -Paul https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py File python/convertrules.py (right): https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py#newcode3751 python/convertrules.py:3751: str = re.sub (r'\bwhiteout\b', 'whiteout-box', str) On 2015/06/02 05:00:30, Keith wrote: Separate rules or a pattern that catches the prefixes-- whatever works to update the LilyPond input without changing comments about the whiteout. And be a bit defensive against cases that you can imagine. You do not want to convert whiteout-box - whiteout-box-box, and to be thorough you might want to catch uses with no spaces like \markup\whiteout1 , although with the new \whiteout being an improvement over the old you can err on the side of being conservative. Scheme functions that do not appear in the docs, such as 'stencil-whiteout', probably do not need convert-ly rules, but if the conversion is easy to do without false-positives it should be fine. Ok, thanks, fixed in next patch set (4th). https://codereview.appspot.com/236480043/diff/40001/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): https://codereview.appspot.com/236480043/diff/40001/scm/define-grob-properties.scm#newcode1147 scm/define-grob-properties.scm:1147: equivalent to a value of 3. Usually #f by default.) On 2015/06/02 05:00:30, Keith wrote: The 'boolean-or-number' parameter made sense for getting two types of behavior through one parameter, but it is unusual to have #t be a synonym for 3. '3' is easier to type than '##t'. If this is a backward-compatibility help for people whose fingers know \override Xx #'whiteout = ##t , it would be less mysterious to say For compatibility with former behavior (now available with @code{whiteout-box}) the value #t is treated as 3.0. Ok, I've fixed the wording in the next patch set (4th). I was thinking about backwards compatibility like you describe and also that it's easier on users if they can get a sensible default with ##t without having to remember a number and what the units are. Also for parity with whiteout-box which accepts ##t for its default (and may take a number in the future to customize its outline thickness). Isn't it just multiple of the staff-line thickness? That is, if you #(set-global-staff-size 5) doesn't the white-out stay relatively thicker, to match the relatively thicker staff-lines? Yep, done. https://codereview.appspot.com/236480043/diff/40001/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): https://codereview.appspot.com/236480043/diff/40001/scm/define-markup-commands.scm#newcode710 scm/define-markup-commands.scm:710: #:properties ((thickness 4)) On 2015/06/02 05:00:30, Keith wrote: I just guessed the number 3 staffline thicknesses, and apparently typo'd it as 4. If you like that default, fine. Ok, I tried both and went with 3 in the next patch set (4th). https://codereview.appspot.com/236480043/diff/40001/scm/define-markup-commands.scm#newcode720 scm/define-markup-commands.scm:720: \\whiteout whiteout On 2015/06/02 05:00:30, Keith wrote: \\override #'(thickness . 1.5) \\whiteout whiteout otherwise there is no indication of what thickness does, and the default thickness around 'whiteout' looks jagged on the black background. Done. https://codereview.appspot.com/236480043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)
On 2015/05/31 01:07:47, Keith wrote: I realize I am changing my opinion here on a few points: 1) leave the old whiteout as a simple box and give the new function a new name 2) enabling either type of whiteout through a single property looks better 3) the thickness should be in terms of line-thickness rather than staff-space I hope you don't mind figuring out which of these were the wiser opinions. I'll just say this all looks good to me. Ok, see below. On 2015/05/29 04:03:06, http://paul_paulwmorris.com wrote: Question: should the whiteout property be a number rather than a boolean, so that it sets the thickness of the white outline (and #0 replaces ##f)? Or should the whiteout property accept either a number or a boolean? Yes, and more yes to the second option. There is a type-checking predicate 'boolean-or-symbol?' from which you could make the analogous 'boolean-or-number?'. Ok, sounds good. Existing uses of Xx.whiteout=##t could get the simple box, while Xx.whiteout=1 gives the outline style. I suspect the users who want the outline will often want to adjust the thickness. I see the appeal of having just one property and avoiding a convert-ly rule. It does seem a little odd if the kind of whiteout changes whether the property is a number or boolean (but that may just be a matter of getting used to it). One potential problem is if we wanted to allow the old box whiteout function to also take an optional thickness argument to provide some padding for the whiteout box around its grob/stencil. I don't see a good way to do that with one property, but if there were two properties then it would be easy. Most thicknesses are in terms of the staff-line thickness. Using this unit, the range of thicknesses for which the function was intended, and works best without excessive copies of the stencil, are simple numbers \override Xx.whiteout = 4 %{ staffline thicknesses, rather than =#0.3 staff-spaces %} Also, the whiteout border will more likely do the right thing if it scales with the staffline thickness, rather than staff-space, when people scale the output to make a pocket score for example. Inconveniently for this patch, the stencil-translate works in units of staff-spaces, and by design the stencil language keeps some things in units of line-thickness, others in staff-spaces. Each grob or markup is built for a particular layout, so we can convert units where the grob or markup asks for whiteout (see below). Using staffline thickness does seem like the better approach. I need to look closer at the implementation details. (A convert-ly rule may be needed… maybe in any case to change whiteout to whiteout-box? ...or maybe not?) Looking at the existing uses of \whiteout Xxx.whiteout, it does seem best to leave the plain box whiteout in existing scores. Convert-ly to \whiteout-box one way to preserve the behavior, but it would be better to leave the old markup function \whiteout for the old behavior, and add \whiteout-outline. Here I'd prefer a convert-ly rule for \whiteout -- \whiteout-box, and then we can use \whiteout for the new outline style whiteout. To sum up, you're suggesting: Grob Property: whiteout (T/F for old box style, numbers for new outline style) Markup functions: \whiteout (old box style) \whiteout-outline (new style) SCM Functions: stencil-whiteout (old box style) stencil-whiteout-outline (new style) Convert-ly: none But I think I prefer the following which has a consistent parallel structure across all the categories and where the new style of whiteout is named in a way that conveys that it is the default, with the old box style getting a more specialized name. This requires convert-ly rules and for users to learn the new names, but I think it's worth it. Grob Properties: whiteout (new style, T/F or numbers) whiteout-box (old style, T/F, could allow numbers for padding) Markup functions: \whiteout (new style) \whiteout-box (old style) SCM Functions: stencil-whiteout (new style) stencil-whiteout-box (old style, optional number arg for padding) Convert-ly: markups: \whiteout -- \whiteout-box grob properties: whiteout -- whiteout-box (Hmmm... seems a convert-ly rule that would replace all whiteout with whiteout-box would do the trick here for grob properties, markup functions, and scm functions.) https://codereview.appspot.com/236480043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)
https://codereview.appspot.com/236480043/diff/20001/Documentation/notation/rhythms.itely File Documentation/notation/rhythms.itely (right): https://codereview.appspot.com/236480043/diff/20001/Documentation/notation/rhythms.itely#newcode555 Documentation/notation/rhythms.itely:555: collide with other objects in a staff. On 2015/05/31 01:07:46, Keith wrote: At first I thought the docs should use the new outline form of whiteout, but here I see that the box is perfect for this case. (The text should say .. properties of objects that should cause a gap in ties. Later we might build this into the grob definitions.) Ok, I'll make that change to the text. https://codereview.appspot.com/236480043/diff/20001/lily/grob.cc File lily/grob.cc (right): https://codereview.appspot.com/236480043/diff/20001/lily/grob.cc#newcode162 lily/grob.cc:162: On 2015/05/31 01:07:46, Keith wrote: We could let \override Xx.whiteout = 3 give a white outline 3 times the staffline thickness, by looking up the staffline thickness here. if scm_is_number(get_property(whiteout) { SCM wh_proc = y_lily_module_constant (stencil-whiteout-outline); Real thickness = robust_scm2double (get_property(whiteout), 3.0) * layout ()-get_dimension (ly_symbol2scm (line-thickness)); retval = *unsmobStencil( scm_call_2(wh_proc, retval.smobbed_copy, thickness)); } Ok, thanks for this code. I'll work it into the next patch set. https://codereview.appspot.com/236480043/diff/20001/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): https://codereview.appspot.com/236480043/diff/20001/scm/define-markup-commands.scm#newcode722 scm/define-markup-commands.scm:722: (stencil-whiteout (interpret-markup layout props arg))) On 2015/05/31 01:07:47, Keith wrote: Whatever the name for \whiteout-outline you can let it read the 'thickness' property, to use the interface for optional arguments that we have in the markup language g^\markup \override #'(thickness . 5) \whiteout-outline hello (define-markup-command (whiteout-outline layout props arg) (markup?) #:category other #:properties ((thickness 4)) (stencil-whiteout-outline (interpret-markup layout props arg) (* thickness (ly:output-def-lookup layout 'line-thickness Sounds good. Thanks for the code to do this. https://codereview.appspot.com/236480043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)
New patch set uploaded for review (3rd one): https://codereview.appspot.com/236480043 Please review, especially the convert-ly rule, grob.cc, and doc strings. Includes commits: convert-ly: add whiteout - whiteout-box rhythms.itely: improve text for whiteout-box let \whiteout use the thickness property make whiteout grob property boolean-or-number? grob.cc: allow numbers in whiteout property stencil.scm: fix typo add boolean-or-number? predicate https://codereview.appspot.com/236480043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)
I updated convertrules.py as follows: @rule ((2, 19, 22), whiteout - whiteout-box) def conv(str): str = re.sub (r\whiteout, r\whiteout-box, str) str = re.sub (r.whiteout, r.whiteout-box, str) str = re.sub (r#'whiteout, r#'whiteout-box, str) str = re.sub (rstencil-whiteout, rstencil-whiteout-box, str) return str but when I run scripts/auxiliar/update-with-convert-ly.sh I get this: make: *** No rule to make target `python-modules'. Stop. xargs: ./out/bin/convert-ly: No such file or directory xargs: ./out/bin/convert-ly: No such file or directory I'm running LilyDev3 in a virtual machine. (Feeling like I'm in over my head with this one.) https://codereview.appspot.com/236480043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)
https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py File python/convertrules.py (right): https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py#newcode3751 python/convertrules.py:3751: str = re.sub (r'\bwhiteout\b', 'whiteout-box', str) On 2015/06/01 23:10:20, dak wrote: That seems like a rather overenthusiastic conversion pattern. Ah yes, good point. Did you actually try running scripts/auxiliar/update-with-convert-ly.sh with this? Um... well, no... I got in a hurry and didn't follow the CG thoroughly enough. Let me give it another try. (I had actually already edited all occurrences manually (except for snippets), and so wasn't thinking about convert-ly being run on the documentation. I now see the recommended way is to start with convert-ly and then follow up with any needed manual edits.) I suppose the most robust approach would be to have more than one rule to match: \whiteout for the markup command .whiteout for grob property overrides #'whiteout for the older property syntax stencil-whiteout for the scheme function. https://codereview.appspot.com/236480043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)
On 2015/05/25 13:06:26, Valentin Villenave wrote: I’ve always been unhappy with the current method for whiteout (which I have also been using in rounded-box-stencil, nevertheless). LilyPond scores’ background is _not_ white, but alpha transparent. Look for example at the way Frescobaldi displays PDF scores by default, or at the picture attached to http://lists.gnu.org/archive/html/lilypond-user/2015-05/msg00522.html So, ideally \whiteout should mimic this behavior rather than cover stuff with a layer of white paint. Not sure how to do that, though. Me neither... In any case, this seems like a separate issue. https://codereview.appspot.com/236480043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)
On 2015/05/25 22:24:01, Keith wrote: The additional copies of the stencils do make the PDF files noticeably larger. 8% larger when I used 16-copy whiteout around all dynamics and text marks in a string quartet score. The compilation time increased by 2%, desipte the repeated computations of sines and cosines. With \pointAndClickOn, the file-size increased by 30% when putting the new white-out on dynamics and text marks. Each copy of the stencil gets a grob-cause mark, and these take up a lot of space. If we use this for the built-in Xx.whiteout=##t we would have to re-order the operations in grob.cc:get_print_stencil() so that the 'whiteout option is applied before 'color and 'grob-cause are added. Thanks for looking closer at the performance and file size implications. Sounds like editing the C++ is the way to minimize these effects. https://codereview.appspot.com/236480043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)
Thanks for the reviews. On 2015/05/24 22:56:44, Keith wrote: I think whiteout-stencil should simply call this function, with the defaults suggested, so that the existing \set Xx.whiteout=##t and \markup\whiteout use this new method. We could provide access to the old whiteout method with \set Xx.block-whiteout and \markup\block-whiteout for the former extent-based whiteout. Sounds good to me. In that case... would it make sense to name this new function stencil-whiteout so that it replaces the current function, and then the current function could be renamed stencil-box-whiteout or similar? Then we would have: \set Xx.box-whiteout \markup \box-whiteout I like box rather than block since block can more easily be misread as a verb and box is shorter. https://codereview.appspot.com/236480043/diff/1/scm/stencil.scm File scm/stencil.scm (right): https://codereview.appspot.com/236480043/diff/1/scm/stencil.scm#newcode700 scm/stencil.scm:700: (angle-increments 12) (radial-increments 3)) On 2015/05/24 22:56:43, Keith wrote: I am so far happy testing (angle-increments 16) (radial-increments 1) with 16 copies instead of 36. I tried this out, and found it to work well, even on glyphs with thin lines like treble clef and time signatures. https://codereview.appspot.com/236480043/diff/1/scm/stencil.scm#newcode749 scm/stencil.scm:749: (ly:stencil-extent stil Y)) On 2015/05/24 22:56:43, Keith wrote: The whiteout-stencil still affects the position of objects outside the staff, because LilyPond uses the outlines stencils (not extents) for several spacing tasks. There are other situations (\page-ref and \with-dimensions) where we need one stencil for spacing, and another for final printing, so we delay the formation of the final-printing stencil. That method seems appropriate here as well: (let ((whiteout-expr (ly:stencil-expr (stencil-with-color (radial-plot offset stil empty-stencil) color (ly:stencil-add (ly:make-stencil `(delay-stencil-evaluation ,(delay whiteout-expr))) stil) Thanks. I was not aware of this, but glad there's an easy fix. https://codereview.appspot.com/236480043/diff/1/scm/stencil.scm#newcode749 scm/stencil.scm:749: (ly:stencil-extent stil Y)) On 2015/05/25 22:24:01, Keith wrote: When I tried making stencil-whitéout' call the new improved function, I had trouble with \override Staff.Clef.whiteout = ##t \override Staff.Clef.color = #red the whiteout was also red because the code in grob.cc applies the color before handing the stencil to stencil-whiteout-outline, and in composed stencils with color applied at different levels, the innermost 'color applies. I added code to remove any 'color at top-level so I can continue using this. Ideally we would want code to remove any 'color from the stencil before using it as whiteout, but it might be more appropriate to apply 'whiteout before 'color in grob.cc. I am not that comfortable working in the C++ side of things, so if we go that route (and that sounds like the better approach) I won't be much help. https://codereview.appspot.com/236480043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)
Reviewers: , Message: Please review, thanks. See discussion here for background and usage demo: http://lists.gnu.org/archive/html/lilypond-user/2015-05/msg00522.html I have not yet done a regtest, but will add one if this is reviewed favorably for inclusion in LilyPond. Description: add stencil-whiteout-outline function clean up code formatting in stencil-whiteout Please review this at https://codereview.appspot.com/236480043/ Affected files (+55, -5 lines): M scm/stencil.scm Index: scm/stencil.scm diff --git a/scm/stencil.scm b/scm/stencil.scm index 03259cc3f53e90a9e1057a1e84a128964d003730..948ee221bc55b60d14413620a566f6c1dc3c427e 100644 --- a/scm/stencil.scm +++ b/scm/stencil.scm @@ -688,15 +688,65 @@ box, remains the same. (define-public (stencil-whiteout stencil) (let* ((x-ext (ly:stencil-extent stencil X)) - (y-ext (ly:stencil-extent stencil Y)) - - ) + (y-ext (ly:stencil-extent stencil Y))) (ly:stencil-add (stencil-with-color (ly:round-filled-box x-ext y-ext 0.0) white) - stencil) -)) + stencil))) + +(define*-public (stencil-whiteout-outline + stil #:optional (thickness 0.3) (color white) + (angle-increments 12) (radial-increments 3)) + This function works by creating a series of white or @var{color} +stencils radially thickness from the original stencil with angles from +0 to 2*pi, at an increment of @code{angle-inc}, and with radii +from @code{radial-inc} to @var{thickness}. @var{thickness} is how big +the white outline is in staff-spaces. @var{radial-increments} is how +many copies of the white stencil we make on our way out to thickness. +@var{angle-increments} is how many copies of the white stencil +we make between 0 and 2*pi. + (if (or (not (positive? angle-increments)) + (not (positive? radial-increments))) + (begin + (ly:warning Both angle-increments and radial-increments must be positive numbers.) + stil) + (let* ((2pi 6.283185307) + (angle-inc (/ 2pi angle-increments)) + (radial-inc (/ thickness radial-increments))) + +(define (circle-plot ang dec radius original-stil new-stil) + ;; ang (angle) and dec (decrement) are in radians, not degrees + (if (= ang 0) + new-stil + (circle-plot (- ang dec) dec radius original-stil +(ly:stencil-add + new-stil + (ly:stencil-translate original-stil + (cons +(* radius (cos ang)) +(* radius (sin ang + +(define (radial-plot radius original-stil new-stil) + (if (= radius 0) + new-stil + (ly:stencil-add new-stil +(radial-plot + (- radius radial-inc) + original-stil + (circle-plot 2pi angle-inc + radius original-stil empty-stencil) + +(let ((whiteout-stil + (ly:stencil-add +(stencil-with-color + (radial-plot thickness stil empty-stencil) + color) +stil))) + (ly:make-stencil + (ly:stencil-expr whiteout-stil) + (ly:stencil-extent stil X) + (ly:stencil-extent stil Y)) (define-public (arrow-stencil-maker start? end?) Return a function drawing a line from current point to @code{destination}, ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add stencil-flip function (issue 235090043 by paulwmor...@gmail.com)
https://codereview.appspot.com/235090043/diff/60001/input/regression/stencil-flip.ly File input/regression/stencil-flip.ly (right): https://codereview.appspot.com/235090043/diff/60001/input/regression/stencil-flip.ly#newcode1 input/regression/stencil-flip.ly:1: \version 2.19.19 On 2015/05/07 04:44:53, lemzwerg wrote: I would rename the file to `flip-stencil.ly' now. And everywhere you need s/stencil-flip/flip-stencil/ of course. Ah, of course! Done. https://codereview.appspot.com/235090043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add stencil-flip function (issue 235090043 by paulwmor...@gmail.com)
Here's another round of edits, also: WARNING: could not change issue labels; please email lilypond-devel with the issue number: 4370 No permission to edit issue Thanks, -Paul https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm File scm/stencil.scm (right): https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode665 scm/stencil.scm:665: (define-public (stencil-flip axis stil) On 2015/05/06 23:51:16, thomasmorley651 wrote: I'd call it 'flip-stencil', would be more in line with the majority of namings here in stencil.scm Done. I was thinking of ly:stencil-rotate, but you're right most of the names in this file are the other way around, so lets go with that. https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode668 scm/stencil.scm:668: An @var{axis} of Y or 1 will flip it vertically. On 2015/05/06 06:01:30, lemzwerg wrote: What about: Value @code{X} (or @code{0}) for @var{axis} flips it horizontally. Value @code{Y} (or @code{1}) flips it vertically. Exactly defined values for a variable should be represented with `@code'. And I'm a great fan of avoiding future tense in documentation :-) Done. https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode668 scm/stencil.scm:668: An @var{axis} of Y or 1 will flip it vertically. On 2015/05/06 23:51:15, thomasmorley651 wrote: Why not simply delete these two lines. I feel they are pretty much redundant. Well Werner requested the horizontal and vertical language so I'll leave these lines in. I'm ok with erring on the side of a little redundancy for the sake of clarity. https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode669 scm/stencil.scm:669: @var{stil} is flipped in place. Its position, the On 2015/05/06 06:01:30, lemzwerg wrote: I suggest s/./;/ Done. https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode670 scm/stencil.scm:670: X and Y coordinates of its bounding box, remains the same. On 2015/05/06 23:51:16, thomasmorley651 wrote: I'd delete X and Y in order not to lead to confusing with the meaning of X- and Y-axis in LilyPond Done. https://codereview.appspot.com/235090043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add stencil-flip function (issue 235090043 by paulwmor...@gmail.com)
Thanks for the feedback! I've improved the doc strings, and revised the doc string for ly:stencil-scale so that it covers negative arguments. -Paul https://codereview.appspot.com/235090043/diff/1/input/regression/stencil-scale.ly File input/regression/stencil-scale.ly (right): https://codereview.appspot.com/235090043/diff/1/input/regression/stencil-scale.ly#newcode8 input/regression/stencil-scale.ly:8: its bounding box.) On 2015/05/05 03:53:05, lemzwerg wrote: Please use two spaces after a period that ends a sentence. Thanks for the reminder. https://codereview.appspot.com/235090043/diff/1/input/regression/stencil-scale.ly#newcode8 input/regression/stencil-scale.ly:8: its bounding box.) On 2015/05/05 04:16:37, Carl wrote: I don't think information about stencil-flip should be put in this regtest, unless stencil-flip is also added to the regtest. The headers of the regtests should only discuss the particular regtest, in my opinion. Makes sense, and done. https://codereview.appspot.com/235090043/diff/1/scm/stencil.scm File scm/stencil.scm (right): https://codereview.appspot.com/235090043/diff/1/scm/stencil.scm#newcode467 scm/stencil.scm:467: (is-absolute (if (memq head-raw On 2015/05/05 04:16:38, Carl wrote: Probably a nice touch, since absolute? is probably not really a predicate. Yep, just cleaning up after myself here. https://codereview.appspot.com/235090043/diff/1/scm/stencil.scm#newcode667 scm/stencil.scm:667: @var{axis} is 0 for X-axis, 1 for Y-axis. On 2015/05/05 04:16:38, Carl wrote: Perhaps you could also use X and Y instead of 0 and 1 (see the code below where you use X) Done. https://codereview.appspot.com/235090043/diff/1/scm/stencil.scm#newcode667 scm/stencil.scm:667: @var{axis} is 0 for X-axis, 1 for Y-axis. On 2015/05/05 03:53:05, lemzwerg wrote: Can you somehow add the words `horizontally' and `vertically'? Done. Additionally, I think it helps to tell the reader where the mirroring axis is positioned. I think I have this covered now, although with different wording. https://codereview.appspot.com/235090043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add stencil-flip function (issue 235090043 by paulwmor...@gmail.com)
Please review, thanks. https://codereview.appspot.com/235090043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: absolute pitch entry: accept an offset octave (issue 235010043 by k-ohara5...@oco.net)
On 2015/05/03 20:04:28, dak wrote: There is no such thing as '' or s'' as a recognizable element in LilyPond's syntax and there is not even a tangible representation of it in Scheme. I don't think that such a feature warrants both messing with the parser as well as introducing new Scheme entities. So at least for me, those proposals are non-starters. Good points. I was only thinking about what might be best from a user's perspective and hadn't considered feasibility. I agree that this wouldn't warrant such invasive changes. (Although, it's too bad this won't work since only the octave is relevant in Keith's patches... I haven't thought through the other proposals on the table.) https://codereview.appspot.com/235010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: absolute pitch entry: accept an offset octave (issue 235010043 by k-ohara5...@oco.net)
On 2015/05/03 13:23:15, Dan Eble wrote: If people don't like numbers (I'm with you) are there any other feasible ways to indicate an octave only? Maybe bare ''' and ,,,? Maybe with an unpitched name? \absolute '' { a b c } \absolute s'' { a b c } I was thinking along the same lines. It's simpler for users if they can indicate the (absolute) octave using ' and , in the way the always do. It would be easy to see that the following would be the same: \absolute '' { a b c } \absolute s'' { a b c } \absolute { a'' b'' c'' } https://codereview.appspot.com/235010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel