Re: New format for autobeaming rules

[Carl Sorensen]

On 7/22/09 4:15 PM, "address@hidden" <address@hidden> 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, "address@hidden" <address@hidden> 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 header file that defined SCM through git
grep.  Do you know which file I need to include?

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

Done.  Nice catch.

> 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

Fixed.

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

OK -- fixed (and tested).

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

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

This is (almost) equivalent to

((* . (3))
 ((1 16) . (4 4 4))
 ((1 32) . (8 8 8))
 ((1 64) . (16 16 16))
 ((1 128) . (32 32 32)))

but it's written more succinctly.

(At least it's equivalent, as far as I can determine.)

As far as beaming is concerned, it's equivalent.  But the
measure-grouping-engraver uses the default for doing measure
grouping.  So I changed my mind and went to the settings
listed above.


> 
> http://codereview.appspot.com/88155/diff/3101/4041#newcode155
> Line 155: ;;;; Functions for overriding beam settings
> indentation of function bodies

I think I've got it right now.
> 
> 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

Fixed.

> 
> http://codereview.appspot.com/88155