Opened 15 years ago

Closed 8 years ago

#626 closed defect (fixed)

[MSW] Wrong selection events in multi selection tree control

Reported by: haribeau Owned by:
Priority: normal Milestone:
Component: wxMSW Version:
Keywords: Cc: haribeau, hansvl, ghazel, hockkn, leio, wx@…, wxscary@…, net147@…
Blocked By: Blocking:
Patch: yes

Description

Situation: wxListCtrl/wxTreeCtrl each with multiselect
allowed (wxTR_MULTIPLE style) on Win32

when I press the ctrl-button during selecting icons with
the mouse each unselected icon is added (when
clicked) and each selected icon is deselected (when
clicked).

Problem: when I deselect an icon, I do not get a
EVT_TREE_SEL_CHANGED. The reason seems to be
that the deselected item still has the focus and so it is
still seen as selected, even if it was deselected. The
problem is similar on wxListCtrl and wxTreeCtrl.

the problem can be seen on the wxListCtrl demo.

Attachments (6)

treectrl_events_fix.patch download (55.0 KB) - added by net147 8 years ago.
Events patch
treectrl_events_fix_trunk_r58959_multiple_selection.patch download (56.6 KB) - added by net147 8 years ago.
Events patch for multiple selection only
tree_events_fix.diff download (6.7 KB) - added by vadz 8 years ago.
Fix events generation (against r59332)
treectrl_alt_fix_trunk_59369.diff download (636 bytes) - added by net147 8 years ago.
ALT key fix.
treectrl_setfocus_fix_trunk_r59406.patch download (2.5 KB) - added by net147 8 years ago.
SetFocus fix.
treectrl_events_fix_trunk_r59406.patch download (16.3 KB) - added by net147 8 years ago.
Events patch for r59406.

Download all attachments as: .zip

Change History (57)

comment:1 Changed 13 years ago by hansvl

1.) wxTR_MULTIPLE is a style only for wxTreeCtrl, it should
not be used for wxListCtrl as it has a different meaning there.
2.) Also EVT_TREE_SEL_CHANGED is wxTreeCtrl only,
EVT_LIST_ITEM_SELECTED/EVT_LIST_ITEM_DESELECTED is used for
a wxListCtrl.
3.) I don't see the problem in the wxListCtrl demo (wxMSW
2.5.x from CVS) like stated... Could be I'm doing something
wrong as the explanation on how to reproduce it is a bit
cryptic/vague.

For what version of wxWindows was this? And can you still
reproduce the problem with 2.4.x or 2.5.x? And was this
problem only seen with wxMSW or also with other ports (I'm
asking because the bug category is set to common and you
only speak of Win32 in the bug report)?

comment:2 Changed 13 years ago by hansvl

Ok, I just checked the wxTreeCtrl sample and can confirm
that wxTreeCtrl is not reporting a selection changes (=
generating a
wxEVT_COMMAND_TREE_SEL_CHANGING/wxEVT_COMMAND_TREE_SEL_CHANGED
event) when deselecting (using Ctrl+mouse click) the last
selected item (i.e. the item with focus box)...

comment:3 Changed 11 years ago by ghazel

In WX_2_6_BRANCH today I noticed the following behaviour:
wxListCtrl works as expceted in every way (surrounding this
issue, obviously).

wxTreeCtrl:

  • fires SEL_CHANG(ED/ING) when the -focus- of items

changes. Hold Ctrl and use the arrow keys, you'll see
events even though the highlighted selection is not
changing (and should not change, the bug is the events)

  • does not fire SEL_CHANG(ED/ING) when selecting or

unselecting the focused item (as documented by hansvl),
however GetSelections() reports the correct items if you
check periodically, in an Idle handler for example.

So penidng a fix, at the very least the events could be
called
wxEVT_COMMAND_TREE_FOCUS_CHANGING/wxEVT_COMMAND_TREE_FOCUS_C
HANGED to be consistant with the wxListCtrl terminology.

comment:4 Changed 11 years ago by vadz

The problems are still present in 2.7.2 wxTreeCtrl.

comment:5 Changed 9 years ago by mspacek

  • Cc wx@… added
  • Status changed from new to confirmed

See also #2307

comment:6 Changed 9 years ago by mspacek

#4448 regarding EVT_LEFT_DOWN is probably related

