T2026: Use new (scm markup-facility-defs) scheme module for markup commands.

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.

[Carl, 2011-12-19 19:38:04]

Hi Ian,

I feel pretty much unqualified to comment on this, since the details are way
over my head.  Since it works (and you've tested it on both 1.8 and 2), I assume
that most things about it are OK.

I have a couple of questions:

1) How do we decide when we need to include (use-modules  (scm
markup-facility-defs)) in our input files?  Is it to be included any time we
define our own markup commands?

2) It seems you've replaced format with fancy-format in multiple places, even
though in your patch set you only describe doing it once.  I understand the need
of using fancy-format when doing the roman numeral formatting, but I'm not sure
why you've changed it in the other places.

Thanks,

Carl

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.sc...
scm/define-markup-commands.scm:1989: prev-result)))
The previous spacing was correct.  prev-result should align with (char-list

http://codereview.appspot.com/5464045/diff/2001/scm/define-markup-commands.sc...
scm/define-markup-commands.scm:2194: ;(start-repl)
Is this a leftover that should be removed?

[dak, 2011-12-19 19:43:27]

On 2011/12/19 19:38:04, Carl wrote:
> 1) How do we decide when we need to include (use-modules  (scm
> markup-facility-defs)) in our input files?  Is it to be included any time we
> define our own markup commands?

I feel rather strongly that this should not be required in user LilyPond files
but rather already be loaded on startup.

Is that the case?

[dak, 2011-12-19 20:52:56]

http://codereview.appspot.com/5464045/diff/2001/Documentation/snippets/three-...
File Documentation/snippets/three-sided-box.ly (right):

http://codereview.appspot.com/5464045/diff/2001/Documentation/snippets/three-...
Documentation/snippets/three-sided-box.ly:20: #(use-modules (scm
markup-facility-defs))
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?

http://codereview.appspot.com/5464045/diff/2001/Documentation/snippets/three-...
Documentation/snippets/three-sided-box.ly:34: (define (NWS-box-stencil stencil
thickness padding)
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.

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...
input/regression/bookparts.ly:15: (interpret-markup layout props (fancy-format
#f "~@r" page-number))))
Any idea why this would have worked before?

http://codereview.appspot.com/5464045/diff/2001/input/regression/profile-prop...
File input/regression/profile-property-access.ly (right):

http://codereview.appspot.com/5464045/diff/2001/input/regression/profile-prop...
input/regression/profile-property-access.ly:37: (map (lambda (x) (fancy-format
#f "~30a: ~6@a" (car x) (cdr x)))
This change is quite unrelated to the markup business.  We probably should have
a different patch for the fancy-format fixes.

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))
This file does not appear to define or call a single macro.  Why is
markup-facility-defs needed here nevertheless?

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)
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.

[Ian Hulin (gmail), 2011-12-20 11:13:27]

http://codereview.appspot.com/5464045/diff/2001/Documentation/snippets/three-...
File Documentation/snippets/three-sided-box.ly (right):

http://codereview.appspot.com/5464045/diff/2001/Documentation/snippets/three-...
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-...
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...
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...
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-prop...
File input/regression/profile-property-access.ly (right):

http://codereview.appspot.com/5464045/diff/2001/input/regression/profile-prop...
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.sc...
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.sc...
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).

[dak, 2011-12-20 11:34:07]

In general, people will view a patch like this as reference for what changes are
required in order to keep things working in future.

So you should keep actually independent cleanups and issues and fixes separate
from markup in separate patches.

Personally, I tend to push that kind of small-fry cleanup (if it really is that)
to staging without much of a review.

[dak, 2011-12-21 18:32:50]

http://codereview.appspot.com/5464045/diff/8002/input/regression/markup-cycli...
File input/regression/markup-cyclic-reference.ly (right):

http://codereview.appspot.com/5464045/diff/8002/input/regression/markup-cycli...
input/regression/markup-cyclic-reference.ly:2: #(use-modules (scm
markup-facility-defs))
Is this required?

http://codereview.appspot.com/5464045/diff/8002/ly/toc-init.ly
File ly/toc-init.ly (right):

http://codereview.appspot.com/5464045/diff/8002/ly/toc-init.ly#newcode4
ly/toc-init.ly:4: %% scheme code now defined in (scm markup-facility-defs)
Can you explain why it was necessary to move the stuff in here to
markup-facility-defs?  It looks like the kind of thing that should continue
working.

