Re: Issue 4654: offset-fret modifies its definition (issue 276810043 by d...@gnu.org)
On 2015/11/07 10:26:39, dak wrote: Just opencode the offsetting LGTM https://codereview.appspot.com/276810043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: fix unwanted capo indication in fret diagrams (issue 272150043 by thomasmorle...@gmail.com)
On 2015/10/26 21:47:14, thomasmorley651 wrote: fix failed make check, clean up LGTM. https://codereview.appspot.com/272150043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix for several musicxml2ly bugs. (issue 5697059)
Hey Julien, thanks a million! Cheers, patrick http://codereview.appspot.com/5697059/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix for several musicxml2ly bugs. (issue 5697059)
On 2012/02/29 22:33:45, Julien Rioux wrote:
This patch is not associated with any issue in the bug tracker. It
will not get
a proper review until it is added there, and the automatic testing
shows that it
does not cause any unexpected problems. Should it be added to issue
1983, or is
it sufficiently different that we should open a new issue for it?
Well it would certainly be easier for me if we opened a new issue for
this patch as it contains fixes for several bugs I reported lately. For
some of them no issues were opened. Here are the bugs / reports at
stake:
-) http://code.google.com/p/lilypond/issues/detail?id=1983
-)
http://old.nabble.com/musicxml2ly%3A-chordnames-placement-bug-td33309393.html
-) http://lists.gnu.org/archive/html/bug-lilypond/2012-02/msg00519.html
I also added 2 lines to Usage as the new command line options -m and
--midi hadn't been added yet.
Beyond that I have a partial fix for
(http://code.google.com/p/lilypond/issues/detail?id=1969). Should I make
a new patch or can I add the code here?
There are a lot of formatting changes to musicxml.py that are
unnecessary and
make it difficult to see the meaningful changes in your patch. You
also
introduce whitespace errors. So please revert the changes to
musicxml.py except
for...
http://codereview.appspot.com/5697059/diff/1/scripts/musicxml2ly.py
File scripts/musicxml2ly.py (right):
http://codereview.appspot.com/5697059/diff/1/scripts/musicxml2ly.py#newcode25
scripts/musicxml2ly.py:25: # Store command-line options in a global
variable, so
we can access them everywhere
...this typo...
OK. Will do. But I'll leave out the former lines 477 and 478 as this
_is_ a meaningful change.
Thanks for your help!
patrick
BTW: I should mention that the bugs in this patch were fixed by René
Fauck.
http://codereview.appspot.com/5697059/diff/1/scripts/musicxml2ly.py#newcode534
scripts/musicxml2ly.py:534: return None
...and this thinko.
http://codereview.appspot.com/5697059/diff/1/scripts/musicxml2ly.py#newcode2570
scripts/musicxml2ly.py:2570: p.version = ('''%prog (LilyPond)
2.15.24\n\n'''
Especially, don't change this.
http://codereview.appspot.com/5697059/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Fix for several musicxml2ly bugs. (issue 5697059)
Reviewers: , Description: Fix for several musicxml2ly bugs. musicxml2ly: title, chord symbol and midi bug Titles and headers can now contain single words followed by a punctuation mark (.,!:). See issue 1983. Chord symbols are now placed above staffs instead of below. musicxml2ly now includes an out-commented midi-block in every .ly-file. Docs: Added command line options -m and --midi to Usage Please review this at http://codereview.appspot.com/5697059/ Affected files: M Documentation/usage/external.itely M python/musicexp.py M python/musicxml.py M scripts/musicxml2ly.py ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicexp.py: Fix for issue 1985 (issue 5303063)
On 2011/10/24 09:47:45, Reinhold wrote: LGTM. Hi, I know it's only a one-liner but it still fixes a bus error. Is there any specific reason why this patch has never been pushed? Thanks patrick http://codereview.appspot.com/5303063/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicexp.py: Fix for issue 1985 (issue 5303063)
On 2012/02/11 21:01:04, Graham Percival wrote: On 2012/02/11 20:55:34, pl_s wrote: I know it's only a one-liner but it still fixes a bus error. Is there any specific reason why this patch has never been pushed? It was pushed a week ago? http://code.google.com/p/lilypond/issues/detail?id=1985 You're the only person that can close this codereview issue. - Graham Thanks, will close it. http://codereview.appspot.com/5303063/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
musicexp.py: Fix for issue 1985 (issue 5303063)
Reviewers: Reinhold,
Message:
I uploaded a patch for Issue 1985: musicxml2ly: group-symbol none leads
to bus error.
Description:
musicexp.py: Fix for issue 1985
The musicxml value 'none' of the group-symbol element is now converted
to 'SystemStartBar' instead of 'f', the compilation of which lead to a
bus error/seg fault.
Please review this at http://codereview.appspot.com/5303063/
Affected files:
M python/musicexp.py
Index: python/musicexp.py
diff --git a/python/musicexp.py b/python/musicexp.py
index
d105ef333d2bfb8f50f2dba428b4fbb00caf29e0..8610f8aa44f138886c5a1bed5cb4963a3f43a34f
100644
--- a/python/musicexp.py
+++ b/python/musicexp.py
@@ -1793,7 +1793,7 @@ class StaffGroup:
if self.spanbar == no:
printer.dump (\\override SpanBar #'transparent = ##t)
brack = {brace: SystemStartBrace,
- none: f,
+ none: SystemStartBar,
line: SystemStartSquare}.get (self.symbol, None)
if brack:
printer.dump (systemStartDelimiter = #'%s % brack)
___
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)
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)
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)
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)
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
