Created attachment 115557 [details] [review]
Nautilus patch for this bug

Modified get_rubberband function to have three arguments as we now need also the previous focused icon..

Created attachment 115558 [details] [review]
Eel patch for this bug

Add new function to obtain a rectangle that can contain the two rectangles passed as parameters.

Thanks for your efforts Nelson!

Maybe you could briefly sketch the precise policy you are implementing with this code? For instance, I do not understand what eel_direct_superset is supposed to do, since it takes the MAX of lower boundaries and the MAX of upper boundaries of both rectangles. What is the corresponding geometric operation?

The current policy is to draw a rectangle from the icon where you first pressed the shift key to the currently focused icon (as denoted by the dash line surrounding it), i.e. you effectively have keyboard rubberbanding and define two of its corners.

If I get you right in the initial bug report, you now want to have a wrap-around behavior at the end of lines. I like the idea, but a full analysis of some selection scenarios before hacking could help :).

Created attachment 115611 [details]
sketch for the example case commented above

Hi Christian!, I made a sketch of the example I commented above, in the sketch:

 - The blue , green and pink files are, in that order, the three icons passed to get_rubberband function.. before it only passed the blue and green files icons which correspond to start and last icon of the selection..

 - Before, the selection returned by get_rubberband was the green rectangle that contains the blue and green files, was calculated with eel_drect_union (&ret, &rect1, &rect2);

 - Now, we calculate the two green rectangles, which are the union between the first(blue) and last icon selected(green) and the first(blue) and previous_to_last icon selected(pink).Corresponding code is:
eel_drect_union (&union1_2, &rect1, &rect2);
eel_drect_union (&union1_3, &rect1, &rect3);

- Then we pass the two green rectangles to the eel_drect_superset function which returns us the red rectangle, which is, a rectangle that contains the two rectangles passed as parameters, so is a superset of those rectangles. That red rectangle is what get_rubberband function returns so other code selects all files in that area.Corresponding code is:
eel_drect_superset (&ret, &union1_2, &union1_3);
return ret;

In non-manual layout, the example above(and the corresponding RTL one) is the only interesting scenario.

In manual layout (like the Desktop window) it could be more possible scenarios but this patch won't affect that because it first needs to have the same behaviour we implement in bug 316117 , I've just discovered that that fix don't work for manual layout, will file a bug about that.

Thanks for the visualization. Now that's an issue where I'd prefer personal contact instead of writing an essay...

I do still do not understand the geometrical operation you are implementing with eel_drect_superset(). As I pointed out before, it just contains MAX() calls for calculating the boundaries in the case where both of the rectangles are not empty.

Geometrically, this means that you make a union wrt the RHS and bottom boundary (by taking the largest x2/y2) and an intersection wrt the LHS and top boundary (by taking the largest x1/y1). This can not be right and probably fixes the issue by accident, and I'm sure it introduces other selection bugs. Again: I'm all for selection interaction improvements, but I don't like how you tried to solve the issue, because it does not look well thought-out.

Let's try to formulate first what the actual issue is:

* The rubberbanding works by defining two corners of a rectangle.
* In the case where the last line is filled, it always works as expected.
* In the case where the last line is not filled, the behavior depends on the way the user defines the rectangle (we'll treat the LTR case):
(A) if you first define the top-right (or the bottom-left) corner, and then move to the bottom-left (or top-right), this also works as expected. In your example, you could for instance select F6 and then move straight down
(B) if you define the top-left corner (i.e. F4), and then move to the bottom-right, you can not define a rectangle spanning F4, F5, F6, F7, i.e. the L shape.

Can we agree that the actual issue is described correctly as case (B)?

Now, we just have to specify the precise desired selection policy. The rectangle definition obviously has to fail for these last lines [it will also fail in the compact view].

For instance, we internally always define the rectangle through its top-right and bottom-left corners (for RTL, vice versa). This needs some serious thinking, and I'm looking forward to your proposals :).

(In reply to comment #5)
[..]
> but this patch won't affect that because it first needs to have the same
> behaviour we implement in bug 316117 , I've just discovered that that fix don't
> work for manual layout, will file a bug about that.

 Well, it's not certainly a bug, it's just that in manual layout the fuction to
focus the following icon is "closest_in_90_degrees" while in non-manual layout,
for pressing the cursor down key, it's "same_column_below_highest" function.

So while they have different logic to select the next icon, both manual and
non_manual layout shares the same logic for SHIFT + selecting, that is the
get_rubberband function, that function is used by both modes.

With my patch I'm modifying the get_rubberband function to give it a
wrap-around[1] behaviour (as Christian said), and I think this would benefit
both modes, although specially the non-manual mode because its select_next_icon
logic it's more similar to this wrap-around behaviour. For the manual layout
this wrap-around just makes it to some times select some more icons than
before, depending on the situation of the previous_to_last selected icon.. 

[1] really it's just a wrap around the previous_to_last icon selected, not a
full wrap around of all the files you're passing through with SHIFT key
pressed..

P.D. I'll probably file a new bug report to improve the cursor key navigation
in the manual layout (at least a glitch I've seen)... 

I think I just fixed this by moving to linear selection when pressing shift, instead of rectangular selection (which is still available with ctrl-shift):

http://svn.gnome.org/viewvc/nautilus?view=revision&revision=14435

I also removed the wrap-around for the rectangle selection, since it is confusing. I'm closing the bug report for now. Feel free to reopen it.

Note that there is still a small issue in vertical layout mode, where we do not move from the penultimate column to the last one when pressing the right key in a row below the last filled row of the last column.

Ok,, will test your fix on monday because I have my linux box at work..  thanks for working on this..

(In reply to comment #8)
> I think I just fixed this by moving to linear selection when pressing shift,
> instead of rectangular selection (which is still available with ctrl-shift):
> 
> http://svn.gnome.org/viewvc/nautilus?view=revision&revision=14435

 Ok, thanks Christian!, indeed it fixes the bug..