Bugzilla – Full Text Bug Listing
| Summary: | win32 tree with SWT.FULL_SELECTION hard to see on dark theme | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Mike Marchand <mmarchand> | ||||
| Component: | SWT | Assignee: | Niraj Modi <niraj.modi> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | alexandr.miloslavskiy, Lars.Vogel, lshanmug, niraj.modi, rolf.theunissen, wayneb | ||||
| Version: | 4.16 | Flags: | lshanmug:
review+
|
||||
| Target Milestone: | 4.18 RC1 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows 10 | ||||||
| See Also: |
https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/172564 https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/172768 https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=4a6583aa496f9afe2ad4208bfc7e72addf2602f6 |
||||||
| Whiteboard: | |||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 566539, 568585 | ||||||
| Attachments: |
|
||||||
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/172564 New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/172768 Hi Alexandr, Created two separate patches because of RC1: First one seems safer for RC1 addresses only the use-case of the bug. Second one improves selection and hot state as well, but with limited testing from my side. WDYT ? The new patch makes sense to me.
As far as I can see, both 'explorerTheme' and 'TVS_TRACKSELECT' depend on 'OS.IsAppThemed()'. The latter is basically almost always true, see [1] which says:
"In Windows 8, it is not possible to turn off visual styles."
Somewhere on my TODO list (at a very low priority though) is an idea to clean up SWT code, assuming that 'OS.IsAppThemed()' is always true.
So, if noise is removed, code boils down to
if (OS.IsWindowEnabled (handle))
OS.SetTextColor (hDC, getForegroundPixel ());
Which is completely reasonable: foreground color is there exactly to affect the text color if Tree items. I have no idea why 'COLOR_WINDOWTEXT' can ever make sense instead of 'getForegroundPixel()'. In default theme the colors are expected to be equal, see 'Control.defaultForeground ()'. But if user actually specified 'getForegroundPixel()', I would expect it to be used instead.
I blamed and arrived at commit acdaa6aa, but this doesn't quite explain anything.
Regarding which patch to pick: I have good feelings about second patch. But yes, RC1... On the other hand, with the updated insight, the first patch looks slightly incorrect to me. I would expect it to take 'getForegroundPixel()' branch instead of not doing anything. So I actually consider second patch as less risky. Or you could improve first patch to fix this problem. If it was my choice, I would collect my bravery and merge second patch.
[1] https://docs.microsoft.com/en-us/windows/win32/api/uxtheme/nf-uxtheme-isappthemed
(In reply to Alexandr Miloslavskiy from comment #4) > The new patch makes sense to me. > > As far as I can see, both 'explorerTheme' and 'TVS_TRACKSELECT' depend on > 'OS.IsAppThemed()'. The latter is basically almost always true, see [1] > which says: > "In Windows 8, it is not possible to turn off visual styles." > > Somewhere on my TODO list (at a very low priority though) is an idea to > clean up SWT code, assuming that 'OS.IsAppThemed()' is always true. > > So, if noise is removed, code boils down to > if (OS.IsWindowEnabled (handle)) > OS.SetTextColor (hDC, getForegroundPixel ()); > > Which is completely reasonable: foreground color is there exactly to affect > the text color if Tree items. I have no idea why 'COLOR_WINDOWTEXT' can ever > make sense instead of 'getForegroundPixel()'. In default theme the colors > are expected to be equal, see 'Control.defaultForeground ()'. But if user > actually specified 'getForegroundPixel()', I would expect it to be used > instead. > > I blamed and arrived at commit acdaa6aa, but this doesn't quite explain > anything. > > Regarding which patch to pick: I have good feelings about second patch. But > yes, RC1... On the other hand, with the updated insight, the first patch > looks slightly incorrect to me. I would expect it to take > 'getForegroundPixel()' branch instead of not doing anything. So I actually > consider second patch as less risky. Or you could improve first patch to fix > this problem. If it was my choice, I would collect my bravery and merge > second patch. > > [1] > https://docs.microsoft.com/en-us/windows/win32/api/uxtheme/nf-uxtheme- > isappthemed Thanks Alexandr for your inputs, will go with new patch. Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/172768 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=4a6583aa496f9afe2ad4208bfc7e72addf2602f6 (In reply to Eclipse Genie from comment #6) > Gerrit change > https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/172768 was > merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=4a6583aa496f9afe2ad4208bfc7e72addf2602f6 Resolving now. Verified on Win10 using Build id: I20201125-1800 *** Bug 562783 has been marked as a duplicate of this bug. *** *** Bug 574054 has been marked as a duplicate of this bug. *** |
Created attachment 284369 [details] Tree with dark selected row The selected text of a tree with SWT.FULL_SELECTION is dark but it should be light. I have narrowed it down to these lines of code in Tree.java if ((bits & OS.TVS_TRACKSELECT) != 0) { if ((style & SWT.FULL_SELECTION) != 0 && (selected || hot)) { OS.SetTextColor (hDC, OS.GetSysColor (OS.COLOR_WINDOWTEXT)); } else { OS.SetTextColor (hDC, getForegroundPixel ()); } } I've attached a screenshot that shows the black text in the tree. There's a slight difference in tree's that I've created and the problems view, on my trees, even the first column in the selected row is black. If I remove the condition that uses COLOR_WINDOWTEXT and just always use getForegroundPixel(), it's a better result. Since I'm not familiar with this code, I'm not certain what to propose for a permanent solution.