[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (i
From: |
janek . lilypond |
Subject: |
Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044) |
Date: |
Sun, 28 Apr 2013 21:59:37 +0000 |
changes i see generally LGTM (but i may be missing something, in
particular Keith seems to make a good point).
Let Stencil::is_empty use Box::is_empty [...]
I think the desription of this commit could use some more detail, for
example that we need (1 . -1) stencils for backspacing.
Let Stencil::add_at_edge skip offsets
for half-empty stencils [...]
I think "half-empty" is confusing (i at least was confused). I think
"empty along only one axis" would be clearer.
Let Stencil::align_to check axis-specific
emptiness before punting
could you use a more odrinary word? I had to check 'punt'
in a dictionary, and i consider my English to be fairly good.
https://codereview.appspot.com/8869044/diff/9001/lily/box.cc
File lily/box.cc (right):
https://codereview.appspot.com/8869044/diff/9001/lily/box.cc#newcode66
lily/box.cc:66: return interval_a_[a][LEFT] == empty[LEFT]
Do we need to define 'empty'? I.e. why not just write
interval_a_[a][LEFT] == infinity_f ?
Are we expecting that the definition of empty extent will change
further?
https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc
File lily/stencil.cc (right):
https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc#newcode253
lily/stencil.cc:253:
whitespace error
https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc#newcode259
lily/stencil.cc:259: offset += d * padding;
I think this is worth a comment with an example. It's not obvious why
sometimes we want padding and sometimes not.
https://codereview.appspot.com/8869044/diff/9001/scm/markup.scm
File scm/markup.scm (right):
https://codereview.appspot.com/8869044/diff/9001/scm/markup.scm#newcode67
scm/markup.scm:67: between the end of each stencil and the reference
point of the following
Hmm, shouldn't this read "between the end of the stencil and the
*beginning* of the following stencil"?
https://codereview.appspot.com/8869044/
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044), janek . lilypond, 2013/04/28
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044), dak, 2013/04/28
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044), k-ohara5a5a, 2013/04/28
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044),
janek . lilypond <=
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044), k-ohara5a5a, 2013/04/28
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044), dak, 2013/04/28
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044), dak, 2013/04/28
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044), k-ohara5a5a, 2013/04/28
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044), janek . lilypond, 2013/04/29
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044), janek . lilypond, 2013/04/29
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044), k-ohara5a5a, 2013/04/29
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044), dak, 2013/04/29
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044), dak, 2013/04/29
- Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044), k-ohara5a5a, 2013/04/29