`wxListCtrl` fixes in wxMSW (PR #25961)
VZ
This is #25950 with minor changes and split into several commits.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/25961
Commit Summary
- bde6b7a Fix wxListCtrl::GetSubItemRect(n, 0) when columns are reordered
- 4883c94 Draw wxListCtrl subitems in visual, not logical, order
- d76f3c1 Erase the whole list item row before drawing sub-items
- c1e3c92 Make wxListCtrl dark mode closer to native, white mode rendering
File Changes
(1 file)
- M src/msw/listctrl.cpp (175)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/25961.patch
- https://github.com/wxWidgets/wxWidgets/pull/25961.diff
—
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.
> + wxRect rectSubItem; + if ( !listctrl->GetSubItemRect(item, col, rectSubItem, wxLIST_RECT_BOUNDS) ) + continue; + RECT rcSubItem; + wxCopyRectToRECT(rectSubItem, rcSubItem);
Can get the RECT directly like:
- wxRect rectSubItem; - if ( !listctrl->GetSubItemRect(item, col, rectSubItem, wxLIST_RECT_BOUNDS) ) - continue; - RECT rcSubItem; - wxCopyRectToRECT(rectSubItem, rcSubItem); + RECT rcSubItem; + if ( !wxGetListCtrlSubItemRect(nmcd.hdr.hwndFrom, item, col, LVIR_BOUNDS, rcSubItem) ) + continue;
> {
pLVCD->iSubItem = col;
- HandleSubItemPrepaint(listctrl, pLVCD, hfont, colCount);
+
+ // get the rectangle of this entire subitem
+ wxRect rectSubItem;
+ if ( !listctrl->GetSubItemRect(item, col, rectSubItem, wxLIST_RECT_BOUNDS) )
+ continue;
+ RECT rcSubItem;
+ wxCopyRectToRECT(rectSubItem, rcSubItem);
+
+ HandleSubItemPrepaint(listctrl, pLVCD, hfont, rcSubItem, pLVCD->clrTextBk);
You already pass pLVCD to the function, so pLVCD->clrTextBk is redundant.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Crementif
@Crementif commented on this pull request.
> + wxRect rectSubItem; + if ( !listctrl->GetSubItemRect(item, col, rectSubItem, wxLIST_RECT_BOUNDS) ) + continue; + RECT rcSubItem; + wxCopyRectToRECT(rectSubItem, rcSubItem);
ListCtrl::GetSubItemRect() does additional math to normalize the output of wxGetListCtrlSubItemRect, since its fully incorrect.
—
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.
> + wxRect rectSubItem; + if ( !listctrl->GetSubItemRect(item, col, rectSubItem, wxLIST_RECT_BOUNDS) ) + continue; + RECT rcSubItem; + wxCopyRectToRECT(rectSubItem, rcSubItem);
Ignore this, it won't work with the fixes in GetSubItemRect about column order.
—
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.
> + wxRect rectSubItem; + if ( !listctrl->GetSubItemRect(item, col, rectSubItem, wxLIST_RECT_BOUNDS) ) + continue; + RECT rcSubItem; + wxCopyRectToRECT(rectSubItem, rcSubItem);
ListCtrl::GetSubItemRect() does additional math to normalize the output of wxGetListCtrlSubItemRect, since its fully incorrect.
Sorry missed your message. Indeed, I also noticed wrong x-offset when reordering columns.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 1 commit.
- 4b2a57c Make wxListCtrl dark mode closer to native, white mode rendering
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> {
pLVCD->iSubItem = col;
- HandleSubItemPrepaint(listctrl, pLVCD, hfont, colCount);
+
+ // get the rectangle of this entire subitem
+ wxRect rectSubItem;
+ if ( !listctrl->GetSubItemRect(item, col, rectSubItem, wxLIST_RECT_BOUNDS) )
+ continue;
+ RECT rcSubItem;
+ wxCopyRectToRECT(rectSubItem, rcSubItem);
+
+ HandleSubItemPrepaint(listctrl, pLVCD, hfont, rcSubItem, pLVCD->clrTextBk);
Thanks, I agree that it's simpler to remove this parameter, force-pushed now.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25961)@Crementif If you don't have any objections, I'll merge this soon.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#25961)I have one more suggestion. In #25950 I mentioned the right-aligned text padding. What about fixing it like this.
Result: https://imgsli.com/NDI3NDY3
diff --git "a/src/msw/listctrl.cpp" "b/src/msw/listctrl.cpp" index b800fdb9b2e..71500d78f85 100644 --- "a/src/msw/listctrl.cpp" +++ "b/src/msw/listctrl.cpp" @@ -3055,7 +3055,7 @@ constexpr int PADDING_RIGHT_SIDE = 2; constexpr int GAP_AFTER_CHECKBOX = 2; constexpr int GAP_BEFORE_IMAGE = 2; constexpr int GAP_BEFORE_CHECKBOX = 4; -constexpr int PADDING_LEFT_FOR_TEXT = 1; +constexpr int PADDING_FOR_TEXT = 1; bool HandleSubItemPrepaint(wxListCtrl* listctrl, @@ -3204,6 +3204,7 @@ HandleSubItemPrepaint(wxListCtrl* listctrl, { case LVCFMT_LEFT: fmt |= DT_LEFT; + rc.left += PADDING_FOR_TEXT; break; case LVCFMT_CENTER: @@ -3212,13 +3213,12 @@ HandleSubItemPrepaint(wxListCtrl* listctrl, case LVCFMT_RIGHT: fmt |= DT_RIGHT; + rc.right -= PADDING_FOR_TEXT; break; } } //else: failed to get alignment, assume it's DT_LEFT (default) - rc.left += PADDING_LEFT_FOR_TEXT; - if ( rc.left < rc.right ) DrawText(hdc, text, -1, &rc, fmt);
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 1 commit.
- 7caaa1a Improve right-aligned text positioning in wxListCtrl dark mode
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25961)Oops, sorry, I had missed that. Pushed too now.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Crementif
@Crementif requested changes on this pull request.
> - if ( !col && listctrl->HasCheckBoxes() ) + if ( !col && listctrl->HasCheckBoxes() && availableWidth > 0 )
I guess I wasn't super clear in my PR, but a large part of what I was also intending to fix (for Cemu) was that columns would still correctly (invisibly) render when their width is set to 0.
That is to say that you might not have checked this issue as much. But this code looks like it'd fail when the availableWidth is set to a value between 1-3. After GAP_BEFORE_CHECKBOX (4 pixels of padding) gets added, the ImageList_DrawEx should really not be called at all, since there's no column width left.
I personally think its better to move this around the ImageList_DrawEx check, like I had.
> ImageList_GetIconSize(himl, &cbWidth, &cbHeight); - cbX = listctrl->FromDIP(GAP_BETWEEN_CHECKBOX_AND_TEXT); - cbY = rc.top + ((rc.bottom - rc.top) / 2 - cbHeight / 2); + + // center checkbox vertically + int cbY = rc.top + ((rc.bottom - rc.top) / 2 - cbHeight / 2); + + // prevent drawing checkbox farther than the column width + cbWidth = wxClip(cbWidth, 0, availableWidth);
availableWidth isn't the current remaining space that's available in the column, since we've added GAP_BEFORE_CHECKBOX to it already. The checkbox is now able to overflow the next column by 4 pixels.
> @@ -3137,6 +3203,7 @@ bool HandleSubItemPrepaint(wxListCtrl* listctrl, LPNMLVCUSTOMDRAW pLVCD, HFONT h
{
case LVCFMT_LEFT:
fmt |= DT_LEFT;
+ rc.left += PADDING_FOR_TEXT;
I think PADDING_FOR_TEXT isn't very clear by itself. I think re-adding the comment about the padding being used as the inner padding for an individual row-item's background color would be useful for someone looking to understand all of these magic offsets.
Might just be a bit of a nitpick, so you can also leave it if that's clearer for others/you.
—
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 ( !col && listctrl->HasCheckBoxes() ) + if ( !col && listctrl->HasCheckBoxes() && availableWidth > 0 )
Sorry, I've indeed missed that. Why is it important to still render them when the width is 0? I thought it would be better to just skip this in this case, what do we gain by still doing it?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
> @@ -3137,6 +3203,7 @@ bool HandleSubItemPrepaint(wxListCtrl* listctrl, LPNMLVCUSTOMDRAW pLVCD, HFONT h
{
case LVCFMT_LEFT:
fmt |= DT_LEFT;
+ rc.left += PADDING_FOR_TEXT;
I think it's relatively clear as it's only used in this one place. I did remove
// text is drawn 1 unit away from the background rectanglecomment intentionally because it seemed to just repeat what the code was quite clearly doing.
OTOH looking at this code again, I think we should apply padding (at both sides) for centered text too, shouldn't we?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
> ImageList_GetIconSize(himl, &cbWidth, &cbHeight); - cbX = listctrl->FromDIP(GAP_BETWEEN_CHECKBOX_AND_TEXT); - cbY = rc.top + ((rc.bottom - rc.top) / 2 - cbHeight / 2); + + // center checkbox vertically + int cbY = rc.top + ((rc.bottom - rc.top) / 2 - cbHeight / 2); + + // prevent drawing checkbox farther than the column width + cbWidth = wxClip(cbWidth, 0, availableWidth);
Oops, thanks, I've indeed missed it. But how to fix this depends on the fix for the comment above, i.e. I would just replace availableWidth > 0 with availableWidth > GAP_BEFORE_CHECKBOX and use availableWidth - GAP_BEFORE_CHECKBOX here, but not if we remove that check completely...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Crementif
@Crementif commented on this pull request.
> - if ( !col && listctrl->HasCheckBoxes() ) + if ( !col && listctrl->HasCheckBoxes() && availableWidth > 0 )
I meant the opposite, sorry. Ultimately it should follow the native white mode behavior, which is that it always cuts the content off correctly.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Crementif
> @@ -3137,6 +3203,7 @@ bool HandleSubItemPrepaint(wxListCtrl* listctrl, LPNMLVCUSTOMDRAW pLVCD, HFONT h
{
case LVCFMT_LEFT:
fmt |= DT_LEFT;
+ rc.left += PADDING_FOR_TEXT;
Hard to say without testing, but maybe?
—
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 ( !col && listctrl->HasCheckBoxes() ) + if ( !col && listctrl->HasCheckBoxes() && availableWidth > 0 )
I might be missing something (it's getting late here...), but I don't see any obvious difference in behaviour between light and dark modes with the current version, the checkbox does get clipped when the column width becomes very small. Maybe it's because you've also changed code to overdraw the columns by the following ones?
Anyhow, what about this diff?
diff --git a/src/msw/listctrl.cpp b/src/msw/listctrl.cpp index e77b4450d5..9a219ba11c 100644 --- a/src/msw/listctrl.cpp +++ b/src/msw/listctrl.cpp @@ -3079,8 +3079,9 @@ HandleSubItemPrepaint(wxListCtrl* listctrl, rc.right -= PADDING_RIGHT_SIDE; const int availableWidth = wxMax(rc.right - rc.left, 0); + const int availableForCheckbox = availableWidth - GAP_AFTER_CHECKBOX; - if ( !col && listctrl->HasCheckBoxes() && availableWidth > 0 ) + if ( !col && listctrl->HasCheckBoxes() && availableForCheckbox > 0 ) { const HIMAGELIST himl = ListView_GetImageList(hwndList, LVSIL_STATE); @@ -3095,7 +3096,7 @@ HandleSubItemPrepaint(wxListCtrl* listctrl, int cbY = rc.top + ((rc.bottom - rc.top) / 2 - cbHeight / 2); // prevent drawing checkbox farther than the column width - cbWidth = wxClip(cbWidth, 0, availableWidth); + cbWidth = wxClip(cbWidth, 0, availableForCheckbox); // When using style flag ILD_SELECTED or ILD_FOCUS, the checkboxes // for selected items are drawn with a blue background, which we want to avoid.
I don't see any visual differences but it does seem to be more logical to do it like this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Crementif
@Crementif commented on this pull request.
> ImageList_GetIconSize(himl, &cbWidth, &cbHeight); - cbX = listctrl->FromDIP(GAP_BETWEEN_CHECKBOX_AND_TEXT); - cbY = rc.top + ((rc.bottom - rc.top) / 2 - cbHeight / 2); + + // center checkbox vertically + int cbY = rc.top + ((rc.bottom - rc.top) / 2 - cbHeight / 2); + + // prevent drawing checkbox farther than the column width + cbWidth = wxClip(cbWidth, 0, availableWidth);
The latter solution would introduce a very minor, but present behavior where it'd draw an image or the text if it can't fit GAP_BEFORE_CHECKBOX. I am not sure if this is what the white mode does, but if I had to guess it doesn't do any "if X doesn't fit, render Y" tricks.
You could still reduce the amount of code it has to run if you split it up into 2 if statements, and I think it'd be a lot more accurate. But I am speculating.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 2 commits.
- ff79835 Make HandleSubItemPrepaint() void
- 26e2bd6 Skip drawing the rest of the item if there is not enough space
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> ImageList_GetIconSize(himl, &cbWidth, &cbHeight); - cbX = listctrl->FromDIP(GAP_BETWEEN_CHECKBOX_AND_TEXT); - cbY = rc.top + ((rc.bottom - rc.top) / 2 - cbHeight / 2); + + // center checkbox vertically + int cbY = rc.top + ((rc.bottom - rc.top) / 2 - cbHeight / 2); + + // prevent drawing checkbox farther than the column width + cbWidth = wxClip(cbWidth, 0, availableWidth);
What about the latest version? I've removed availableWidth to get a bit closer back to the original version, but I've also added returns to make sure we don't try drawing anything which is certain not to fit.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Crementif
@Crementif commented on this pull request.
> - if ( !col && listctrl->HasCheckBoxes() ) + if ( !col && listctrl->HasCheckBoxes() && availableWidth > 0 )
I think the latest version looks good to be merged.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Merged #25961 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.