Bugzilla – Full Text Bug Listing
| Summary: | [win32][Dark theme] Text border color is too bright for dark theme | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Alexandr Miloslavskiy <alexandr.miloslavskiy> | ||||||||||
| Component: | SWT | Assignee: | Alexandr Miloslavskiy <alexandr.miloslavskiy> | ||||||||||
| Status: | VERIFIED FIXED | QA Contact: | Niraj Modi <niraj.modi> | ||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | alexandr.miloslavskiy, Lars.Vogel, nikita, niraj.modi, ts-swt | ||||||||||
| Version: | 4.14 | ||||||||||||
| Target Milestone: | 4.16 M3 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows 10 | ||||||||||||
| See Also: |
https://git.eclipse.org/r/158043 https://git.eclipse.org/r/158042 https://git.eclipse.org/r/158041 https://bugs.eclipse.org/bugs/show_bug.cgi?id=576614 |
||||||||||||
| Whiteboard: | |||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 562193 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Alexandr Miloslavskiy
New Gerrit change created: https://git.eclipse.org/r/158043 New Gerrit change created: https://git.eclipse.org/r/158042 New Gerrit change created: https://git.eclipse.org/r/158041 Created attachment 281874 [details]
Screenshot with old code
Created attachment 281875 [details]
Screenshot with new code
I really disagree with the direction you're taking: spreading dirty hacks all over SWT fake a dark theme. We only need a single knob: native dark theme enabled/disabled. Exactly how to make the broken native win32 dark theme functional is a matter of debate, but I think a few native function hooks will get us there with no changes to SWT code at all. Have you considered this approach? I did some experiments, and it appears very promising. Considering your reverse engineering work, I'd expect you to be the first person to propose uxtheme hooking. We do use SWT, but not for Eclipse. We also basically want a dark theme, but it's a different dark theme compared to what Eclipse does. If a binary configuration is to be made, I will apparently have to choose compatibility with _our_ dark theme. Otherwise, a more fine-tuned configuration is needed. That's what I currently aim for. As for uxtheme hooks, 1) I am not aware of a way to hook native functions with SWT. 2) I think that native hooks are a complicated beast to make really reliable. With some extra time to think, I still believe that my approach is better: 1) Again, I'm not aware of ready-to-use native hooking in SWT. A possible solution here would be to add some native hooking library to SWT's native library and then make some APIs to install Java-side handler for this hook. 2) Again, I'm rather worried about how reliable it's going to be. I would expect various kinds of problems. For example, multi-threading: while SWT itself tends to handle UI from one thread, Windows does sometime use additional threads. Then we'll have to either ignore these hook calls (and some parts of hooked drawing will sometimes draw old stuff) or make larger portions of SWT multithread-ready. At least the GC stuff. And even then, we'll need to make sure to avoid deadlocks, because windows thread hook can fire at unpredictable times. This issue is just one of many that I immediately feel worried about. 3) One obvious target for hooking would be 'uxtheme!DrawThemeBackground'. However, we still need an implementation for that drawing! That is, partially implementing owner draw. Let's for example discuss scrollbars. It's doable to draw them similar to what Windows does, but that's quite a bit of effort. The other tempting idea is to use images from actual Windows dark theme, but that would be subject to copyright issues. 4) Some window parts (I believe including window borders addressed by this patch) are handled in kernel and simply can't be hooked from user space. That you call "dirty hacks" I would rather call additional configuration options. To me, selecting a type of window border is definitely a configuration option and not a hack. Afterall, in native it's a configuration parameter to an API. And to me it sounds perfectly reasonable to have more configurable (rather then hardcoded) options, especially when they solve real world problems. (In reply to Alexandr Miloslavskiy from comment #8) The purpose of hooks is to implement complete, quasi-native dark theme, i.e. to complement or replace Win10 native dark (AllowDarkModeForWindow), Everything will happen in native code, below SWT level, and the only API is setTheme(DARK/LIGHT), just like Cocoa. I think it's impossible to deliver a quality dark theme on Windows without hooking. Hooking public WinAPI functions in your own process is both simple in reliable. I'm using Detours library (MIT license) for my experiments. I could roll my own hooking, but there's no point (and no money). Win32 controls don't draw on background threads. We can either ignore (pass-through) non-main thread calls or make hooked functions thread-safe. It's not a problem in practice. You're right that DrawThemeBackgroundEx is the most complex function to implement. All other functions are basically system color queries. Still, DrawThemeBackgroundEx is much simpler than owner drawing controls. It either draws a single bitmap or a single color. There's no logic beyond picking the bitmap/color for a given the part/state. To maintain native look and feel, dark control bitmaps have to be derived from Windows bitmaps, there's no way around it. Since Win10 theme is rather flat, I hope to cut the number of required bitmaps. E.g. buttons can be drawn with fills and borders, so are edit/combo boxes and scrollbars (except the arrows). I might even try to draw checkbox/etc glyphs from fonts to save the effort of crafting 5 DPI variants of every bitmap. Regardign borders, most of the borders come from the theme. Old-style borders can be overridden in WM_NCPAINT (probably). DefWindowProc has to be hooked anyway, for WM_CTLCOLOR and WM_ERASEBKGND. Adding platform-specific custom properties to override native painting is totally a hack, especially when the true goal is not extra configurability, but faking a dark theme. (In reply to Nikita Nemkin from comment #9) > (In reply to Alexandr Miloslavskiy from comment #8) > The purpose of hooks is to implement complete, quasi-native dark theme, i.e. > to complement or replace Win10 native dark (AllowDarkModeForWindow), > Everything will happen in native code, below SWT level, and the only API is > setTheme(DARK/LIGHT), just like Cocoa. > > I think it's impossible to deliver a quality dark theme on Windows without > hooking. > > Hooking public WinAPI functions in your own process is both simple in > reliable. I'm using Detours library (MIT license) for my experiments. I > could roll my own hooking, but there's no point (and no money). Hi Nikita, For proper evaluation of both the approaches, we will need more information on the experiment with 3rd party library you mentioned above. If not patches, then may be sharing the steps via an article on SWT website would be a good starting point for others to try: git://git.eclipse.org/gitroot/www.eclipse.org/swt.git > Win32 controls don't draw on background threads From some previous research, I was sure that `AnimateWindow()` uses additional threads for painting. However, now I have tested `DrawThemeBackground` and `DrawThemeBackgroundEx` specifically and indeed, I didn't get any hits from other threads. > For proper evaluation of both the approaches, we will need more information on > the experiment with 3rd party library you mentioned above. I would also like to point out that it would probably take quite some time before the new approach is ready for publishing (if it proves to be reasonable). At the same time, my patches are ready to use today, and will improve user experience for Eclipse's dark theme right now, especially the dark scrollbar part. We will continue to use my patches in our product for now. While we'd like to have less patches on our fork, that's not the top priority. My patches work well for us and I have many other things to work on, so without some promising movement, these dark theme patches are going to just stay as is: Bug 444560 - scrollbars Bug 560316 - this patch Bug 560284 - table header Bug 560358 - menu To summarize, I'm not ready to delve in to a large-scale project that will only give very little benefit to us, because my patches already do the job. If someone else is willing to do the work, I'm of course happy to get the results for free. My intuition says that the approach is promising, but will have many problems to overcome before it really works. Created attachment 282824 [details]
Eclipse (old)
Created attachment 282825 [details]
Eclipse (with patch)
Unfortunately for Eclipse, its dark theme is somewhat inconsistent and it mixes dark parts with grey parts. The patch's effect probably makes it better for dark part and worse for grey part. See screenshots and find (1)(2)(3)(4) on it for comparison.
In our product, dark theme is actually dark, and the patch gives good effect. (In reply to Alexandr Miloslavskiy from comment #14) > Created attachment 282825 [details] > Eclipse (with patch) > > Unfortunately for Eclipse, its dark theme is somewhat inconsistent and it > mixes dark parts with grey parts. The patch's effect probably makes it > better for dark part and worse for grey part. See screenshots and find > (1)(2)(3)(4) on it for comparison. I think it looks better. Can you upload Gerrit for Platform UI to enable it? Looks like you managed to set it for your testing. :-) Thank you for using the Eclipse IDE as test case. It wasn't too easy to figure how to run published Eclipse with my own SWT, but I managed to do that yesterday. So yes, now I can test Eclipse with SWT patches. I'm updating patches one by one, so unless something goes wrong, we'll have this one merged until tomorrow. (In reply to Alexandr Miloslavskiy from comment #17) > It wasn't too easy to figure how to run published Eclipse with my own SWT, > but I managed to do that yesterday. So yes, now I can test Eclipse with SWT > patches. I think if you import SWT plug-ins into your Eclipse workspace you can select Run As-> Eclipse application on one of them and it will start a runtime Eclipse with your patches. I'm using IDEA for development.
This is the way I found to inject my SWT into Eclipse:
1) Add 'C:\Eclipse\plugins' to classpath
2) Set 'org.eclipse.equinox.launcher.Main' as main class
3) Add this to arguments ('C:\SWT\.classes' is SWT compiled from sources)
-dev C:\SWT\.classes
(In reply to Alexandr Miloslavskiy from comment #20) > I'm using IDEA for development. > > This is the way I found to inject my SWT into Eclipse: > 1) Add 'C:\Eclipse\plugins' to classpath > 2) Set 'org.eclipse.equinox.launcher.Main' as main class > 3) Add this to arguments ('C:\SWT\.classes' is SWT compiled from sources) > -dev C:\SWT\.classes Sounds harder than doing it via Eclipse. In Eclipse: 1.) Import projects into workspace 2.) Select Run as-> Eclipse application This is probably not enough to convince me to learn and switch to a new IDE :) Changes are merged in master, resolving now. Thanks! Verified using Build id: I20200518-2220 on Win10. Details on how to disable this feature: On Windows 10 all the dark theme tweaks, including the text border tweak can be disabled using the org.eclipse.swt.internal.win32.disableCustomThemeTweaks Java property. For Example. add this VM argument in eclipse.ini or on the command line after -vmargs: -Dorg.eclipse.swt.internal.win32.disableCustomThemeTweaks=true |