comment:7 Changed 9 years ago by Scary

  • Cc wxscary@… added

comment:8 Changed 9 years ago by Scary

I have added a patch to #9570 which rewrites the multi-select functionality on MSW for wxTreeCtrl. Please give it a try an let me know what you think. Thanks a lot.
-Chris Edwards

comment:9 Changed 8 years ago by net147

  • Cc net147@… added

comment:10 Changed 8 years ago by net147

  • Patch set

I've attached a patch which attempts to fix the issues defects described by this ticket as well as the following additional tickets:
#2307 - wxTreeCtrl + wxTR_MULTIPLE flag: SEL_CHANGED only fires on item focus change
#4448 - wxTR_MULTIPLE breaks EVT_LEFT_DOWN on MS Windows
#9570 - wxTreeCtrl bugs (SVN version from beginning of june)

The patch is against the latest 2.8 stable branch.
Any comments and additional testing of this patch are welcome.

comment:11 Changed 8 years ago by net147

Patch updated to fix triggering of label edit for multiple selection so when you click on an item, click on another control and click back on the selected item it will not trigger a label edit.

comment:12 Changed 8 years ago by net147

Some of the main differences between the attached treectrl_events_fix.patch and the patch in #9570 are:
-Selection changing event behaviour is not changed
-Focus drawing behaviour is not changed
-The selected items when the selection changing event is triggered is now correct when previously the wxTreeCtrl would sometimes trigger the event when the selection was in an intermediate state of being changed

-Jonathan Liu

comment:13 Changed 8 years ago by net147

I've updated my patch against the latest SVN trunk.

The patch rewrites handling of selection events so that it does not rely on selection change events from Windows. This seems to be needed as relying on Windows events causes problems with multiple selection events.

As the events generated by Windows occur when there is a focus change rather than a selection change, the state of selected items when event is handled may be in an intermediate state of change when the focus has changed but before the selection change has completed.

The nature of selection change events on Windows also makes difficult to handle vetoing of a range selection. For example if you have multiple items selected and you click a single item then that would cause an unselect all followed by a select. Vetoing the selection changing event from Windows would not veto the unselect all operation.

Summary of bugs fixed by this patch with multiple selection mode:

  • User interface
    • Clicking on a single item, clicking on another control then clicking back on same item causes label edit to start when it shouldn't
    • Pressing up arrow key when first item selected causes deselection of all items
    • Pressing down arrow key when last item selected causes deselection of all items
  • Selection state of items during selection changing event
    • Expanding selection using shift arrow keys causes old item to be unselected
    • Moving focus using CTRL + arrow keys causes two events
      • First event: old focused item is unselected
      • Second event: selection states are now correct
    • If one item is selected and selection is expanded using SHIFT + down arrow:
      • Holding ALT and pressing down causes all selected items except the focused item to be selected
  • Missing events
    • Selecting multiple items and then clicking on the focused item does not generate a selection changing event
  • Event handling
    • Vetoing a selection change event just prevents change in focus not change in selection
  • Jonathan Liu

comment:14 follow-up: Changed 8 years ago by vadz

  • Cc vadz removed

Jonathan, thanks a lot for your patch and detailed explanations, they were very helpful.

