On 7/22/09 4:15 PM, "joenee...@gmail.com" <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" <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 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
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