http://codereview.appspot.com/5464045/diff/8002/scm/define-event-classes.scm
File scm/define-event-classes.scm (right):

http://codereview.appspot.com/5464045/diff/8002/scm/define-event-classes.scm#...
scm/define-event-classes.scm:22: (if (not (defined?
'ly:is-listened-event-class)) (use-modules (lily)) )
Uh, why the conditional here?  Isn't it normal to use-modules whenever one needs
something defined in a different module, and let use-modules cater for loading
the stuff if not already done?

http://codereview.appspot.com/5464045/diff/8002/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/5464045/diff/8002/scm/define-markup-commands.sc...
scm/define-markup-commands.scm:370: x    laboris nisi ut aliquip ex ea commodo
consequat.
What's with the x?

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm
File scm/markup-facility-defs.scm (right):

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:34: markup* )
markup*?

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:94: Use `markup*' in a \\notemode context."
Why are you reintroducing markup*?

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:169: (ly:debug "Defined ~s function in ~s\n"
,(symbol->string command-name) (current-module))
Is there a reason to insert this debug output including the current module?

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:198: (ly:debug "Defined ~s function in ~s\n"
,(symbol->string make-markup-name) (current-module))
Same here.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:392: (defmacro*-public markup* (#:rest body)
Again: is there a reason you reintroduce markup*?

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:413: ;; (eval-when (load compile eval) ; Guile V2
only
Is there a reason you keep this line in after commenting it out?  It does not
seem to convey useful information.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:424: (ly:message "Current module: ~s"
(current-module))
You throw a signal here that I don't see being caught anywhere.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:458: ;;)   ; Guile V2 only
What is Guile V2 only here?

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:690: (set-current-module lilypond-module)
Is this line necessary?  What happens with it when loading, what when compiling?
Makes me nervous.

[Ian Hulin (gmail), 2011-12-27 00:46:40]

Remove debugging cruft, also other changes in patch-set one not needed since
patchset two.

Add handler to handle conditions thrown from markup module code in lily.scm.

http://codereview.appspot.com/5464045/diff/8002/input/regression/markup-cycli...
File input/regression/markup-cyclic-reference.ly (right):

http://codereview.appspot.com/5464045/diff/8002/input/regression/markup-cycli...
input/regression/markup-cyclic-reference.ly:2: #(use-modules (scm
markup-facility-defs))
On 2011/12/21 18:32:50, dak wrote:
> Is this required?
Not with latest patch-set, will remove

http://codereview.appspot.com/5464045/diff/8002/ly/toc-init.ly
File ly/toc-init.ly (right):

http://codereview.appspot.com/5464045/diff/8002/ly/toc-init.ly#newcode4
ly/toc-init.ly:4: %% scheme code now defined in (scm markup-facility-defs)
On 2011/12/21 18:32:50, dak wrote:
> Can you explain why it was necessary to move the stuff in here to
> markup-facility-defs?  It looks like the kind of thing that should continue
> working.
Reverted and removed corresponding changes in markup-facility-defs.scm

http://codereview.appspot.com/5464045/diff/8002/scm/define-event-classes.scm
File scm/define-event-classes.scm (right):

http://codereview.appspot.com/5464045/diff/8002/scm/define-event-classes.scm#...
scm/define-event-classes.scm:22: (if (not (defined?
'ly:is-listened-event-class)) (use-modules (lily)) )
On 2011/12/21 18:32:50, dak wrote:
> Uh, why the conditional here?  Isn't it normal to use-modules whenever one
needs
> something defined in a different module, and let use-modules cater for loading
> the stuff if not already done?
Left-over cruft from debugging.  Removing.

http://codereview.appspot.com/5464045/diff/8002/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/5464045/diff/8002/scm/define-markup-commands.sc...
scm/define-markup-commands.scm:370: x    laboris nisi ut aliquip ex ea commodo
consequat.
On 2011/12/21 18:32:50, dak wrote:
> What's with the x?
x est res icognita. . .
OK it's a typo, will remove.

http://codereview.appspot.com/5464045/diff/8002/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/5464045/diff/8002/scm/lily.scm#newcode829
scm/lily.scm:829: (set! failed (append (list failed-file) failed)))))
Needs to be modified to handle conditions thrown from markup module.  (See also
comment below)

http://codereview.appspot.com/5464045/diff/8002/scm/lily.scm#newcode873
scm/lily.scm:873: (lambda (x . args) (handler x file-name))))
Enhance to handle conditions thrown from markup module, also
handler assignment above in (let* block in lilypond-all

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm
File scm/markup-facility-defs.scm (right):

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:17: (define-public lilypond-module
(current-module))
To be removed, along with mirroring line at the bottom.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:34: markup* )
On 2011/12/21 18:32:50, dak wrote:
> markup*?
It's cruft. Will lose it.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:94: Use `markup*' in a \\notemode context."
On 2011/12/21 18:32:50, dak wrote:
> Why are you reintroducing markup*?
It's cruft. Will lose it.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:169: (ly:debug "Defined ~s function in ~s\n"
,(symbol->string command-name) (current-module))
On 2011/12/21 18:32:50, dak wrote:
> Is there a reason to insert this debug output including the current module?
Would like to leave this and similar until patch for Issue 1686 is done.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:198: (ly:debug "Defined ~s function in ~s\n"
,(symbol->string make-markup-name) (current-module))
On 2011/12/21 18:32:50, dak wrote:
> Same here.
See above

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:392: (defmacro*-public markup* (#:rest body)
On 2011/12/21 18:32:50, dak wrote:
> Again: is there a reason you reintroduce markup*?
It's cruft. Will lose it.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:413: ;; (eval-when (load compile eval) ; Guile V2
only
On 2011/12/21 18:32:50, dak wrote:
> Is there a reason you keep this line in after commenting it out?  It does not
> seem to convey useful information.
It's a TODO for me - will comment it as such, see matching comment below.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:424: (ly:message "Current module: ~s"
(current-module))
On 2011/12/21 18:32:50, dak wrote:
> You throw a signal here that I don't see being caught anywhere.
Thinko.  It's caught in the version of lily.scm I was using on my Guile V2
system. Will ensure relevant handling code is added to ly:load in lily.scm in
next patch-set.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:458: ;;)   ; Guile V2 only
On 2011/12/21 18:32:50, dak wrote:
> What is Guile V2 only here?
Part of the same TODO as at the top.  Going to have to do something like:
(define (compile-markup-expression-aux <current-args>)
  <current compile-markup-expression code>)

(define (compile-markup-expression <current args>)
  (cond-expand 
     (guile-2
        (eval-when (compile load eval)
           (compile-markup-expression-aux <current-args>)))
     (guile
           (compile-markup-expression-aux <current-args>)))

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#...
scm/markup-facility-defs.scm:690: (set-current-module lilypond-module)
On 2011/12/21 18:32:50, dak wrote:
> Is this line necessary?  What happens with it when loading, what when
compiling?
> Makes me nervous.
It was transitional while the file was still being loaded by lily.scm.  Should
be redundant now lily.scm loads this via (use-modules) will remove this and
matching line 17.

[dak, 2011-12-27 12:05:38]

One problem that I keep having is that I fail to see any documentation of how
this is supposedly interacting with the module system.

What module are user-defined markup functions placed in?  Previously, this would
have been a module local to the compilation unit.  It looks like there are
multiple lookups going on, yet the local module is supposedly starting out as a
copy of the lily module.

So where would be the documentation about what happens with macros when and
where?  What is the difference between code within Lilypond, and code in user
documents?

Without that information, neither developers nor users can be expected to
continue working with confidence.  It is also not clear to me whether macro
definitions will bleed between multiple compilation units on the command line.

[Ian Hulin (gmail), 2011-12-27 13:00:14]

On 2011/12/27 12:05:38, dak wrote:
> One problem that I keep having is that I fail to see any documentation of how
> this is supposedly interacting with the module system.
> 
> What module are user-defined markup functions placed in?  Previously, this
would
> have been a module local to the compilation unit. 
They still are since patch-set 2.
> It looks like there are multiple lookups going on, yet the local module is
supposedly starting out as a
> copy of the lily module.
> 
Markup validation looks for the declaration in the current module, in case the
new markups are nice and self-contained in the document, and then falls back to
looking up in the '(lily) module. That way we are guaranteed access to all the
definitions in (lily) defined in scm/define-markup-commands.scm whatever
shenanigans have been going on in the user document, or lily documents the user
is pulling in via \include.

Base code markup commands - defined in scm/define-markup-commands.scm - are in
the (lily) module.  User-defined markup commands are in the local lily scope
module (anonymous module created when the new source file is processed in
lilypond-file in lily.scm).

> So where would be the documentation about what happens with macros when and
> where?  What is the difference between code within Lilypond, and code in user
> documents?
> 
We maintain the stuff declared in the base code declared in
define-markup-commands.scm.  Stuff in user documents we can advise users on how
to do it right. If lots of users develop a markup that's safe, useful, and
someone asks for that enhancement, we put it in the base code.
> Without that information, neither developers nor users can be expected to
> continue working with confidence.  It is also not clear to me whether macro
> definitions will bleed between multiple compilation units on the command line.
No they won't.  This issue was addressed by patch-set 2.  It now works as it did
and is documented as well/badly as it was.  What information do you feel is
needed and how do you suggest it is supplied?

Ian

[Ian Hulin (gmail), 2011-12-27 13:07:02]

On 2011/12/27 13:00:14, Ian Hulin (gmail) wrote:
> On 2011/12/27 12:05:38, dak wrote:
> > One problem that I keep having is that I fail to see any documentation of
how
> > this is supposedly interacting with the module system.
> > 
> > What module are user-defined markup functions placed in?  Previously, this
> would
> > have been a module local to the compilation unit. 
> They still are since patch-set 2.
> > It looks like there are multiple lookups going on, yet the local module is
> supposedly starting out as a
> > copy of the lily module.
> > 
> Markup validation looks for the declaration in the current module, in case the
> new markups are nice and self-contained in the document, and then falls back
to
> looking up in the '(lily) module. That way we are guaranteed access to all the
> definitions in (lily) defined in scm/define-markup-commands.scm whatever
> shenanigans have been going on in the user document, or lily documents the
user
> is pulling in via \include.
> 
> Base code markup commands - defined in scm/define-markup-commands.scm - are in
> the (lily) module.  User-defined markup commands are in the local lily scope
> module (anonymous module created when the new source file is processed in
> lilypond-file in lily.scm).
> 
> > So where would be the documentation about what happens with macros when and
> > where?  What is the difference between code within Lilypond, and code in
user
> > documents?
> > 
> We maintain the stuff declared in the base code declared in
> define-markup-commands.scm.  Stuff in user documents we can advise users on
how
> to do it right. If lots of users develop a markup that's safe, useful, and
> someone asks for that enhancement, we put it in the base code.
> > Without that information, neither developers nor users can be expected to
> > continue working with confidence.  It is also not clear to me whether macro
> > definitions will bleed between multiple compilation units on the command
line.
> No they won't.  This issue was addressed by patch-set 2.  It now works as it
did
> and is documented as well/badly as it was.  What information do you feel is
> needed and how do you suggest it is supplied?
> 
> Ian

Just noticed, Patch-set 4 should be entitled "Implement feedback from patch-set
3 . . ." Sorry for any confusion.

Ian

[dak, 2012-01-02 14:04:39]

http://codereview.appspot.com/5464045/diff/10002/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/5464045/diff/10002/ly/music-functions-init.ly#n...
ly/music-functions-init.ly:31: %% need (scm markup-facility-defs)for markup?
Wasn't this supposed to get removed?  This was not supposed to be needed in
user-level docs, right?

http://codereview.appspot.com/5464045/diff/10002/ly/titling-init.ly
File ly/titling-init.ly (right):

http://codereview.appspot.com/5464045/diff/10002/ly/titling-init.ly#newcode1
ly/titling-init.ly:1: \version "2.15.20"
Isn't the version string wrong here?

http://codereview.appspot.com/5464045/diff/10002/scm/define-event-classes.scm
File scm/define-event-classes.scm (right):

http://codereview.appspot.com/5464045/diff/10002/scm/define-event-classes.scm...
scm/define-event-classes.scm:20: (ice-9 optargs)
Why is optargs needed here?  There is a define* below, but that looks like it
should rather be plain define.

http://codereview.appspot.com/5464045/diff/10002/scm/framework-ps.scm
File scm/framework-ps.scm (right):

http://codereview.appspot.com/5464045/diff/10002/scm/framework-ps.scm#newcode20
scm/framework-ps.scm:20: #:duplicates (replace))
What is with the duplicate?  Should this be fixed?

http://codereview.appspot.com/5464045/diff/10002/scm/framework-ps.scm#newcode429
scm/framework-ps.scm:429: (fancy-format port "/~a (~a)\n" field (metadata-encode
(markup->string val))))))
Why would we need fancy-format here?  The format codes are usual?

