Re: lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
Hi Reinhold, On 2012/01/24 17:06:57, Reinhold wrote: What about --use-source-file-names? I just tested this. I used lilypond-book --use-source-file-names --output=out file1, where `file1' includes `file2'. In the output, we link to `file2', so that part looks correct. But the file `file2' is not copied to the `out' dir. I also tested on 2.15.25, and it didn't work back then either. I was rather talking about e.g. when the papersize=... snippet option is used in a texi file... (I think that doesn't work currently, either, though). The line width is implicitly specified by the paper size. Then we need to derive the line-width from the papersize, and we need to do so in exactly the same way lilypond itself would do. Then adjust this line-width according to the padding. This is obviously not yet implemented, also not back in 2.15.25, but it shouldn't be terribly hard to do. So I think we have two new bugs in need of issue numbers. Thanks and regards, Julien http://codereview.appspot.com/5553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py File python/book_snippets.py (left): http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#oldcode806 python/book_snippets.py:806: self.ext = os.path.splitext (os.path.basename (self.filename))[1] On 2012/01/22 00:08:45, Julien Rioux wrote: The '.ly' is hardcoded elsewhere already, so that when you include a .ily, lilypond-book actually writes a lily-.ly file for it, and this is what we link to. On the other hand leaving the current code in leads to broken links to a lily-.ily file. What about --use-source-file-names? http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py File python/book_snippets.py (right): http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#newcode124 python/book_snippets.py:124: line-width = #(- line-width (* mm %(padding_mm)f) (* mm 1))''', On 2012/01/22 00:08:45, Julien Rioux wrote: On 2012/01/21 23:54:36, Reinhold wrote: > Aren't there cases when neither LINE_WIDTH nor QUOTE is used? Yes, that's the case for html-inline-no-option.html I was rather talking about e.g. when the papersize=... snippet option is used in a texi file... (I think that doesn't work currently, either, though). > In that case we don't subtract the padding... Why would a padding be relevant when there is no specified line width? The line width is implicitly specified by the paper size. http://codereview.appspot.com/5553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
Thanks for reviewing it. Regards, Julien http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py File python/book_snippets.py (left): http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#oldcode806 python/book_snippets.py:806: self.ext = os.path.splitext (os.path.basename (self.filename))[1] On 2012/01/21 23:54:36, Reinhold wrote: Why are you removing a generic statement and hardcoding .ly above? This will break e.g. if you try to include a .ily file as a snippet! It does not break. I tested it. This self.ext is used only in output, in links to the source file. The '.ly' is hardcoded elsewhere already, so that when you include a .ily, lilypond-book actually writes a lily-.ly file for it, and this is what we link to. On the other hand leaving the current code in leads to broken links to a lily-.ily file. http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py File python/book_snippets.py (right): http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#newcode124 python/book_snippets.py:124: line-width = #(- line-width (* mm %(padding_mm)f) (* mm 1))''', On 2012/01/21 23:54:36, Reinhold wrote: On 2012/01/21 19:14:15, Julien Rioux wrote: > This fixes the first problem by adjusting line-width directly where it is > defined. Aren't there cases when neither LINE_WIDTH nor QUOTE is used? Yes, that's the case for html-inline-no-option.html In that case we don't subtract the padding... Why would a padding be relevant when there is no specified line width? http://codereview.appspot.com/5553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
LGTM in general. Some comments though. Plus: The example in the comment above will now compile, but it will be missing the additional padding... Another thing about the hard-coded .ly extension: I would leave the extension extraction in the file snippet class, so that the base class sets a default of .ly, but for file snippets the extension will be extracted from the file name. http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py File python/book_snippets.py (left): http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#oldcode184 python/book_snippets.py:184: line-width = #(- line-width (* mm %(padding_mm)f) (* mm 1)) On 2012/01/21 21:18:00, janek wrote: Ah, so the problem was that sometimes line-width wasn't defined and thus trying to adjust it failed? Yes, the problem is that I don't know of any way to get the default line-width, because there are cases where line-width is not explicitly defined in the snippets. In those cases we also need to subtract the padding... http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#oldcode806 python/book_snippets.py:806: self.ext = os.path.splitext (os.path.basename (self.filename))[1] Why are you removing a generic statement and hardcoding .ly above? This will break e.g. if you try to include a .ily file as a snippet! http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#oldcode890 python/book_snippets.py:890: return result On 2012/01/21 19:14:15, Julien Rioux wrote: ...this part. Yeah, appending the original extension makes more sense (and also works when you have a compressed .xml file where you explicitly specify --compress). http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py File python/book_snippets.py (right): http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#newcode124 python/book_snippets.py:124: line-width = #(- line-width (* mm %(padding_mm)f) (* mm 1))''', On 2012/01/21 19:14:15, Julien Rioux wrote: This fixes the first problem by adjusting line-width directly where it is defined. Aren't there cases when neither LINE_WIDTH nor QUOTE is used? In that case we don't subtract the padding... http://codereview.appspot.com/5553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
On Sat, Jan 21, 2012 at 4:18 PM, wrote: > On 2012/01/21 19:14:15, Julien Rioux wrote: >> >> Here was the first problem: line-width is being adjusted. This is written to >> the >> .ly file generated by lilypond-book, but it is not clear whether >> %(paper_string)s above will contain a definition for line-width. And for >> HTML, >> it does not. This gave an error while running lilypond, mentioned in the bug >> report. > > > Ah, so the problem was that sometimes line-width wasn't defined and thus > trying to adjust it failed? > > http://codereview.appspot.com/5553056/ Yes. From the initial bug report for issue the failed output shows: Running lilypond... GNU LilyPond 2.15.25 Processing `snippet-map-1372012142.ly' Parsing... Processing `html-inline-no-options.html' Parsing... html-inline-no-options.html:16:16: error: GUILE signaled an error for the expression beginning here line-width = # (- line-width (* mm 3.00) (* mm 1)) Unbound variable: line-width We see that line-width was used before it was defined. Cheers, Julien ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
Thanks for explanations, Julien! Janek http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py File python/book_snippets.py (left): http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#oldcode184 python/book_snippets.py:184: line-width = #(- line-width (* mm %(padding_mm)f) (* mm 1)) On 2012/01/21 19:14:15, Julien Rioux wrote: Here was the first problem: line-width is being adjusted. This is written to the .ly file generated by lilypond-book, but it is not clear whether %(paper_string)s above will contain a definition for line-width. And for HTML, it does not. This gave an error while running lilypond, mentioned in the bug report. Ah, so the problem was that sometimes line-width wasn't defined and thus trying to adjust it failed? http://codereview.appspot.com/5553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
Thanks for explanations, Julien! Janek http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py File python/book_snippets.py (left): http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#oldcode184 python/book_snippets.py:184: line-width = #(- line-width (* mm %(padding_mm)f) (* mm 1)) On 2012/01/21 19:14:15, Julien Rioux wrote: Here was the first problem: line-width is being adjusted. This is written to the .ly file generated by lilypond-book, but it is not clear whether %(paper_string)s above will contain a definition for line-width. And for HTML, it does not. This gave an error while running lilypond, mentioned in the bug report. Ah, so the problem was that sometimes line-width wasn't defined and thus trying to adjust it failed? http://codereview.appspot.com/5553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
There were actually two issues. The second was discovered after fixing the first. I intend to push as two separate commits explaining each. http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py File python/book_snippets.py (left): http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#oldcode184 python/book_snippets.py:184: line-width = #(- line-width (* mm %(padding_mm)f) (* mm 1)) Here was the first problem: line-width is being adjusted. This is written to the .ly file generated by lilypond-book, but it is not clear whether %(paper_string)s above will contain a definition for line-width. And for HTML, it does not. This gave an error while running lilypond, mentioned in the bug report. http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#oldcode890 python/book_snippets.py:890: return result ...this part. http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py File python/book_snippets.py (right): http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#newcode124 python/book_snippets.py:124: line-width = #(- line-width (* mm %(padding_mm)f) (* mm 1))''', This fixes the first problem by adjusting line-width directly where it is defined. http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#newcode336 python/book_snippets.py:336: self.ext = '.ly' On 2012/01/21 18:32:05, Graham Percival wrote: this Here was the second problem, discovered after fixing the first: Somewhere in the HTML output, a filename and extension is needed. This was not defined for inline lilypond code. It gave a python error. http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#newcode506 python/book_snippets.py:506: override['padding_mm'] = self.global_options.padding_mm This is needed for the line-width adjustement thingy with default settings otherwise python errors. http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#newcode662 python/book_snippets.py:662: result.append (base + self.ext) On 2012/01/21 18:32:05, Graham Percival wrote: this Just some clean up, simplification, with the same meaning as... http://codereview.appspot.com/5553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
2012/1/21 : > I believe the problem had to do with filename extensions; as such, I > think the following three places constitute the actual fix for . Do i understand correctly that line 336 http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#newcode336 adds .ly extension to snippet files, and line 661 http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#newcode661 checks for it so that lilypond-book doesn't try to compile non-ly files? If so, would it be worth mentioning in code and/or commit message for code readability's sake? thanks, Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
I believe the problem had to do with filename extensions; as such, I think the following three places constitute the actual fix for . But he also cleaned up a few other little bits, which seems sensible and ok since it's a pretty small patch. http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py File python/book_snippets.py (right): http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#newcode336 python/book_snippets.py:336: self.ext = '.ly' this http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#newcode662 python/book_snippets.py:662: result.append (base + self.ext) this http://codereview.appspot.com/5553056/diff/1/python/book_snippets.py#newcode849 python/book_snippets.py:849: self.ext = os.path.splitext (os.path.basename (self.filename))[1] this http://codereview.appspot.com/5553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
Hi Julien, could you please explain to me how your patch fixes this issue? I've read it but don't understand why it works :( thanks, Janek http://codereview.appspot.com/5553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
LGTM http://codereview.appspot.com/5553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
LGTM. Carl http://codereview.appspot.com/5553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
lilypond-book: Group line-width settings together (issue 2222). (issue 5553056)
Reviewers: Reinhold,
Message:
Please review.
Description:
lilypond-book:
Group line-width settings together (issue ).
Also, specify filename and ext for all snippets.
Please review this at http://codereview.appspot.com/5553056/
Affected files:
M python/book_snippets.py
Index: python/book_snippets.py
diff --git a/python/book_snippets.py b/python/book_snippets.py
index
e5a43110ffa268ae6733d09e7c96dc768503ef61..127453d6afa6f79c9c84db7e6792926f9001a8f0
100644
--- a/python/book_snippets.py
+++ b/python/book_snippets.py
@@ -109,11 +109,19 @@ snippet_options = {
},
##
+# TODO: Remove the 1mm additional padding in the line-width
+# once lilypond creates tighter cropped images!
PAPER: {
PAPERSIZE: r'''#(set-paper-size "%(papersize)s")''',
INDENT: r'''indent = %(indent)s''',
-LINE_WIDTH: r'''line-width = %(line-width)s''',
-QUOTE: r'''line-width = %(line-width)s - 2.0
* %(exampleindent)s''',
+LINE_WIDTH: r'''line-width = %(line-width)s
+ %% offset the left padding, also add 1mm as lilypond creates cropped
+ %% images with a little space on the right
+ line-width = #(- line-width (* mm %(padding_mm)f) (* mm 1))''',
+QUOTE: r'''line-width = %(line-width)s - 2.0 * %(exampleindent)s
+ %% offset the left padding, also add 1mm as lilypond creates cropped
+ %% images with a little space on the right
+ line-width = #(- line-width (* mm %(padding_mm)f) (* mm 1))''',
RAGGED_RIGHT: r'''ragged-right = ##t''',
NORAGGED_RIGHT: r'''ragged-right = ##f''',
},
@@ -164,8 +172,6 @@ def classic_lilypond_book_compatibility (key, value):
return (None, None)
-# TODO: Remove the 1mm additional padding in the line-width, once lilypond
-# creates tighter cropped images!
PREAMBLE_LY = ''' Generated by %(program_name)s
Options: [%(option_string)s]
\\include "lilypond-book-preamble.ly"
@@ -179,9 +185,6 @@ PREAMBLE_LY = ''' Generated by %(program_name)s
\paper {
%(paper_string)s
- %% offset the left padding, also add 1mm as lilypond creates cropped
- %% images with a little space on the right
- line-width = #(- line-width (* mm %(padding_mm)f) (* mm 1))
}
\layout {
@@ -329,6 +332,8 @@ class IncludeSnippet (Snippet):
class LilypondSnippet (Snippet):
def __init__ (self, type, match, formatter, line_number,
global_options):
Snippet.__init__ (self, type, match, formatter, line_number,
global_options)
+self.filename = ''
+self.ext = '.ly'
os = match.group ('options')
self.parse_snippet_options (os, self.type)
@@ -498,6 +503,7 @@ class LilypondSnippet (Snippet):
override[EXAMPLEINDENT] = r'0.4\in'
override[LINE_WIDTH] = '5\\in'
override.update (self.formatter.default_snippet_options)
+override['padding_mm'] = self.global_options.padding_mm
option_string = ','.join (self.get_outputrelevant_option_strings
())
compose_dict = {}
@@ -651,7 +657,10 @@ printing diff against existing file." % filename)
def additional_files_to_consider (self, base, full):
return []
def additional_files_required (self, base, full):
-return []
+result = [];
+if self.ext != '.ly':
+result.append (base + self.ext)
+return result
def all_output_files (self, output_dir, output_dir_files):
@@ -803,7 +812,6 @@ class LilypondFileSnippet (LilypondSnippet):
def __init__ (self, type, match, formatter, line_number,
global_options):
LilypondSnippet.__init__ (self, type, match, formatter,
line_number, global_options)
self.filename = self.substring ('filename')
-self.ext = os.path.splitext (os.path.basename (self.filename))[1]
self.contents = file (BookBase.find_file (self.filename,
global_options.include_path,
global_options.original_dir)).read ()
@@ -838,6 +846,7 @@ class MusicXMLFileSnippet (LilypondFileSnippet):
LilypondFileSnippet.__init__ (self, type, match, formatter,
line_number, global_options)
self.compressed = False
self.converted_ly = None
+self.ext = os.path.splitext (os.path.basename (self.filename))[1]
self.musicxml_options_dict = {
'verbose': '--verbose',
'lxml': '--lxml',
@@ -881,14 +890,6 @@ class MusicXMLFileSnippet (LilypondFileSnippet):
return ('\\sourcefilename \"%s\"\n\\sourcefileline 0\n%s'
% (name, self.converted_ly))
-def additional_files_required (self, base, full):
-result = [];
-if self.compressed:
-result.append (base + '.mxl')
-else:
-result.append (base + '.xml')
-return result
-
def write_ly (self):
base = self.basename ()
path = os.path.join (self.global_options.lily_output_dir, base)
___
lilypond-devel mailing list
lilypond-d
