Re: Swap 'polite' and 'l2r' variable definitions (issue 42000044)

[janek . lilypond]

Reviewers: Keith,

Message:
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#oldcode739
> 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#oldcode790
> 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

Description:
Swap 'polite' and 'l2r' variable definitions

Obviously they were misplaced before.

Please review this at https://codereview.appspot.com/42000044/

Affected files (+2, -2 lines):
  M lily/axis-group-interface.cc


Index: lily/axis-group-interface.cc
diff --git a/lily/axis-group-interface.cc b/lily/axis-group-interface.cc
index  
0a6155b6eb25ab7b997e87984fcec5f340772a33..11e6ccfce4b03cdbaea5361f3d30a1babf0091b8  
100644
--- a/lily/axis-group-interface.cc
+++ b/lily/axis-group-interface.cc
@@ -735,10 +735,10 @@ add_grobs_of_one_priority (Grob *me,
   SCM directive
     = valid_outside_staff_placement_directive (me);

-  bool l2r = ((directive == ly_symbol2scm ("left-to-right-greedy"))
+  bool polite = ((directive == ly_symbol2scm ("left-to-right-greedy"))
               || (directive == ly_symbol2scm ("left-to-right-polite")));

-  bool polite = ((directive == ly_symbol2scm ("left-to-right-polite"))
+  bool l2r = ((directive == ly_symbol2scm ("left-to-right-polite"))
                  || (directive == ly_symbol2scm ("right-to-left-polite")));

   vector<Box> boxes;