Re: [1/2] comctl32: Implement highlighting (marquee) selection support in listview
Owen Rudge wrote:
These patches add support for selecting multiple items in a multi-selection listview using the mouse (by dragging and highlighting).
--- dlls/comctl32/listview.c | 172 ++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 157 insertions(+), 15 deletions(-) Hi, Owen.
Some comments. --- @@ -201,6 +201,7 @@ typedef struct tagITEM_INFO LPARAM lParam; INT iIndent; ITEM_ID *id; + UINT orig_state; /* state before highlighting operation begins */ } ITEM_INFO; --- Why do you need this restoring code? If you're starting selection box with some items selected they should be deselected if not Shift pressed and leaved untouched if Shift used. I don't see a reason to restore it - when you want to toggle selected state moving rectangle with Ctrl pressed you could just: LISTVIEW_GetItemState() and place inverted state with following LISTVIEW_SetItemState(). Also such large cycles shouldn't be used if you could manage not to use them. Currently we're not doing right sometimes with ownerdata lists which leads to huge response times (see http://bugs.winehq.org/show_bug.cgi?id=14303). Anyway this part: --- + /* Take a snapshot of all the item states */ + for (i = 0; i < infoPtr->nItemCount; i++) + { + hdpaSubItems = DPA_GetPtr(infoPtr->hdpaItems, i); + lpItem = DPA_GetPtr(hdpaSubItems, 0); + + if (lpItem) + lpItem->orig_state = lpItem->state; + } --- is wrong. You can't do it that way cause selected state could also be cached at parent side with corresponding callback bit set (look at LISTVIEW_GetItemT()). P.S. No need to split in two patches here, fist is useless if we can't see rectangle.
Hi Nikolay,
Why do you need this restoring code?
I tried to replicate the way that Windows did the selections. Testing various manners of selection with Windows Explorer, that code was needed for a relatively specific scenario: If you're starting a selection box with some items already selected and Shift is pressed, then the items should, as you say, remain selected. If you then highlight some different items, but then un-highlight them before releasing the mouse button, the original items should remain selected. However, if in that time you highlight some of the already highlighted items, and then unhighlight them, they should not remain highlighted. Due to the fact that, with this code, I'm deselecting all the items (if neither shift nor ctrl are pressed) whenever the bounding box is altered, we obviously need to keep a record of how the items were highlighted before the selection was started. The alternative would be, I believe, to maintain a list of all the items that have become highlighted or de-highlighted during the selection process, so that they can be restored if need. This would probably be more efficient, at least with large numbers of items, but it would be more complex to implement. If need be, of course, I can try to reimplement the code in this manner.
If you're starting selection box with some items selected they should be deselected if not Shift pressed and leaved untouched if Shift used. I don't see a reason to restore it - when you want to toggle selected state moving rectangle with Ctrl pressed you could just:
LISTVIEW_GetItemState() and place inverted state with following LISTVIEW_SetItemState().
The issue with doing it this way is it would result in massive flickering every time you moved the mouse, unless I perhaps misunderstood you. Otherwise I guess there would have to be some way of tracking that the pointer is still within the hit area of a certain item, and has not moved out from that area, and hence the state should not be inverted again.
Also such large cycles shouldn't be used if you could manage not to use them.
See above - but that was indeed a concern of mine. I tested with some reasonably large collections of items (in the hundreds), and it did not seem noticeably slow, but I admit to not trying with thousands if items.
Anyway this part: ... is wrong. You can't do it that way cause selected state could also be cached at parent side with corresponding callback bit set (look at LISTVIEW_GetItemT()).
I'll look into that, thanks.
P.S. No need to split in two patches here, fist is useless if we can't see rectangle.
I can of course combine the two patches. However, the first isn't actually useless on its own - even if the rectangle can't be seen, it is obvious where the selection box is due to the highlighting of the icons. :) Thanks for your comments! -- Owen Rudge http://www.owenrudge.net/
Owen Rudge wrote:
Hi Nikolay,
Why do you need this restoring code?
I tried to replicate the way that Windows did the selections. Testing various manners of selection with Windows Explorer, that code was needed for a relatively specific scenario:
If you're starting a selection box with some items already selected and Shift is pressed, then the items should, as you say, remain selected. If you then highlight some different items, but then un-highlight them before releasing the mouse button, the original items should remain selected. However, if in that time you highlight some of the already highlighted items, and then unhighlight them, they should not remain highlighted. Exactly. So why should we deselect everything starting this selecting rectangle (when some threshold distance from initial click position reached)? Why not just let initially selected items as they are and simply alter their state when selection touches them?
Due to the fact that, with this code, I'm deselecting all the items (if neither shift nor ctrl are pressed) whenever the bounding box is altered, This is a problem. I don't think native does it this way - deselecting all already selected items seems quite strange. And this will flicker as a side effect I suppose. This could be easily seen cause native doesn't send any notification while moving selection rectangle without item change. You could of course silence notifications in this case as a workaround too :), but this approach will turn code in crap in future.
Could we try it this way (just a proposal): selecting rectangle has only two working sides with and angle at current cursor position. Each resize leads to two smaller rectangles resize - formed these two sides and previous selection rectangle (that you introduced in second patch). Remaining rectangle stays as it was. What if test for items to change using these two rectangles? Two iterator_frameditems() calls will return items to change, next goes rule to toggle or select/deselect item depending on keys state.
we obviously need to keep a record of how the items were highlighted before the selection was started.
The alternative would be, I believe, to maintain a list of all the items that have become highlighted or de-highlighted during the selection process, so that they can be restored if need. This would probably be more efficient, at least with large numbers of items, but it would be more complex to implement. If need be, of course, I can try to reimplement the code in this manner. You're speaking about selection ranges here which aren't lists, but an efficient in some sense storage for selections.
If you're starting selection box with some items selected they should be deselected if not Shift pressed and leaved untouched if Shift used. I don't see a reason to restore it - when you want to toggle selected state moving rectangle with Ctrl pressed you could just:
LISTVIEW_GetItemState() and place inverted state with following LISTVIEW_SetItemState().
The issue with doing it this way is it would result in massive flickering every time you moved the mouse, unless I perhaps misunderstood you. Why will it flicker? If state didn't change LISTVIEW_SetItemState is smart enough to do not any invalidation. Otherwise I guess there would have to be some way of tracking that the pointer is still within the hit area of a certain item, and has not moved out from that area, and hence the state should not be inverted again. Same selection rule applies here, as for all other items called iterator_frameditems() - don't know if it's nice to use in this case though, cause some intersection allowed here while moving selection rectangle with mouse. You could tweak this by the way passing a bit altered rectangle to this helper (not equally selection rectangle).
Also such large cycles shouldn't be used if you could manage not to use them.
See above - but that was indeed a concern of mine. I tested with some reasonably large collections of items (in the hundreds), and it did not seem noticeably slow, but I admit to not trying with thousands if items. It's something like a known const memory amount that should suffice everyone needs in future ). If we could avoid this we certainly should.
Anyway this part: ... is wrong. You can't do it that way cause selected state could also be cached at parent side with corresponding callback bit set (look at LISTVIEW_GetItemT()).
I'll look into that, thanks.
participants (2)
-
Nikolay Sivov -
Owen Rudge