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

[ianhulin44]

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 "address@hidden" 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: address@hidden" (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