[MSW] Wrong selection events in multi selection tree control

[haribeau]

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.

[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)?

[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)...

[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.

[vadz]

The problems are still present in 2.7.2 wxTreeCtrl.

[mspacek]

See also #2307

[mspacek]

#4448 regarding EVT_LEFT_DOWN is probably related

[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

[net147]

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.

[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.

[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

[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

[vadz]

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!

[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.

[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!

[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?

[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!

[net147]

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

[net147]

Events patch

[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.

[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!

[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?

[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.

[net147]

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

[net147]

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

[net147]

Events patch for multiple selection only

[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

[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?

[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).

[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.

[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.

[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.

[Euan]

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

[vadz]

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!

[net147]

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.

[vadz]

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!

[vadz]

Fix events generation (against r59332)

[net147]

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

[vadz]

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.

[VZ]

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

[net147]

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.

[net147]

ALT key fix.

[VZ]

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

[vadz]

Thanks for finding and fixing this!

[net147]

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.

[vadz]

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!

[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.

[net147]

SetFocus fix.

[net147]

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

Regards,
Jonathan

[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.

[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.

[net147]

Events patch for r59406.

[net147]

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

[VZ]

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

[VZ]

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

[vadz]

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