Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2555)

Issue 42000044: Swap 'polite' and 'l2r' variable definitions

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 4 months ago by janek
Modified:
10 years, 3 months ago
Reviewers:
Keith
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Swap 'polite' and 'l2r' variable definitions Obviously they were misplaced before.

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -2 lines) Patch
M lily/axis-group-interface.cc View 1 chunk +2 lines, -2 lines 2 comments Download

Messages

Total messages: 2
Keith
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
janek
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b