lilypond-book: Group line-width settings together (issue 2222).

lilypond-book:

Group line-width settings together (issue 2222).
Also, specify filename and ext for all snippets.

[Julien Rioux, 2012-01-19 00:17:18]

Please review.

[Carl, 2012-01-19 03:59:30]

LGTM.

Carl

[Graham Percival, 2012-01-20 13:52:01]

LGTM

[janek, 2012-01-21 18:28:19]

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

[Graham Percival, 2012-01-21 18:32:04]

I believe the problem had to do with filename extensions; as such, I think the
following three places constitute the actual fix for 2222.  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

[janek, 2012-01-21 18:54:22]

2012/1/21  <graham@percival-music.ca>:
> I believe the problem had to do with filename extensions; as such, I
> think the following three places constitute the actual fix for 2222.

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

[Julien Rioux, 2012-01-21 19:14:14]

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...

[janek, 2012-01-21 21:17:55]

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?

[Julien Rioux, 2012-01-21 21:38:11]

On Sat, Jan 21, 2012 at 4:18 PM,  <janek.lilypond@gmail.com> 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 2222 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.000000) (* mm 1))
Unbound variable: line-width

We see that line-width was used before it was defined.
Cheers,
Julien

[Reinhold, 2012-01-21 23:54:36]

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...

[Julien Rioux, 2012-01-22 00:08:45]

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?

[Reinhold, 2012-01-24 17:06:57]

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.

[Julien Rioux, 2012-01-24 21:51:10]

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