Re: comctl32: Scroll listview when performing a marquee selection

[Nikolay Sivov]

Owen Rudge wrote:
> This patch adds support for scrolling a listview when performing a 
> marquee selection. In Windows, a listview will scroll around when 
> dragging and highlighting icons, picking up speed as you move the 
> mouse further past the window.

Hi, Owen.

This patch is rather large, so few comments:
> - RECT marqueeRect;         /* absolute coordinates of marquee selection */
> - RECT marqueeDrawRect;     /* relative coordinates for drawing marquee */
> - POINT marqueeOrigin;      /* absolute coordinates of marquee click origin */

I don't think you need such duplication. Why not to use a single 
rectangle? Looks like you always offset it to listview origin,
so these rectangle differ in offset only, that's why you introduced 
another iterator_* helper. If I'm right about that it's better to
add another OffsetRect than consistently maintain two variables with 
same meaning.

comctl32: Scroll listview when performing a marquee selection

[Owen Rudge]

Hi Nikolay,
> I don't think you need such duplication. Why not to use a single 
> rectangle? Looks like you always offset it to listview origin,
> so these rectangle differ in offset only, that's why you introduced 
> another iterator_* helper. If I'm right about that it's better to
> add another OffsetRect than consistently maintain two variables with 
> same meaning.

I think I originally found myself implementing it in this manner, but 
ended up duplicating the offsetting code in a couple of locations while 
drawing/invalidating, so reworked it. However, the patch did grow a bit 
since then, a bit more than I think I expected it would, so it probably 
would be tidier to go back and simplify that again, with a couple of 
OffsetRect calls where appropriate. I'll have a look at tidying it up a 
bit more tomorrow.

Cheers,

Owen

comctl32: Scroll listview when performing a marquee selection

[Nikolay Sivov]

Owen Rudge wrote:
> Hi Nikolay,
> > I don't think you need such duplication. Why not to use a single 
> > rectangle? Looks like you always offset it to listview origin,
> > so these rectangle differ in offset only, that's why you introduced 
> > another iterator_* helper. If I'm right about that it's better to
> > add another OffsetRect than consistently maintain two variables with 
> > same meaning.
>
> I think I originally found myself implementing it in this manner, but 
> ended up duplicating the offsetting code in a couple of locations 
> while drawing/invalidating, so reworked it. However, the patch did 
> grow a bit since then, a bit more than I think I expected it would, so 
> it probably would be tidier to go back and simplify that again, with a 
> couple of OffsetRect calls where appropriate. I'll have a look at 
> tidying it up a bit more tomorrow.

Sure. Another thing, I'm not sure about that, maybe you could tell me - 
if a window loses focus with active timer, what happens? First time I 
thought about that a month ago working on Monthcal. I know that it isn't 
every day case, but probably we need just to KillTimer on WM_KILLFOCUS 
or WM_CAPTURECHANGED...Or LButtonUp event will be fired anyway if I 
didn't even release button?
> Cheers,
>
> Owen

comctl32: Scroll listview when performing a marquee selection

[Owen Rudge]

Hi Nikolay,
> I don't think you need such duplication. Why not to use a single 
> rectangle? Looks like you always offset it to listview origin,
> so these rectangle differ in offset only, that's why you introduced 
> another iterator_* helper. If I'm right about that it's better to
> add another OffsetRect than consistently maintain two variables with 
> same meaning.

On second thoughts, having looked at this a bit, I think that if I 
weren't to have the two separate rectangles, I would still need to 
record the previous offset.

In the MarqueeHighlight function, the old draw rectangle is invalidated 
(which has been offsetted by the previous call), and then the old 
absolute rectangle is used for the inversion of the selected items. Then 
the new rectangle is created, and is adjusted by the present offset.

If we wanted to store only one rectangle, the previous offset - which 
would differ from the present offset - would also need to be stored, in 
order to cancel out the marquee draw and the item selection, which use 
different figures.

So it's either two rectangles being stored, or one rectangle with the 
previous offset. So I may as well leave it as it is, I think.

It might of course just be that I've been staring at this bit of code 
too long and have missed something obvious!

I'll submit a new patch anyway that adds in a check in WM_KILLFOCUS to 
stop a marquee selection if focus is lost.

Cheers,

Owen