Re: New format for autobeaming rules
2009/7/24 Carl Sorensen : >> type check also for settings? > > Settings needs a list? type check, and I haven't seen one available > in c++. It shouldn't segfault, because we do a pair check before we > use it. ly_is_list (which is a wrapper for scm_list_p) (but see below) > > I can't use a pair check for the argument, because '() is valid for > settings. > > I could use pair_or_empty, but it doesn't exist You could use ly_cheap_is_list, which does exactly this. , and when I tried to > add it to flower/include/guile-compatibility.hh, where all the rest of > the types are defined, it gave me errors. You'd need to add it to lily-guile.hh. guile-compatibility.hh is for compatibility with guile 1.6. >> type checks? > > Put in for time_signature, rule_type. > > Can't do one for beam_type, because it needs to be pair-or-symbol, > and I couldn't figure out how to add it. The long way: Add this to lily-guile.hh, inline bool ly_is_symbol_or_pair (SCM x) { return scm_is_symbol (x) || scm_is_pair (x); } and this to init_func_doc () in function-documentation.cc, ly_add_type_predicate ((void*) &ly_is_symbol_or_pair, "symbol or pair"); then you can use LY_ASSERT_TYPE with the new predicate: LY_ASSERT_TYPE (ly_is_symbol_or_pair, beam_type, 4); The shorter way (and better since this is unlikely to be used elsewhere) is to do this: SCM_ASSERT_TYPE (scm_is_symbol (beam_type) || scm_is_pair (beam_type), beam_type, SCM_ARG4, __FUNCTION__, "symbol or pair"); >> sort > But I couldn't find the header file that defined SCM through git > grep. Do you know which file I need to include? I think it's libguile.h, which is included in lily-guile.hh. The following works for me: beam-setting-scheme.cc: #include "beam-settings.hh" #include "context.hh" beam-settings.hh: (inside #define) #include "lily-guile.hh" >> decided not to restore original setting? > > I had decided to restore it in a different form. 1/8 beams are (6), which > is equivalent to (3) in (3 . 4). All shorter beams will be (1 1 1). Ah, so it is. I'll get the hang of the new settings eventually. :) Regards, Neil ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
On 7/22/09 4:15 PM, "joenee...@gmail.com" wrote: > I think this is pretty much ready to commit > > > http://codereview.appspot.com/88155/diff/3101/4032 > File lily/beam-scheme.cc (right): > > http://codereview.appspot.com/88155/diff/3101/4032#newcode2 > Line 2: beam-scheme.cc -- Retrieving beam settings > could you call this beam-grouping-scheme.cc or something like that? > beam-scheme sounds like it contains routines for manipulating Beam > grobs. Changed to beam-setting-scheme.cc. Changed beam-grouping.hh to beam-settings.hh. > > http://codereview.appspot.com/88155/diff/3101/4032#newcode12 > Line 12: LY_DEFINE (ly_beam_settings, "ly:beam-settings", > is this function really necessary? Probably not. Instead of ly_beam_settings(context) we can do context->get_property("beamSettings"); there's no error checking in the current function. So I guess I'll drop it. > > http://codereview.appspot.com/88155/diff/3101/4032#newcode49 > Line 49: ly_grouping_rules(settings,time_signature,rule_type), > formatting Fixed. > > http://codereview.appspot.com/88155/diff/3101/4032#newcode61 > Line 61: SCM settings = ly_beam_settings(context); > formatting Fixed > > http://codereview.appspot.com/88155 On 7/22/09 5:23 PM, "n.putt...@gmail.com" wrote: > > > http://codereview.appspot.com/88155/diff/3101/4027 > File input/new/grouping-beats.ly (right): > > http://codereview.appspot.com/88155/diff/3101/4027#newcode7 > Line 7: Beaming patterns may be altered with the @code{beatGrouping} > property: > new texidoc using \overrideBeamSettings > > http://codereview.appspot.com/88155/diff/3101/4032 > File lily/beam-scheme.cc (right): > > http://codereview.appspot.com/88155/diff/3101/4032#newcode10 > Line 10: #include "beam-grouping.hh" > swap Fixed > > http://codereview.appspot.com/88155/diff/3101/4032#newcode26 > Line 26: " @var{rule-type} in @var{context}.") > no context arg, doc settings > Fixed, 2 places (ly_grouping_rules and ly_beam_grouping). > http://codereview.appspot.com/88155/diff/3101/4032#newcode28 > Line 28: LY_ASSERT_TYPE (ly_is_pair, time_signature, 2); > scm_is_pair Fixed > > http://codereview.appspot.com/88155/diff/3101/4032#newcode30 > Line 30: > type check also for settings? Settings needs a list? type check, and I haven't seen one available in c++. It shouldn't segfault, because we do a pair check before we use it. I can't use a pair check for the argument, because '() is valid for settings. I could use pair_or_empty, but it doesn't exist, and when I tried to add it to flower/include/guile-compatibility.hh, where all the rest of the types are defined, it gave me errors. I'll put a FIXME in. So I'm not doing a type check for settings, at least right now. > > http://codereview.appspot.com/88155/diff/3101/4032#newcode34 > Line 34: ly_assoc_get ((scm_list_2 (time_signature, rule_type)), > excess parens D'OH! I'm not in scheme anymore! Fixed. > > http://codereview.appspot.com/88155/diff/3101/4032#newcode43 > Line 43: "Return grouping for beams of @var{ beam-type} in" > @var{beam-type} > Fixed > http://codereview.appspot.com/88155/diff/3101/4032#newcode45 > Line 45: " @var{rule-type} in @var{context}.") > no context arg, doc settings Fixed > > http://codereview.appspot.com/88155/diff/3101/4032#newcode46 > Line 46: { > type checks? Put in for time_signature, rule_type. Can't do one for beam_type, because it needs to be pair-or-symbol, and I couldn't figure out how to add it. I don't think it would segfault, because it's not dereferenced. I'll put a FIXME in. > > http://codereview.appspot.com/88155/diff/3101/4032#newcode57 > Line 57: { > LY_ASSERT_SMOB (Context, context, 1); > Added. > If you don't do this, then unsmob_context () will return NULL if this > function is passed invalid arguments, leading to a null dereference for > get_property ("timeSignatureFraction") -> segfault Thanks for teaching me about this. > > http://codereview.appspot.com/88155/diff/3101/4033 > File lily/beaming-pattern.cc (right): > > http://codereview.appspot.com/88155/diff/3101/4033#newcode18 > Line 18: #include "beam-grouping.hh" > sort OK, done > > http://codereview.appspot.com/88155/diff/3101/4034 > File lily/include/beam-grouping.hh (right): > > http://codereview.appspot.com/88155/diff/3101/4034#newcode8 > Line 8: > To prevent multiple includes: > > #ifndef BEAM_GROUPING_HH > #define BEAM_GROUPING_HH > > (+ line 14) > > http://codereview.appspot.com/88155/diff/3101/4034#newcode14 > Line 14: > #endif // BEAM_GROUPING_HH > OK, done. > http://codereview.appspot.com/88155/diff/3101/4035 > File lily/measure-grouping-engraver.cc (right): > > http://codereview.appspot.com/88155/diff/3101/4035#newcode14 > Line 14: #include "beam-grouping.hh" > sort When I try to sort, it breaks compile because SCM is not defined when beam-grouping.hh is included. The best thing to do would be to include the proper file to define SCM if it hasn't already been loaded. But I couldn't find the
Re: New format for autobeaming rules
http://codereview.appspot.com/88155/diff/3101/4027 File input/new/grouping-beats.ly (right): http://codereview.appspot.com/88155/diff/3101/4027#newcode7 Line 7: Beaming patterns may be altered with the @code{beatGrouping} property: new texidoc using \overrideBeamSettings http://codereview.appspot.com/88155/diff/3101/4032 File lily/beam-scheme.cc (right): http://codereview.appspot.com/88155/diff/3101/4032#newcode10 Line 10: #include "beam-grouping.hh" swap http://codereview.appspot.com/88155/diff/3101/4032#newcode26 Line 26: " @var{rule-type} in @var{context}.") no context arg, doc settings http://codereview.appspot.com/88155/diff/3101/4032#newcode28 Line 28: LY_ASSERT_TYPE (ly_is_pair, time_signature, 2); scm_is_pair http://codereview.appspot.com/88155/diff/3101/4032#newcode30 Line 30: type check also for settings? http://codereview.appspot.com/88155/diff/3101/4032#newcode34 Line 34: ly_assoc_get ((scm_list_2 (time_signature, rule_type)), excess parens http://codereview.appspot.com/88155/diff/3101/4032#newcode43 Line 43: "Return grouping for beams of @var{ beam-type} in" @var{beam-type} http://codereview.appspot.com/88155/diff/3101/4032#newcode45 Line 45: " @var{rule-type} in @var{context}.") no context arg, doc settings http://codereview.appspot.com/88155/diff/3101/4032#newcode46 Line 46: { type checks? http://codereview.appspot.com/88155/diff/3101/4032#newcode57 Line 57: { LY_ASSERT_SMOB (Context, context, 1); If you don't do this, then unsmob_context () will return NULL if this function is passed invalid arguments, leading to a null dereference for get_property ("timeSignatureFraction") -> segfault http://codereview.appspot.com/88155/diff/3101/4033 File lily/beaming-pattern.cc (right): http://codereview.appspot.com/88155/diff/3101/4033#newcode18 Line 18: #include "beam-grouping.hh" sort http://codereview.appspot.com/88155/diff/3101/4034 File lily/include/beam-grouping.hh (right): http://codereview.appspot.com/88155/diff/3101/4034#newcode8 Line 8: To prevent multiple includes: #ifndef BEAM_GROUPING_HH #define BEAM_GROUPING_HH (+ line 14) http://codereview.appspot.com/88155/diff/3101/4034#newcode14 Line 14: #endif // BEAM_GROUPING_HH http://codereview.appspot.com/88155/diff/3101/4035 File lily/measure-grouping-engraver.cc (right): http://codereview.appspot.com/88155/diff/3101/4035#newcode14 Line 14: #include "beam-grouping.hh" sort http://codereview.appspot.com/88155/diff/3101/4035#newcode66 Line 66: SCM time_signature_fraction = get_property ("timeSignatureFraction"); move to if { } block, then it's only retrieved if required http://codereview.appspot.com/88155/diff/3101/4038 File ly/music-functions-init.ly (right): http://codereview.appspot.com/88155/diff/3101/4038#newcode20 Line 20: (_i "Define @@var{music} as a quotable music expression named rogue extra @'s throughout file http://codereview.appspot.com/88155/diff/3101/4039 File python/convertrules.py (right): http://codereview.appspot.com/88155/diff/3101/4039#newcode2930 Line 2930: str = re.sub("\\set\w+#\'beatGrouping", "\\setBeatGrouping", str) won't get here due to search above (and regexp is broken) http://codereview.appspot.com/88155/diff/3101/4041 File scm/beam-settings.scm (right): http://codereview.appspot.com/88155/diff/3101/4041#newcode48 Line 48: ;; in 3 4 time: decided not to restore original setting? http://codereview.appspot.com/88155/diff/3101/4041#newcode155 Line 155: Functions for overriding beam settings indentation of function bodies http://codereview.appspot.com/88155/diff/3101/4043 File scm/define-context-properties.scm (right): http://codereview.appspot.com/88155/diff/3101/4043#newcode126 Line 126: (beatGrouping ,list? "A list of beatgroups, e.g., in 5/8 time remove http://codereview.appspot.com/88155 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
I think this is pretty much ready to commit http://codereview.appspot.com/88155/diff/3101/4032 File lily/beam-scheme.cc (right): http://codereview.appspot.com/88155/diff/3101/4032#newcode2 Line 2: beam-scheme.cc -- Retrieving beam settings could you call this beam-grouping-scheme.cc or something like that? beam-scheme sounds like it contains routines for manipulating Beam grobs. http://codereview.appspot.com/88155/diff/3101/4032#newcode12 Line 12: LY_DEFINE (ly_beam_settings, "ly:beam-settings", is this function really necessary? http://codereview.appspot.com/88155/diff/3101/4032#newcode49 Line 49: ly_grouping_rules(settings,time_signature,rule_type), formatting http://codereview.appspot.com/88155/diff/3101/4032#newcode61 Line 61: SCM settings = ly_beam_settings(context); formatting http://codereview.appspot.com/88155 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
Reviewers: Neil Puttock, joeneeman, Message: OK, I've responded to Joe's comments. I've created a new file lily/beam-scheme.cc, and put c++ and scheme callable routines in it. Then I've used those calls from auto-beam.scm, measure-grouping-engraver.cc, and beaming-pattern.cc. I also added a new file lily/include/beam-grouping.hh to provide those functions to c++. I've never done this much mucking in the c++ code before, so please make sure I've done things right. Thanks, Carl http://codereview.appspot.com/88155/diff/3092/2039 File ly/music-functions-init.ly (right): http://codereview.appspot.com/88155/diff/3092/2039#newcode699 Line 699: (make-music 'SequentialMusic 'void #t)) On 2009/07/21 18:24:35, joeneeman wrote: I'd feel better if this were finished. At least add FIXME so it can be easily grepped for. OK -- fixed in new patch set. http://codereview.appspot.com/88155/diff/3092/2047 File scm/music-functions.scm (right): http://codereview.appspot.com/88155/diff/3092/2047#newcode546 Line 546: (define (make-default-beaming-rule context) On 2009/07/21 18:24:35, joeneeman wrote: this doesn't seem to be used... Thanks for the catch -- removed Description: New format for autobeaming rules Change autobeaming rules so they are all set in one place and they remain if the time signature is changed. Eliminates override-auto-beam-settings and revert-auto-beam-settings autoBeamSettings property changed to beamSettings property beatGrouping property eliminated (default autobeam rule is now used for beatGrouping) Please review this at http://codereview.appspot.com/88155 Affected files: M Documentation/de/user/rhythms.itely M Documentation/es/user/rhythms.itely M Documentation/fr/user/rhythms.itely M Documentation/topdocs/NEWS.tely M Documentation/user/music-glossary.tely M Documentation/user/rhythms.itely M input/lsr/automatic-beams-two-per-two-in-4-4-or-2-2-time-signature.ly M input/lsr/beam-endings-in-score-context.ly M input/lsr/beam-grouping-in-7-8-time.ly M input/lsr/chordchanges-for-fretboards.ly M input/lsr/compound-time-signatures.ly M input/lsr/conducting-signs,-measure-grouping-signs.ly M input/lsr/grouping-beats.ly M input/lsr/making-slurs-with-complex-dash-structure.ly M input/lsr/non-default-tuplet-numbers.ly M input/lsr/non-traditional-key-signatures.ly M input/lsr/reverting-default-beam-endings.ly M input/manual/fretted-headword.ly M input/mutopia/claop.py A input/new/beam-endings-in-score-context.ly A input/new/beam-grouping-in-7-8-time.ly A input/new/compound-time-signatures.ly A input/new/conducting-signs,-measure-grouping-signs.ly A input/new/grouping-beats.ly A input/new/reverting-default-beam-endings.ly M input/regression/auto-beam-beat-grouping.ly M input/regression/beam-beat-grouping.ly M lily/auto-beam-engraver.cc A lily/beam-scheme.cc M lily/beaming-pattern.cc A lily/include/beam-grouping.hh M lily/measure-grouping-engraver.cc M ly/bagpipe.ly M ly/engraver-init.ly M ly/music-functions-init.ly M python/convertrules.py M scm/auto-beam.scm A scm/beam-settings.scm M scm/c++.scm M scm/define-context-properties.scm M scm/define-music-display-methods.scm M scm/lily.scm M scm/music-functions.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
very nice! http://codereview.appspot.com/88155/diff/3092/2039 File ly/music-functions-init.ly (right): http://codereview.appspot.com/88155/diff/3092/2039#newcode699 Line 699: (make-music 'SequentialMusic 'void #t)) I'd feel better if this were finished. At least add FIXME so it can be easily grepped for. http://codereview.appspot.com/88155/diff/3092/2047 File scm/music-functions.scm (right): http://codereview.appspot.com/88155/diff/3092/2047#newcode546 Line 546: (define (make-default-beaming-rule context) this doesn't seem to be used... http://codereview.appspot.com/88155 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: PATCH: Re: New format for autobeaming rules
On Tue, Jul 21, 2009 at 11:06 AM, Patrick McCarty wrote: > On Tue, Jul 21, 2009 at 11:03 AM, Carl Sorensen wrote: >> >> >> I'm getting ready to push the new autobeaming code. Are there any more >> comments before I do so? >> >> http://codereview.appspot.com/88155 > > Did you address Neil's last comment on the patchset, from 5 days ago? Never mind. :-) Sorry for the noise. -Patrick ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: PATCH: Re: New format for autobeaming rules
On Tue, Jul 21, 2009 at 11:03 AM, Carl Sorensen wrote: > > > I'm getting ready to push the new autobeaming code. Are there any more > comments before I do so? > > http://codereview.appspot.com/88155 Did you address Neil's last comment on the patchset, from 5 days ago? -Patrick ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
PATCH: Re: New format for autobeaming rules
I'm getting ready to push the new autobeaming code. Are there any more comments before I do so? http://codereview.appspot.com/88155 Thanks, Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
On Wed, Jul 15, 2009 at 07:43:56AM -0600, Carl Sorensen wrote: > > On 7/14/09 3:57 PM, "n.putt...@gmail.com" wrote: > > > http://codereview.appspot.com/88155/diff/95/1147#newcode69 > > Line 69: section 1.2.4 Beams, for more information. > > Is it possible to use @ruser{} here? > > I'm not sure. I thought NEWS was supposed to be a standalone document. > Graham, you're the source of all truth about documents; what do you think? Until now, it's been a standalone document, but perhaps that should change. Only after the new unified doc process, unified macros, etc, though. So just leave it as-is for now. :) > > http://codereview.appspot.com/88155/diff/95/1149#newcode1743 > > Line 1743: Beam settings can be reverted to get back to default > > behavior. This > > This looks like it should be an input/new snippet. > > I'm not sure I understand why you think it should it be in input/new instead > of just being in the docs. It doesn't use \set or \override. It explains > the use of a LilyPond command. That's why I thought it should be an inline > snippet. In most doc sections, we'd move tweaking-type stuff (and I think that \overrideBeamSettings would qualify) into snippets. HOWEVER, certain NR sections go into more detail... my traditional example of this is the page about changing automatic beam settings. So I definitely think it's ok to have this inline here. Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
On 7/15/09 3:45 PM, "n.putt...@gmail.com" wrote: > > > http://codereview.appspot.com/88155/diff/2005/3086 > File scm/music-functions.scm (right): > > http://codereview.appspot.com/88155/diff/2005/3086#newcode519 > Line 519: (make-simultaneous-music output) > This breaks all the Festival regression tests which use \time > (song-associated-voice.ly, song-basic.ly, song-breathe.ly, > song-melisma.ly, song-stanzas.ly & song-tempo.ly): > > /usr/local/share/lilypond/2.13.4/scm/song.scm:707:52: In procedure last > in expression (last lst-1): > /usr/local/share/lilypond/2.13.4/scm/song.scm:707:52: Wrong type > argument in position 1 (expecting pair): () > > http://codereview.appspot.com/88155 Aargh -- I thought I had run the regtests after that change, but apparently not. I've got that fixed now, and the regtests run without error. Thanks for saving my bacon, Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
http://codereview.appspot.com/88155/diff/2005/3086 File scm/music-functions.scm (right): http://codereview.appspot.com/88155/diff/2005/3086#newcode519 Line 519: (make-simultaneous-music output) This breaks all the Festival regression tests which use \time (song-associated-voice.ly, song-basic.ly, song-breathe.ly, song-melisma.ly, song-stanzas.ly & song-tempo.ly): /usr/local/share/lilypond/2.13.4/scm/song.scm:707:52: In procedure last in expression (last lst-1): /usr/local/share/lilypond/2.13.4/scm/song.scm:707:52: Wrong type argument in position 1 (expecting pair): () http://codereview.appspot.com/88155 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
2009/7/15 Carl Sorensen : > I'm not sure I understand why you think it should it be in input/new instead > of just being in the docs. It doesn't use \set or \override. It explains > the use of a LilyPond command. That's why I thought it should be an inline > snippet. It's positioned in the middle of a @snippet block, so it looks like a snippet waiting to be moved to LSR. Regards, Neil ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
On 7/15/09 9:30 AM, "John Mandereau" wrote: > Le mercredi 15 juillet 2009 à 07:43 -0600, Carl Sorensen a écrit : >> On 7/14/09 3:57 PM, "n.putt...@gmail.com" wrote: >>> http://codereview.appspot.com/88155/diff/95/1147#newcode69 >>> Line 69: section 1.2.4 Beams, for more information. >>> Is it possible to use @ruser{} here? >> >> I'm not sure. I thought NEWS was supposed to be a standalone document. >> Graham, you're the source of all truth about documents; what do you think? > > There is no @ruser defined in NEWS. However, see top of NEWS.tely: a > macro named @usermanref is defined, but it's currently unused in both > 2.12 and 2.13, so please check whether it works before pushing. I tried, and it didn't work (but it didn't appear to break compilation, either). So for right now, I think I'll leave the reference out. Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
Le mercredi 15 juillet 2009 à 07:43 -0600, Carl Sorensen a écrit : > On 7/14/09 3:57 PM, "n.putt...@gmail.com" wrote: > > http://codereview.appspot.com/88155/diff/95/1147#newcode69 > > Line 69: section 1.2.4 Beams, for more information. > > Is it possible to use @ruser{} here? > > I'm not sure. I thought NEWS was supposed to be a standalone document. > Graham, you're the source of all truth about documents; what do you think? There is no @ruser defined in NEWS. However, see top of NEWS.tely: a macro named @usermanref is defined, but it's currently unused in both 2.12 and 2.13, so please check whether it works before pushing. Fortunately, Graham's proposal of reorganization will lead to clean up and unify our Texinfo xref cooking. Best, John ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
On 7/14/09 3:57 PM, "n.putt...@gmail.com" wrote: > Carl, I haven't commenting on them directly, but there are quite a few > indentation errors in the .scm files. Thanks for noticing. I went through and fixed them up. > > > http://codereview.appspot.com/88155/diff/95/1147 > File Documentation/topdocs/NEWS.tely (right): > > http://codereview.appspot.com/88155/diff/95/1147#newcode69 > Line 69: section 1.2.4 Beams, for more information. > Is it possible to use @ruser{} here? I'm not sure. I thought NEWS was supposed to be a standalone document. Graham, you're the source of all truth about documents; what do you think? > > http://codereview.appspot.com/88155/diff/95/1149 > File Documentation/user/rhythms.itely (right): > > http://codereview.appspot.com/88155/diff/95/1149#newcode1743 > Line 1743: Beam settings can be reverted to get back to default > behavior. This > This looks like it should be an input/new snippet. I'm not sure I understand why you think it should it be in input/new instead of just being in the docs. It doesn't use \set or \override. It explains the use of a LilyPond command. That's why I thought it should be an inline snippet. > > http://codereview.appspot.com/88155/diff/95/1155 > File input/lsr/conducting-signs,-measure-grouping-signs.ly (right): > > http://codereview.appspot.com/88155/diff/95/1155#newcode77 > Line 77: } > missing %begin verbatim > > http://codereview.appspot.com/88155/diff/95/1155#newcode88 > Line 88: } % begin verbatim > rogue %begin verbatim OK, fixed -- trailing whitespace in the input/new version was confusing makelsr.py. Thanks again for the comments, Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
2009/7/14 Carl Sorensen : > It has cleaned up all of the issues that Neil identified, with the exception > of default autobeaming for 3/4 time. I never got any concurrence for > setting it back to (3), so it's left at (1 1 1). I think (3) is preferable, at least for quavers. The Baerenreiter Bach 'cello suite example shows how it should be (though it's got manual beaming just to make sure). Regards, Neil ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
Carl, I haven't commenting on them directly, but there are quite a few indentation errors in the .scm files. http://codereview.appspot.com/88155/diff/95/1147 File Documentation/topdocs/NEWS.tely (right): http://codereview.appspot.com/88155/diff/95/1147#newcode69 Line 69: section 1.2.4 Beams, for more information. Is it possible to use @ruser{} here? http://codereview.appspot.com/88155/diff/95/1149 File Documentation/user/rhythms.itely (right): http://codereview.appspot.com/88155/diff/95/1149#newcode1743 Line 1743: Beam settings can be reverted to get back to default behavior. This This looks like it should be an input/new snippet. http://codereview.appspot.com/88155/diff/95/1155 File input/lsr/conducting-signs,-measure-grouping-signs.ly (right): http://codereview.appspot.com/88155/diff/95/1155#newcode77 Line 77: } missing %begin verbatim http://codereview.appspot.com/88155/diff/95/1155#newcode88 Line 88: } % begin verbatim rogue %begin verbatim http://codereview.appspot.com/88155/diff/95/1170 File lily/measure-grouping-engraver.cc (right): http://codereview.appspot.com/88155/diff/95/1170#newcode71 Line 71: scm_list_2 (time_signature_fraction, same line as ly_assoc_get ( http://codereview.appspot.com/88155/diff/95/1170#newcode72 Line 72: ly_symbol2scm ("end")), indentation http://codereview.appspot.com/88155/diff/95/1170#newcode75 Line 75: grouping_rules, SCM_EOL); indentation http://codereview.appspot.com/88155/diff/95/1177 File scm/c++.scm (right): http://codereview.appspot.com/88155/diff/95/1177#newcode30 Line 30: (define-public (pair-or-symbol? x) also unused http://codereview.appspot.com/88155 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
I've posted a new patch set on Rietveld which I think is a release candidate. It has changes in all the translated rhythms.itely files so that all of the docs build properly. It has the revised snippets in input/new added to the repository. It has cleaned up all of the issues that Neil identified, with the exception of default autobeaming for 3/4 time. I never got any concurrence for setting it back to (3), so it's left at (1 1 1). Please review and let me know of any further issues. Thanks, Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
On 7/12/09 11:13 AM, "n.putt...@gmail.com" wrote: > > http://codereview.appspot.com/88155/diff/43/1045 > File > input/lsr/automatic-beams-two-per-two-in-4-4-or-2-2-time-signature.ly > (right): > > http://codereview.appspot.com/88155/diff/43/1045#newcode1 > Line 1: %% Do not edit this file; it is auto-generated from input/new > If you're keeping the revised rule for 4/4, this file's obsolete (it's > already been unticked in LSR for docs). > > The changes you've made contradict the original intent of this snippet > (and the title's incorrect), so it would be better to have a new snippet > for this. > > Frankly, I'd much prefer returning to the old settings (also for 3/4); > not only are they more in keeping with LilyPond's declared aims with > regard to typesetting style, they also produce better looking examples > in the documentation. > I've returned the default grouping to (2 2) for 4/4. We still don't need this snippet; there's an example in the docs that demonstrates this. I've eliminated this snippet. Thanks, Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
On 7/12/09 11:13 AM, "n.putt...@gmail.com" wrote: > > http://codereview.appspot.com/88155/diff/43/1045 > File > input/lsr/automatic-beams-two-per-two-in-4-4-or-2-2-time-signature.ly > (right): > > http://codereview.appspot.com/88155/diff/43/1045#newcode1 > Line 1: %% Do not edit this file; it is auto-generated from input/new > If you're keeping the revised rule for 4/4, this file's obsolete (it's > already been unticked in LSR for docs). > > The changes you've made contradict the original intent of this snippet > (and the title's incorrect), so it would be better to have a new snippet > for this. > > Frankly, I'd much prefer returning to the old settings (also for 3/4); > not only are they more in keeping with LilyPond's declared aims with > regard to typesetting style, they also produce better looking examples > in the documentation. > I've returned the default grouping to (2 2) for 4/4. We still don't need this snippet; there's an example in the docs that demonstrates this. I've eliminated this snippet. Thanks, Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New format for autobeaming rules
On 7/12/09 11:13 AM, "n.putt...@gmail.com" wrote: > > > http://codereview.appspot.com/88155/diff/43/1044 > File Documentation/user/rhythms.itely (right): > > http://codereview.appspot.com/88155/diff/43/1044#newcode1662 > Line 1662: of the beam, e.g. @code{#'(1 . 16)}, or @code{#'*} to > indicate a > This could be confusing to users unfamiliar with Scheme, since it > implies the hash and quote are required inside the alist. Good catch. This was written in an earlier incarnation, when I didn't have an alist, and they needed the hash for each argument. Thanks! > > http://codereview.appspot.com/88155/diff/43/1045 > File > input/lsr/automatic-beams-two-per-two-in-4-4-or-2-2-time-signature.ly > (right): > > http://codereview.appspot.com/88155/diff/43/1045#newcode1 > Line 1: %% Do not edit this file; it is auto-generated from input/new > If you're keeping the revised rule for 4/4, this file's obsolete (it's > already been unticked in LSR for docs). > > The changes you've made contradict the original intent of this snippet > (and the title's incorrect), so it would be better to have a new snippet > for this. > > Frankly, I'd much prefer returning to the old settings (also for 3/4); > not only are they more in keeping with LilyPond's declared aims with > regard to typesetting style, they also produce better looking examples > in the documentation. I have no problem with returning to the old settings. I tried to make this patch rule-neutral (but I made a mistake in 4 8, which you pointed out earlier). IIRC, Trevor Daniels took charge of cleaning up the old (2.12) autobeaming stuff once beatGrouping started working. I guess there's no sense in trying to recreate the history; let's just fix it. What do you think the default beam grouping for 4 4 and 3 4 should be? for 4 4: (* . (2 2)) ? and for 3 4: (* . (3)) ? > > http://codereview.appspot.com/88155/diff/43/1045#newcode53 > Line 53: } % begin verbatim > There should only be one of these inserted automatically (as above, at > the closing brace for the header). > Thanks, I'll fix that for all the snippets. > > http://codereview.appspot.com/88155/diff/43/1060 > File lily/measure-grouping-engraver.cc (right): > > http://codereview.appspot.com/88155/diff/43/1060#newcode65 > Line 65: SCM time_signature_fraction = > get_property("timeSignatureFraction"); > get_property ( Oh, yes, thanks. > > http://codereview.appspot.com/88155/diff/43/1060#newcode69 > Line 69: > remove blank line Done. > > http://codereview.appspot.com/88155/diff/43/1060#newcode74 > Line 74: scm_from_locale_string ("end"))), > ly_symbol2scm ("end") OK, thanks. I remember seeing that function elsewhere, but I couldn't find it when I was coding. I wish we had a way of telling people where in the source tree the scheme utility functions are. Maybe in the CG...? > > http://codereview.appspot.com/88155/diff/43/1060#newcode77 > Line 77: scm_from_locale_string ("*")), > ly_symbol2scm ("*") Thanks. Fixed > > http://codereview.appspot.com/88155/diff/43/1067 > File scm/c++.scm (right): > > http://codereview.appspot.com/88155/diff/43/1067#newcode34 > Line 34: (or (number? x) (symbol? x))) > unused? Yep, detritus from an earlier incarnation when '* or (1 . 16) were stand-alone arguments, rather than part of an alist. Removed. Thanks, Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel