[wxQt] Fix wxEVT_TREE_SEL_{CHANGING,CHANGED} generation/handling (PR #24600)
AliKet
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/24600
Commit Summary
- 33a76a4 wxQt: Remove trailing white-space
- 6cd9d69 wxQt: Simplify wxTreeCtrl::GetSelection() implementation
- 2c8ca5c wxQt: Fix generating wxEVT_TREE_SEL_{CHANGING,CHANGED} events
File Changes
(1 file)
- M src/qt/treectrl.cpp (157)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/24600.patch
- https://github.com/wxWidgets/wxWidgets/pull/24600.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks for the fixes, I think the logic is correct here, but if we could store the items in, say, wxQTreeWidget, as member variables, it would be better IMHO.
> +static wxTreeItemId gs_currentItem; +static wxTreeItemId gs_previousItem;
Do these really need to be global? I am not 100% sure it's not going to be a problem, e.g. if a handler of some event changes selection in some other tree control.
> @@ -125,6 +125,61 @@ class ImageState Q_DECLARE_METATYPE(TreeItemDataQt) +// We use a custom selection model because we need to generate the +// wxEVT_TREE_SEL_CHANGED event only if it is allowed. that is, only
Nitpick of the day:
⬇️ Suggested change-// wxEVT_TREE_SEL_CHANGED event only if it is allowed. that is, only +// wxEVT_TREE_SEL_CHANGED event only if it is allowed, that is, only
(alternatively, keep the period, but capitalize "That")
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- 73040e9 wxQt: Fix generating wxEVT_TREE_SEL_{CHANGING,CHANGED} events
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet commented on this pull request.
> @@ -125,6 +125,61 @@ class ImageState Q_DECLARE_METATYPE(TreeItemDataQt) +// We use a custom selection model because we need to generate the +// wxEVT_TREE_SEL_CHANGED event only if it is allowed. that is, only
forgot to update this
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@dsa-t It would be really apreciated if you could test this PR (or this patch) before it get merged because I had to change the implementation a little due to problems encountered with wxTreeBook control with the previous version.
TIA.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
Oops! this version reintroduced the infinite loop reported by @dsa-t which can be seen with the notebook sample (File -> Type -> Treebook)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
This is really strange because the treectrl sample works correctly but the notebook (treebook page) does not! maybe the problem is in the wxTreeBook control !?
Needs more debugging...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- 80eb629 wxQt: Fix generating wxEVT_TREE_SEL_{CHANGING,CHANGED} events
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- a77fb17 wxQt: Fix generating wxEVT_TREE_SEL_{CHANGING,CHANGED} events
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
This is really strange because the treectrl sample works correctly but the notebook (treebook page) does not! maybe the problem is in the
wxTreeBookcontrol !?Needs more debugging...
Everything seems to work correctly now: the treectrl and notebook (wxTreebook) samples.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
dsa-t
Works good generally, but I also see a small issue with multiple selections:
- In
wxTR_MULTIPLEmode, selectFile child 1.1toFile child 1.5 - Then, select, for example,
File child 1.2 - Inside
OnSelChanged,GetSelectionswill say "File child 1.1""File child 1.2""File child 1.3""File child 1.4""File child 1.5"
Also the name editor gets triggered (I don't think it should).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
Works good generally, but I also see a small issue with multiple selections in
treetest:
- In
wxTR_MULTIPLEmode, selectFile child 1.1toFile child 1.5- Then, select, for example,
File child 1.2- Inside
OnSelChanged,GetSelectionswill say "File child 1.1""File child 1.2""File child 1.3""File child 1.4""File child 1.5"
Also the name editor gets triggered (I don't think it should).
Thanks for testing, but it works correctly for me!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
dsa-t
Thanks for testing, but it works correctly for me!
In the 2nd step, I mean select just the 1.2 item, not deselect it:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
Thanks for testing, but it works correctly for me!
In the 2nd step, I mean select just the 1.2 item, not deselect it:
Indeed, I can reproduce the problem. And the fix is simple which I'll push soon.
but preventing the editor from being triggered is trickier because this is how Qt interpret SelectedClicked flag passed here https://github.com/wxWidgets/wxWidgets/blob/f8043a62f3319719cbd3a400ea454e4dbb3d23ff/src/qt/treectrl.cpp#L1311-L1315
i.e. It starts editing when clicking on an already selected item
See Qt docs https://doc.qt.io/qt-6/qabstractitemview.html#EditTrigger-enum:~:text=QAbstractItemView%3A%3ASelectedClicked
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- 45d4dce wxQt: Fix generating wxEVT_TREE_SEL_{CHANGING,CHANGED} events
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@dsa-t this patch:
diff --git a/src/qt/treectrl.cpp b/src/qt/treectrl.cpp index ea68681ae0..45be7b0c48 100644 --- a/src/qt/treectrl.cpp +++ b/src/qt/treectrl.cpp @@ -330,11 +330,31 @@ protected: } } + // For compatibility with MSW, selecting an item that is part of a multiple selection + // simply deselects the other items without triggering the edit mode on the item. This + // is not the case with Qt because we already tell it to start editing when clicking + // on an already selected item with setEditTriggers(QTreeWidget::SelectedClicked) call. + bool m_blockEditing = false; + + void selectionChanged(const QItemSelection& selected, + const QItemSelection& deselected) override + { + m_blockEditing = selected.size() == 0; + + QTreeView::selectionChanged(selected, deselected); + } + bool edit(const QModelIndex &index, EditTrigger trigger, QEvent *event) override { // AllEditTriggers means that editor is about to open, not waiting for double click if (trigger == AllEditTriggers) { + if ( m_blockEditing ) + { + m_blockEditing = false; + return false; + } + // Allow event handlers to veto opening the editor wxTreeEvent wx_event( wxEVT_TREE_BEGIN_LABEL_EDIT,
solves the problem with starting edit you mention, but I'm still not sure if we should apply it or not because wxGTK3 also start editing if you do the following:
- Select
File child 1.1 - Hold
Ctrlkey down and selectFile child 1.2 - Now you have 2 item selected (
File child 1.1andFile child 1.2) - Select
File child 1.2with or without holding theCtrlkey down. - The control starts editing the item.
Notice that this happens only if you select the focused item. wxMSW doesn't behave like this. i.e. it doesn't start editing at all.
@vadz any thoughts about this ?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I think wxGTK (i.e. generic implementation) is wrong here, this doesn't seem like a useful behaviour to me.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 6 commits.
- 6bf265c Get rid of CppUnit boilerplate in wxTreeCtrl unit tests
- 583e0a7 Put all WXUISIM_TEST under wxUSE_UIACTIONSIMULATOR in wxTreeCtrl unit tests
- 22083d6 Test vetoing of wxEVT_TREE_SEL_CHANGING in wxTreeCtrl unit tests
- 7a3133f Added SelectItemMultiInteractive test case to TreeCtrlTestCase
- 7e2994e Fix unwanted item edit in wxQt wxTreeCtrl with wxTR_MULTIPLE
- a658536 Fix unwanted item edit in wxGenericTreeCtrl with wxTR_MULTIPLE
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 11 commits.
- 0603834 wxQt: Remove trailing white-space
- 3d61666 wxQt: respect wxTR_HIDE_ROOT style flag for wxTreeCtrl
- e49038c wxQt: Simplify wxTreeCtrl::GetSelection() implementation
- 6a54146 wxQt: Fix generating wxEVT_TREE_SEL_{CHANGING,CHANGED} events
- b3302f1 Get rid of CppUnit boilerplate in wxTreeCtrl unit tests
- 4a5642a Put all WXUISIM_TEST under wxUSE_UIACTIONSIMULATOR in wxTreeCtrl unit tests
- b70e88f Test vetoing of wxEVT_TREE_SEL_CHANGING in wxTreeCtrl unit tests
- 1844af0 Added SelectItemMultiInteractive test case to TreeCtrlTestCase
- dda9ecd Fix unwanted item edit in wxQt wxTreeCtrl with wxTR_MULTIPLE
- d891e7b Fix unwanted item edit in wxGenericTreeCtrl with wxTR_MULTIPLE
- 81bebe0 Fix TreeCtrlTestCase failing under wxMSW after recent changes
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- 41e80cb Fix TreeCtrlTestCase failing under wxMSW after recent changes
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- 2de5654 Fix TreeCtrlTestCase failing under wxMSW after recent changes
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@vadz I have no idea what's going on here, or what to do with these failures?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I'm afraid that at least the Mac failures are real ones, i.e. actually due to the changes here as they seem to be occurring in the affected code. For the MSW ones I'm less sure and I don't have time to debug this right now, sorry...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- c39f309 Temporarily disable the recently added test
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- d643c81 Temporarily disable wxTreeCtrl::SelectionChange testtoo
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- d05dbfb Temporarily disable wxTreeCtrl::SelectionChange testtoo
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 3 commits.
- 00f5022 Added SelectItemMultiInteractive test case to TreeCtrlTestCase
- 2c08bc0 Fix unwanted item edit in wxQt wxTreeCtrl with wxTR_MULTIPLE
- 3b3b126 Fix unwanted item edit in wxGenericTreeCtrl with wxTR_MULTIPLE
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 6 commits.
- 3840b6b Get rid of CppUnit boilerplate in wxTreeCtrl unit tests
- 4fb6244 Put all WXUISIM_TEST under wxUSE_UIACTIONSIMULATOR in wxTreeCtrl unit tests
- 2a99c57 Test vetoing of wxEVT_TREE_SEL_CHANGING in wxTreeCtrl unit tests
- 28a5434 Added SelectItemMultiInteractive test case to TreeCtrlTestCase
- bb32ea3 Fix unwanted item edit in wxQt wxTreeCtrl with wxTR_MULTIPLE
- dd60c9c Fix unwanted item edit in wxGenericTreeCtrl with wxTR_MULTIPLE
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 10 commits.
- 42007fc wxQt: Remove trailing white-space
- cff117b wxQt: respect wxTR_HIDE_ROOT style flag for wxTreeCtrl
- 392729a wxQt: Simplify wxTreeCtrl::GetSelection() implementation
- cd79f04 wxQt: Fix generating wxEVT_TREE_SEL_{CHANGING,CHANGED} events
- cb80ec7 Get rid of CppUnit boilerplate in wxTreeCtrl unit tests
- aa4b3b7 Put all WXUISIM_TEST under wxUSE_UIACTIONSIMULATOR in wxTreeCtrl unit tests
- d80efc2 Test vetoing of wxEVT_TREE_SEL_CHANGING in wxTreeCtrl unit tests
- 1afe3b9 Added SelectItemMultiInteractive test case to TreeCtrlTestCase
- 2a46b3e Fix unwanted item edit in wxQt wxTreeCtrl with wxTR_MULTIPLE
- 8741e64 Fix unwanted item edit in wxGenericTreeCtrl with wxTR_MULTIPLE
You are receiving this because you are subscribed to this thread.