|
|
Descriptionmusicxml2ly: title and subtitle (issue 1913), miscellaneous
musicxml2ly: titles (fixes issue 1913), tagline, conversion-info, <source>, midi-cmd-line-option, center-column long instrument names
1) if XML: <work-title>bli</work-title> AND <movement-title>bla
</movement-title> --> LilyPond: title = bla subtitle = bli
2) the tagline of a piece engraved by LilyPond should not contain any
information as to the encoding software of the .xml file. The standard
Lilypond-tagline should be used.
3) the conversion info should contain the name of the conversion tool
4) the <source>-element is converted to a custom LilyPond-variable
named "source" in the header. (it is usually used for publishing
information)
5) a command line option for a midi block was added
6) multi-lined instrument names are now typeset in center-columns
7) the <miscellaneous>-element is currently translated into a header-
variable "texidoc" which is important for the documentation. I'd suggest
to call it "miscellaneous" by default and call it "texinfo" when
activated via a command line option. (Not implemented yet)
Patch Set 1 #
Total comments: 15
Patch Set 2 : revision #Patch Set 3 : forgotten bits and pieces #
Total comments: 1
MessagesTotal messages: 18
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?
Sign in to reply to this message.
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: printer.print_verbatim ('%% automatically converted with musicxml2ly from %s\n' % filename) To me "converted by musicxml2ly" sounds better..
Sign in to reply to this message.
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 > scripts/musicxml2ly.py:221: # set_if_exists ('tagline', > ids.get_encoding_software ()) > Simply remove the code without comment. > OK > 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. > See above > 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.. OK @Janek: I'll add some comments in the next patch. Thank you both for your comments!
Sign in to reply to this message.
> 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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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)?
Sign in to reply to this message.
passes make and reg tests
Sign in to reply to this message.
This also passes a completely fresh make ; make doc.
Sign in to reply to this message.
Hi Patrick, 2011/9/24 <ptrcklschmdt@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?
Sign in to reply to this message.
On 2011/09/25 10:31:14, janek wrote: > Hi Patrick, > > 2011/9/24 <ptrcklschmdt@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
Sign in to reply to this message.
2011/9/25 <ptrcklschmdt@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
Sign in to reply to this message.
On 2011/09/25 18:30:51, janek wrote: > 2011/9/25 <ptrcklschmdt@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!
Sign in to reply to this message.
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
Sign in to reply to this message.
Pushed as c4d028afdec2ee6e3cbbd661ed750ee9f5cf4c8f on behalf of Patrick/Janek
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
2012/1/16 pls <p.l.schmidt@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
Sign in to reply to this message.
|