Bug 562454 - [win32][Dark theme] Menu is not configurable
Summary: [win32][Dark theme] Menu is not configurable
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.16   Edit
Hardware: PC Windows 10
: P3 normal (vote)
Target Milestone: 4.17 M3   Edit
Assignee: Alexandr Miloslavskiy CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
: 544366 574972 (view as bug list)
Depends on: 562043
Blocks: 563540 566401
  Show dependency tree
 
Reported: 2020-04-24 04:08 EDT by Niraj Modi CLA
Modified: 2022-01-27 03:03 EST (History)
6 users (show)

See Also:


Attachments
capture of eclipse with ownerdraw menus (40.40 KB, image/png)
2020-04-25 07:48 EDT, Nicolas Mahoudeaux CLA
no flags Details
PatchV1, dark (186.88 KB, image/png)
2020-06-15 11:42 EDT, Alexandr Miloslavskiy CLA
no flags Details
PatchV1, light (177.76 KB, image/png)
2020-06-15 11:42 EDT, Alexandr Miloslavskiy CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Niraj Modi CLA 2020-04-24 04:08:02 EDT
Created this bug based on the discussion in https://bugs.eclipse.org/bugs/show_bug.cgi?id=560358#c7
Comment 1 Niraj Modi CLA 2020-04-24 04:10:41 EDT
This bug is to investigate on how to style the menu on Windows for dark theme purpose.
Comment 2 Nicolas Mahoudeaux CLA 2020-04-25 07:48:07 EDT
Created attachment 282557 [details]
capture of eclipse with ownerdraw menus

I have a working POC with ownerdraw menus (see attachment)

I think there still is some edge cases to address and a lot of refactoring to do.

For information, right now I'm mainly modifying MenuItem.java and will isolate the changes to be only available when customTheme is enable to avoid regressions when it is not enabled.

What to you think ? Is it interesting for you that I continue to work on it ? And is there any chance it will be merged in SWT ?

If so, is there a sample with all the menus variations in order to test all edge cases ? Right now I'm using 'TextEditor' and .. Eclipse and the menus seems fine.
Comment 3 Alexandr Miloslavskiy CLA 2020-04-27 12:31:08 EDT
This screenshot looks nice enough to me. The only issue here is dark > icons that open popup menus. Luckily, I already did the research and know how to replace them. Will share it once this is the last issue standing.

Now I think it's time to explain the rather weird situation in Bug 560358.

I'm working for a company that uses SWT for some other product not connected to Eclipse. More then a year ago, we did some dark theme improvements, including dark window title, menu bar, menus themselves and scrollbars. The result looked very good to us.

Title, menu and scrollbar patches are Win10 specific. I shared our patch for dark scrollbar in Bug 444560 on 2019-01-31 (So 1 year and 3 months ago), because dark scrollbars were very sought after in Eclipse. The demand for dark scrollbars still didn't stop SWT team from burying the patch in endless discussions of how the API to enable it should look like.

At roughly the same time, I had patch for dark menu bar to share, but I saw no reason submitting it before dark scrollbars made their way, which they didn't until very recently.

Now, why didn't I consider fully owner-drawing all menus instead of using win10-specific tricks? Simply because it meant more effort to make menus look exactly the same as before, and I foreseen that it won't be so welcome if setting menu's color also changes its appearance. At the same time, our product already had perfect menus through the other Win10-specific patch. So I seen that I fixed menu bar (which isn't affected by menu patch) in a very cheap way. However, what I couldn't foresee is that SWT team will be unable to arrive at some decision about the yes/no API for over a year! This shifted cost-efficiency calculations in a very weird way, where total costs already surpass effort required to fully owner-draw menu.

That said, at this point, Eclipse will benefit from fully owner drawing, if there's somebody willing to contribute the work to make it happen.

On the other hand, dark menu isn't much help without dark scrollbars, and current dark scrollbars approach requires Win10, and I have a patch that already makes menu perfect on Win10. The patch is rather undocumented right now, but I still expect Microsoft to release their dark API (which I'm currently using undocumented) somewhat soon. If they do, that will probably render ownerdraw menu code rather useless (unless you want to have purple menus).
Comment 4 Alexandr Miloslavskiy CLA 2020-04-27 12:40:16 EDT
For reference, this is the JNA code we're using to get perfect dark menus:

----------------
final Kernel32 kernel32 = Kernel32.INSTANCE;

final WinDef.HMODULE uxthemeDll = kernel32.GetModuleHandle("uxtheme.dll");
if (uxthemeDll == null) {
	// Probably running application with 'Disable Visual Themes' compatibility setting
	return false;
}

// Function is only exported by Ordinal#135
final Pointer ptrSetPreferredAppMode = kernel32.GetProcAddress(uxthemeDll, 135);
if (ptrSetPreferredAppMode == null) {
	// Shouldn't happen
	return false;
}

final Function funcSetPreferredAppMode = Function.getFunction(ptrSetPreferredAppMode, Function.ALT_CONVENTION);

// In Win10 build 17763 signature is AllowDarkModeForApp(BOOL)
// In Win10 build 18334 signature is SetPreferredAppMode(enum PreferredAppMode)
//     enum PreferredAppMode {Default = 0, AllowDark = 1, ForceDark = 2, ForceLight = 3}
// Using PreferredAppMode::ForceDark works for both.
final int PreferredAppMode_ForceDark = 2;
funcSetPreferredAppMode.invoke(new Object[]{PreferredAppMode_ForceDark});
return true;
----------------
Comment 5 Alexandr Miloslavskiy CLA 2020-04-27 12:42:57 EDT
This code can only be used on recent Win10. As far as I remember, it needs to be invoked before any menus are created.
Comment 6 Nicolas Mahoudeaux CLA 2020-04-27 15:50:11 EDT
Hi ! 
First of all, Thanks for the background story, it helps me better understand your eagerness to have your patch merged and why your patch for full dark menus is too low level. Make perfect sense.

