Jpv/edition engraver

[jpvoigt]

Allow addressing context with shorter/easier names for edition-engraver mods.

This allows for using different paths to address a context. If a context \consists \editionEngraver my.tag.path editionMods can be placed with (measure 1, position 0/4) \editionMod target 1 0/4 my.tag.path <mod e.g. \override ...>

This may apply the mod more than once, as Staff, Voice and other contexts might have the same tag-id!

The path may now be specified with:

.ContextName .ContextId .ContextName.ContextId .ContextName.ContextCounter All paths are checked in this order! This might cause performance-slowdown, as these are checked on every timestep ... But it works ;)

[uliska]

The failed test doesn't mean anything - this simply failed because the to-be-merged branch is too old and doesn't contain the testing infrastructure.

The successful test doesn't mean anything either because the edition-engraver isn't included in the test suite so far.

[uliska]

OK, update after adding edition-engraver to our new automated test-suite:

• The push test failed for the same reason as before (branch too old) - don't worry about that
• The pr (Pull Request) still failed, and this is an issue with the Pull Request: See the Details page (you would also get there by clicking on "Details" and on the following page on the red number for the failed build test). On this page you'll see the log for the test run, and at the end of that log you'll see the detailed console output from the failed test: Obviously there's something incompatible with current dev version of LilyPond.

@jpvoigt please have a look at this. Of course we need the edition-engraver to be compatible with both stable and development version. Just in case: let me remind you of the lilypond-version-predicates that let you run code conditionally depending on the executed LilyPond version

[jpvoigt]

OK, there is a change in parser accessibility. Now the parser is another argument to (add-edmod ...)

[uliska]

So can you fix this? Do you have to use the LP version switch for that?
Anyway, this is the first real-world example of why the automated test integration started by Matteo is a great thing :-)

[jpvoigt]

It is fixed and compiles with 2.18 and 2.19 :) The switch is not needed, as it is now more consistent and doesnt make use of a weakness(?) in pre-2.19.

[uliska]

Great! and now the build proves that it is fixed.

@kierenmacmillan when you review this (I'd be glad if you could do it) please notice the comments about the failed "push" build that doesn't matter. The proposed change is "correct" now to the extent that the example file compiles without errors on 2.18.2 and 2.19.15.

[uliska]

@jpvoigt Sorry that this has been pending for so long. After having assigned the PR to Kieren I didn't look into it any further.

Today I have one question: Is this a "breaking" change? That is, would it affect existing code? If so I'd suggest not to modify the current edition-engraver but to implement your modified version as a copy inside the new library structure, maybe moving the other helper modules there on the occasion.

[jpvoigt]

@uliska no, this isn't a breaking change. I introduced the same addition in lalily and compiled old stuff successfully. So the old "complete" notation is still possible (e.g. ...Staff.A)

[kierenmacmillan]

Hello all,

@jpvoigt Sorry that this has been pending for so long. After having assigned the PR to Kieren I didn't look into it any further.

I apologise for my total lack of assistance at this time. I am under the gun for an important production of a [Lily-engraved!] musical next Thursday, and then a big [Lily-engraved!] classical commission due to be premiered on July 10.

I am very interested and excited by what’s going on here, and the moment I can roll up my sleeves and help, I will do so.

Thank you, Kieren.

---

Kieren MacMillan, composer www: http://www.kierenmacmillan.info email: info@kierenmacmillan.info

[uliska]

Ok. I'll try to review and merge this to make way for the next step.

Knowing it doesn't break existing code makes things easier.

Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

[uliska]

Now I have looked into it and tried to understand it, but to no avail. I will give that an even closer look when we will try to move the tool to its better location in the new infrastructure.

For the time being: The example file compiles fine, but when trying to compile existing files I get an error:

% at the beginning of the log
openlilylib/editorial-tools/edition-engraver/definitions.ily:36:2: error: GUILE signaled an error for the expression beginning here
#
 (translator-property-description 'edition-id list? "edition id (list)")

% and at the end:
Converting to `./ClarinetI.pdf'...
Wrong type (expecting string): edition-id

So I don't have a clue what this can mean. I will try to build a new file from scratch and see what happens.

[uliska]

OK, I compiled a different real-world file, and that worked. I can see two differences to the files that failed:

1. the failed files include the edition-engraver as well as auto-transpose, and both have this copy of translator-property-description. Maybe this is an issue? (well, we should remove that redundancy anyway when moving the files)
2. the failed files add several editions, so the edition-engraver is loaded multiple times. I can imagine that is an issue.

Finally I notice that in the failed files I get a number of output lines

edition-id: #f

[jpvoigt]

I'll have a look at the issues later. The translator-property-description part should be public in lilypond - it shouldn't be a problem to have a copy in some files. The "edition-id: #f" are an indication, that the edition id is not a context-property. I shall document this new possibility: If you consist an edition-engraver with no id/path (e.g. ##f), it first looks, if it finds a context-property "edition-id" - if it doesn't, it looks, like it does before, in the parent contexts edition-engraver. I hope to have alook at it soon!

[jpvoigt]

I created a tiny example, that uses both, edition-engraver and auto-transpose:

%%%%%%%%%%%%%%%%% \version "2.18.2"

\include "editorial-tools/auto-transpose/definitions.ily" \include "editorial-tools/edition-engraver/definitions.ily"

\layout { \context { \Voice \consists \editionEngraver ##f % if no context-property is found and no id/path is given here, it results in a message: % "edition-id: #f" % this is also the case, if the id is inherited from the parent contexts edition-engraver } }

\addEdition lalala % ed mod on Staff level \editionMod lalala 1 1/4 la.la.la.Johann \once \override NoteHead.color = #green % ed mod on Voice level \editionMod lalala 1 2/4 la.la.la.Voice \once \override NoteHead.color = #red

\new Staff = "Johann" \with { % you can decide, whether you want set the edition-id as a context-property: %edition-id = #'(la la la) %\consists \editionEngraver ##f % ... or in the engraver: \consists \editionEngraver la.la.la \autoTranspose } \new Voice \relative c'' { \transposition bes bes4 a c b } %%%%%%%%%%%%%%%%%%%%%%%%%%%%

[uliska]

@jpvoigt I assume all of this has been ported to the separate edition-engraver repo?

If that's correct we can close this PR, isn't it?