Improve consistency of checkboxes in wxListCtrl (PR #23890)
Maarten
Improve consistency of checkboxes in wxListCtrl between virtual and report mode, and between the generic and MSW implementation, see #23869.
Also disable WS_EX_COMPOSITED for virtual wxListCtrl because it generates an endless stream of cache events, see #23878.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/23890
Commit Summary
- b29927f Generate wxEVT_LIST_ITEM_[UN]CHECKED events for MSW virtual wxListCtrl
- 4908c77 Support using IsItemChecked in MSW virtual wxListCtrl
- e054aef Don't use internal check state for generic virtual wxListCtrl
- 41adea8 Ignore wxListCtrl CheckItem/IsItemChecked when checkboxes are disabled
- 00657ed Update listctrl example to recent wxListCtrl checkbox changes
- 3d43876 Disable WS_EX_COMPOSITED for virtual wxListCtrl
- ee36b32 Update documentation of wxListCtrl IsItemChecked and CheckItem
File Changes
(6 files)
- M include/wx/generic/private/listctrl.h (3)
- M interface/wx/listctrl.h (6)
- M samples/listctrl/listtest.cpp (33)
- M samples/listctrl/listtest.h (3)
- M src/generic/listctrl.cpp (42)
- M src/msw/listctrl.cpp (28)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/23890.patch
- https://github.com/wxWidgets/wxWidgets/pull/23890.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
This could also be backported to 3.2.
Not sure if the change to include/wx/generic/private/listctrl.h will cause abi/api problems, but this change could be omitted. It is an precaution to prevent the internal checkbox state from being out-of-sync with the external state from OnGetItemIsChecked.
—
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 am not sure we can backport this to 3.2 as this changes the behaviour in not quite obvious way, but we should definitely apply this to master, I'd just like to clarify a couple of things before doing it.
> + if ( !HasCheckBoxes() ) + return;
Should this be a wxCHECK_RET() instead? I.e. isn't it a programmers error to call this if there are no checkboxes?
> @@ -257,6 +257,11 @@ bool wxListCtrl::Create(wxWindow *parent,
if ( !MSWCreateControl(WC_LISTVIEW, wxEmptyString, pos, size) )
return false;
+ // LISTVIEW generates and endless stream of LVN_ODCACHEHINT event for a
+ // virtual wxListCtrl, so disabled WS_EX_COMPOSITED.
Tiny nitpick, but you probably meant
⬇️ Suggested change- // virtual wxListCtrl, so disabled WS_EX_COMPOSITED. + // virtual wxListCtrl, so disable WS_EX_COMPOSITED.
> + if ( !HasCheckBoxes() ) + return false;
Same question as above.
> +#if defined(__WXMSW__) + // LISTVIEW generates and endless stream of LVN_ODCACHEHINT event for a + // virtual wxListCtrl, so disabled WS_EX_COMPOSITED. + if ( IsVirtual() ) + MSWDisableComposited(); +#endif +
Sorry, I don't understand this: we're not using the native control here, why do we need this?
> @@ -1494,12 +1499,31 @@ bool wxListCtrl::EnableCheckBoxes(bool enable)
void wxListCtrl::CheckItem(long item, bool state)
{
- ListView_SetCheckState(GetHwnd(), (UINT)item, (BOOL)state);
+ if ( !HasCheckBoxes() )
+ return;
+
+ if ( IsVirtual() )
+ {
+ wxListEvent le(state ? wxEVT_LIST_ITEM_CHECKED : wxEVT_LIST_ITEM_UNCHECKED, GetId());
I'd probably add a comment saying that ListView_SetCheckState() generates the same event that we have to generate ourselves here because we can't call it for the virtual controls.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Maarten, should I make the changes indicated above myself and merge this or will you make them yourself or do you think they shouldn't be done at all?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
I will make the changes.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
Yes, I added it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> +#if defined(__WXMSW__) + // LISTVIEW generates and endless stream of LVN_ODCACHEHINT event for a + // virtual wxListCtrl, so disabled WS_EX_COMPOSITED. + if ( IsVirtual() ) + MSWDisableComposited(); +#endif +
I was testing the generic implementation on Windows, then it is needed. But normally this isn't possible so this code is indeed not needed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 7 commits.
- 07df143 Generate wxEVT_LIST_ITEM_[UN]CHECKED events for MSW virtual wxListCtrl
- eca7ec0 Support using IsItemChecked in MSW virtual wxListCtrl
- fa26c87 Don't use internal check state for generic virtual wxListCtrl
- 1b8f498 Ignore wxListCtrl CheckItem/IsItemChecked when checkboxes are disabled
- abdbe66 Update listctrl example to recent wxListCtrl checkbox changes
- 3987323 Disable WS_EX_COMPOSITED for virtual wxListCtrl
- f68bb39 Update documentation of wxListCtrl IsItemChecked and CheckItem
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> + if ( !HasCheckBoxes() ) + return false;
Not sure about this one. Sometimes it is easier to just call IsItemChecked() even if checkboxes are disabled. Like I do now in the code below. This would have to be changed to HasCheckBoxes() && IsItemChecked(line).
—
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.
OK, thanks, let's leave it like this, but I'll add a note about this (not necessarily obvious) behaviour to the docs.
—
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.
> + if ( !HasCheckBoxes() ) + return false;
Oops, actually this is already documented, sorry for not realizing 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'm going to merge this to master soon but I think it should also be backported to 3.2.
Am I correct in thinking that everything but 3987323 should be safe to backport?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
Yes, I think so.
I was questioning earlier if the changes to include/wx/generic/private/listctrl.h would cause abi/api problems. But wxListLineData is not exported, so it should be fine.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Merged #23890 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.