wxMSW:darkmode - wxTreeCtrl, wxDataViewCtrl and wxListCtrl (Issue #25633)
herb52
herb52 created an issue (wxWidgets/wxWidgets#25633)Hello,
First of all: Thanks for the darkmode feature in wxMSW.
I have updated my Windows 10 project from wxWidgets 3.2.1 to 3.3.0 and
I started some tests with darkmode.
For my used controls I have seen the following in darkmode:
wxTreeCtrl
for selected and focused item it uses a light gray
for selected but unfocused item it uses a dark gray
which is nearly identcal to background
wxDataViewCtrl
for selected and focused item it uses a dark blue
for selected and unfocused item it uses black
which is also nearly identcal to used background
wxListCtrl
for selected item it uses blue
I also have retested this behaviour with the released samples for dataview, treeview and listview.
The only modification is: MSWEnableDarkMode(wxApp::DarkMode_Always, nullptr); inside OnInit()
Now I was asked whether it is possible
- to enhance the visibility of selected unfocused items and
- to harmonize the used colors
This leads to question:
Is it possible to change/modify ONLY these used colors?
If yes, any hint where is appreciated.
I know that wxWidgets uses native Windows controls when possible, so maybe some colors cannot be changed.
But I do not have an overview.
I think wxDataViewCtrl is not a native control.
Thanks for your answers/comments in advance
Best regards
herb
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
PBfordev left a comment (wxWidgets/wxWidgets#25633)Back then when I was trying to fix some wxListCtrl issues, I thought that we need to introduce some pseudo-system named colors for Win32 dark mode, e.g., to wxDarkModeSettings or elsewhere. The system-provided ones may not be enough these days.
Now we may have them spread hardcoded all over the code base. I originally meant colors for grid lines (wxListView or wxGrid), but of course as in this Issue for various states of items in different controls too...
—
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#25633)As you notice, it should be possible to improve things for wxDVC as it's entirely under our control. Doing it for wx{Tree,List}Ctrl risks being trickier, especially for the list one, as I spent a lot of time on it and the current look was the best I could find, even if it has known problems, see this comment:
Of course, any improvements would be welcome.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Takasa Technologies
Takasa left a comment (wxWidgets/wxWidgets#25633)First, fantastic work on darkmode, it's great!
Rather than start a new ticket could I report a darkmode issue for wxMSW::wxlistctrl here?
I am setting a different text colour for each column using (as a test) the following
wxListItemAttr* cPointInfoList::OnGetItemColumnAttr(long item, long column) const
{
if (column == 0) m_attr.SetTextColour(wxColour(0xFF,0x00,0x00));
if (column == 1) m_attr.SetTextColour(wxColour(0x00,0xFF,0x00));
if (column == 2) m_attr.SetTextColour(wxColour(0x00,0x00,0xFF));
return const_cast<wxItemAttr*>(&m_attr);
}
In lightmode this works as you would expect. In darkmode all columns get the last colour from column2.
I've followed it through to line 3240 in src/msw/listctrl.cpp HandleItemPaint
// we could use CDRF_NOTIFYSUBITEMDRAW here but it results in weird repaint
// problems so just draw everything except the focus rect from here instead
const int colCount = Header_GetItemCount(ListView_GetHeader(hwndList));
for ( int col = 0; col < colCount; col++ )
{
pLVCD->iSubItem = col;
HandleSubItemPrepaint(listctrl, pLVCD, hfont, colCount);
}
::SetTextColor(hdc, colTextOld);
pLVCD->clrText - is correct just before the col loop. Even though HandleItemPaint is called per column, this loop at the end redraws all columns for each call, I think. Which is where it goes wrong.
I continue to investigate.
I was hoping to see an obvious difference between dark and lite mode but I cannot.
Thanks all - and apologies if jumping on this existing issue was the wrong call.
Mike.
—
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#25633)pLVCD->clrText - is correct just before the col loop. Even though HandleItemPaint is called per column, this loop at the end redraws all columns for each call, I think. Which is where it goes wrong
I can see the problem too. Like you suspected, for each row, It is using the attribute of the column that is being updated to redraw all the columns.
I made this quick hack to get the attributes of each individual column when redrawing. It is a hack because I had to make DoGetItemColumnAttr public (this is not in the diff). And it is duplicating some code from HandleItemPrepaint.
And the improvements from #25950 would also need to be integrated into this.
diff --git "a/src/msw/listctrl.cpp" "b/src/msw/listctrl.cpp" index 0511836bf6e..8d78314c15e 100644 --- "a/src/msw/listctrl.cpp" +++ "b/src/msw/listctrl.cpp" @@ -3216,26 +3216,6 @@ void HandleItemPaint(wxListCtrl* listctrl, LPNMLVCUSTOMDRAW pLVCD, HFONT hfont) } HDC hdc = nmcd.hdc; - RECT rc = GetCustomDrawnItemRect(nmcd); - - if ( nmcd.uItemState & CDIS_SELECTED ) - { - pLVCD->clrText = wxColourToRGB(wxSystemSettings::GetColour(wxSYS_COLOUR_HIGHLIGHTTEXT)); - pLVCD->clrTextBk = wxColourToRGB(wxSystemSettings::GetColour(wxSYS_COLOUR_HIGHLIGHT)); - } - else - { - // use normal colours from pLVCD - - // do not draw item background colour under the checkbox/image - RECT rcIcon; - wxGetListCtrlItemRect(nmcd.hdr.hwndFrom, nmcd.dwItemSpec, LVIR_ICON, rcIcon); - if ( !::IsRectEmpty(&rcIcon) ) - rc.left = rcIcon.right + listctrl->FromDIP(GAP_BETWEEN_CHECKBOX_AND_TEXT); - } - - COLORREF colTextOld = ::SetTextColor(hdc, pLVCD->clrText); - ::FillRect(hdc, &rc, AutoHBRUSH(pLVCD->clrTextBk));
// we could use CDRF_NOTIFYSUBITEMDRAW here but it results in weird repaint
// problems so just draw everything except the focus rect from here instead
@@ -3243,10 +3223,49 @@ void HandleItemPaint(wxListCtrl* listctrl, LPNMLVCUSTOMDRAW pLVCD, HFONT hfont) for ( int col = 0; col < colCount; col++ ) { pLVCD->iSubItem = col; - HandleSubItemPrepaint(listctrl, pLVCD, hfont, colCount); - } - ::SetTextColor(hdc, colTextOld); + RECT rcSubItem; + if (!wxGetListCtrlSubItemRect(nmcd.hdr.hwndFrom, item, col, LVIR_BOUNDS, rcSubItem)) + continue; + + wxItemAttr* attr = listctrl->DoGetItemColumnAttr(item, col); + wxFont font; + if ( attr && attr->HasFont() ) + { + font = attr->GetFont(); + font.WXAdjustToPPI(listctrl->GetDPI()); + } + pLVCD->clrText = attr && attr->HasTextColour() + ? wxColourToRGB(attr->GetTextColour()) + : wxColourToRGB(listctrl->GetTextColour()); + pLVCD->clrTextBk = attr && attr->HasBackgroundColour() + ? wxColourToRGB(attr->GetBackgroundColour()) + : wxColourToRGB(listctrl->GetBackgroundColour()); + + if ( nmcd.uItemState & CDIS_SELECTED ) + { + pLVCD->clrText = wxColourToRGB(wxSystemSettings::GetColour(wxSYS_COLOUR_HIGHLIGHTTEXT)); + pLVCD->clrTextBk = wxColourToRGB(wxSystemSettings::GetColour(wxSYS_COLOUR_HIGHLIGHT)); + } + else + { + // use normal colours from pLVCD + + // do not draw item background colour under the checkbox/image + RECT rcIcon; + wxGetListCtrlSubItemRect(nmcd.hdr.hwndFrom, item, col, LVIR_ICON, rcIcon); + if ( !::IsRectEmpty(&rcIcon) ) + rcSubItem.left = rcIcon.right + listctrl->FromDIP(GAP_BETWEEN_CHECKBOX_AND_TEXT); + } + + ::FillRect(hdc, &rcSubItem, AutoHBRUSH(pLVCD->clrTextBk)); + + COLORREF colTextOld = ::SetTextColor(hdc, pLVCD->clrText); + + HandleSubItemPrepaint(listctrl, pLVCD, font.IsOk() ? GetHfontOf(font) : nullptr, colCount); + + ::SetTextColor(hdc, colTextOld); + } HandleItemPostpaint(nmcd); }
—
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#25633)Rather than start a new ticket could I report a darkmode issue for wxMSW::wxlistctrl here?
It doesn't look like the same problem as was originally reported here, unless I'm missing something, so I'd rather create a separate issue for it. Also, please use triple backticks around the code snippets for them to be formatted correctly/remain readable. TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Takasa Technologies
Takasa left a comment (wxWidgets/wxWidgets#25633)Amazing thanks MaartenBent, I will see if I can patch my copy.
Vadz will do in future thanks for the advice.
—
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#25633)I created #25962 to fix this.
—
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#25633)Is this issue still relevant, with all the latest fixes in master?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Takasa Technologies
Takasa left a comment (wxWidgets/wxWidgets#25633)It's was all fixed, although I noticed a regression with one of the latest commits and a difference between lightmode and darkmode on a selected row. I'm applying this patch which resolves it for me, not sure it's correct however.
diff --git a/src/msw/listctrl.cpp b/src/msw/listctrl.cpp
index 6025d95d53..c55c4286c1 100644
--- a/src/msw/listctrl.cpp
+++ b/src/msw/listctrl.cpp
@@ -3337,7 +3337,11 @@ void HandleItemPaint(wxListCtrl* listctrl, LPNMLVCUSTOMDRAW pLVCD)
if ( nmcd.uItemState & CDIS_SELECTED )
{
- pLVCD->clrText = wxColourToRGB(wxSystemSettings::GetColour(wxSYS_COLOUR_HIGHLIGHTTEXT));
+ if (attr && attr->HasTextColour()) {
+ pLVCD->clrText = wxColourToRGB(attr->GetTextColour());
+ } else {
+ pLVCD->clrText = wxColourToRGB(wxSystemSettings::GetColour(wxSYS_COLOUR_HIGHLIGHTTEXT));
+ }
pLVCD->clrTextBk = wxColourToRGB(wxSystemSettings::GetColour(wxSYS_COLOUR_HIGHLIGHT));
}
else
@@ -3374,6 +3378,13 @@ WXLPARAM HandleItemPrepaint(wxListCtrl* listctrl, LPNMLVCUSTOMDRAW pLVCD)
// nothing to do for this item
return CDRF_DODEFAULT;
}
+ // set the colours to use for text drawing
+ pLVCD->clrText = attr->HasTextColour()
+ ? wxColourToRGB(attr->GetTextColour())
+ : wxColourToRGB(listctrl->GetTextColour());
+ pLVCD->clrTextBk = attr->HasBackgroundColour()
+ ? wxColourToRGB(attr->GetBackgroundColour())
+ : wxColourToRGB(listctrl->GetBackgroundColour());
// select the font if non default one is specified
if ( attr->HasFont() )
—
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#25633)I think this patch is for an older commit, and this has all been fixed after #25999 was merged yesterday.
If not, please share screenshots of the differences so it is easier to see the problems.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Takasa Technologies
Takasa left a comment (wxWidgets/wxWidgets#25633)Updating to latest and removing my patch, the issue on selection remains in dark mode. It's correct in light.
This simpler patch seems to fix it for my use case and is consistent between the two modes where a text fg attributes overrides the highlight text colour.
—
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#25633)I mean a screenshot of the wxListCtrl in light and dark mode, showing the different behaviour, not a screenshot of the patch.
But you are correct, in dark-mode the foreground colour is replaced by the selection colour, in light-mode it is not. I can create a Pull Request with this simpler patch:
diff --git "a/src/msw/listctrl.cpp" "b/src/msw/listctrl.cpp" index 795e216a9c5..cd4cb23f9cc 100644 --- "a/src/msw/listctrl.cpp" +++ "b/src/msw/listctrl.cpp" @@ -3408,7 +3408,7 @@ void HandleItemPaint(wxListCtrl* listctrl, LPNMLVCUSTOMDRAW pLVCD) ? wxColourToRGB(attr->GetTextColour()) : wxColourToRGB(listctrl->GetTextColour()); pLVCD->clrTextBk = clrFullBG; - if ( nmcd.uItemState & CDIS_SELECTED ) + if ( nmcd.uItemState & CDIS_SELECTED && !(attr && attr->HasTextColour()) ) pLVCD->clrText = wxColourToRGB(wxSystemSettings::GetColour(wxSYS_COLOUR_HIGHLIGHTTEXT)); if ( !(nmcd.uItemState & CDIS_SELECTED) && !(nmcd.uItemState & CDIS_HOT) && attr && attr->HasBackgroundColour() )
—
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#25633)I can apply the patch above if that's the only change needed, please let me know if I should.
—
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#25633)This is the only change needed, please apply it. Thank you!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Takasa Technologies
Takasa left a comment (wxWidgets/wxWidgets#25633)"I mean a screenshot of the wxListCtrl in light and dark mode, showing the different behavior, not a screenshot of the patch."
Duh, sorry. More obvious in retrospect!
Thanks for the fixes, your work is really appreciated.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Closed #25633 as completed via aaa15aa.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.