LilyIssues / Issues / #5174 Entire \tempo marking should avoid excessive printing commands

Description has changed:

Diff:

--- old
+++ new
@@ -1,8 +1,8 @@
-Currently, separate components of the `tempo` tag are held in different `<text` tags, each with a `<tspan` without attributes. This is wasteful, and can make text ugly. KCC.cropped.svg shows how this fragmentation causes a bad result (ignoring issue 3778 with `\override Score.MetronomeMark.vertical-skylines = #'()`). The text is separated into 5 text blocks, whereas only 2 are necessary (before/after note).
+Currently, separate components of the `tempo` tag are held in different `<text` tags, each with a `<tspan` without attributes. This is wasteful, and can make text ugly. KCC.cropped.svg shows how this fragmentation causes a bad result (ignoring issue [#3778] with `\override Score.MetronomeMark.vertical-skylines = #'()`). The text is separated into 5 text blocks, whereas only 2 are necessary (before/after note).

 There are a few issues at play here:
 * Attributes such as the font-family and style should be in the `<tspan` tags, and so the opening parenthesis may be combined in the same text tag. This would prevent the excessive spacing seen in the example.
-* The equal sign is inexplicably unlinked to the metronome marking. I couldn't find where this goes wrong when reading `scm/translation-functions.scm` for issue 4678 as all the components seem to be combined there, and when there is not a range of beat markings (e.g. "4 = 100", not "4 = 100 - 120"), the equal sign is combined with the rest.
+* The equal sign is inexplicably unlinked to the metronome marking. I couldn't find where this goes wrong when reading `scm/translation-functions.scm` for issue [#4678] as all the components seem to be combined there, and when there is not a range of beat markings (e.g. "4 = 100", not "4 = 100 - 120"), the equal sign is combined with the rest.
 * The closing parenthesis is also separated from the rest of the metronome mark, leaving a strange gap as seen, and should be combined with the rest of the text.

 I can't seem to find where the issue is occuring for points 2 & 3, but by specifying the position of each component (and problems in concatenating them, the spacing is off and the syntax messier.

Issues: ~~#3778~~
Issues: ~~#4678~~

Description has changed:

Diff:

--- old
+++ new
@@ -1,6 +1,7 @@
 Currently, separate components of the `tempo` tag are held in different `<text` tags, each with a `<tspan` without attributes. This is wasteful, and can make text ugly. KCC.cropped.svg shows how this fragmentation causes a bad result (ignoring issue [#3778] with `\override Score.MetronomeMark.vertical-skylines = #'()`). The text is separated into 5 text blocks, whereas only 2 are necessary (before/after note).

 There are a few issues at play here:
+
 * Attributes such as the font-family and style should be in the `<tspan` tags, and so the opening parenthesis may be combined in the same text tag. This would prevent the excessive spacing seen in the example.
 * The equal sign is inexplicably unlinked to the metronome marking. I couldn't find where this goes wrong when reading `scm/translation-functions.scm` for issue [#4678] as all the components seem to be combined there, and when there is not a range of beat markings (e.g. "4 = 100", not "4 = 100 - 120"), the equal sign is combined with the rest.
 * The closing parenthesis is also separated from the rest of the metronome mark, leaving a strange gap as seen, and should be combined with the rest of the text.

Issues: ~~#3778~~
Issues: ~~#4678~~

Étienne Beaulé - 2017-09-02

summary: Entire \tempo marking should be in one tag --> Entire \tempo marking should avoid excessive printing commands
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

This problem is not just limited to SVGs, and does affect the PS backend. There, the print_glyphs should be used at a minimum. An example (in PS is provided below for \tempo "Adagio" 4=72-80:

19.2264 -7.0260 moveto /TeXGyreSchola-Bold 3.86523438 output-scale div selectfont
1.3316 0.0000 0.0000 /o
0.8194 0.0000 0.0000 /i
1.3316 0.0000 0.0000 /g
1.3316 0.0000 0.0000 /a
1.4682 0.0000 0.0000 /d
1.6730 0.0000 0.0000 /A
6 print_glyphs
27.7818 -7.0260 moveto /TeXGyreSchola-Regular 3.86523438 output-scale div selectfont
0.7170 0.0000 0.0000 /parenleft
1 print_glyphs
28.4988 -6.5373 moveto 0.0772 2.4743 1.0642 0.1791 0.0386 draw_round_box
28.4988 -6.5373 moveto magfontemmentaler-18mXVo /noteheads.s2 glyphshow
29.6594 -7.0260 moveto /TeXGyreSchola-Regular 3.86523438 output-scale div selectfont
0.6146 0.0000 0.0000 /space
1.3316 0.0000 0.0000 /equal
0.6146 0.0000 0.0000 /space
3 print_glyphs
32.2202 -7.0260 moveto /TeXGyreSchola-Regular 3.86523438 output-scale div selectfont
1.2292 0.0000 0.0000 /zero
1.2292 0.0000 0.0000 /eight
0.6146 0.0000 0.0000 /space
1.2292 0.0000 0.0000 /endash
0.6146 0.0000 0.0000 /space
1.2292 0.0000 0.0000 /two
1.2292 0.0000 0.0000 /seven
7 print_glyphs
39.5951 -7.0260 moveto /TeXGyreSchola-Regular 3.86523438 output-scale div selectfont
0.7170 0.0000 0.0000 /parenright
1 print_glyphs

Further study of the tempo section of translation-functions.scm is required.

Last edit: Étienne Beaulé 2017-09-02

Étienne Beaulé - 2017-09-02

Limit use of make-concat-markup in tempo tag

Calling make-concat-markup when the list of components of the tempo
tag is incomplete causes the printing command to be unnecessarily
fragmented, causing problems, mostly in the SVG backend. This change
combines all components of the metronome mark in one list before
calling make-concat-markup.

The make-simple-markup statements are also combined where possible, and
comments have been added where characters can be confused. None have
modified.

This patch provides one part for the fix of bug 5174. Further patches
will be needed to combine text of different styles/fonts or to fix
text spacing in the SVG backend.

Bug: 5174

http://codereview.appspot.com/328440043

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2017-09-03

assigned_to: Étienne Beaulé

Needs: -->

Patch: new --> needs_work

Type: --> Enhancement
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Fails make check on a few files:

e.g.

../regression/metronome-mark-broken-bound.ly

Processing `./4b/lily-a6faa2f9.ly'
Parsing...
Renaming input to: `/home/james/lilypond-git/input/regression/metronome-mark-broken-bound.ly'
Interpreting music...
fatal error: make-concat-markup: Invalid argument in position 1.  Expect: markup list, found: ((#<procedure simple-markup (layout props str)> "(") (#<procedure general-align-markup (layout props axis dir arg)> 1 -1 (#<procedure smaller-markup (layout props arg)> (#<procedure note-by-number-markup (layout props log dot-count dir)> 2 0 1))) (#<procedure simple-markup (layout props str)> " = ") #<procedure simple-markup (layout props str)> "90" (#<procedure simple-markup (layout props str)> ")")).

../regression/song-tempo.ly

Processing `./8b/lily-6e6407f6.ly'
Parsing...
Renaming input to: `/home/james/lilypond-git/input/regression/song-tempo.ly'
Writing Festival XML file song-tempo.xml...song-tempo<?xml version="1.0"?>
<!DOCTYPE SINGING PUBLIC "-//SINGING//DTD SINGING mark up//EN" "Singing.v0_1.dtd" []>
<SINGING BPM="90.0">
<DURATION BEATS="1.0"><PITCH NOTE="C5">do</PITCH></DURATION>
<DURATION BEATS="1.0"><PITCH NOTE="E5">re</PITCH></DURATION>
<DURATION BEATS="1.0"><PITCH NOTE="G5">mi</PITCH></DURATION>
<DURATION BEATS="1.5"><PITCH NOTE="C5">do</PITCH></DURATION>
<DURATION BEATS="1.5"><PITCH NOTE="E5">re</PITCH></DURATION>
<DURATION BEATS="1.5"><PITCH NOTE="G5">mi</PITCH></DURATION>
</SINGING>
Interpreting music...
fatal error: make-line-markup: Invalid argument in position 1.  Expect: markup list, found: (((#<procedure general-align-markup (layout props axis dir arg)> 1 -1 (#<procedure smaller-markup (layout props arg)> (#<procedure note-by-number-markup (layout props log dot-count dir)> 2 0 1))) (#<procedure simple-markup (layout props str)> " = ") #<procedure simple-markup (layout props str)> "60")).

../regression/metronome-mark-loose-column.ly

Processing `./dd/lily-f33a45f3.ly'
Parsing...
Renaming input to: `/home/james/lilypond-git/input/regression/metronome-mark-loose-column.ly'
Interpreting music...
fatal error: make-line-markup: Invalid argument in position 1.  Expect: markup list, found: (((#<procedure general-align-markup (layout props axis dir arg)> 1 -1 (#<procedure smaller-markup (layout props arg)> (#<procedure note-by-number-markup (layout props log dot-count dir)> 2 0 1))) (#<procedure simple-markup (layout props str)> " = ") #<procedure simple-markup (layout props str)> "60")).

Étienne Beaulé - 2017-09-03

Fix list appending

http://codereview.appspot.com/328440043

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2017-09-03

Attachments has changed:

Diff:

--- old +++ new @@ -1 +1,2 @@ KCC.cropped.svg (23.9 kB; image/svg+xml) +Screenshot.png (54.9 kB; image/png)

Needs: -->

Patch: new --> review

Type: --> Enhancement
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2017-09-03

Passes make, make check and a full make doc.

Reg test diff attached.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Étienne Beaulé - 2017-09-03

Type: Enhancement --> Ugly
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Étienne Beaulé - 2017-09-03

I must disagree with the type of this bug. This is not adding a new feature, rather a problem that was only apparent in the SVG backend. For the PS backend, this could be categorized as an enhancement, but the root problem was there, just not apparent. I'll be bold and change the type. The reg-test output did not show anything significant as the selected backend did not show the problem to begin with. Is it possible to get the regtests using different backends?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2017-09-04

Patch: review --> countdown
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2017-09-04

Patch on countdown for September 7th.

@Etienne - I always defer to Developers. When git-cl is used it seems to 'blank' out the field or default to 'Enhancement' (I cannot remember) so I just put anything that isn't 'Doc' and not immediately obvious (to me) as anything else as an 'Enhancement'. Feel free to change anything I do. Just make sure that you say why so the others can see.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Étienne Beaulé - 2017-09-04

Hope my reasoning was good :)

BTW, can the regression tests also use the SVG backend, alongside PS? This change was geared towards SVG output, so the diff would show much more with that backend. (Still important with PDF).

Thank you!

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2017-09-07

Patch: countdown --> push
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2017-09-07

Patch counted down - please push.

As for having a set of SVG reg tests, I don't think that is a bad thing but I personally wouldn't know how to go about doing that.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2017-09-07

labels: --> Fixed_2_21.0

status: Started --> Fixed

Patch: push -->
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2017-09-07

author Étienne Beaulé beauleetienne0@gmail.com
Sat, 2 Sep 2017 21:32:41 +0100 (17:32 -0300)
committer James Lowe pkx166h@gmail.com
Thu, 7 Sep 2017 16:11:19 +0100 (16:11 +0100)
commit 4785a3ed7ce03c950f4f1f90ef6f412d038cedc7

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Malte Meyn - 2018-08-24

labels: Fixed_2_21.0 --> Fixed_2_21_0
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Entire \tempo marking should avoid excessive printing commands

Issue Tracker for LilyPond

Searches

Help

#5174 Entire \tempo marking should avoid excessive printing commands

Related

Discussion

Related

Related