Re: Set the sequence name in MIDI using title information from \header block (issue 256230045 by ht.lilypond.developm...@gmail.com)
Oh one thing to add, I am seeing a lot of this in the reg test: --snip-- input/regression/midi/staff-map-instrument.midi @@ -1,4 +1,4 @@ -track 0 ev (0, (255, 3, 'control track')) +track 0 ev (0, (255, 3, '')) ev (0, (255, 1, 'creator: ')) ev (0, (255, 88, '\x04\x02\x12\x08')) ev (0, (255, 81, '\x0fB@')) --snip-- I assume that is expected? https://codereview.appspot.com/256230045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Set the sequence name in MIDI using title information from \header block (issue 256230045 by ht.lilypond.developm...@gmail.com)
https://codereview.appspot.com/256230045/diff/1/lily/performance.cc File lily/performance.cc (right): https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode41 lily/performance.cc:41: header_ (SCM_EOL) On 2015/08/06 11:22:27, dak wrote: On 2015/08/06 11:05:56, ht wrote: I added the Performance::mark_smob member function that should handle the new SCM member variable. That's wrong. mark_smob is not a virtual member function of Music_output so there is no reason Performance::mark_smob will ever be called. Ok, so I overlooked the significance of Music_output and used an incorrect smob class (Score) as a model for extending Performance when I'd been better off with using (for example) Paper_book instead. Thanks for the correction, I hope to have got it right this time. https://codereview.appspot.com/256230045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Set the sequence name in MIDI using title information from \header block (issue 256230045 by ht.lilypond.developm...@gmail.com)
https://codereview.appspot.com/256230045/diff/1/lily/performance.cc File lily/performance.cc (right): https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode41 lily/performance.cc:41: header_ (SCM_EOL) On 2015/08/06 11:05:56, ht wrote: On 2015/08/06 02:30:37, Dan Eble wrote: I don't work with Guile frequently enough to tell you whether or not you need to do something to make sure that garbage collection works properly with this SCM member. David K. could probably tell you at a glance. Thanks for this observation, I added the Performance::mark_smob member function that should handle the new SCM member variable. That's wrong. mark_smob is not a virtual member function of Music_output so there is no reason Performance::mark_smob will ever be called. Music_output provides the derived_mark virtual member function for the purpose of adding your own marking to classes derived from Music_output. https://codereview.appspot.com/256230045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Set the sequence name in MIDI using title information from \header block (issue 256230045 by ht.lilypond.developm...@gmail.com)
https://codereview.appspot.com/256230045/diff/1/lily/include/performance.hh File lily/include/performance.hh (right): https://codereview.appspot.com/256230045/diff/1/lily/include/performance.hh#newcode44 lily/include/performance.hh:44: void write_output (string filename, const string name) const; On 2015/08/06 02:30:36, Dan Eble wrote: It's not obvious what name is the name of. A more descriptive name or a comment would be helpful. Changed name of argument to performance_name. https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc File lily/performance-scheme.cc (right): https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc#newcode39 lily/performance-scheme.cc:39: Performance *p = unsmobPerformance (performance); On 2015/08/06 02:30:37, Dan Eble wrote: LY_ASSERT_SMOB returns a pointer, doesn't it? So this unsmob is redundant. I just adapted this code from score-scheme.cc (which makes similar unsmob calls), so maybe there's a need for a more general clean-up if this coding pattern must be optimized. Being not that experienced with how the smob classes work internally on the C++ level, I'd rather be safe and follow the example than take responsibility that a direct reinterpret_cast (which is what unchecked_unsmob appears to do) will continue to work here. https://codereview.appspot.com/256230045/diff/1/lily/performance.cc File lily/performance.cc (right): https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode41 lily/performance.cc:41: header_ (SCM_EOL) On 2015/08/06 02:30:37, Dan Eble wrote: I don't work with Guile frequently enough to tell you whether or not you need to do something to make sure that garbage collection works properly with this SCM member. David K. could probably tell you at a glance. Thanks for this observation, I added the Performance::mark_smob member function that should handle the new SCM member variable. https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode81 lily/performance.cc:81: if (i == 0 !s-audio_items_. empty()) On 2015/08/06 02:30:37, Dan Eble wrote: no space after the dot; space before () Done. https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode92 lily/performance.cc:92: text-text_string_ = name; On 2015/08/06 02:30:37, Dan Eble wrote: On 2015/08/05 03:58:25, lemzwerg wrote: This code allows overwriting `control track' only once. I guess this is intended, however, it looks limiting. Wouldn't it be better to identify the control track by another property, say Audio_text::CONTROL_TRACK_NAME, instead of a comparison with a string that the user might modify? In addition to that, recognizing the control track with (i == 0) seems fragile. (Please forgive me for criticizing without offering an alternative. I'm in a hurry.) The original implementation rested on my interpretation of the guidelines concerning the structure of a format 1 MIDI file (see my comment in http://lists.gnu.org/archive/html/lilypond-user/2015-08/msg00048.html in the thread that led to this patch), and I trusted the existing LilyPond implementation to follow these guidelines. I agree that while the assumptions regarding the staff structure probably rarely break in practice (provided that no Control_track_performers are added to or removed from any contexts in a LilyPond input file, which I think that no normal user will do accidentally), the code just looks fragile... I've now implemented the recognition of the control track staff here in a more explicit way by defining a new Audio_staff subclass (Audio_control_track_staff), and changing the Control_track_performer to create an instance of this subclass, so the staff can then be any of the staves in a performance (although it really should always be the first one). I also changed the tests to verify the audio element structure of the staff into assertions since the control track is expected to follow the structure that's defined in Control_track_performer::initialize. https://codereview.appspot.com/256230045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Set the sequence name in MIDI using title information from \header block (issue 256230045 by ht.lilypond.developm...@gmail.com)
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Hello Heikki On 04/08/15 23:35, ht.lilypond.developm...@gmail.com wrote: Reviewers: , Message: This patch adds support for setting the MIDI sequence name for MIDI output files using the value of the midititle field from a relevant \header block, and falling back to using the title field if midititle has not been defined. (This should be analogous to how pdftitle and title work for customizing PDF metadata.) Does this patch have a google tracker? I couldn't find one. If not can you create one (with the summary and the link to this Rietveld) so that I can keep track of its progress while it is tested and reviewed? Thanks James -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQIcBAEBAgAGBQJVwcAiAAoJEP8yVoKoS9i++rMP/3AyA5tI7OqmIT11kWPM+uOw XCKAZ0bKy+1Inh+L1Eo9CuxcRP4hCnWcvjEEFFZ1AO4KnTwtPU4gpj496HJqEiNM c3SnfKCyk73AIOf9JifVe1YNXB68GgD2Vl1bdKQvHu1byWn9Q69oDnH6q29ov9aN WuH917ztTBUweW7M/sKdpXfE9vMOCLl1EPIsfU19ZWhSKqSwT33luynkQ9dwYVk0 Y40XwpsQAF4fOp6VyYAMfI1Ruts8PiIivA0JQtlHCeK0TqxKxJrkjpO8Zd3gqdqe ztAG/QwpBjf0GPomLeZUcuoNrVXVq+eLLStuyq0cSj2lpBEZUZ80Ap3k3j958C4G TopbB46WHsfYDtBJy0VezX/L587GEfThPBlR7ssYVdS5bBJnrlT/+PIv9gnderk8 CBIwugjqjlqUX01TRCH6Vf+wTeIREANTYRqrJ+xOEokw1AJersFtm2VcICQfReCn LG7paq1Sv0ct5RlgunuWcuPBGgdmdY2tazcNTuO2OdnTfpjlumtUeaRNUXOCr7dk l8Xll06vjC5GQAIpfSY3myGFgy189jUHgVDSTcWKMG7TTBH/kjulCiVVzjITKxF3 mo/M/qHO5aAG58tPLBsJ9vJsG2A4ud2+67GdwWttm00mLHm7xRQmNdhEvOB1/g1b rO5Y70xweqHlwJcMRrqt =Prhk -END PGP SIGNATURE- ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Set the sequence name in MIDI using title information from \header block (issue 256230045 by ht.lilypond.developm...@gmail.com)
On 2015/08/05 08:05:44, ht wrote: On 2015/08/05 07:50:08, http://pkx_gnu.org wrote: Does this patch have a google tracker? I couldn't find one. If not can you create one (with the summary and the link to this Rietveld) so that I can keep track of its progress while it is tested and reviewed? I added a new issue manually: https://code.google.com/p/lilypond/issues/detail?id=4539 (When initially running git-cl, I think that I mistyped my Google account password, so the command just aborted after that - this is probably the reason why no corresponding Google issue had been created. Using git-cl so rarely, I always get confused about in which state things get left if an error occurs during the procedure... Sorry about this.) Actually there is a problem using git-cl to create issue trackers since they changed their authentication method. So you can only use git-cl to upload to Rietveld and all tracker issues have to be done manually I am afraid (creation and updating). Anyway, I have it now and am testing it. https://codereview.appspot.com/256230045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Set the sequence name in MIDI using title information from \header block (issue 256230045 by ht.lilypond.developm...@gmail.com)
Fails make
https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc
File lily/performance-scheme.cc (right):
https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc#newcode46
lily/performance-scheme.cc:46: Write @var{performance} to
@var{filename} with name @{name}.)
... with name @var{name}...
This breaks make.
https://codereview.appspot.com/256230045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Set the sequence name in MIDI using title information from \header block (issue 256230045 by ht.lilypond.developm...@gmail.com)
https://codereview.appspot.com/256230045/diff/1/lily/include/performance.hh File lily/include/performance.hh (right): https://codereview.appspot.com/256230045/diff/1/lily/include/performance.hh#newcode44 lily/include/performance.hh:44: void write_output (string filename, const string name) const; It's not obvious what name is the name of. A more descriptive name or a comment would be helpful. https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc File lily/performance-scheme.cc (right): https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc#newcode39 lily/performance-scheme.cc:39: Performance *p = unsmobPerformance (performance); LY_ASSERT_SMOB returns a pointer, doesn't it? So this unsmob is redundant. https://codereview.appspot.com/256230045/diff/1/lily/performance.cc File lily/performance.cc (right): https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode41 lily/performance.cc:41: header_ (SCM_EOL) I don't work with Guile frequently enough to tell you whether or not you need to do something to make sure that garbage collection works properly with this SCM member. David K. could probably tell you at a glance. https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode81 lily/performance.cc:81: if (i == 0 !s-audio_items_. empty()) no space after the dot; space before () https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode92 lily/performance.cc:92: text-text_string_ = name; On 2015/08/05 03:58:25, lemzwerg wrote: This code allows overwriting `control track' only once. I guess this is intended, however, it looks limiting. Wouldn't it be better to identify the control track by another property, say Audio_text::CONTROL_TRACK_NAME, instead of a comparison with a string that the user might modify? In addition to that, recognizing the control track with (i == 0) seems fragile. (Please forgive me for criticizing without offering an alternative. I'm in a hurry.) https://codereview.appspot.com/256230045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Set the sequence name in MIDI using title information from \header block (issue 256230045 by ht.lilypond.developm...@gmail.com)
I don't have enough knowledge for a LGTM, but reading the source code I noticed one detail... https://codereview.appspot.com/256230045/diff/1/lily/performance.cc File lily/performance.cc (right): https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode92 lily/performance.cc:92: text-text_string_ = name; This code allows overwriting `control track' only once. I guess this is intended, however, it looks limiting. Wouldn't it be better to identify the control track by another property, say Audio_text::CONTROL_TRACK_NAME, instead of a comparison with a string that the user might modify? https://codereview.appspot.com/256230045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Set the sequence name in MIDI using title information from \header block (issue 256230045 by ht.lilypond.developm...@gmail.com)
Reviewers: ,
Message:
This patch adds support for setting the MIDI sequence name for MIDI
output files using the value of the midititle field from a relevant
\header block, and falling back to using the title field if
midititle
has not been defined. (This should be analogous to how pdftitle and
title work for customizing PDF metadata.)
The purpose of the change is to improve the previous behavior where the
name of every MIDI sequence created by LilyPond used to be shown by MIDI
synthesizers as control track (previously, this string was hard-coded
as the name of every initial track of MIDI files created by LilyPond by
Control_track_performer).
The patch
* extends every Performance instance with a reference to a \header
block associated with the performance, adds Scheme library routines for
getting and setting the associated \header (modeled after corresponding
routines available for the Score class), and updates the
Book::process_score function to initialize the header information of
Performance objects attached to a score;
* adds a name parameter to the Performance output routines, used for
updating the track name in the performance's first Audio_staff (assumed
to represent the control track) before outputting MIDI; and
* changes the write-performances-midis function (in scm/midi.scm) to
query the MIDI sequence name for a performance from the performance's
\header block (adapted from the handle-metadata function in
scm/framework-ps.scm).
For testing, I used the following LilyPond code:
\version 2.19.25
\header {
title = Top-level title
}
% Book A without a title defined in a \header block
\book {
\bookOutputName A
% A: score without a title in Book A -- will get the top-level title
\score {
\new Staff { c1 }
\midi { }
}
% A-1: score with a title defined in a nested \header block (in Book
A) --
% will get the title in this \header block
\score {
\new Staff { c1 }
\header { title = Score in Book A }
\midi { }
}
% A-2: score with a title and a separate MIDI title defined in a
nested
% \header block (in Book A) -- will get the MIDI title in this \header
block
\score {
\new Staff { c1 }
\header {
title = Another score in Book A
midititle = MIDI title
}
\midi { }
}
}
% Book B with a title defined in a \header block
\book {
\bookOutputName B
\header { title = Book B }
% B: score without a title in Book B -- will get the title of Book B
\score {
\new Staff { c1 }
\midi { }
}
% B-1: score with a \header block (and title) of its own (in Book B)
-- will
% get the title in this \header block
\score {
\new Staff { c1 }
\header { title = Score in Book B }
\midi { }
}
}
% Book C: book with a title defined in a \header block with book parts
\book {
\bookOutputName C
\header { title = Book C }
% Book part in Book C with a title of its own
\bookpart {
\header { title = Part 1 in Book C }
% C: score without a title (in Part 1 of Book C) -- will get the
title of
%its enclosing book part
\score {
\new Staff { c1 }
\midi { }
}
% C-1: score with a \header (and a title) of its own (in Part 1 of
Book C)
% -- will get this title
\score {
\new Staff { c1 }
\header { title = Score in Part 1 of Book C }
\midi { }
}
}
% C-2: score without a title outside any book part of Book C -- will
get the
% title of Book C
\score {
\new Staff { c1 }
\midi { }
}
% Book part in Book C with no \header block
\bookpart {
% C-3: score without a \header block (in Part 2 of Book C) -- will
get the
% title of Book C
\score {
\new Staff { c1 }
\midi { }
}
% C-4: score with an explicit title (in Part 2 of Book C) -- will
get this
% title
\score {
\new Staff { c1 }
\header { title = Score in Part 2 of Book C }
\midi { }
}
}
}
Description:
Set the sequence name in MIDI using title information from \header block
Please review this at https://codereview.appspot.com/256230045/
Affected files (+92, -14 lines):
M lily/book.cc
M lily/control-track-performer.cc
M lily/include/performance.hh
M lily/performance.cc
M lily/performance-scheme.cc
M scm/midi.scm
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