What is funny is that I was under the impression that YOU were the maintainer of SWT ...

About the menus, I still don't have a solution that I can share, lot of hard coded values (colors!) and I still have to figure out how to share the code.

On the architecture standpoint, I've separated the rendering (wmDraw & wmMeasure) from the rest of the MenuItem code so that, 1. I can keep the existing code as is (when no custom theme) and, 2. if microsoft release the information and the user is on windows10, it will be possible to have a third implementation if needed.


I'll work a bit more on my code and I'll share it so we can discuss it.

Nicolas.
Comment 7 Alexandr Miloslavskiy CLA 2020-04-27 16:31:26 EDT
> I was under the impression that YOU were the maintainer of SWT

That's partially correct and incorrect. In SWT, there is a team of "committers"
who can commit directly to repository. I'm on that team. However, there are
other committers too. I joined the team some year and a half ago.

Here's some overview of it:
https://www.eclipse.org/projects/handbook/#contributing

This document explains the steps necessary to submit a patch:
https://wiki.eclipse.org/Gerrit

From it, pay attention to these sections:
1) User Account
2) Git over HTTPS
3) Install the commit-msg hook in your repository

Take your time and finish the patch so it's good enough.
Comment 9 Lars Vogel CLA 2020-05-19 08:57:04 EDT
*** Bug 544366 has been marked as a duplicate of this bug. ***
Comment 10 Eclipse Genie CLA 2020-06-05 03:26:48 EDT
New Gerrit change created: https://git.eclipse.org/r/164225
Comment 11 Nicolas Mahoudeaux CLA 2020-06-05 03:29:17 EDT
Hi

Lars, thanks from the link, it was well explained.

Today I tried my first merge request, now let's see what happens.
Comment 12 Niraj Modi CLA 2020-06-05 08:02:04 EDT
(In reply to Eclipse Genie from comment #10)
> New Gerrit change created: https://git.eclipse.org/r/164225

Targeting for 4.17 M1
Comment 13 Alexandr Miloslavskiy CLA 2020-06-15 11:42:07 EDT
Created attachment 283286 [details]
PatchV1, dark
Comment 14 Alexandr Miloslavskiy CLA 2020-06-15 11:42:26 EDT
Created attachment 283287 [details]
PatchV1, light
Comment 15 Alexandr Miloslavskiy CLA 2020-06-15 11:42:36 EDT
Sorry that it has taken me so long to reply. All of sudden, I'm stormed with other things to solve.

I have tried the patch with Eclipse and unfortunately it doesn't look so good, please see attached screenshots.

I'm testing on DPI 200%, I feel that this has to do with what I see.
Comment 16 Alexandr Miloslavskiy CLA 2020-06-15 11:43:53 EDT
To be specific, Dark theme (where patch is active):
1) Doesn't draw menubar properly
2) Popup menus as smaller and cramped
Comment 17 Nicolas Mahoudeaux CLA 2020-06-15 15:18:31 EDT
Good catch, I tested only up to 150% but I reproduce it at 175 & 200% (what kind of screen have you ? :) ).

Is it possible that there is a bug in the SWT scaling ?
Here is what I found while debuging :

gc.stringExtent("&File"):
100% : Size of &File => 28 15
125% : Size of &File => 35 20
150% : Size of &File => 40 25
175% : Size of &File => 24 15
200% : Size of &File => 28 16

At 175% it seems the size is shrinking


"&File" size : stringExtentInPixels vs stringExtent
100% : 28,15 => 28,15
200% : 56,32 => 28,16

I think the real size at 200% is 56,32 but the methods stringExtent returns autoscaledown values of stringExtentInPixels (which is not public)

What do you think ? Maybe I'm not using the intended methods to handle hight DPI, but it was a public method that seemed to work
Comment 18 Alexandr Miloslavskiy CLA 2020-06-15 16:44:47 EDT
Sorry, I'm still trying to deal with my backlog, so can't be of much assistance at the moment.

I'm using a 200% screen, my boss "forced" me to have it so I can see any HiDPI problems in our product :)
Comment 19 Alexandr Miloslavskiy CLA 2020-08-19 13:09:57 EDT
Note that since Bug 562043 SWT will also have dark popup menus from Windows native dark theme.
Comment 20 Lars Vogel CLA 2020-08-19 15:22:05 EDT
Done via Bug 562043, the menu is now dark under windows in the Eclipse dark theme. Thanks, Alexandr. 

I suggest to open new bugs for remaining issues.
Comment 21 Lars Vogel CLA 2020-08-19 15:23:56 EDT
And thanks to Nicolas for his patch, even though the solution was different.

Nicolas, do you still want to make the menu configuration more flexible? If so, please open a new bug and attach your Gerrit to it.
Comment 22 Eclipse Genie CLA 2020-08-20 03:49:12 EDT
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/167993
Comment 24 Niraj Modi CLA 2020-08-26 03:40:45 EDT
Verified in Build id: I20200825-1800 on Win10, will add SWT N&N entry shortly.
Comment 25 Eclipse Genie CLA 2020-08-26 03:48:26 EDT
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/168234
Comment 27 Niraj Modi CLA 2022-01-27 03:03:47 EST
*** Bug 574972 has been marked as a duplicate of this bug. ***