http://codereview.appspot.com/5464045/diff/10002/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/5464045/diff/10002/scm/lily.scm#newcode47
scm/lily.scm:47: (define-public lilypond-module (resolve-module '(lily))) ;;
should be equivalent to (current-module) for Guile V1.8
What is this equivalent to in GuileV2?  Or rather: what module are we in if not
in lily?

http://codereview.appspot.com/5464045/diff/10002/scm/markup-facility-defs.scm
File scm/markup-facility-defs.scm (right):

http://codereview.appspot.com/5464045/diff/10002/scm/markup-facility-defs.scm...
scm/markup-facility-defs.scm:92: Use `markup*' in a \\notemode context."
You are still not using the current definition of markup as your source here,
correct?  The doc string refers to markup*.

http://codereview.appspot.com/5464045/diff/10002/scm/markup.scm
File scm/markup.scm (right):

http://codereview.appspot.com/5464045/diff/10002/scm/markup.scm#newcode56
scm/markup.scm:56: ;; Use `markup*' in a \\notemode context."
The outcommented parts are not from the current source.  Why are they
outcommented instead of removed?

[Ian Hulin (gmail), 2012-01-02 21:19:24]

I've had to do some major mangling with my local git rep, so the next patch-set
is coming up as a new Rietveld issue.  

It's http://codereview.appspot.com/5504107

Will also update Google tracker.

Ian

http://codereview.appspot.com/5464045/diff/10002/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/5464045/diff/10002/ly/music-functions-init.ly#n...
ly/music-functions-init.ly:31: %% need (scm markup-facility-defs)for markup?
On 2012/01/02 14:04:39, dak wrote:
> Wasn't this supposed to get removed?  This was not supposed to be needed in
> user-level docs, right?
No.  This isn't a user-level document, it's part of the run-time initialization
we do for each user document.  
Regression tests will break without it.

User document testing.ly with this (copy of a definition from
music-functions-init.ly):

\version "2.14.0"
myFootnoteGrob =
#(define-music-function  (parser location grob-name offset text footnote)
   (symbol? number-pair? markup? markup?)
   (_i "Attach @var{text} to @var{grob-name} at offset @var{offset},
with @var{text} referring to @var{footnote} (use like @code{\\once})")
   (make-music 'FootnoteEvent
               'automatically-numbered #f
	       'symbol grob-name
	       'X-offset (car offset)
	       'Y-offset (cdr offset)
	       'text text
	       'footnote-text footnote))

\relative c' {
\time 4/4
c4 d e f |
g a b c \bar "|."
}

Still compiles OK.

http://codereview.appspot.com/5464045/diff/10002/ly/titling-init.ly
File ly/titling-init.ly (right):

http://codereview.appspot.com/5464045/diff/10002/ly/titling-init.ly#newcode1
ly/titling-init.ly:1: \version "2.15.20"
On 2012/01/02 14:04:39, dak wrote:
> Isn't the version string wrong here?

Done.

http://codereview.appspot.com/5464045/diff/10002/scm/define-event-classes.scm
File scm/define-event-classes.scm (right):

http://codereview.appspot.com/5464045/diff/10002/scm/define-event-classes.scm...
scm/define-event-classes.scm:20: (ice-9 optargs)
On 2012/01/02 14:04:39, dak wrote:
> Why is optargs needed here?  There is a define* below, but that looks like it
> should rather be plain define.

Done.

http://codereview.appspot.com/5464045/diff/10002/scm/define-event-classes.scm...
scm/define-event-classes.scm:165: (define* (simplify e)
Will change to (define

http://codereview.appspot.com/5464045/diff/10002/scm/framework-ps.scm
File scm/framework-ps.scm (right):

http://codereview.appspot.com/5464045/diff/10002/scm/framework-ps.scm#newcode20
scm/framework-ps.scm:20: #:duplicates (replace))
On 2012/01/02 14:04:39, dak wrote:
> What is with the duplicate?  Should this be fixed?
Some modules occasionally need to re-import (lily) via a (use-modules (lily)). 
The #:duplicates (replace) stops guile throwing out warnings during
initialization, particularly about us over-riding the guile base interface
definition for (format). This works round that. If you feel the whole issue
about modules needing to re-import (lily) needs looking at, lets look at it as a
separate issue.

http://codereview.appspot.com/5464045/diff/10002/scm/framework-ps.scm#newcode429
scm/framework-ps.scm:429: (fancy-format port "/~a (~a)\n" field (metadata-encode
(markup->string val))))))
On 2012/01/02 14:04:39, dak wrote:
> Why would we need fancy-format here?  The format codes are usual?
Another weirdo. Also reverted on my local repository.  Maybe git-cl upload isn't
doing what I think it does.

http://codereview.appspot.com/5464045/diff/10002/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/5464045/diff/10002/scm/lily.scm#newcode47
scm/lily.scm:47: (define-public lilypond-module (resolve-module '(lily))) ;;
should be equivalent to (current-module) for Guile V1.8
On 2012/01/02 14:04:39, dak wrote:
> What is this equivalent to in GuileV2?  Or rather: what module are we in if
not
> in lily?
For Guile V2, eventually we will want to do 
$guild compile --output=/scm/out/lily.go -L /scm lily.scm
in the build.  As it won't be able to run up the Lily image to create the (lily)
module in the main.cc code, (current-module) will be set to an "anonymous"
module by the guile code within its compile-file function.
So I can't just do 
(define-public lilypond-module (current-module))

http://codereview.appspot.com/5464045/diff/10002/scm/markup-facility-defs.scm
File scm/markup-facility-defs.scm (right):

http://codereview.appspot.com/5464045/diff/10002/scm/markup-facility-defs.scm...
scm/markup-facility-defs.scm:92: Use `markup*' in a \\notemode context."
On 2012/01/02 14:04:39, dak wrote:
> You are still not using the current definition of markup as your source here,
> correct?  The doc string refers to markup*.
Ouch! Nice catch.

http://codereview.appspot.com/5464045/diff/10002/scm/markup.scm
File scm/markup.scm (right):

http://codereview.appspot.com/5464045/diff/10002/scm/markup.scm#newcode56
scm/markup.scm:56: ;; Use `markup*' in a \\notemode context."
On 2012/01/02 14:04:39, dak wrote:
> The outcommented parts are not from the current source.  Why are they
> outcommented instead of removed?
OK. Cleanup time.

[dak, 2012-01-02 21:33:07]

http://codereview.appspot.com/5464045/diff/10002/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/5464045/diff/10002/ly/music-functions-init.ly#n...
ly/music-functions-init.ly:31: %% need (scm markup-facility-defs)for markup?
On 2012/01/02 21:19:24, Ian Hulin (gmail) wrote:
> On 2012/01/02 14:04:39, dak wrote:
> > Wasn't this supposed to get removed?  This was not supposed to be needed in
> > user-level docs, right?
> No.  This isn't a user-level document, it's part of the run-time
initialization
> we do for each user document.  

Argl.  I just don't get along with Rietveld: I thought this was
input/regression/bookparts.ly.

[Ian Hulin, 2012-01-02 21:47:10]

Hi David,
On 02/01/12 21:33, dak@gnu.org wrote:
> 
> http://codereview.appspot.com/5464045/diff/10002/ly/music-functions-init.ly
>
> 
File ly/music-functions-init.ly (right):
> 
>
http://codereview.appspot.com/5464045/diff/10002/ly/music-functions-init.ly#n...
>
>  ly/music-functions-init.ly:31: %% need (scm
> markup-facility-defs)for markup? On 2012/01/02 21:19:24, Ian Hulin
> (gmail) wrote:
>> On 2012/01/02 14:04:39, dak wrote:
>>> Wasn't this supposed to get removed?  This was not supposed to
>>> be
> needed in
>>> user-level docs, right?
>> No.  This isn't a user-level document, it's part of the run-time
> initialization
>> we do for each user document.
> 
> Argl.  I just don't get along with Rietveld: I thought this was 
> input/regression/bookparts.ly.
> 
Don't worry, input/regression/bookparts.ly has gone from the patch
entirely.  A new stream-lined set of files has been uploaded.

They pass the reg-tests locally.

New patch-set is at http://codereview.appspot.com/5504107.

Cheers,

Ian