The old variable definitions followed the corresponding words in the options. I see the bug ...
10 years, 4 months ago
(2014-01-01 04:21:51 UTC)
#1
The old variable definitions followed the corresponding words in the options.
I see the bug in the old code, so left-to-right did not behave symmetrically to
right-to-left.
If the actual behavior fits the chosen options with your change, then good
enough.
https://codereview.appspot.com/42000044/diff/1/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (left):
https://codereview.appspot.com/42000044/diff/1/lily/axis-group-interface.cc#o...
lily/axis-group-interface.cc:739: || (directive == ly_symbol2scm
("left-to-right-polite")));
The original l2r meant that one of the two "left-to-right-*" options was chosen.
Below, it means the objects closer to the left end of a line are placed before
those further right, so the items on the left tend to go closer to the staff
when there are overlaps.
https://codereview.appspot.com/42000044/diff/1/lily/axis-group-interface.cc#o...
lily/axis-group-interface.cc:790: if (x_extent[LEFT] <= last_end[dir] && polite)
The old "polite" meant that if we are about to place an object that would
overlap one just placed, ask this object wait politely until we first to place
all objects that fit without overlap.
Placing each object on the first pass was called "greedy".
(The old logic here was not quite right, because if we are placing from right to
left -- the old 'ltr==false' -- then the test for overlap concerns the
x_extent[RIGHT] of the object we are about to place. It should have been
x_extent[ltr?LEFT:RIGHT] and the converse below. As it is, the loop is
quadratic, but does not hurt too badly because it handles objects in just one
after-breaking line.)
Keith, first of all: i apologize for the delay. I shouldn't have put this up ...
10 years, 3 months ago
(2014-01-12 22:36:32 UTC)
#2
Keith,
first of all: i apologize for the delay. I shouldn't have put this up for
review when being too busy to reply in time, it's my mistake. I thought the
patch was obvious (while it wasn't, and in fact i have misunderstood the code) -
i should know better now...
On 2014/01/01 04:21:51, Keith wrote:
> The old variable definitions followed the corresponding words in the options.
>
> I see the bug in the old code, so left-to-right did not behave symmetrically
to
> right-to-left.
Indeed, it seems to be a bug.
> If the actual behavior fits the chosen options with your change, then good
> enough.
No, your comments made me realize that i was mistaken. My patch is wrong.
> https://codereview.appspot.com/42000044/diff/1/lily/axis-group-interface.cc
> File lily/axis-group-interface.cc (left):
>
>
https://codereview.appspot.com/42000044/diff/1/lily/axis-group-interface.cc#o...
> lily/axis-group-interface.cc:739: || (directive == ly_symbol2scm
> ("left-to-right-polite")));
> The original l2r meant that one of the two "left-to-right-*" options was
chosen.
> Below, it means the objects closer to the left end of a line are placed
before
> those further right, so the items on the left tend to go closer to the staff
> when there are overlaps.
I'm dumb as a brick. I misunderstood the code...
>
https://codereview.appspot.com/42000044/diff/1/lily/axis-group-interface.cc#o...
> lily/axis-group-interface.cc:790: if (x_extent[LEFT] <= last_end[dir] &&
polite)
> The old "polite" meant that if we are about to place an object that would
> overlap one just placed, ask this object wait politely until we first to place
> all objects that fit without overlap.
>
> Placing each object on the first pass was called "greedy".
>
> (The old logic here was not quite right, because if we are placing from right
to
> left -- the old 'ltr==false' -- then the test for overlap concerns the
> x_extent[RIGHT] of the object we are about to place. It should have been
> x_extent[ltr?LEFT:RIGHT] and the converse below. As it is, the loop is
> quadratic, but does not hurt too badly because it handles objects in just one
> after-breaking line.)
Indeed, old code seems to be wrong, but my patch was wrong as well (i.e. it was
based on a misunderstanding and it's results weren't intended). Thank goodness
you reviewed it and stopped me from pushing! I've made an idiot out of myself.
thanks a lot,
Janek
Issue 42000044: Swap 'polite' and 'l2r' variable definitions
Created 10 years, 4 months ago by janek
Modified 10 years, 3 months ago
Reviewers: Keith
Base URL:
Comments: 2