Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
2012/1/16 pls p.l.schm...@gmx.de: BTW: http://codereview.appspot.com/5303063/ is still open and hasn't been pushed. Oops! Looks like this patch slipped through the cracks because it wasn't mentioned in tracker issue 1985. I'm adding it now, it should be checked soon. thanks, Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
Hi Janek, done! Thanks patrick BTW: http://codereview.appspot.com/5303063/ is still open and hasn't been pushed. Am 16.01.2012 um 00:07 schrieb Janek Warchoł: Hi Patrick, your patch was pushed when i was absent; now i see that Rietveld issue is still opened. Could you close it please? http://codereview.appspot.com/5096050/ Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
Hi Patrick, your patch was pushed when i was absent; now i see that Rietveld issue is still opened. Could you close it please? http://codereview.appspot.com/5096050/ Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
On 2011/09/25 18:30:51, janek wrote: 2011/9/25 ptrcklsch...@googlemail.com: Hi Janek, I was talking about uploading some changes of this patch to rietveld. I forgot to add some bits and pieces I had announced in the description of this patch to the reworked version. I would have had to adjust only 2 of the 3 files in the patch. That's why I asked whether it's possible to upload only these 2 changed files in a new patch, but probably not... It is possible. As I'm not really experienced with git I would have to redo the whole patch. That surely won't be necessary. In fact, it'd be undesired also from our point of view. I use Lilydev and git. IIRC I had some problems with lily-git the last time I used it, but I will give it another try... If you are able to use git via command line, there's no point in switching to lily-git :) Lily-git is only an easy tool for people that may have trouble with command line. So, here is what you need to know about git in your current situation. For clarity i'll describe how everything is done from the very beginning: 1) work begins by cloning git repository from official server to your Lilydev 2) make changes in files inside working directory (i.e. files that appear inside lilypond-git) 3) make a commit: 'git commit -a' 4) update your repository: 'git pull -r' 5) upload your patch to Rietveld: 'git cl upload origin/master' After this step the newly created Rietveld issue will show the differencies between your local git repository (which contains your patch, since you have committed it in step 3) and official master repository. There are differencies in 3 files. This is the step you are in. 6) make the changes you forgot about to the files in working directory, save those files 7) commit the changes: 'git commit -a -m write your commit message inside quotation marks ' (you can also omit -m option and write commit message in the editor that appears) 8) update your repository: 'git pull -r' 9) upload new version of the patch to Rietveld: 'git cl upload origin/master' What exactly does this do? First of all, it doesn't create a new Rietveld issue, but updates the issue that already exists (that's good). It sends the difference between your local git repository and official repository. It doesn't matter that it was done as two commits; the Rietveld tool will look at the diff send and think there is a difference in two files between what was previously uploaded and the current state of things and update these two files. We will be able to see everything - both newest versions of these two files and your version of the third file (which is already uploaded) - when we go to http://codereview.appspot.com/5096050/ I hope this makes things clear. If you have any questions, ask! Good luck, Janek Thanks! http://codereview.appspot.com/5096050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
LGTM. Thanks! http://codereview.appspot.com/5096050/diff/20001/python/musicexp.py File python/musicexp.py (right): http://codereview.appspot.com/5096050/diff/20001/python/musicexp.py#newcode1510 python/musicexp.py:1510: # Print out the style if we have ome, but the '() should only be The typo that was fixed in the previous version of the patch is back :P http://codereview.appspot.com/5096050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
This also passes a completely fresh make ; make doc. http://codereview.appspot.com/5096050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
Hi Patrick, 2011/9/24 ptrcklsch...@googlemail.com: I just discovered that I forgot to add the changes related to conversion-info and \center-column. Do I have to make a completely new patch containing all 3 files (musicxml2ly.py, musicxml.py and musicexp.py) or is it ok to upload only the files with these changes (i.e. musicexp.py and musicxml2ly.py)? I'm not sure what you mean. Are you talking about uploading changes to Rietveld, or fixing a different issue separately from current one? cheers, Janek PS are you using Lilydev and lily-git? ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
On 2011/09/25 10:31:14, janek wrote: Hi Patrick, 2011/9/24 ptrcklsch...@googlemail.com: I just discovered that I forgot to add the changes related to conversion-info and \center-column. Do I have to make a completely new patch containing all 3 files (musicxml2ly.py, musicxml.py and musicexp.py) or is it ok to upload only the files with these changes (i.e. musicexp.py and musicxml2ly.py)? I'm not sure what you mean. Are you talking about uploading changes to Rietveld, or fixing a different issue separately from current one? cheers, Janek PS are you using Lilydev and lily-git? Hi Janek, I was talking about uploading some changes of this patch to rietveld. I forgot to add some bits and pieces I had announced in the description of this patch to the reworked version. I would have had to adjust only 2 of the 3 files in the patch. That's why I asked whether it's possible to upload only these 2 changed files in a new patch, but probably not... As I'm not really experienced with git I would have to redo the whole patch. But I think it's better to include these changes in the next patch. I use Lilydev and git. IIRC I had some problems with lily-git the last time I used it, but I will give it another try... Thanks for your help! patrick http://codereview.appspot.com/5096050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
2011/9/25 ptrcklsch...@googlemail.com: Hi Janek, I was talking about uploading some changes of this patch to rietveld. I forgot to add some bits and pieces I had announced in the description of this patch to the reworked version. I would have had to adjust only 2 of the 3 files in the patch. That's why I asked whether it's possible to upload only these 2 changed files in a new patch, but probably not... It is possible. As I'm not really experienced with git I would have to redo the whole patch. That surely won't be necessary. In fact, it'd be undesired also from our point of view. I use Lilydev and git. IIRC I had some problems with lily-git the last time I used it, but I will give it another try... If you are able to use git via command line, there's no point in switching to lily-git :) Lily-git is only an easy tool for people that may have trouble with command line. So, here is what you need to know about git in your current situation. For clarity i'll describe how everything is done from the very beginning: 1) work begins by cloning git repository from official server to your Lilydev 2) make changes in files inside working directory (i.e. files that appear inside lilypond-git) 3) make a commit: 'git commit -a' 4) update your repository: 'git pull -r' 5) upload your patch to Rietveld: 'git cl upload origin/master' After this step the newly created Rietveld issue will show the differencies between your local git repository (which contains your patch, since you have committed it in step 3) and official master repository. There are differencies in 3 files. This is the step you are in. 6) make the changes you forgot about to the files in working directory, save those files 7) commit the changes: 'git commit -a -m write your commit message inside quotation marks ' (you can also omit -m option and write commit message in the editor that appears) 8) update your repository: 'git pull -r' 9) upload new version of the patch to Rietveld: 'git cl upload origin/master' What exactly does this do? First of all, it doesn't create a new Rietveld issue, but updates the issue that already exists (that's good). It sends the difference between your local git repository and official repository. It doesn't matter that it was done as two commits; the Rietveld tool will look at the diff send and think there is a difference in two files between what was previously uploaded and the current state of things and update these two files. We will be able to see everything - both newest versions of these two files and your version of the third file (which is already uploaded) - when we go to http://codereview.appspot.com/5096050/ I hope this makes things clear. If you have any questions, ask! Good luck, Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
On 2011/09/23 15:36:15, p-l-s wrote: Hi Reinhold and Janek, I hope I got it right this time. I didn't include the bit about miscellaneous. This will be part of a seperate patch. BTW: I noticed that the midi-block is not always inserted in every .ly file (this is not related to my patch). I will do some testing... Thanks for your help! patrick I just discovered that I forgot to add the changes related to conversion-info and \center-column. Do I have to make a completely new patch containing all 3 files (musicxml2ly.py, musicxml.py and musicexp.py) or is it ok to upload only the files with these changes (i.e. musicexp.py and musicxml2ly.py)? http://codereview.appspot.com/5096050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
passes make and reg tests http://codereview.appspot.com/5096050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
On 2011/09/22 13:26:54, Reinhold wrote:
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:
reinhold@curie:~$ cat .bin/musicxml2ly
#!/bin/sh
exec ~/lilypond/lilypond/out/bin/musicxml2ly $@
You can then simply call musicxml2ly, and the built musicxml2ly will
be called.
OK, I basically use the same method, but the make-trick was new to me.
I obviously modified the program files directly in the build directory
and copied them to the source tree because 'git status' couldn't find
any modified files.
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.
OK
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.
Hm, I didn't understand all of this. What do I have to change in
musicxml2ly? My idea was to use the description misc field for a
custom header variable named 'miscellaneous' by default. I was thinking
about implementing a cmd line option ('-t' and '--texinfo') to be able
to use the 'texinfo' variable when needed. But if at all this should be
implemented in a different patch...
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
OK
http://codereview.appspot.com/5096050/diff/1/scripts/musicxml2ly.py#newcode221
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
Hm, I didn't understand all of this. What do I have to change in
musicxml2ly? My
idea was to use the description misc field for a custom header
variable named
'miscellaneous' by default. I was thinking about implementing a cmd
line option
('-t' and '--texinfo') to be able to use the 'texinfo' variable when
needed. But
if at all this should be implemented in a different patch...
My suggestion is more general: It copies every miscellaneous-field to
the lilypond file as a header variable with the same name. This way,
nothing gets lost from the MusicXML file. And the 'description' fields
is duplicated as 'texidoc', too.
I don't think we need a --texinfo cmd line option, see my comment below.
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
On 2011/09/22 13:26:54, Reinhold wrote:
Of course, you'll have to adjust musicxml2ly.py, too.
I already gave you the spot where you would have to modify it. I'll add
some pseudocode there to make it clearer what I envisioned...
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#newcode231
scripts/musicxml2ly.py:231: #set_if_exists ('miscellaneous',
ids.get_miscellaneous ());
On 2011/09/22 13:26:54, Reinhold wrote:
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.
What I'm thinking of is something like:
misc = ids.get_miscellaneous (); # This is a hash with fieldname -
value
for i in misc.keys ():
# Create a header variable for each miscellaneous-field:
set_if_exists (i, misc[i]);
if (i == 'description'):
set_if_exists ('texidoc', misc[i]);
This will create a header variable for each miscellaneous-field, plus
one texidoc header variable for the description. I don't think we need a
cmd line argument to switch on/off texidoc creation. If someone really
uses a miscellaneous-field with 'description' type for a MusicXML file
that is not part of our regtest suite, it still doesn't hurt to have one
more header variable. The user won't need it, but it also doesn't cause
any problems.
http://codereview.appspot.com/5096050/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
Hi Reinhold and Janek, I hope I got it right this time. I didn't include the bit about miscellaneous. This will be part of a seperate patch. BTW: I noticed that the midi-block is not always inserted in every .ly file (this is not related to my patch). I will do some testing... Thanks for your help! patrick http://codereview.appspot.com/5096050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
Some concerns and a handful of questions (as usual in my case).
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')
Isn't this a mistake?
If not, can it not be hardcoded?
http://codereview.appspot.com/5096050/diff/1/python/musicexp.py#newcode283
python/musicexp.py:283: return False
Can you add a comment saying what does this do?
I'd appreciate it, because i don't know :)
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#newcode176
python/musicxml.py:176: for r in source:
What does this do?
Can you add a comment explaining this?
http://codereview.appspot.com/5096050/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: title and subtitle (issue 1913), miscellaneous (issue 5096050)
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:
reinhold@curie:~$ 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:
