Reviewers: carl.d.sorensen_gmail.com, dak,
http://codereview.appspot.com/5464045/diff/2001/Documentation/snippets/three-sided-box.ly File Documentation/snippets/three-sided-box.ly (right): http://codereview.appspot.com/5464045/diff/2001/Documentation/snippets/three-sided-box.ly#newcode20 Documentation/snippets/three-sided-box.ly:20: #(use-modules (scm markup-facility-defs)) On 2011/12/19 20:52:56, dak wrote:
I don't think we should require this use-modules in a normal document.
Is there
a reason we can't just preload the module in the course of normal
Lilypond
initialization?
This change in this file is cruft after the validation fix in the markup module. Removing. http://codereview.appspot.com/5464045/diff/2001/Documentation/snippets/three-sided-box.ly#newcode34 Documentation/snippets/three-sided-box.ly:34: (define (NWS-box-stencil stencil thickness padding) On 2011/12/19 20:52:56, dak wrote:
Does this reorganization mean that a markup command must not use a
command
defined before it in a file?
I don't think we can explain this to users in a satisfactory way.
You could do it either way, pre-patch or post-patch. As I had the patch before in patchset 1, the original NWS-box-stencil declaration would have been declared in the current Lilypond scope scheme module, which would already have access to the (lily) module public interface, but the (define-markup-command was getting in under the radar and declaring it within the (lily) module direct, so the code in the (define-markup-command block referencing NWS-box-stencil could not see the NWS-box-stencil declaration. I think the original tenet of the original example was: "Here are some bits you can lift from two places in the base LilyPond code and hack them around a bit to produce something different." The original box-stencil is a (define-public declaration because it's a part of the base Lily code and has to be part of the (lily) module's exported interface. This example demonstrates a user customization at LilyPond document level, it is only used by the markup command and does not need to be global as it is only used by the new custom NWS-box markup command. So the define-public aspect of the NWS-box-stencil declaration in the original was rather crufty. If NWS-box-stencil is a thing local to the markup declaration, why not include it inside the markup declaration? It's an example in a snippet document, I don't think we explained it in a satisfactory way in the first place. I'm not prepared to die in a ditch over this one, though. I'll do one of the following: 1) Revert the example and re-instate the (define-public 2) Revert the example but change the (define-public to (define. 3) Use the nested definition and beef up the comments if necessary All three work with patch-set 2, what are the preferences as this is supposed to be model coding for customizers? Ian http://codereview.appspot.com/5464045/diff/2001/input/regression/bookparts.ly File input/regression/bookparts.ly (right): http://codereview.appspot.com/5464045/diff/2001/input/regression/bookparts.ly#newcode2 input/regression/bookparts.ly:2: #(use-modules (scm markup-facility-defs)) This isn't needed with patchset 2 - removing http://codereview.appspot.com/5464045/diff/2001/input/regression/bookparts.ly#newcode15 input/regression/bookparts.ly:15: (interpret-markup layout props (fancy-format #f "~@r" page-number)))) On 2011/12/19 20:52:56, dak wrote:
Any idea why this would have worked before?
Looks like need for this was specific to patch-set one changes. Reverting. http://codereview.appspot.com/5464045/diff/2001/input/regression/profile-property-access.ly File input/regression/profile-property-access.ly (right): http://codereview.appspot.com/5464045/diff/2001/input/regression/profile-property-access.ly#newcode37 input/regression/profile-property-access.ly:37: (map (lambda (x) (fancy-format #f "~30a: ~6@a" (car x) (cdr x))) On 2011/12/19 20:52:56, dak wrote:
This change is quite unrelated to the markup business. We probably
should have
a different patch for the fancy-format fixes.
Not necessary. This change can safely be reverted with patch-set 2, http://codereview.appspot.com/5464045/diff/2001/ly/titling-init.ly File ly/titling-init.ly (right): http://codereview.appspot.com/5464045/diff/2001/ly/titling-init.ly#newcode2 ly/titling-init.ly:2: #(use-modules (scm markup-facility-defs)) On 2011/12/19 20:52:56, dak wrote:
This file does not appear to define or call a single macro. Why is markup-facility-defs needed here nevertheless?
No, but it references interpret-markup on line 27 which is internal to the markup module. Normally only used inside (define-markup-command) or (define-markup-list-command). Will comment to this effect. http://codereview.appspot.com/5464045/diff/2001/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): http://codereview.appspot.com/5464045/diff/2001/scm/define-markup-commands.scm#newcode1989 scm/define-markup-commands.scm:1989: prev-result))) On 2011/12/19 19:38:04, Carl wrote:
The previous spacing was correct. prev-result should align with
(char-list Done. http://codereview.appspot.com/5464045/diff/2001/scm/define-markup-commands.scm#newcode2194 scm/define-markup-commands.scm:2194: ;(start-repl) On 2011/12/19 19:38:04, Carl wrote:
Is this a leftover that should be removed?
Too right. Nice catch. Done http://codereview.appspot.com/5464045/diff/2001/scm/display-lily.scm File scm/display-lily.scm (right): http://codereview.appspot.com/5464045/diff/2001/scm/display-lily.scm#newcode325 scm/display-lily.scm:325: ;; now defined here (to aid Guile V2 migration) On 2011/12/19 20:52:56, dak wrote:
I think we really need to get a hang about Guile's equivalents to
"include",
documented or not. We can't just stuff everything into one large file
because
Guile can't be bothered documenting basic functionality. We use a lot
of other
basic, undocumented stuff as well. While Guile is documented as awful
as it is,
it can't be avoided. If you know about the functions/macros (likely
by seeing
what Guile itself does), use them and report the missing docs on the
Guile bug
list.
It's not just about the (include-from-path) not being documented and not being available in Guile V1.8. I want to be able to process this module in the build outside the lily image and produce a single .go file. This will keep things simpler during the migration. If you like, I can add a TODO and a new issue to re-partition the file and add back the (include-from-path) and all the make-file complications later. Ian (P.S. I've filed guile bug reports about (include) and (include-from-path) documentation). Description: T2026: Use new (scm markup-facility-defs) scheme module for markup commands. This affects the following files. New file scm/markup-utility-defs.scm ly/music-functions-init.ly Add #(use-modules (scm markup-facility-defs)) statement. ly/titling-init.ly Add #(use-modules (scm markup-facility-defs)) statement. scm/lily.scm Add (use-modules (scm markup-facility-defs)) statement. Group I18n declarations together at the beginning. Add some comments. Add better --loglevel=DEBUG statement when running with Guile V2. scm/markup.scm Gutted. scm/markup-macros.scm (will be deleted) scm/define-markup-commands.scm Add (use-modules (scm markup-facility-defs)) statement. scm/chord-ignatzek-names Add (use-modules (scm markup-facility-defs)) statement. scm/define-event-classes.scm Ensure ly:is-listened-event-class is defined scm/define-woodwind-diagrams.scm Add (use-modules (scm markup-facility-defs)) statement. scm/display-woodwind-diagrams.scm Add (use-modules (scm markup-facility-defs)) statement. scm/font.scm Add (use-modules (oops goops)) statement. scm/fret-diagrams.scm Add (use-modules (scm markup-facility-defs)) statement. scm/harp-pedals.scm Add (use-modules (scm markup-facility-defs)) statement. scm/tablature.scm Add (use-modules (scm markup-facility-defs)) statement. Regression tests: input/regression/bookparts.ly Add #(use-modules (scm markup-facility-defs)) statement. input/regression/markup-cyclic-references.ly Add #(use-modules (scm markup-facility-defs)) statement. input/regression/profile-property-access.ly Needs to use fancy-format rather than format to avoid "use (ice-9 format)" diagnostics. Please review this at http://codereview.appspot.com/5464045/ Affected files: M Documentation/snippets/three-sided-box.ly M input/regression/bookparts.ly M input/regression/markup-cyclic-reference.ly M input/regression/profile-property-access.ly M ly/music-functions-init.ly M ly/titling-init.ly M ly/toc-init.ly M scm/chord-ignatzek-names.scm M scm/define-event-classes.scm M scm/define-markup-commands.scm M scm/define-woodwind-diagrams.scm M scm/display-lily.scm M scm/display-woodwind-diagrams.scm M scm/font.scm M scm/framework-ps.scm M scm/fret-diagrams.scm M scm/harp-pedals.scm M scm/lily.scm A scm/markup-facility-defs.scm M scm/markup.scm M scm/output-ps.scm M scm/page.scm M scm/tablature.scm _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