I do see that your patch fixes the problems mentioned above (as a side note, the first one, with edit starting incorrectly, also exists in the generic version), thanks! However unfortunately it also changes the behaviour in single selection mode and I'd like to avoid this. To be precise, now (testing in the treetest sample):

  1. The selection changing/changed events are generated on left mouse down, not up and the visual feedback is different too (focus rectangle doesn't remain around the previously selected item as long as the mouse is pressed).
  2. Selecting item A and right clicking item B changes selection to B while originally it only visually highlighted B but returned the highlight to A on right mouse up and (correctly) didn't generate any events.

So I wonder if it would be possible to keep the old behaviour for !wxTR_MULTIPLE case? I think it would be easier for you to update your patch to do it rather than for me to try to do it myself, wouldn't it?

And if you do redo the patch, a couple of minor comments on the patch itself:

  1. SelectRange() calls are even more readable now with 2 boolean parameters and not one. As usual I'd prefer to have a single flags parameter which could be a combination of SELECT_ONLY and SELECT_SIMULATE or something like this.
  2. Style quibbles: could you please avoid writing the body of if statements on the same line and, when you do write it on the next line(s) put the opening curly brace on a line by its own?

Thanks in advance!

comment:15 Changed 8 years ago by net147

Much of the behaviour of this patch is based on the observations from the generic wxTreeCtrl with wxGTK. The generic wxTreeCtrl generates the selection changing/changed events after left mouse down and before left mouse up. I will update my patch with the behaviour you mentioned though because that's the standard behaviour for TreeView control on Windows.

I'll fix the other things you mentioned as well.

Should I modify the code to use the original code for !wxTR_MULTIPLE?

With the default wxTreeCtrl behaviour in single selection mode, I noticed deleting all items would sometimes cause selection changing/changed events.

Test case

  1. Run wxTreeCtrl sample
  2. Expand Root
  3. Expand Folder child 3
  4. Click File child 3.3
  5. Tree -> Delete all items

For the above test, selection changing/changed events would only be generated if hidden root is enabled. Even then I don't see how these events would be useful if they don't always occur when an item is selected when delete all items is called.

Using common code for both single selection and multiple selection would reduces inconsistencies in behaviour in cases such as these.

comment:16 Changed 8 years ago by vadz

I don't have any definitive answer to the question of whether we should use the original code for !wxTR_MULTIPLE case but I believe that this indeed would be better as it would allow us to remain as native as possible, even if the future Windows versions change how the (single item) selection works in this control. It would IMO be hard to emulate Windows behaviour exactly ourselves and it would be simpler to filter out the unwanted events (like the ones happening during deletion) instead.

Thanks again!

comment:17 Changed 8 years ago by net147

I've stumbled upon an issue with using the default handler for wxTreeCtrl. Using the default mouse handler creates these bugs in both single selection and multiple selection mode:

  • Left mouse up event not received when left-clicking an item
  • Right mouse up event not received when right-clicking an item

I don't know a way to fix these without overriding the default mouse processing for wxTreeCtrl which overrides the default behaviour of wxTreeCtrl with my own code (which emulates Windows TreeView control behaviour).

Any ideas for a workaround for this that doesn't involve overriding default mouse processing (and hence having to write code which emulates TreeView behaviour) for both selection modes?

comment:18 Changed 8 years ago by vadz

Sorry, I don't understand the problem, why can't you let the default handler execute only in single selection mode and not run it when wxTR_MULTIPLE?

Maybe you could post to wx-dev to discuss this further?

TIA!

comment:19 Changed 8 years ago by net147

Using the default handler for single selection causes left mouse up and right mouse up events to not be generated.

Changed 8 years ago by net147

Events patch

comment:20 in reply to: ↑ 14 Changed 8 years ago by net147

Replying to vadz:
I've fixed the issues you mentioned, implemented the suggestion to use a single flags parameter for SelectRange and fixed the code style. Single selection still does not use the original code because left mouse up and right mouse up events are not being generated correctly when clicking an item. This can be seen in the treectrl sample using 2.9 SVN.

comment:21 Changed 8 years ago by vadz

I think left mouse up events can be synthesized from NM_CLICK notification. Or we could just add new ITEM_CLICK event and recommend using it instead of LEFT_UP. As for the right up ones, it was a conscious choice to live without them as they're not very important because we already have ITEM_RIGHT_CLICK and CONTEXT_MENU events anyhow (which are better/more convenient) and the native behaviour was deemed to be more important.

IOW I'd still prefer to use the default handler for single selection case if possible.

Thanks again!

comment:22 Changed 8 years ago by net147

In that case would it be a good idea to omit the mouse up events for multiple selection mode so it is more consistent with single selection mode in the cases where single selection doesn't omit mouse up events?

comment:23 Changed 8 years ago by vadz

Honestly, I don't know, I see arguments both for and against it. As long as it's clearly documented (although it's not clear just where exactly to document it...) that these events are not generated only for single selection mode or never at all both solutions are acceptable.

comment:24 Changed 8 years ago by net147

I've attached patch that uses original code for single selection and only overrides behaviour for multiple selection.

comment:25 Changed 8 years ago by net147

Multiple selection patch updated to fix missing item right click event and make multiple selection mouse up events consistent with single selection mode.

Changed 8 years ago by net147

Events patch for multiple selection only

comment:26 Changed 8 years ago by net147

Updated patch with the following additional fixes for issues that occur in 2.8/2.9 SVN:

  • Assertion failure when pressing enter in multiple selection mode
  • Assertion failure when pressing space or enter after unselect all in single selection mode
  • Can't activate item using space in multiple selection mode
  • Client object not set for events not generated from wxTreeCtrl::MSWOnNotify
  • Item activated event generated before key down event in single selection mode when pressing space or enter

comment:27 follow-up: Changed 8 years ago by Euan

In wxTreeCtrl::Unselect, you're asserting HasFlag(wxTR_MULTIPLE) but you have code that depends on that being false. Should this assert be changed?

comment:28 in reply to: ↑ 27 Changed 8 years ago by Euan

Replying to Euan:

HasFlag(wxTR_MULTIPLE)

(Sorry for the spam) Just to clarify, the "!" (not) in that line isn't appearing in trac so that comment is probably confusing.  I'm talking about line 1770 of src/msw/treectrl.cpp after applying the patch (line 1667 before patching).

comment:29 Changed 8 years ago by net147

That code is there so that the code will work properly regardless of whether the assert is ignored or not. If you want the code not to handle events properly if the user ignores the assert then you can remove the code that depends on wxTR_MULTIPLE being false. If the code is removed the events will be generated twice if the assert is ignored.

comment:30 Changed 8 years ago by net147

For example in release builds the wxASSERT_MSG won't do anything. In that case all assertions are ignored and I figured we still want it to behave correctly even in release builds when the assertions would be silently ignored.

comment:31 Changed 8 years ago by net147

In the code before patching, if the assertion is ignored the selection changing/changed events would still be generated correctly. The patch preserves this behaviour by still working correctly even if the assertion is ignored.

comment:32 Changed 8 years ago by Euan

Fair enough, put that down to cultural differences about asserts and release build behaviour. :-)

comment:33 Changed 8 years ago by vadz

  • Resolution set to fixed
  • Status changed from confirmed to closed

Thanks again for all your work, I've committed a slightly modified version of the patch as r59336. My changes were done mainly to simplify code a little (e.g. to avoid a 1000-line MSWWindowProc()) and more could still be done about it probably, e.g. I think mouse and keyboard handling should ideally be somehow rationalized. But in the meanwhile the patch seems to work well, please let me know if I broke anything while applying it.

Thanks!

comment:34 Changed 8 years ago by net147

  • Resolution fixed deleted
  • Status changed from closed to reopened

There is an issue with the committed patch in MSWWindowProc when handling WM_KEYDOWN:

        processed = HandleTreeEvent(keyEvent);
        if ( !processed )
        {
            // update the selection if key was left unprocessed
            processed = MSWHandleSelectionKey(wParam);
        }

The issue is that when you press a key in multiple selection mode that isn't handled by MSWHandleSelectionKey, the tree key down event will be generated twice - MSWHandleSelectionKey returns false which then triggers TVN_KEYDOWN after MSWWindowProc finishes.

We only want to generate a tree event here if we are handling the key in MSWHandleSelectionKey. For other keys (default branch label in MSWHandleSelectionKey), we don't generate a tree event and return false so it is then handled in TVN_KEYDOWN. The only way I could think of to achieve this was to put a HandleTreeEvent at the beginning of each branch in the switch statement except the default one.

Some keys (e.g. the context menu keyboard button) don't generate some events properly if handled in WM_KEYDOWN instead of TVN_KEYDOWN. The context menu button generates right mouse down and right mouse up events.

comment:35 follow-up: Changed 8 years ago by vadz

  • Status changed from reopened to infoneeded_new

I see, thanks, sorry for failing to realize this. I'm not sure if it's actually that important to generate key events from TVN_KEYDOWN handler rather than from WM_KEYDOWN one but it's probably better to do it like this just in case the tree control ever decides to do something smart (or stupid) here. So I've tried correcting the problem and the result is the attached patch (which also gets rid of some more code duplication) -- do you see anything wrong with it?

Thanks again!

Changed 8 years ago by vadz

Fix events generation (against r59332)

comment:36 in reply to: ↑ 35 Changed 8 years ago by net147

  • Status changed from infoneeded_new to new

Replying to vadz:

Your patch fixes the issue. Feel free to commit the patch.
Thanks for taking the time to work through the large patch.

Regards,
Jonathan

comment:37 Changed 8 years ago by vadz

  • Resolution set to fixed
  • Status changed from new to closed

Thanks for the review, committing.

P.S. Please don't hesitate to submit other patches to wxTreeCtrl selection code (or anything else, of course) -- if the patches are small they'd be applied more quickly, it took a long time to apply this one because it was so big.

comment:38 Changed 8 years ago by VZ

(In [59369]) fix duplicate events for selection keys after the last change (see #626)

comment:39 Changed 8 years ago by net147

  • Resolution fixed deleted
  • Status changed from closed to reopened

The ALT key now doesn't work properly when pressing ALT + another key.
The issue was caused by changeset [59369].

Patch attached to fix the issue.

Changed 8 years ago by net147

ALT key fix.

comment:40 Changed 8 years ago by VZ

(In [59406]) fix generation of key events with Alt pressed broken by r59369 (see #626)

comment:41 Changed 8 years ago by vadz

  • Resolution set to fixed
  • Status changed from reopened to closed

Thanks for finding and fixing this!

comment:42 Changed 8 years ago by net147

  • Resolution fixed deleted
  • Status changed from closed to reopened

There is an issue when calling SetFocus on wxTreeCtrl. When SetFocus is called, a selection change event is generated to select the first item if no items are selected. In multiple selection mode however, selection chang-ing/ed events aren't generated for this because I didn't expect the selection to change during SetFocus. I think SetFocus shouldn't change which items are selected.

Also when selecting the first item in the treectrl sample with single selection mode, two selection chang-ing/ed events are generated.

The attached patch fixes these issues.

comment:43 Changed 8 years ago by vadz

  • Status changed from reopened to confirmed

Sorry, I disagree with the change of behaviour of the single selection control again: now it's possible to have such control have focus but not show any selection which is wrong/inconsistent with all the other (single selection) Windows controls. A single selection control really should always have the selection.

I could apply the patch with a check for !HasFlag(wxTR_MULTIPLE) of course but I wonder if it's possible to do it in some better way for multiselection case only as your comment seems to imply. If I misunderstood you, please let me know and we'll use this patch but for multiselection controls only.

Thanks!

comment:44 Changed 8 years ago by net147

Yes I see that now. It just seems a bit odd in the treectrl sample if I select an item, unselect everything, click to another control and tab back to the tree control that is changes the selection. The generic tree control doesn't have this behaviour; it doesn't change the selection on SetFocus.

Applying for multiple selection only seems like the best way to handle it for now. Though it will generate extra events in single selection mode when you click the first item.

Changed 8 years ago by net147

SetFocus fix.

comment:45 Changed 8 years ago by net147

I've updated the patch to only modify multiple selection SetFocus behaviour.

Regards,
Jonathan

comment:46 Changed 8 years ago by net147

I think a good way to properly fix this would be to modify the multiple selection event handling so it only overrides the selection chang-ing/ed events when it needs to rather than overriding for all and trying to account for every single case. This way it will still work properly if the control does things like change selection during SetFocus.

I'll work on implementing this and post an updated patch when i'm done.

comment:47 Changed 8 years ago by net147

I've created an updated patch which fixes the following issues with multiple selection:

  • Handling key down and not calling event.Skip() does not prevent selection change - broken by r59369
  • Selection chang-ing/ed events not generated when SetFocus causes selection change
  • Clicking on nothing doesn't cancel label edit
  • Double-clicking on nothing to cancel label edit, then clicking back on focused item doesn't trigger label edit
  • Pressing down when nothing is focused will select the second item which is different from single selection behaviour

The updated patch is attached.

Changed 8 years ago by net147

Events patch for r59406.

comment:48 Changed 8 years ago by net147

In addition, unselecting all in multiple selection mode will keep the current focused item as it does in wxMSW 2.8.9.

comment:49 Changed 8 years ago by VZ

(In [59570]) add a menu item to test setting focus (see #626 comment:42)

comment:50 Changed 8 years ago by VZ

(In [59600]) more bug fixes to multiple selection mode (see #626, comment 47)

comment:51 Changed 8 years ago by vadz

  • Priority changed from high to normal
  • Resolution set to fixed
  • Status changed from confirmed to closed

Thanks, applied with a minor change: use a class to set/unset the variable instead of doing it manually.

Note: See TracTickets for help on using tickets.