Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)

[reinhold . kainhofer]

Welcome to the wonderful world of LilyPond development!
Most changes look fine, but there are some things that can't be
submitted yet. See my comments.

Most important: Please edit the source file in scripts/musicxml2ly.py
and don't modify the installed musicxml2ly file and copy it over to the
source tree! In particular, the source code contains identifiers like
@TOPLEVEL_VERSION@, which will be replaced by the build system with
proper values, so that we don't have to hardcode things like version or
python path!

So,
-) Modify only the scripts/musicxml2ly.py and python/musicexp.py and
python/musicxml.py
-) Run make (you can kill it after a few seconds, as soon as all python
files are processed, which happends right at the beginning)
-) You can't run scripts/musicxml2ly.py directly, but rather the built
out/bin/musicxml2ly

For the last item, on my computer,  I have created a simply wrapper
script ~/.bin/musicxml2ly (if ~/.bin is in your PATH environment
variable), which contains only:

address@hidden:~$ cat .bin/musicxml2ly
#!/bin/sh
exec ~/lilypond/lilypond/out/bin/musicxml2ly "$@"

You can then simply call musicxml2ly, and the built musicxml2ly will be
called.



http://codereview.appspot.com/5096050/diff/1/python/musicexp.py
File python/musicexp.py (right):

http://codereview.appspot.com/5096050/diff/1/python/musicexp.py#newcode63
python/musicexp.py:63: self.print_verbatim ('\\version "2.15.13"')
On 2011/09/22 12:50:43, janek wrote:
> Isn't this a mistake?

I suppose it is a mistake. The source should contain @TOPLEVEL_VERSION@,
which will be replaced by the current version by make.

http://codereview.appspot.com/5096050/diff/1/python/musicexp.py#newcode283
python/musicexp.py:283: return False
These functions should not be placed here. The pitch language functions
are here, because the Pitch class is next. The midi functions don't
belong here.

http://codereview.appspot.com/5096050/diff/1/python/musicxml.py
File python/musicxml.py (right):

http://codereview.appspot.com/5096050/diff/1/python/musicxml.py#newcode252
python/musicxml.py:252: return None
Please don't exactly duplicate get_file_description!

A much cleaner solution would be to rename get_file_description to
get_miscellaneous and return a hash of all miscellaneous fields (note
that the 'name' attribute is REQUIRED in the MusicXML specification)...

Something like:
def get_miscellaneous (self):
    misc = self.get_named_children ('miscellaneous')
    ret = []
    for m in misc:
        misc_fields = m.get_named_children ('miscellaneous-field')
        for mf in misc_fields:
            if hasattr (mf, 'name'):
                ret[mf.name] = mf.get_text ()
            else:
                // Print a warning here...
    return ret

Instead of the if hasattr you can also use mf.get('name', '').

Of course, you'll have to adjust musicxml2ly.py, too. But at least this
solution is more general, and the logic to abuse a "description"  misc
field for the texidoc is implemented in the main file, not in a library
file.

http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py
File scripts/musicxml2ly.py (right):

http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode1
scripts/musicxml2ly.py:1: #!/usr/bin/python
Please don't modify  the compiled musicxml2ly and copy it to the source
tree. Rather modify the scripts/musicxml2ly.py in the source tree and
call make. To run it, simply call out/bin/musicxml2ly.

The source code MUST have the @TARGET_PYTHON@, @relocate-preamble@, etc.

http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode33
scripts/musicxml2ly.py:33: """
Same as above: Needs to be @relocate-preamble@ in the code!

http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode210
scripts/musicxml2ly.py:210: set_if_exists ('subtitle',
movement_title.get_text ())
"else"  is missing (if there is no work, then no title will be set at
all, even if movement_title exists). I would structure the if as follows
(pseudocode):

if work:
    'title' = work_title
    if movement_title:
        'subtitle' = movement_title
elif movement_title:
    'title' = movement_title

http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode221
scripts/musicxml2ly.py:221: #        set_if_exists ('tagline',
ids.get_encoding_software ())
Simply remove the code without comment.

http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode231
scripts/musicxml2ly.py:231: #        set_if_exists ('miscellaneous',
ids.get_miscellaneous ());
If you change get_miscellaneous to a hash, you can extract the
'description' here for the texidoc, and loop through all fields to
insert custom header fields for them.

http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode2596
scripts/musicxml2ly.py:2596: p.version = ('''%prog (LilyPond)
2.15.13\n\n'''
@TOPLEVEL_VERSION@

http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode2797
scripts/musicxml2ly.py:2797: printer.print_verbatim ('%% automatically
converted with musicxml2ly from %s\n' % filename)
To me "converted by musicxml2ly" sounds better..

http://codereview.appspot.com/5096050/