Bug 546859 - [API] Provide APIs for Dark theme support
Summary: [API] Provide APIs for Dark theme support
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.12   Edit
Hardware: PC All
: P3 normal with 1 vote (vote)
Target Milestone: 4.24 M1   Edit
Assignee: Niraj Modi CLA
QA Contact:
URL:
Whiteboard:
Keywords: api, triaged
: 550003 (view as bug list)
Depends on: 444560 549733
Blocks: 577357 578260 549898 550003
  Show dependency tree
 
Reported: 2019-04-30 08:02 EDT by Lakshmi P Shanmugam CLA
Modified: 2022-11-09 02:19 EST (History)
17 users (show)

See Also:


Attachments
Colors in light theme (26.25 KB, text/plain)
2022-07-12 14:08 EDT, Christopher Mindus CLA
no flags Details
Colors in dark theme (27.19 KB, text/plain)
2022-07-12 14:09 EDT, Christopher Mindus CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lakshmi P Shanmugam CLA 2019-04-30 08:02:08 EDT
From https://bugs.eclipse.org/bugs/show_bug.cgi?id=540357#c12,
- Some other platforms have a native dark mode, too, so we could try to make to cross-platform api (as far as possible) with methods to:
   - query if the platform has native dark mode support at all
   - query what the current system setting is
   - set the requested app appearance.

From https://bugs.eclipse.org/bugs/show_bug.cgi?id=540357#c15,
Mac & GTK provide native dark theme support. SWT on Mac & GTK provide internal APIs that are used by Platform UI to use the native dark theme. This bug is to provide SWT APIs to set/get the theme.

One use case for the API is: Eclipse can automatically start in light or dark theme based on the OS theme.
Comment 1 Eric Williams CLA 2019-04-30 09:19:25 EDT
By public API, do you mean exposing some form of "setDarkThemePreferred" in something like Display? Or is this intended to be per-widget theming?
Comment 2 Lakshmi P Shanmugam CLA 2019-05-02 07:47:01 EDT
(In reply to Eric Williams from comment #1)
> By public API, do you mean exposing some form of "setDarkThemePreferred" in
> something like Display? Or is this intended to be per-widget theming?

Hi Eric,
The goal is to launch Eclipse automatically in dark or light theme based on the System theme. For this, Platform UI will need an API to query the system theme based on which it can set the Eclipse theme. Something like isSystemDarkTheme() or getSystemTheme(). Is it possible to check this info on GTK?

I'm not sure yet if we need to expose an API in SWT to set the dark theme. At least there is no use-case from Eclipse POV since UI already has access to OS.setDarkThemePreferred.
For SWT applications, I think they should be able to automatically launch with the System theme. This works on Mac and I added a new System property "org.eclipse.swt.display.useSystemTheme" to control this behaviour (Bug 540357).
WDYT?
Comment 3 Eric Williams CLA 2019-05-02 09:14:41 EDT
(In reply to Lakshmi Shanmugam from comment #2)
> (In reply to Eric Williams from comment #1)
> > By public API, do you mean exposing some form of "setDarkThemePreferred" in
> > something like Display? Or is this intended to be per-widget theming?
> 
> Hi Eric,
> The goal is to launch Eclipse automatically in dark or light theme based on
> the System theme. For this, Platform UI will need an API to query the system
> theme based on which it can set the Eclipse theme. Something like
> isSystemDarkTheme() or getSystemTheme(). Is it possible to check this info
> on GTK?

I think it's possible, I'd have to look into it a bit further. However I'm a bit confused about the idea of getSystemTheme(), why would we need this? What theme would SWT be using if not the system theme?

 
> I'm not sure yet if we need to expose an API in SWT to set the dark theme.
> At least there is no use-case from Eclipse POV since UI already has access
> to OS.setDarkThemePreferred.
> For SWT applications, I think they should be able to automatically launch
> with the System theme. This works on Mac and I added a new System property
> "org.eclipse.swt.display.useSystemTheme" to control this behaviour (Bug
> 540357).
> WDYT?

On GTK SWT will always run using the colors defined by the system theme. This can be overridden via some environment variables native to GTK, but the fall-back is always the system theme. Maybe it works differently on Mac (I'm not sure), but where else would SWT load its colors from?
Comment 4 Lakshmi P Shanmugam CLA 2019-05-03 05:49:08 EDT
(In reply to Eric Williams from comment #3)
> (In reply to Lakshmi Shanmugam from comment #2)
> > (In reply to Eric Williams from comment #1)
> > > By public API, do you mean exposing some form of "setDarkThemePreferred" in
> > > something like Display? Or is this intended to be per-widget theming?
> > 
> > Hi Eric,
> > The goal is to launch Eclipse automatically in dark or light theme based on
> > the System theme. For this, Platform UI will need an API to query the system
> > theme based on which it can set the Eclipse theme. Something like
> > isSystemDarkTheme() or getSystemTheme(). Is it possible to check this info
> > on GTK?
> 
> I think it's possible, I'd have to look into it a bit further. However I'm a
> bit confused about the idea of getSystemTheme(), why would we need this?
> What theme would SWT be using if not the system theme?
> 
Display.isSystemDarkTheme() is specific to dark theme and will return false when system theme is not dark.

Display.getSystemTheme() can return something like SWT.Theme_Dark or SWT.Theme_Light depending on whether the system theme is light or dark. It allows to support other theme values in the future if required.

In https://git.eclipse.org/r/#/c/136091/ Alexandr proposed a similar API to check the theme - Display.isSystemThemeAvailable(SWT.THEME_SYSTEMUI_DARK).

Any other proposals for the API are welcome.

>  
> > I'm not sure yet if we need to expose an API in SWT to set the dark theme.
> > At least there is no use-case from Eclipse POV since UI already has access
> > to OS.setDarkThemePreferred.
> > For SWT applications, I think they should be able to automatically launch
> > with the System theme. This works on Mac and I added a new System property
> > "org.eclipse.swt.display.useSystemTheme" to control this behaviour (Bug
> > 540357).
> > WDYT?
> 
> On GTK SWT will always run using the colors defined by the system theme.
> This can be overridden via some environment variables native to GTK, but the
> fall-back is always the system theme. Maybe it works differently on Mac (I'm
> not sure), but where else would SWT load its colors from?

On Mac, the application can have its appearance(theme) different from the System appearance. For Example, if System is set to Dark theme, application can choose to use Light mode and vice-versa. SWT gets the colors from the UI Toolkit based on the appearance that is set to the application (https://developer.apple.com/documentation/appkit/nsappearance?language=objc).
Comment 5 Alexandr Miloslavskiy CLA 2019-05-06 05:17:07 EDT
Newest Win10 also aims to support four settings:
* Default (legacy)
* DarkOnDark (dark is system UI is dark, light otherwise)
* ForceDark
* ForceLight

According to Eric, Mac already supports these as well. On GTK, this should also be possible by playing with second parameter of gtk_css_provider_get_named.

So I think that 3 APIs should be provided:
* Display.getSystemTheme() - as already described by Eric.
* Display.isThemeAvailable() - should be helpful for compatibility issues.
* Display.setTheme() - will accept {Default, DarkOnDark, ForceDark, ForceLight}.

I would remove 'Display.isSystemDarkTheme' and 'Display.setDarkThemePreferred' as too specific.

'org.eclipse.swt.display.useSystemTheme' should probably be replaced with explicit call to 'Display.setTheme'.

I don't think that defaulting to 'DarkOnDark' is a good idea, most applications won't be ready for that.
Comment 6 Eric Williams CLA 2019-05-06 09:43:58 EDT
(In reply to Alexandr Miloslavskiy from comment #5)
> * Display.isThemeAvailable() - should be helpful for compatibility issues.

What would this API return? Whether or not a dark theme is available, or whether *any* theme is available?

> * Display.setTheme() - will accept {Default, DarkOnDark, ForceDark,
> ForceLight}.
> 
> I would remove 'Display.isSystemDarkTheme' and
> 'Display.setDarkThemePreferred' as too specific.
> 
> 'org.eclipse.swt.display.useSystemTheme' should probably be replaced with
> explicit call to 'Display.setTheme'.
> 
> I don't think that defaulting to 'DarkOnDark' is a good idea, most
> applications won't be ready for that.

We have to be mindful of applications running outside of Eclipse that don't care about dark themes and just want to use the system theme for native look and feel out of the box. With this in mind, any of these APIs should default to whatever the system is running, at least until a client calls them.
Comment 7 Alexandr Miloslavskiy CLA 2019-05-06 09:59:37 EDT
It will be:
boolean Display.isThemeAvailable(int themeID)

By the way, why doesn't SWT use java enums?
Comment 8 Eric Williams CLA 2019-05-06 16:27:11 EDT
(In reply to Alexandr Miloslavskiy from comment #7)
> It will be:
> boolean Display.isThemeAvailable(int themeID)

And what will theme ID be?


> By the way, why doesn't SWT use java enums?

If I had to guess, probably because SWT was written pre-Java 1.5.
Comment 9 Lakshmi P Shanmugam CLA 2019-05-07 07:18:09 EDT
(In reply to Alexandr Miloslavskiy from comment #5)
> 
> So I think that 3 APIs should be provided:
> * Display.getSystemTheme() - as already described by Eric.
> * Display.isThemeAvailable() - should be helpful for compatibility issues.
What does theme availability mean? Can you please give an example of how this API will be useful/used?

> * Display.setTheme() - will accept {Default, DarkOnDark, ForceDark,
> ForceLight}.
> 
I'm not sure if SWT can support these multiple options on all platforms. It's possible on Mac, but not sure about GTK.
If supported, the values to pass to setTheme() could be DEFAULT, SYSTEM_THEME, DARK_THEME and LIGHT_THEME.

> I would remove 'Display.isSystemDarkTheme' and
> 'Display.setDarkThemePreferred' as too specific.
> 
> 'org.eclipse.swt.display.useSystemTheme' should probably be replaced with
> explicit call to 'Display.setTheme'.
> 
> I don't think that defaulting to 'DarkOnDark' is a good idea, most
> applications won't be ready for that.
Yes, at least initially when the SWT Dark theme support work is not complete.
That's why having a system property is useful. It allows SWT applications to try-out or switch to dark mode without making code changes. But once the SWT Dark theme support is stable, using the System theme should be the default. As Eric pointed out it's already the default on GTK.
Comment 10 Eric Williams CLA 2019-05-07 09:46:37 EDT
(In reply to Lakshmi Shanmugam from comment #9)
> (In reply to Alexandr Miloslavskiy from comment #5)
> > 
> > So I think that 3 APIs should be provided:
> > * Display.getSystemTheme() - as already described by Eric.
> > * Display.isThemeAvailable() - should be helpful for compatibility issues.
> What does theme availability mean? Can you please give an example of how
> this API will be useful/used?
> 
> > * Display.setTheme() - will accept {Default, DarkOnDark, ForceDark,
> > ForceLight}.
> > 
> I'm not sure if SWT can support these multiple options on all platforms.
> It's possible on Mac, but not sure about GTK.
> If supported, the values to pass to setTheme() could be DEFAULT,
> SYSTEM_THEME, DARK_THEME and LIGHT_THEME.

Not sure about light theme, as GTK only has a notion of "dark" and "non-dark". Also DEFAULT and SYSTEM_THEME should really be the same here, at least that's how it works on GTK.

> 
> > I would remove 'Display.isSystemDarkTheme' and
> > 'Display.setDarkThemePreferred' as too specific.
> > 
> > 'org.eclipse.swt.display.useSystemTheme' should probably be replaced with
> > explicit call to 'Display.setTheme'.
> > 
> > I don't think that defaulting to 'DarkOnDark' is a good idea, most
> > applications won't be ready for that.
> Yes, at least initially when the SWT Dark theme support work is not complete.
> That's why having a system property is useful. It allows SWT applications to
> try-out or switch to dark mode without making code changes. But once the SWT
> Dark theme support is stable, using the System theme should be the default.
> As Eric pointed out it's already the default on GTK.

It's worth pointing out that on GTK, if the system theme is dark then SWT will use that. I guess in that case the dark theme would be the default?
Comment 11 Eric Williams CLA 2019-05-07 09:53:30 EDT
Also: on GTK we only support the default theme [1]. So any bugs caused by non-standard (i.e. non-Adwaita) themes will require community support.

The reason we have this is that unlike Windows/Mac, GTK doesn't guarantee theme compatibility outside of Adwaita, and anyone can write their own theme and use it, making support for all themes a huge effort.

1: https://www.eclipse.org/projects/project-plan.php?planurl=http://www.eclipse.org/eclipse/development/plans/eclipse_project_plan_4_12.xml#target_environments
Comment 12 Alexandr Miloslavskiy CLA 2019-05-07 09:56:11 EDT
-----------------------
Clarified suggested API
-----------------------
/**
 * Tries to match historical appearance that existed
 * before light/dark separation.
 */
int SWT.THEME_LEGACY   = 0;

/**
 * Matches operating system's UI.
 */
int SWT.THEME_MATCH_OS = 1;

/**
 * Dark theme, even if operating system's UI is light.
 * NOTE: when not supported, THEME_MATCH_OS could still work.
 */
int SWT.THEME_DARK     = 2;

/**
 * Light theme, even if operating system's UI is dark.
 * NOTE: when not supported, THEME_MATCH_OS could still work.
 */
int SWT.THEME_LIGHT    = 3;

/**
 * Returns whether a theme is supported by system.
 *
 * @param themeID one of SWT.THEME_XXX
 * @return true if theme is supported
 */
static boolean Display.isSystemThemeSupported(int themeID);

/**
 * Returns SWT theme closest to theme used by system.
 *
 * @return one of SWT.THEME_XXX
 */
static int Display.getSystemTheme();

/**
 * Instructs SWT to use specified system theme.
 *
 * @param themeID one of SWT.THEME_XXX
 */
void Display.setSystemTheme(int themeID);

-----------------------
Notes
-----------------------
1) Dark Theme appears (undocumented) in Win10 1809, but you can't force dark if OS UI is light.
2) In Win10 1903 (undocumented) it's now possible to force light/dark.
3) On Windows, default doesn't match system, and I think will never be - too many applications would look ugly, and Windows actually cares about compatibility (I'm looking at you, GTK!)
Comment 13 Eric Williams CLA 2019-05-07 16:27:10 EDT
(In reply to Alexandr Miloslavskiy from comment #12)
> -----------------------
> Clarified suggested API
> -----------------------
> /**
>  * Tries to match historical appearance that existed
>  * before light/dark separation.
>  */
> int SWT.THEME_LEGACY   = 0;

I don't really know what you mean by this, can you clarify?

 
> /**
>  * Matches operating system's UI.
>  */
> int SWT.THEME_MATCH_OS = 1;

I think this is doable, but we'd have to check if gtk_css_provider_new() returns the GtkCssProvider for the default system theme. It also gets a bit complicated when GTK_THEME is specified, so supporting this is a hard "maybe" for now IMO.

> 
> /**
>  * Dark theme, even if operating system's UI is light.
>  * NOTE: when not supported, THEME_MATCH_OS could still work.
>  */
> int SWT.THEME_DARK     = 2;

Doable, just load the :dark variant of the current theme.

 
> /**
>  * Light theme, even if operating system's UI is dark.
>  * NOTE: when not supported, THEME_MATCH_OS could still work.
>  */
> int SWT.THEME_LIGHT    = 3;

I don't think there is such a thing as a "light" variant of a GTK theme, though I could be wrong.

 
> /**
>  * Returns whether a theme is supported by system.
>  *
>  * @param themeID one of SWT.THEME_XXX
>  * @return true if theme is supported
>  */
> static boolean Display.isSystemThemeSupported(int themeID);

Shouldn't be an issue on GTK.

 
> /**
>  * Returns SWT theme closest to theme used by system.
>  *
>  * @return one of SWT.THEME_XXX
>  */
> static int Display.getSystemTheme();

I actually don't think we can implement this...GTK has very little knowledge of what theme the system default is. We can guess based on what theme SWT is run with, but that isn't definitive. Also, if a user runs SWT with GTK_THEME then it all falls apart.

 
> /**
>  * Instructs SWT to use specified system theme.
>  *
>  * @param themeID one of SWT.THEME_XXX
>  */
> void Display.setSystemTheme(int themeID);

Should work on GTK, but only for dark variants.


IMO I think this API will be a bit of a headache on GTK. Since SWT only officially supports Adwaita, I'd be inclined to make it Adwaita/Adwaita:dark only, and specify that the API works only as a hint on GTK. Otherwise it will become too cumbersome to support. That said, if we can address the GTK concerns or design it in a way that will make it work, I am not against it.
Comment 14 Alexandr Miloslavskiy CLA 2019-05-08 02:57:50 EDT
Eric, you're thinking GTK only, that's why the suggested API looks strange to you. And I'm actually trying to align Windows and GTK, and also what I know about macOS.

> > int SWT.THEME_LEGACY   = 0;
> I don't really know what you mean by this, can you clarify?

This is basically the "default", or "current", where SWT doesn't try to do anything. On Windows, it will most likely match SWT.THEME_LIGHT, even on dark system theme, because I really doubt that Windows will ever force dark theme on applications due to compatibility reasons. On GTK this will mean to just take system theme as is.

> > int SWT.THEME_MATCH_OS = 1;
> It also gets a bit complicated when GTK_THEME is specified,
> so supporting this is a hard "maybe" for now IMO.

I would say that if GTK_THEME is specified, then it overrides what we consider to be the system theme. So you can simply ignore this issue. And if you don't want to support it - OK, just return false from 'Display.isSystemThemeSupported'. My impression that on GTK this is a non-issue, 
SWT.THEME_MATCH_OS basically means to do nothing special. On Windows however it will do additional work, like forcing dark scrollbars if system is dark.

> > int SWT.THEME_DARK     = 2;
> Doable, just load the :dark variant of the current theme.

I expected that this goes down to 'setDarkThemePreferred'.

> > int SWT.THEME_LIGHT    = 3;
> I don't think there is such a thing as a "light" variant of a GTK theme,
> though I could be wrong.

Return false from 'Display.isSystemThemeSupported' if so. On Windows there is such a thing, and I understand that macOS also has it.

> > static int Display.getSystemTheme();
> I actually don't think we can implement this...GTK has very little knowledge
> of what theme the system default is. We can guess based on what theme SWT is
> run with, but that isn't definitive. Also, if a user runs SWT with GTK_THEME
> then it all falls apart.

Again, GTK_THEME is to be ignored, because user explicitly wanted to mess with theme. I would suggest to reply based on whether 'dark' variant of theme is loaded. Or you can simply reply with SWT.THEME_MATCH_OS. Yes, I agree, GTK has too many themes without explicitly differentiating them into light and dark, but Windows and macOS has this difference.

> > void Display.setSystemTheme(int themeID);
> Should work on GTK, but only for dark variants.

I think loading non-dark-variant when dark is system's default should also be easy. Otherwise, just return false from 'Display.isSystemThemeSupported'.

> IMO I think this API will be a bit of a headache on GTK.

Nay, it's providing a uniform cross-platform API is what a real headache is ;)
Comment 15 Lakshmi P Shanmugam CLA 2019-05-08 07:06:08 EDT
(In reply to Alexandr Miloslavskiy from comment #12)
> -----------------------
> Clarified suggested API
> -----------------------
> /**
>  * Tries to match historical appearance that existed
>  * before light/dark separation.
>  */
> int SWT.THEME_LEGACY   = 0;
> 
> /**
>  * Matches operating system's UI.
>  */
> int SWT.THEME_MATCH_OS = 1;
How about THEME_MATCH_SYSTEM?

> 
> /**
>  * Dark theme, even if operating system's UI is light.
>  * NOTE: when not supported, THEME_MATCH_OS could still work.
>  */
> int SWT.THEME_DARK     = 2;
> 
> /**
>  * Light theme, even if operating system's UI is dark.
>  * NOTE: when not supported, THEME_MATCH_OS could still work.
>  */
> int SWT.THEME_LIGHT    = 3;
> 
> /**
>  * Returns whether a theme is supported by system.
>  *
>  * @param themeID one of SWT.THEME_XXX
>  * @return true if theme is supported
>  */
> static boolean Display.isSystemThemeSupported(int themeID);
If I understand correctly, client code would call this API with the themeID, and if it returns true it'll then call setTheme(themeID). 
Given the usecase I think the API should just be isThemeSupported(themeID). It should return true/false based on whether the themeID will be honoured by SWT's setTheme(themeID).

> 
> /**
>  * Returns SWT theme closest to theme used by system.
>  *
>  * @return one of SWT.THEME_XXX
>  */
> static int Display.getSystemTheme();
I think this API should return only return Theme_Dark or Theme_Light based on the current system theme. Returning match_os or legacy for system theme doesn't seem correct.
One use case of this API is to determine if the system theme is a dark variant based on which the Eclipse application can set its theme (Bug 547038). 

If we have to a get the themeID used by SWT or value set by the client, we could have a getter Display.getTheme() to match Display.setTheme().
 
> /**
>  * Instructs SWT to use specified system theme.
>  *
>  * @param themeID one of SWT.THEME_XXX
>  */
> void Display.setSystemTheme(int themeID);
> 
As the javadoc says, this API only instructs SWT to use a specific theme, but doesn't set/change the system/OS theme. So, it should be Display.setTheme().
Comment 16 Alexandr Miloslavskiy CLA 2019-05-08 08:49:26 EDT
> > int SWT.THEME_MATCH_OS = 1;
> How about THEME_MATCH_SYSTEM?
Currently, Win10 only supports dark theme for Windows Explorer and a few other applications. 'SYSTEM' felt somewhat vague to me. 'OS' felt like a better description.

> > static boolean Display.isSystemThemeSupported(int themeID);
> Given the usecase I think the API should just be isThemeSupported(themeID). It
> should return true/false based on whether the themeID will be honoured by SWT's
> setTheme(themeID).
I thought about it and decided to go with 'System' because on Windows it will only set individual items (currently only scrollbars are proposed), while the rest will remain as is. I would say, there are two layers of themes in SWT: system-supported part and on top of it individual .setColor() overrides for specific widgets. On Windows, there wasn't such thing as dark theme for a while now, yet Eclipse can be (mostly) dark by overriding colors with .setColor()/etc. On the other hand, in the future I would expect Windows to provide more theming by default. So maybe we could use .setTheme() and just say that currently it's only barely supported?

> > static int Display.getSystemTheme();
> I think this API should return only return Theme_Dark or Theme_Light based on
> the current system theme. Returning match_os or legacy for system theme doesn't
> seem correct. One use case of this API is to determine if the system theme is a
> dark variant based on which the Eclipse application can set its theme (Bug 547038). 
> If we have to a get the themeID used by SWT or value set by the client, we
> could have a getter Display.getTheme() to match Display.setTheme().
As we just discussed with Eric, on GTK it could be hard to label current theme "light" or "dark". Returning THEME_MATCH_OS sounds reasonable in this case.

> > void Display.setSystemTheme(int themeID);
> As the javadoc says, this API only instructs SWT to use a specific theme, but
> doesn't set/change the system/OS theme. So, it should be Display.setTheme().
See my comments for 'isSystemThemeSupported'
Comment 17 Eric Williams CLA 2019-05-08 10:00:41 EDT
Okay, I think I understand it better now. Will happily review patches. :)
Comment 18 Till Brychcy CLA 2019-05-09 02:37:20 EDT
The comments read like the "query if the platform has native dark mode support at all"-part is not handled yet. It's hard to make an API without a real use case, so I'll try to explain that part from comment 0

In Bug 544039 Comment 10 I wrote that it would be great if we could simplify Eclipse's over-complicated preference page "General > Appearance".

I really think it should look like this:

Theme:
 [x] Match System settings 
 [ ] Light
 [ ] Dark
 [ ] Other: "Classic" [Choose....]

where "Match System settings" is only available, if the system has a native dark mode switching support (and is default in this case) - for this the api "query if the platform has native dark mode support at all" is required.

If "Match System settings" is selected, then the "query what the current system setting is"-api is required for setting matching eclipse-theme (which as side effect requests the matching app-level design via swt using the "set the requested app appearance." but of course still configures eclipse-ui colors and syntax highlighting colors)
Comment 19 Alexandr Miloslavskiy CLA 2019-05-09 02:54:25 EDT
This is why I suggested 'Display.isSystemThemeSupported' API.

But I do not agree that "Match System settings" should only be available if dark theme is present. To me, if system only has light theme, then the only theme you have actually matches system. Same goes for GTK, where by default whatever heme you have also matches system (but it's not necessarily light, it can be dark blue or anything else instead).
Comment 20 Till Brychcy CLA 2019-05-09 03:02:04 EDT
(In reply to Alexandr Miloslavskiy from comment #19)
> This is why I suggested 'Display.isSystemThemeSupported' API.

Good.

> 
> But I do not agree that "Match System settings" should only be available if
> dark theme is present.

Hmm at least on MacOS before 10.14 there simply is no "system setting" that could be matched, so showing a label like this that wouldn't make sense.

But maybe it could be always be shown if the label would be "Best for System Design" or something like maybe "Choose automatically"
Comment 21 Alexandr Miloslavskiy CLA 2019-05-09 03:06:08 EDT
On macOS < 10.14 there's just one system theme, and Eclipse will "match" this only theme. When user upgrades to 10.14 (and possibly select dark theme), Eclipse will continue to match system theme.
Comment 22 Till Brychcy CLA 2019-05-09 03:25:52 EDT
(In reply to Alexandr Miloslavskiy from comment #21)
> On macOS < 10.14 there's just one system theme, and Eclipse will "match"
> this only theme. When user upgrades to 10.14 (and possibly select dark
> theme), Eclipse will continue to match system theme.

Technically, Yes.  But from the user point of view, there is not "one system theme" but there is simply no concept of "system theme" at all.

So I just meant we cannot refer to a "system setting" or a "system theme" in the label if the system doesn't have that concept.

My solution would be not to show the choice if it doesn't make sense, but I agree we can always show it if we find a label that works.
Comment 23 Alexandr Miloslavskiy CLA 2019-05-09 03:33:36 EDT
As a user, I wouldn't be too surprised if I see this 
(.) Match OS
( ) Light
( ) Dark (disabled)

when system only supports light.
Anyways, specific label wording is a separate issue, not very much related to API itself.
Comment 24 Till Brychcy CLA 2019-05-09 03:43:38 EDT
(In reply to Alexandr Miloslavskiy from comment #23)
> As a user, I wouldn't be too surprised if I see this 
> (.) Match OS
> ( ) Light
> ( ) Dark (disabled)
> 
> when system only supports light.
> Anyways, specific label wording is a separate issue, not very much related
> to API itself.

On MacOS, we have a dark theme even if the system doesn't have one, so the "Dark" button would never be disabled. (using custom drawn buttons etc.)

The question here is just if an API to decide if the system has dark mode setting at all is needed. If the button is always shown, we don't really need it in Eclipse. But maybe other users will want it and it should be added anyway.
Comment 25 Alexandr Miloslavskiy CLA 2019-05-09 03:55:38 EDT
'Display.isSystemThemeSupported' sounds reasonable to have, whether or not Eclipse is going to use it. Also, on Windows (at least with current version), there's an internal need for this API, because trying to set dark theme when it is not available will make things worse.
Comment 26 Lakshmi P Shanmugam CLA 2019-05-09 06:54:18 EDT
(In reply to Alexandr Miloslavskiy from comment #16)
> > > static int Display.getSystemTheme();
> > I think this API should return only return Theme_Dark or Theme_Light based on
> > the current system theme. Returning match_os or legacy for system theme doesn't
> > seem correct. One use case of this API is to determine if the system theme is a
> > dark variant based on which the Eclipse application can set its theme (Bug 547038). 
> > If we have to a get the themeID used by SWT or value set by the client, we
> > could have a getter Display.getTheme() to match Display.setTheme().
> As we just discussed with Eric, on GTK it could be hard to label current
> theme "light" or "dark". Returning THEME_MATCH_OS sounds reasonable in this
> case.

One specific use case for this API is Bug 547038 - when eclipse is starting up, detect if the System is in dark theme and then set Eclipse to dark theme automatically by applying the required css styling.
I was planning to use Display.getSystemTheme() to detect this. But, if the API returns THEME_MATCH_OS then how to determine if the System theme is dark? Returning THEME_MATCH_OS will be ambiguous, returning Dark or Light is required. 
For GTK, may be we can check if the current theme is a dark variant and return Dark. For other cases it can return Light or may a different constant like THEME_OTHER.

I think in general an application (not just Eclipse) may be interested to know if the OS is in Dark theme and may want to adjust/set the application's colors/theme accordingly. So, an API is required to check if the system is in dark theme.
Comment 27 Eric Williams CLA 2019-05-09 09:33:28 EDT
(In reply to Till Brychcy from comment #18)
> The comments read like the "query if the platform has native dark mode
> support at all"-part is not handled yet. It's hard to make an API without a
> real use case, so I'll try to explain that part from comment 0
> 
> In Bug 544039 Comment 10 I wrote that it would be great if we could simplify
> Eclipse's over-complicated preference page "General > Appearance".
> 
> I really think it should look like this:
> 
> Theme:
>  [x] Match System settings 
>  [ ] Light
>  [ ] Dark
>  [ ] Other: "Classic" [Choose....]
> 
> where "Match System settings" is only available, if the system has a native
> dark mode switching support (and is default in this case) - for this the api
> "query if the platform has native dark mode support at all" is required.
> 
> If "Match System settings" is selected, then the "query what the current
> system setting is"-api is required for setting matching eclipse-theme (which
> as side effect requests the matching app-level design via swt using the "set
> the requested app appearance." but of course still configures eclipse-ui
> colors and syntax highlighting colors)

Doesn't "Match system settings" behave the same way as "Disable theming" does now?
Comment 28 Till Brychcy CLA 2019-05-09 09:46:25 EDT
(In reply to Eric Williams from comment #27)
> Doesn't "Match system settings" behave the same way as "Disable theming"
> does now?

No, the .css specifies some colors even in light mode, so at least it looks a bit different. 

But more important, e.g. syntax highlighting colors for dark mode are set via the .css theming engine. 

(I actually tried running eclipse unstyled in native dark mode, see Bug 540357 Comment 7)
Comment 29 Alexandr Miloslavskiy CLA 2019-05-09 11:03:45 EDT
> But, if the API returns THEME_MATCH_OS then how to determine if the
> System theme is dark?
This isn't really an API question.
Let's take some GTK theme, for example "Ambiance".
Is it light or dark? Even humans will have a hard time answering.
And if humans fail, you can't expect more from code :)

> For other cases it can return Light or may a different constant like
> THEME_OTHER.
Both are somewhat bad, I think. Returning Light for themes that are quite dark
would be wrong. Returning 'THEME_OTHER' will not allow to pass it to .setTheme().
Returning 'THEME_MATCH_OS' sounds like a reasonable compromise.

> Doesn't "Match system settings" behave the same way as "Disable theming" does now?
Not on Windows, because dark Windows still sets applications to light theme.
Comment 30 Till Brychcy CLA 2019-05-09 14:42:56 EDT
(In reply to Alexandr Miloslavskiy from comment #29)
> Let's take some GTK theme, for example "Ambiance".
> Is it light or dark? Even humans will have a hard time answering.
> And if humans fail, you can't expect more from code :)

If GTK has no concept of light and dark theme, the "query if the platform has native dark mode support at all" should return false.

But there is OS.setDarkThemePreferred(boolean) in /org.eclipse.swt/Eclipse SWT PI/gtk/org/eclipse/swt/internal/gtk/OS.java, so it looks like GTK does have such a concept!?
Comment 31 Lakshmi P Shanmugam CLA 2019-05-14 08:31:37 EDT
(In reply to Alexandr Miloslavskiy from comment #29)
> > But, if the API returns THEME_MATCH_OS then how to determine if the
> > System theme is dark?
> This isn't really an API question.
> Let's take some GTK theme, for example "Ambiance".
> Is it light or dark? Even humans will have a hard time answering.
> And if humans fail, you can't expect more from code :)
> 
> > For other cases it can return Light or may a different constant like
> > THEME_OTHER.
> Both are somewhat bad, I think. Returning Light for themes that are quite
> dark
> would be wrong. Returning 'THEME_OTHER' will not allow to pass it to
> .setTheme().
> Returning 'THEME_MATCH_OS' sounds like a reasonable compromise.

I'm not sure if I understand how the proposed API getSystemTheme() can be used to query if the OS's theme is dark or not.
getSystemTheme() is not the getter associated with setTheme(). Display.getSystemTheme() will query and return the OS theme, where-as display.setTheme(themeID) sets a SWT application's theme based on the themeID that it's called with. isThemeSupported(themeID) should return true/false based on whether the themeID will be honoured by SWT's setTheme(themeID).
Comment 32 Thomas Singer CLA 2019-05-20 09:29:59 EDT
(In reply to Alexandr Miloslavskiy from comment #29)
> > But, if the API returns THEME_MATCH_OS then how to determine if the
> > System theme is dark?
> This isn't really an API question.
> Let's take some GTK theme, for example "Ambiance".
> Is it light or dark? Even humans will have a hard time answering.
> And if humans fail, you can't expect more from code :)

In SmartGit we use following code

	private static boolean isDarkSystemTheme(Display display) {
		final Color backgroundColor = display.getSystemColor(SWT.COLOR_WIDGET_BACKGROUND);
		final Color foregroundColor = display.getSystemColor(SWT.COLOR_WIDGET_FOREGROUND);
		final int backgroundGray = toGrayValue(backgroundColor);
		final int foregroundGray = toGrayValue(foregroundColor);
		return backgroundGray < foregroundGray;
	}

	private static int toGrayValue(Color color) {
		return color.getRed() + color.getGreen() + color.getBlue();
	}

Maybe it is not perfect, e.g. for a red on blue theme, but for the majority of themes it should work.


@macOS experts: Can macOS 10.14 blend automatically from light to dark theme depending on the day time? Good applications are expected to react immediately and not just on restart, right?
Comment 33 Alexandr Miloslavskiy CLA 2019-05-20 09:34:40 EDT
(In reply to Lakshmi Shanmugam from comment #31)
> I'm not sure if I understand how the proposed API getSystemTheme() can be
> used to query if the OS's theme is dark or not.

If it returns THEME_DARK, then OS is dark.
If it returns THEME_LIGHT, then OS is light.
If it returns THEME_MATCH_OS, this is the weird case when we can't say if it's dark or not.

> getSystemTheme() is not the getter associated with setTheme().
> Display.getSystemTheme() will query and return the OS theme, where-as
> display.setTheme(themeID) sets a SWT application's theme based on the
> themeID that it's called with.

This is correct. Maybe it's better to rename it to 'getOsTheme' to avoid the feeling that it could be a getter for 'setTheme'.

> isThemeSupported(themeID) should return
> true/false based on whether the themeID will be honoured by SWT's
> setTheme(themeID).

In your opinion, what is the difference from my suggestion? My understanding is that "will be honoured by SWT" is incorrect definition, because it's macOS/Win32/GTK is what honors the setting, not SWT.
Comment 34 Lakshmi P Shanmugam CLA 2019-06-26 02:48:34 EDT
(In reply to Alexandr Miloslavskiy from comment #33)

Sorry for the delayed response, this bug somehow got lost during the 4.12 release work.

> (In reply to Lakshmi Shanmugam from comment #31)
> > I'm not sure if I understand how the proposed API getSystemTheme() can be
> > used to query if the OS's theme is dark or not.
> 
> If it returns THEME_DARK, then OS is dark.
> If it returns THEME_LIGHT, then OS is light.
> If it returns THEME_MATCH_OS, this is the weird case when we can't say if
> it's dark or not.

The concern here is that the return value THEME_MATCH_OS is ambiguous. Since, the API is returning the system theme it would always match OS. For example, if System theme is Light, then return values THEME_LIGHT and THEME_MATCH_OS are both valid. Having a return value such as THEME_OTHER can make it clear that the System theme is not Dark or Light, but something else. I think THEME_MATCH_OS is more suitable for setting the theme but not as a return value for getSystemTheme().

> 
> > getSystemTheme() is not the getter associated with setTheme().
> > Display.getSystemTheme() will query and return the OS theme, where-as
> > display.setTheme(themeID) sets a SWT application's theme based on the
> > themeID that it's called with.
> 
> This is correct. Maybe it's better to rename it to 'getOsTheme' to avoid the
> feeling that it could be a getter for 'setTheme'.
The Display class already uses System in a lot of it's APIs. On the same lines we could have Display.getSystemTheme() and change display.setTheme(themeID) to display.setApplicationTheme(themeID) for clarity.

> 
> > isThemeSupported(themeID) should return
> > true/false based on whether the themeID will be honoured by SWT's
> > setTheme(themeID).
> 
> In your opinion, what is the difference from my suggestion? My understanding
> is that "will be honoured by SWT" is incorrect definition, because it's
> macOS/Win32/GTK is what honors the setting, not SWT.

There is no difference, I added it here for clarity of the API.
isThemeSupported(themeID) should return true/false based on whether the themeID will be honored by OS. If the return value is true, then calling setApplicationTheme(themeID) should make the required OS calls based on the themeID to set the theme and the OS sets the theme.
Comment 35 Alexandr Miloslavskiy CLA 2019-07-01 11:48:00 EDT
Indeed, it's quite hard to make uniform theme API on different OSes. 
I think we should concentrate on "Dark" feature and simplify everything else.

-----------------------
New API suggestion
-----------------------
/**
 * Currently, this is anything that is not explicitly "Dark".
 */
int SWT.THEME_GENERIC = 0;

/**
 * Theme that is explicitly dark.
 */
int SWT.THEME_DARK    = 1;

/**
 * Returns SWT theme closest to theme used by OS UI. This is not the same
 * as default application theme: for example, on Dark Mode Windows most
 * applications are still light.
 *
 * Windows   - THEME_DARK if dark mode is activated in Windows settings,
 *             THEME_GENERIC otherwise.
 * Linux-GTK - THEME_DARK if GTK theme has '-dark' suffix in its name,
 *             THEME_GENERIC otherwise.
 *             I think this should read global GTK settings and ignore
 *             environment variable 'GTK_THEME'.
 * macOS     - THEME_DARK when dark mode is activated in macOS settings,
 *             THEME_GENERIC otherwise.
 *
 * @return one of SWT.THEME_XXX
 */
static int Display.getOsUiTheme();

/**
 * Returns whether a theme is supported by system.
 *
 * Windows   - THEME_DARK is supported (undocumented) since Win10 1809.
 * Linux-GTK - THEME_DARK is supported if used theme has '-dark' variant.
 * macOS     - THEME_DARK is supported since 10.14 Mojave.
 *
 * @param themeID one of SWT.THEME_XXX
 * @return true if theme is supported
 */
static boolean Display.isThemeSupported(int themeID);

/**
 * Instructs SWT to use specified system theme.
 * For simplicity, we could demand that it's called early and once.
 * Internally guarded with Display.isThemeSupported().
 *
 * @param themeID one of SWT.THEME_XXX
 * @return Display.isThemeSupported().
 */
static boolean Display.setTheme(int themeID);

-----------------------
Notes
-----------------------
"Light" theme is ignored in current API. Can be added later.
All GTK themes that are not explicitly '-dark' will be 'THEME_GENERIC'.
Comment 36 Niraj Modi CLA 2019-07-03 06:38:06 EDT
(In reply to Alexandr Miloslavskiy from comment #35)
> Indeed, it's quite hard to make uniform theme API on different OSes. 
> I think we should concentrate on "Dark" feature and simplify everything else.
We should also consider "Light" theme when designing the new APIs.

> static int Display.getOsUiTheme();
IMO, getSystemTheme() is more in synch with SWT's naming convention.

-------------------------------------------------------------------------
So, here's my take on the new APIs w.r.t. to Themes:
int SWT.THEME_LIGHT   = 1; // LIGHT
int SWT.THEME_DARK    = 2; // DARK
int SWT.THEME_SYSTEM = 0;  // When no explicit theme is specified from SWT application side
int SWT.THEME_OTHER = -1; // for any theme which is neither Light nor Dark.

Method APIs:
static boolean Display.isThemeSupported(int themeID); // possible arguments THEME_LIGHT or THEME_DARK

static int Display.getSystemTheme();// possible returns THEME_LIGHT, THEME_DARK or THEME_OTHER (if it's neither Light nor Dark)

static int Display.getApplicationTheme();// possible returns THEME_SYSTEM if not set by SWT application otherwise THEME_LIGHT or THEME_DARK

static boolean Display.setApplicationTheme(int themeID) throws ThemeNotSupportException; // possible arguments THEME_SYSTEM to get away with SWT application level theme settings, THEME_LIGHT or THEME_DARK and this method throws checked exception.
Comment 37 Nikita Nemkin CLA 2019-07-03 07:37:06 EDT
I like Niraj's proposal in general. Some comments

 * It's not obvious from the name that isThemeSupported belongs to application theme group.
 * I suggest to drop "application" bit and call the methods getTheme/setTheme/isThemeSuported.
 * setApplicationTheme shouldn't throw, since failure is likely and expected. A boolean return value is enough.
 * It's not clear how a custom component would query which theme is actually in effect. Is it something like this?

  int theme = Display.getApplicationTheme();
  if (theme == SWT.THEME_SYSTEM) theme = Display.getSystemTheme();
Comment 38 Niraj Modi CLA 2019-07-03 07:54:43 EDT
(In reply to Nikita Nemkin from comment #37)
> I like Niraj's proposal in general. Some comments
> 
>  * It's not obvious from the name that isThemeSupported belongs to
> application theme group.
Another case for using "application" to add clarity:
static boolean Display.isApplicationThemeSupported(int themeID);

>  * I suggest to drop "application" bit and call the methods
> getTheme/setTheme/isThemeSuported.
Using "application" suggests that theme effect is limited to an SWT application and getTheme/setTheme does give a false impression that you are actually setting system wide theme.

>  * setApplicationTheme shouldn't throw, since failure is likely and
> expected. A boolean return value is enough.
Am fine with or without "ThemeNotSupportException".

>  * It's not clear how a custom component would query which theme is actually
> in effect. Is it something like this?
>   int theme = Display.getApplicationTheme();
>   if (theme == SWT.THEME_SYSTEM) theme = Display.getSystemTheme();
Yes, that would be two step process.
Comment 39 Nikita Nemkin CLA 2019-07-03 08:29:46 EDT
(In reply to Niraj Modi from comment #38)
> Using "application" suggests that theme effect is limited to an SWT
> application and getTheme/setTheme does give a false impression that you are
> actually setting system wide theme.

I agree that it's a bit confusing, but the presence of getSystemTheme reduces confustion. Having overlong method names isn't great either.

> >  * It's not clear how a custom component would query which theme is actually
> > in effect. Is it something like this?
> >   int theme = Display.getApplicationTheme();
> >   if (theme == SWT.THEME_SYSTEM) theme = Display.getSystemTheme();
> Yes, that would be two step process.

I've just realized that this code would fail on Windows. Windows 10 has system-wide dark mode, but no dark theme for classic applicaitons (undocumented and broken private API doesn't count). In fact, it's useless to know that the system is dark if the app theme is always light.

Maybe rename getSystemTheme() to getDefaultTheme() to reflect that?
Comment 40 Alexandr Miloslavskiy CLA 2019-07-03 08:37:50 EDT
It is not useless at all(In reply to Nikita Nemkin from comment #39)
> In fact, it's useless to know that the system is dark if the
> app theme is always light.

It is not useless at all. Let me remind that Eclipse already has the dark theme and we're trying to make a good API for that. If Windows is set to dark, then it's good to know that, because then SWT applications, such as Eclipse or our SmartGit, can automatically become dark as well.

This is why I suggested .getOsUiTheme().

I can imagine these application scenarios:
1) Applications that don't care about themes
2) Applications that require user to select a theme. These will use explicit .setTheme(), they dont' really care about .getOsUiTheme().
3) Applications that try to fit with current OS settings. These will use .getOsUiTheme(), then decide which .setTheme() to use.
Comment 41 Nikita Nemkin CLA 2019-07-03 09:58:22 EDT
(In reply to Alexandr Miloslavskiy from comment #40)
> It is not useless at all(In reply to Nikita Nemkin from comment #39)
> > In fact, it's useless to know that the system is dark if the
> > app theme is always light.
> 
> It is not useless at all. Let me remind that Eclipse already has the dark
> theme and we're trying to make a good API for that. If Windows is set to
> dark, then it's good to know that, because then SWT applications, such as
> Eclipse or our SmartGit, can automatically become dark as well.
> 
> This is why I suggested .getOsUiTheme().
> 
> I can imagine these application scenarios:
> 1) Applications that don't care about themes
> 2) Applications that require user to select a theme. These will use explicit
> .setTheme(), they dont' really care about .getOsUiTheme().
> 3) Applications that try to fit with current OS settings. These will use
> .getOsUiTheme(), then decide which .setTheme() to use.

This bug is for apps that want to respect (or enable) the native dark theme, which is a real concern on macOS and Linux. Windows classic apps _don't have a native dark theme_ to respect or enable.

I agree that it's useful to know if UWP dark is enabled. Let's provide a separate, Windows-specific API for that. Something ugly like Display.getData("org.eclipse.swt.uwp.darkmode") to match the ugly state of native win32 theming.
Comment 42 Alexandr Miloslavskiy CLA 2019-07-03 10:23:57 EDT
I understand that you despise Windows, yet this is where most of our users are.

We have implemented dark theme using undocumented APIs and are quite happy with how the application looks.

I expect that soon enough those undocumented APIs will become documented and available to everyone, so I think it's best to design the API as if they were already present. Afterall, macOS also got dark theme support only very recently. If this discussion took place 2 years ago, you could have said the same about macOS.
Comment 43 Nikita Nemkin CLA 2019-07-03 11:03:49 EDT
(In reply to Alexandr Miloslavskiy from comment #42)
> I understand that you despise Windows, yet this is where most of our users
> are.

I quite like Windows. All I'm saying is let's not expose dark mode on Windows until setTheme(SWT.DARK) actually works. Because having getDefaultTheme() = SWT.DARK but setTheme(SWT.DARK) = false creates problems.

> We have implemented dark theme using undocumented APIs and are quite happy
> with how the application looks.

Does all of ControlExample work with undocumented dark mode? Completeness and correctness is the main issue, not the lack of documentation.

> I expect that soon enough those undocumented APIs will become documented and
> available to everyone, so I think it's best to design the API as if they
> were already present.

That's exactly what I propose. Design the API for systems with a dark theme. When Windows implements it, it will fit rigth in.
Comment 44 Alexandr Miloslavskiy CLA 2019-07-03 11:11:37 EDT
(In reply to Nikita Nemkin from comment #43)

> getDefaultTheme() = SWT.DARK but setTheme(SWT.DARK) = false creates problems.

This is not the case. getOsUiTheme() = SWT.DARK will mean that setTheme(SWT.DARK) will apply at least a limited support of dark theme.

Currently, on Windows, we use 3 undocumented APIs:
1) Make scrollbars dark. This can be used in SWT, see Bug 444560
2) Make menus dark. This should probably wait until APIs are public.
3) Make window title dark. This should probably wait until APIs are public.

Scrollbars are the biggest problem, and it can be solved *now*. This is what setTheme(SWT.DARK) will do.
Comment 45 Alexandr Miloslavskiy CLA 2019-07-03 11:35:14 EDT
(In reply to Nikita Nemkin from comment #43)

> Does all of ControlExample work with undocumented dark mode? Completeness
> and correctness is the main issue, not the lack of documentation.

When I tested it, everything looked fine. Also, I'm now using dark mode SmartGit for a few months and it looks great. You can try it yourself: download 19.1 preview ant run it on Win10 1809+
Comment 46 Eric Williams CLA 2019-07-03 12:01:20 EDT
(In reply to Niraj Modi from comment #36)
> (In reply to Alexandr Miloslavskiy from comment #35)
> > Indeed, it's quite hard to make uniform theme API on different OSes. 
> > I think we should concentrate on "Dark" feature and simplify everything else.
> We should also consider "Light" theme when designing the new APIs.
> 
> > static int Display.getOsUiTheme();
> IMO, getSystemTheme() is more in synch with SWT's naming convention.
> 
> -------------------------------------------------------------------------
> So, here's my take on the new APIs w.r.t. to Themes:
> int SWT.THEME_LIGHT   = 1; // LIGHT
> int SWT.THEME_DARK    = 2; // DARK
> int SWT.THEME_SYSTEM = 0;  // When no explicit theme is specified from SWT
> application side
> int SWT.THEME_OTHER = -1; // for any theme which is neither Light nor Dark.
> 
> Method APIs:
> static boolean Display.isThemeSupported(int themeID); // possible arguments
> THEME_LIGHT or THEME_DARK
> 
> static int Display.getSystemTheme();// possible returns THEME_LIGHT,
> THEME_DARK or THEME_OTHER (if it's neither Light nor Dark)
> 
> static int Display.getApplicationTheme();// possible returns THEME_SYSTEM if
> not set by SWT application otherwise THEME_LIGHT or THEME_DARK
> 
> static boolean Display.setApplicationTheme(int themeID) throws
> ThemeNotSupportException; // possible arguments THEME_SYSTEM to get away
> with SWT application level theme settings, THEME_LIGHT or THEME_DARK and
> this method throws checked exception.

In general the proposal is fine, but on GTK it might be hard to quantify what is "a light theme". IMO it's more accurate to say dark and non-dark. On GTK this basically means "does the specified theme have a *-dark variant".
Comment 47 Niraj Modi CLA 2019-07-04 07:09:39 EDT
(In reply to Alexandr Miloslavskiy from comment #44)
> (In reply to Nikita Nemkin from comment #43)
> 
> > getDefaultTheme() = SWT.DARK but setTheme(SWT.DARK) = false creates problems.
> 
> This is not the case. getOsUiTheme() = SWT.DARK will mean that
> setTheme(SWT.DARK) will apply at least a limited support of dark theme.

We will have to call out such limitations in JavaDoc of Theme constants.

(In reply to Eric Williams from comment #46)
> (In reply to Niraj Modi from comment #36)
> In general the proposal is fine, but on GTK it might be hard to quantify
> what is "a light theme". IMO it's more accurate to say dark and non-dark. On
> GTK this basically means "does the specified theme have a *-dark variant".

IMO, if unable to categorize any theme as LIGHT/DARK on GTK or any platform, then it shall fall into SWT.THEME_OTHER(which is neither Light nor Dark).
Comment 48 Nikita Nemkin CLA 2019-07-04 07:34:41 EDT
(In reply to Niraj Modi from comment #47)
> (In reply to Alexandr Miloslavskiy from comment #44)
> > (In reply to Nikita Nemkin from comment #43)
> > 
> > > getDefaultTheme() = SWT.DARK but setTheme(SWT.DARK) = false creates problems.
> > 
> > This is not the case. getOsUiTheme() = SWT.DARK will mean that
> > setTheme(SWT.DARK) will apply at least a limited support of dark theme.
> 
> We will have to call out such limitations in JavaDoc of Theme constants.

Why implement badly broken setTheme(SWT.DARK) on Windows? Makes no sense to me.

If dark scrollbar hack is so essential, put in behind a system property or Display.setData.

> (In reply to Eric Williams from comment #46)
> > (In reply to Niraj Modi from comment #36)
> > In general the proposal is fine, but on GTK it might be hard to quantify
> > what is "a light theme". IMO it's more accurate to say dark and non-dark. On
> > GTK this basically means "does the specified theme have a *-dark variant".
> 
> IMO, if unable to categorize any theme as LIGHT/DARK on GTK or any platform,
> then it shall fall into SWT.THEME_OTHER(which is neither Light nor Dark).

As someone mentioned earlier in this thread, simple color comparison heuristic allows to classify almost any theme as light or dark.
Comment 49 Alexandr Miloslavskiy CLA 2019-07-04 09:02:31 EDT
(In reply to Nikita Nemkin from comment #48)
> If dark scrollbar hack is so essential, put in behind a system property or
> Display.setData.

As far as I understand it, the whole point of this discussion is to have some uniform API instead of different system properties for each platform. Theming is wildly different on different platforms, so we're trying to extract at least some common stuff into public API.
Comment 50 Lakshmi P Shanmugam CLA 2019-07-05 06:53:08 EDT
Thanks Alexandr, Eric, Nikita, Niraj for your inputs.

Looks like we more or less have a consensus on how the APIs should look. 
Regarding the implementation, I think we can do it in 2 parts. 

In the first part we can implement the getSystemTheme() API on all platforms and the required constants. If Windows doesn't have any API for this yet, we will just return THEME_LIGHT for now and mark the API as a HINT in the javadoc. This will unblock the Eclipse bug 547038.

The APIs to set the theme can be implemented in part 2. I agree that we shouldn't implement setTheme() and the related APIs on Windows using undocumented/non-public APIs. They may change/break anytime and the SWT APIs using it will break too. Let's wait for the required Windows APIs to be available. But since we know how the API will look, we can work on the Mac and GTK implementation in the meantime.
Comment 51 Lakshmi P Shanmugam CLA 2019-07-05 08:23:01 EDT
Combined comment#35 and comment#36 and updated the javadoc:

/**
 * Theme constant for light variant
 */
int SWT.THEME_LIGHT = 0;

/**
 * Theme constant for dark variant
 */
int SWT.THEME_DARK = 1;

/**
 * Theme constant when the theme is neither light or dark.
 */
int SWT.THEME_OTHER = 2;

/**
 * Returns the theme constant that matches the appearance of the theme used by the OS.
 *
 * Windows   - THEME_DARK if dark mode is activated in Windows settings,
 *             THEME_LIGHT otherwise.
 * Linux-GTK - THEME_DARK if GTK theme has '-dark' suffix in its name,
 *             THEME_OTHER otherwise.
 *             I think this should read global GTK settings and ignore
 *             environment variable 'GTK_THEME'.
 * macOS     - THEME_DARK when dark appearance is selected in OS settings,
 *             THEME_LIGHT otherwise.
 *
 * HINT: This API is not yet implemented on Windows and returns THEME_LIGHT
 *
 * @return one of SWT.THEME_XXX
 */
static int Display.getSystemTheme();

-------------------------------------------------------------------------------
Notes:

1. We could use SWT.THEME_CUSTOM or some other name instead of SWT.THEME_OTHER. 

2. One open question is what should getSystemTheme() return on GTK when the theme is non-dark variant: THEME_LIGHT or THEME_OTHER/THEME_CUSTOM?

@Alexandr, would you be interested to implement the GTK and Windows part of the API?
Comment 52 Thomas Singer CLA 2019-07-05 13:58:23 EDT
(In reply to Lakshmi Shanmugam from comment #50)
> The APIs to set the theme can be implemented in part 2. I agree that we
> shouldn't implement setTheme() and the related APIs on Windows using
> undocumented/non-public APIs. They may change/break anytime and the SWT APIs
> using it will break too. Let's wait for the required Windows APIs to be
> available. But since we know how the API will look, we can work on the Mac
> and GTK implementation in the meantime.

So the Eclipse team does not want to have the dark theme on Windows 10 have dark scrollbars, too? If it is too dangerous for you, why not mark the method as experimental?
Comment 53 Alexandr Miloslavskiy CLA 2019-07-08 05:32:54 EDT
(In reply to Lakshmi Shanmugam from comment #51)

> 1. We could use SWT.THEME_CUSTOM or some other name instead of
> SWT.THEME_OTHER. 

I think that 'SWT.THEME_OTHER' better describes the variety of GTK themes. 'THEME_CUSTOM' sounds like it's possible to customize things.

> 2. One open question is what should getSystemTheme() return on GTK when the
> theme is non-dark variant: THEME_LIGHT or THEME_OTHER/THEME_CUSTOM?

I think that THEME_LIGHT would be wrong. GTK has lots of themes.

> @Alexandr, would you be interested to implement the GTK and Windows part of
> the API?

Our interest was to reduce the number of private patches, such as Bug 444560. Now that this discussion is more or less against our goals, I'm much less interested.
Comment 54 Eric Williams CLA 2019-07-08 09:21:11 EDT
On SWT-GTK we were using non-public API for years without issue. The only caveat to doing so is that someone needs to be on top of making sure things don't break (or get fixed when they do) whenever a new version of the native toolkit is released.

Thomas/Alexandr: if you're willing to make sure the API works when new versions of Win32 are released then I don't have any objection to implementing the full SWT API now.
Comment 55 Alexander Kurtakov CLA 2019-07-08 09:34:03 EDT
If we have the API working on Mac and Gtk we should go with it even if Win32 implementation is not there yet. This has happened multiple times and there are probably still cases where we lack implementation of an API on some platform. E.g. bug 109276 .
With that said - it's really bad practice but if Windows doesn't provide us with the api now using some internals doesn't sound so bad as it will allow the functionality for all our users.
Comment 56 Alexander Kurtakov CLA 2019-07-08 09:40:00 EDT
Can someone please point to these undocumented calls for windows so we can decide how risky they can be? I failed to get clean picture from the comments in this and related bugs.
Comment 57 Alexandr Miloslavskiy CLA 2019-07-08 09:46:13 EDT
Yes, if we choose to implement the Windows part with non-documented APIs, I will keep an eye on it for at least a couple years. By then, I expect that public API will become available already.

I'm currently only talking about dark scrollbars. This is both the most important and least risky change. Most important because light scrollbars in dark UI is the most ugly part. Least risky is because it uses documented API, just with a "secret" theme string name. I also use safeguards to verify that this theme string is recognized, so I don't expect any problems worth worrying about.

The core of the change can be seen in [1], where 'SetWindowTheme' is a well-known documented API and 'DARKMODE_EXPLORER' is the secret theme name.

We also utilize private patches to get dark Window Title and dark Menus, but these patches are more risky, so I don't currently suggest to merge them.

[1] https://git.eclipse.org/r/c/136091/2/bundles/org.eclipse.swt/Eclipse+SWT/win32/org/eclipse/swt/widgets/Control.java
Comment 58 Eric Williams CLA 2019-07-08 14:39:22 EDT
(In reply to Alexandr Miloslavskiy from comment #57)
> Yes, if we choose to implement the Windows part with non-documented APIs, I
> will keep an eye on it for at least a couple years. By then, I expect that
> public API will become available already.

This satisfies my concerns. We were doing this in SWT-GTK with GTK CSS for background/foreground colors for a few years, before the GTK CSS API was made public. Generally we didn't have an issue.

 
> I'm currently only talking about dark scrollbars. This is both the most
> important and least risky change. Most important because light scrollbars in
> dark UI is the most ugly part. Least risky is because it uses documented
> API, just with a "secret" theme string name. I also use safeguards to verify
> that this theme string is recognized, so I don't expect any problems worth
> worrying about.
> 
> The core of the change can be seen in [1], where 'SetWindowTheme' is a
> well-known documented API and 'DARKMODE_EXPLORER' is the secret theme name.
> 
> We also utilize private patches to get dark Window Title and dark Menus, but
> these patches are more risky, so I don't currently suggest to merge them.
> 
> [1]
> https://git.eclipse.org/r/c/136091/2/bundles/org.eclipse.swt/Eclipse+SWT/
> win32/org/eclipse/swt/widgets/Control.java

Please prepare a Gerrit with the full setSystemTheme() and getSystemTheme() API, and I'll be happy to review it, and possibly even help with the GTK side of it. We can always mark it as a hint if behaviour is not guaranteed on all platforms.
Comment 59 Lakshmi P Shanmugam CLA 2019-07-09 04:40:33 EDT
(In reply to Alexandr Miloslavskiy from comment #53)
> > @Alexandr, would you be interested to implement the GTK and Windows part of
> > the API?
> 
> Our interest was to reduce the number of private patches, such as Bug
> 444560. Now that this discussion is more or less against our goals, I'm much
> less interested.

The goal of the bug is to provide SWT APIs for theming on all platforms leveraging the OS support and it was not just about having dark scrollbars on Windows. AFAICS the discussion so far was in the same direction of implementing the native theme APIs.

AFAIK, in case of Windows there are no APIs available yet to set dark theme in win32 application.
The undocumented approach used in [1] only sets the scrollbars (and Tree/Table?) to dark. So, putting the implementation to just set dark scrollbars in the setTheme() API on Windows seems wrong to me and will result in an incomplete API. This is in addition to the problem I already mentioned related to implementing and maintaining SWT APIs based on non-APIs or undocumented values.
So, -1 from me for having incomplete and experimental setTheme() implementation on Windows based on this.

We could provide the Mac and GTK implementation for setTheme() now and provide the Windows implementation when we have the APIs or a more complete implementation.

Dani, what do you think?

I understand that having dark scrollbars is the real problem here since there is no way to set the color for the scroll bar. IMO, providing a way to set the dark scrollbars may be a better option. SWT already provides something like OS.setDarkModePreferred for setting the dark theme and it's used by Eclipse. Will having OS.setDarkScrollBar() or OS.setDarkMode() work for you? This implementation can be reused by the setTheme() API later as API becomes available.


[1] https://git.eclipse.org/r/c/136091/2/bundles/org.eclipse.swt/Eclipse+SWT/win32/org/eclipse/swt/widgets/Control.java
Comment 60 Thomas Singer CLA 2019-07-09 05:30:15 EDT
(In reply to Lakshmi Shanmugam from comment #59)
> I understand that having dark scrollbars is the real problem here since
> there is no way to set the color for the scroll bar. IMO, providing a way to
> set the dark scrollbars may be a better option. SWT already provides
> something like OS.setDarkModePreferred for setting the dark theme and it's
> used by Eclipse. Will having OS.setDarkScrollBar() or OS.setDarkMode() work
> for you? This implementation can be reused by the setTheme() API later as
> API becomes available.

I would prefer to have a platform independent API, so I have not to use reflection.
Comment 61 Alexandr Miloslavskiy CLA 2019-07-09 06:14:33 EDT
After internal discussion, I have decided to step away from this bug.

For now, I will concentrate my effort on re-implementing patch for Bug 444560 to have something like Display.setDarkScrollBars() and a system property that automatically activates it (to avoid the need for reflection)
Comment 62 Eric Williams CLA 2019-07-09 09:22:25 EDT
I don't understand why we can't implement the API and have it marked as a hint, especially when we have company backed committers who have agreed to watch over and maintain it as needed on the affected platform (Windows in this case not having full dark theme support). The whole idea of marking API as a hint is that it allows us to introduce cutting edge API that may not exist on all three platforms. We already have far more significant (than theme related) API in SWT  marked as a hint.

IMO the benefit of having this API outweighs any potential inconsistencies, especially since it would have been marked as a hint anyway. With Apple introducing a desktop dark mode, it makes sense that Windows would not be far behind, and their introduction of internal dark theme support signals that. It's disappointing that we're missing out on an opportunity to make SWT ahead of such a trend, and are instead waiting to react.
Comment 63 Nikita Nemkin CLA 2019-07-09 10:00:49 EDT
(In reply to Eric Williams from comment #62)
> I don't understand why we can't implement the API and have it marked as a
> hint

Because making only a few widgets dark in an otherwise light app is not a good implementation of this "hint".

> With Apple introducing a desktop dark mode, it makes sense that Windows would
> not be far behind, and their introduction of internal dark theme support
> signals that.

You're forgetting that MS is focused on UWP, not the legacy widgets that SWT uses. Legacy widgets got just enough dark theme support (via a private API) to make Explorer and Common Dialogs work. Personally, I see no indication that this support will ever be opened and completed.
Comment 64 Thomas Singer CLA 2019-07-10 02:52:13 EDT
(In reply to Nikita Nemkin from comment #63)
> (In reply to Eric Williams from comment #62)
> > I don't understand why we can't implement the API and have it marked as a
> > hint
> 
> Because making only a few widgets dark in an otherwise light app is not a
> good implementation of this "hint".

Would it be OK for (all of) you to make scrollbars on Windows dark using a system property until Microsoft decides to provide a full dark theme for all controls?

In general I have the feeling that setting individual colors to all kind of controls to provide a dark theme for an otherwise light system (or visa versa) is only an ugly workaround. Often the controls require more than a background/foreground color pair which the API does not support. Especially on macOS text fields and comboboxes cause large problems.

Hence I would prefer to concentrate on using the system theme and only apply tiny changes, and discourage changing the complete look of the application (because it does not look good). Of course, the alternative would be to allow the tweaking of every color used for a certain control (which is platform specific), e.g. the selection colors in a text field (focused/unfocused) or the colors of the popup of a combobox.
Comment 65 Nikita Nemkin CLA 2019-07-10 03:53:37 EDT
(In reply to Thomas Singer from comment #64)
> Would it be OK for (all of) you to make scrollbars on Windows dark using a
> system property until Microsoft decides to provide a full dark theme for all
> controls?

There's also Display.setProperty, which provides reflection-free access to private/platform-specific properties. I'm fine with either.

> In general I have the feeling that setting individual colors to all kind of
> controls to provide a dark theme for an otherwise light system (or visa
> versa) is only an ugly workaround. Often the controls require more than a
> background/foreground color pair which the API does not support. Especially
> on macOS text fields and comboboxes cause large problems.

> Hence I would prefer to concentrate on using the system theme and only apply
> tiny changes, and discourage changing the complete look of the application
> (because it does not look good).

Hell yes. Faking dark themes is a disaster and some questionable code (and API) has accumulated in SWT, trying to recolor unrecolorable.

> Of course, the alternative would be to
> allow the tweaking of every color used for a certain control (which is
> platform specific), e.g. the selection colors in a text field
> (focused/unfocused) or the colors of the popup of a combobox.

This approach is a dead end. Win32 and GTK widget painting can't be reduced to a set of colors. On Windows, themed controls use 9-patch bitmaps for backgrounds. GTK has arbitrary CSS.
Comment 66 Lakshmi P Shanmugam CLA 2019-07-10 05:40:20 EDT
(In reply to Thomas Singer from comment #64)
> (In reply to Nikita Nemkin from comment #63)
> > (In reply to Eric Williams from comment #62)
> > > I don't understand why we can't implement the API and have it marked as a
> > > hint
> > 
> > Because making only a few widgets dark in an otherwise light app is not a
> > good implementation of this "hint".
> 
> Would it be OK for (all of) you to make scrollbars on Windows dark using a
> system property until Microsoft decides to provide a full dark theme for all
> controls?
> 

@Thomas, here is an idea.
SWT has the useSystemTheme property [1] that was introduced in 4.12. When it's set to true, SWT application on Mac is set to start with the theme matching the system theme. With this set, SWT app starts in dark theme if OS appearance is set to Dark.

If you want to have dark scrollbars only when Windows Dark mode is enabled, may be you can use the same property.
When useSystemTheme=true on Windows, SWT can do this:
1. Check if Windows is in Light mode or Dark mode.
2. If in Dark mode, enable the Dark scrollbars and widgets.
3. If in Light mode, do nothing

What do you think?

You could also create a separate system property to enable the dark scrollbars, but the problem is that the implementation using SetWindowTheme(handle, Display.DARKMODE_EXPLORER) currently doesn't limit itself to theming scrollbars alone, a few other widgets are affected too. Also, we cannot say for sure if/when the scope of this "secret" string would expand further to more widgets. So, having it under a generic property may be better.

[1] https://www.eclipse.org/eclipse/news/4.12/platform_isv.php
Comment 67 Eric Williams CLA 2019-07-10 09:05:16 EDT
(In reply to Nikita Nemkin from comment #65)
> (In reply to Thomas Singer from comment #64)
> > Would it be OK for (all of) you to make scrollbars on Windows dark using a
> > system property until Microsoft decides to provide a full dark theme for all
> > controls?
> 
> There's also Display.setProperty, which provides reflection-free access to
> private/platform-specific properties. I'm fine with either.

+1, in SWT-GTK we have built upon this and allow per-widget skinning by using Widget.setData() with valid GTK CSS.

 
> > In general I have the feeling that setting individual colors to all kind of
> > controls to provide a dark theme for an otherwise light system (or visa
> > versa) is only an ugly workaround. Often the controls require more than a
> > background/foreground color pair which the API does not support. Especially
> > on macOS text fields and comboboxes cause large problems.
> 
> > Hence I would prefer to concentrate on using the system theme and only apply
> > tiny changes, and discourage changing the complete look of the application
> > (because it does not look good).
> 
> Hell yes. Faking dark themes is a disaster and some questionable code (and
> API) has accumulated in SWT, trying to recolor unrecolorable.

What do you mean by faking a dark theme? And can you elaborate on the questionable code and API point?
Comment 68 Dani Megert CLA 2019-07-10 09:45:26 EDT
(In reply to Lakshmi Shanmugam from comment #59)
> (In reply to Alexandr Miloslavskiy from comment #53)
> > > @Alexandr, would you be interested to implement the GTK and Windows part of
> > > the API?
> > 
> > Our interest was to reduce the number of private patches, such as Bug
> > 444560. Now that this discussion is more or less against our goals, I'm much
> > less interested.
> 
> The goal of the bug is to provide SWT APIs for theming on all platforms
> leveraging the OS support and it was not just about having dark scrollbars
> on Windows. AFAICS the discussion so far was in the same direction of
> implementing the native theme APIs.
> 
> AFAIK, in case of Windows there are no APIs available yet to set dark theme
> in win32 application.
> The undocumented approach used in [1] only sets the scrollbars (and
> Tree/Table?) to dark. So, putting the implementation to just set dark
> scrollbars in the setTheme() API on Windows seems wrong to me and will
> result in an incomplete API. This is in addition to the problem I already
> mentioned related to implementing and maintaining SWT APIs based on non-APIs
> or undocumented values.
> So, -1 from me for having incomplete and experimental setTheme()
> implementation on Windows based on this.
> 
> We could provide the Mac and GTK implementation for setTheme() now and
> provide the Windows implementation when we have the APIs or a more complete
> implementation.
> 
> Dani, what do you think?
That's fine with me but we should also put a focus on bug 444560.
Comment 69 Thomas Singer CLA 2019-07-10 12:50:34 EDT
(In reply to Lakshmi Shanmugam from comment #66)
> @Thomas, here is an idea.
> SWT has the useSystemTheme property [1] that was introduced in 4.12. When
> it's set to true, SWT application on Mac is set to start with the theme
> matching the system theme. With this set, SWT app starts in dark theme if OS
> appearance is set to Dark.
> 
> If you want to have dark scrollbars only when Windows Dark mode is enabled,
> may be you can use the same property.

This does not apply well for Windows, because for Windows we currently need to set a complete dark theme, too, AND the dark scrollbars. So IMHO the dark scrollbars are some kind of display option like setting some background/foreground colors for every Composite.

> When useSystemTheme=true on Windows, SWT can do this:
> 1. Check if Windows is in Light mode or Dark mode.
> 2. If in Dark mode, enable the Dark scrollbars and widgets.
> 3. If in Light mode, do nothing
> 
> What do you think?

Because Windows does not (yet?) support a real dark theme for normal applications, this makes not much sense for it. IMHO the dark scrollbar bug is independent of this bug. This bug makes much more sense for macOS 10.14+ and Linux which have light/dark themes.
Comment 70 Lakshmi P Shanmugam CLA 2019-07-22 02:13:59 EDT
Opened Bug 549451 to implement a API to detect the current system setting.

Since this bug has useful discussion on the complete API for the Theme support, leaving it open. We will revisit when we have more clarity on the Windows API. 
If anyone is interested please feel free to take it up.
Comment 71 Lars Vogel CLA 2019-08-13 07:45:49 EDT
(In reply to Lakshmi Shanmugam from comment #70)
> Opened Bug 549451 to implement a API to detect the current system setting.
> 
> Since this bug has useful discussion on the complete API for the Theme
> support, leaving it open. We will revisit when we have more clarity on the
> Windows API. 
> If anyone is interested please feel free to take it up.

Platform UI would like to have one unique point to call. Currently we have:

GKT: OS.setDarkThemePreferred(isDark));
Mac: OS.setTheme(isDark));

Can you provide one method? For Windows you can decided later how to communicate with the OS, we "only" need the Java part to call. Typically much methods are present and documented that they do not work on certain OS in their Javadoc.
Comment 72 Lars Vogel CLA 2019-08-13 07:47:02 EDT
Please consider for M3.
Comment 73 Eric Williams CLA 2019-08-13 08:35:22 EDT
(In reply to Lars Vogel from comment #71)
> (In reply to Lakshmi Shanmugam from comment #70)
> > Opened Bug 549451 to implement a API to detect the current system setting.
> > 
> > Since this bug has useful discussion on the complete API for the Theme
> > support, leaving it open. We will revisit when we have more clarity on the
> > Windows API. 
> > If anyone is interested please feel free to take it up.
> 
> Platform UI would like to have one unique point to call. Currently we have:
> 
> GKT: OS.setDarkThemePreferred(isDark));
> Mac: OS.setTheme(isDark));
> 
> Can you provide one method? For Windows you can decided later how to
> communicate with the OS, we "only" need the Java part to call. Typically
> much methods are present and documented that they do not work on certain OS
> in their Javadoc.

Do you mean a unified SWT API to set the dark theme?
Comment 74 Lars Vogel CLA 2019-08-13 08:40:58 EDT
(In reply to Eric Williams from comment #73)
> Do you mean a unified SWT API to set the dark theme?

Yes
Comment 75 Eric Williams CLA 2019-08-13 08:50:17 EDT
(In reply to Lars Vogel from comment #74)
> (In reply to Eric Williams from comment #73)
> > Do you mean a unified SWT API to set the dark theme?
> 
> Yes

Lakshmi, what do you think?
Comment 76 Lakshmi P Shanmugam CLA 2019-08-14 08:14:23 EDT
The discussion in the bug was around having a generic API to get/set the application light/dark theme. But, in the end there were no volunteers to implement it.

The API isSystemDarkTheme() to check if OS theme is dark has already been implemented for 4.13. So, we could have an API to set dark theme API too.

I'll not be able to work on the Mac part at least for 4.13. 
I'm not sure if the Mac OS.setTheme() can be directly used in the API. One has to check if all the controls have the correct colors and also if there are any refresh issues.
This is not a problem with Eclipse as this API is used along with the css files in Eclipse dark theme. Refresh is also taken care of by Eclipse.
Comment 77 Lars Vogel CLA 2019-08-14 09:37:36 EDT
(In reply to Lakshmi Shanmugam from comment #76)
> The discussion in the bug was around having a generic API to get/set the
> application light/dark theme. But, in the end there were no volunteers to
> implement it.

AFAIK we already have OS specific methods.

GKT: OS.setDarkThemePreferred(isDark));
Mac: OS.setTheme(isDark));

Why not create a Java API which calls these methods based on the running platform?
Comment 78 Eric Williams CLA 2019-08-14 19:05:25 EDT
(In reply to Lars Vogel from comment #77)
> (In reply to Lakshmi Shanmugam from comment #76)
> > The discussion in the bug was around having a generic API to get/set the
> > application light/dark theme. But, in the end there were no volunteers to
> > implement it.
> 
> AFAIK we already have OS specific methods.
> 
> GKT: OS.setDarkThemePreferred(isDark));
> Mac: OS.setTheme(isDark));
> 
> Why not create a Java API which calls these methods based on the running
> platform?

I'm in favour of exposing such an API on GTK. We can probably do it in time for 4.13.
Comment 79 Lakshmi P Shanmugam CLA 2019-08-16 02:48:52 EDT
(In reply to Lars Vogel from comment #77)
> Why not create a Java API which calls these methods based on the running
> platform?

As a mentioned in my comment we can do this.

> The API isSystemDarkTheme() to check if OS theme is dark has already been
> implemented for 4.13. So, we could have an API to set dark theme API too.

But,

> I'm not sure if the Mac OS.setTheme() can be directly used in the API. One
> has to check if all the controls have the correct colors and also if there
> are any refresh issues.
> This is not a problem with Eclipse as this API is used along with the css
> files in Eclipse dark theme. Refresh is also taken care of by Eclipse.

For example, when I quickly tried this on ControlExample, it had the foreground color of the TabFolder wrong. Selecting it gave it the correct color.

> I'll not be able to work on the Mac part at least for 4.13.
Comment 80 Lars Vogel CLA 2019-08-16 02:59:06 EDT
Lakshmi, I don't think having one API for all platform will create new issues. It will only simplify platform UI handing, as we can combine the call instead of having platform specific bundles. 

Any issues with the dark theme should be already present, no matter if we call OS.setTheme(isDark)) or the new API.
Comment 81 Lakshmi P Shanmugam CLA 2019-08-16 03:51:07 EDT
(In reply to Lars Vogel from comment #80)
> Lakshmi, I don't think having one API for all platform will create new
> issues. It will only simplify platform UI handing, as we can combine the
> call instead of having platform specific bundles.

Yes, I understand that it'll simplify platform UI code.

> Any issues with the dark theme should be already present, no matter if we
> call OS.setTheme(isDark)) or the new API.

OS.setTheme() is an internal method an only SWT and Platform UI code can use it. 
But, if we are making it an API, then it can be used anywhere. So, we should take into account its issues and limitations.

One problem I see is that SWT initialises the colors for the widgets based on the light theme. The color initialization happens when Display is created. When client code calls the OS.setTheme(true), the app theme changes to dark, but the colors are not refreshed or re-initialized in SWT. This causes the wrong colors. It can be seen in the ControlExample Colors tab.

This problem is not seen in Eclipse dark theme because I think css theming sets the correct colors.
It's also not a problem when starting SWT application with the useSystemTheme=true property because this property is checked before the colors are initialised.

I'm not sure if this is a problem on GTK too.
Comment 82 Lars Vogel CLA 2019-08-16 03:55:02 EDT
(In reply to Lakshmi Shanmugam from comment #81)
 
> OS.setTheme() is an internal method an only SWT and Platform UI code can use
> it. 
> But, if we are making it an API, then it can be used anywhere. So, we should
> take into account its issues and limitations.

Ah, I see. Thanks for the explanation. 

What about a new provisional API shared across all platforms which Platform UI can use and remove the x-internal (or annotation/Javadoc) once the known problems have been solved?
Comment 83 Lakshmi P Shanmugam CLA 2019-08-16 05:35:26 EDT
(In reply to Lars Vogel from comment #82)
> (In reply to Lakshmi Shanmugam from comment #81)
>  
> > OS.setTheme() is an internal method an only SWT and Platform UI code can use
> > it. 
> > But, if we are making it an API, then it can be used anywhere. So, we should
> > take into account its issues and limitations.
> 
> Ah, I see. Thanks for the explanation. 
> 
> What about a new provisional API shared across all platforms which Platform
> UI can use and remove the x-internal (or annotation/Javadoc) once the known
> problems have been solved?

Sorry, I didn't follow what you mean by provisional API. Do you mean internal API? OS.setTheme() is already an internal API. 
Can you please give an example?
Comment 84 Lars Vogel CLA 2019-08-16 10:01:20 EDT
See https://wiki.eclipse.org/Provisional_API_Guidelines
Comment 85 Eric Williams CLA 2019-08-16 10:10:38 EDT
We could re-initialize the system colors when the API is called, I don't think that's a big deal. We'd have to make it clear though that widgets won't be refreshed on their own -- that's up to the client to do. Clients using a theming engine (like in platform UI) or purely custom painted widgets won't have to really worry about this anyway.
Comment 86 Lakshmi P Shanmugam CLA 2019-08-23 06:16:46 EDT
Moving out of 4.13.
Comment 87 Lakshmi P Shanmugam CLA 2019-08-23 06:27:55 EDT
(In reply to Eric Williams from comment #85)
> We could re-initialize the system colors when the API is called, I don't
> think that's a big deal.

Yes, the system colors should be reinitialized.

> We'd have to make it clear though that widgets
> won't be refreshed on their own -- that's up to the client to do.Clients
> using a theming engine (like in platform UI) or purely custom painted
> widgets won't have to really worry about this anyway.

I'm not sure if we should leave the refresh to the client. It's not a problem for Eclipse because it already has the code to do this. But, other applications will have to implement refresh.
Comment 88 Eric Williams CLA 2019-09-05 14:13:57 EDT
(In reply to Lakshmi Shanmugam from comment #87)
> I'm not sure if we should leave the refresh to the client. It's not a
> problem for Eclipse because it already has the code to do this. But, other
> applications will have to implement refresh.

We could add a note in the API that doing a "theme change" will require all Widgets to be redrawn.
Comment 89 Lakshmi P Shanmugam CLA 2019-11-21 07:10:54 EST
Resetting target.
Comment 90 Lars Vogel CLA 2020-03-30 11:21:46 EDT
We now have 

GKT: OS.setDarkThemePreferred(isDark));
Mac: OS.setTheme(isDark));
Win32: display.setData("org.eclipse.swt.internal.win32.enableDarkScrollbars", true);

which forces Platform UI to have three plug-ins to call this API.

Can the SWT team please provide one unique API?
Comment 91 Lars Vogel CLA 2020-03-30 11:30:32 EDT
*** Bug 550003 has been marked as a duplicate of this bug. ***
Comment 92 Niraj Modi CLA 2020-03-31 08:59:57 EDT
(In reply to Lars Vogel from comment #90)
> We now have 
> 
> GKT: OS.setDarkThemePreferred(isDark));
> Mac: OS.setTheme(isDark));
> Win32:
> display.setData("org.eclipse.swt.internal.win32.enableDarkScrollbars", true);
> 
> which forces Platform UI to have three plug-ins to call this API.
> 
> Can the SWT team please provide one unique API?

With latest update on SWT Windows(https://bugs.eclipse.org/bugs/show_bug.cgi?id=444560#c61)
Enabling of this feature is controlled via below System Property(when done from the Eclipse VM args options):
-Dorg.eclipse.swt.internal.win32.enableDarkScrollbars=true

Using Code:
System.setProperty("org.eclipse.swt.internal.win32.enableDarkScrollbars", Boolean.TRUE.toString());
Comment 93 Niraj Modi CLA 2020-04-03 06:11:33 EDT
(In reply to Lars Vogel from comment #90)
> Can the SWT team please provide one unique API?

Having a unique API have below challenges:
1. Since the Dark theme support varies platform to platform, say for example:
Dark theme support on GTK and MAC is dynamic in nature, where as on Windows the feature has to be enabled during the widget creation/initialization only.

2. GTK and MAC API actually sets the theme to Dark, whereas on Windows it just turn the Scrollbar to Dark-themed.
------------------------------------------------------------------------------
Currently for platform UI, I am planning to add below API on Windows which best justifies it's purpose:
Win: OS.enableDarkThemeFeatures (isDark)

I share a gerrit for discussion.
Comment 94 Alexandr Miloslavskiy CLA 2020-04-03 06:14:29 EDT
(In reply to Niraj Modi from comment #93)
> where as on Windows
> the feature has to be enabled during the widget creation/initialization only.

This is not correct and my original patch for Bug 444560 allowed to change theme on the fly.
Comment 95 Eclipse Genie CLA 2020-04-03 06:20:46 EDT
New Gerrit change created: https://git.eclipse.org/r/160426
Comment 96 Niraj Modi CLA 2020-04-03 07:59:50 EDT
(In reply to Alexandr Miloslavskiy from comment #94)
> (In reply to Niraj Modi from comment #93)
> > where as on Windows
> > the feature has to be enabled during the widget creation/initialization only.
> 
> This is not correct and my original patch for Bug 444560 allowed to change
> theme on the fly.

The original patch was actually adding new API(s) to SWT(as we depend on an undocumented native API for this feature and adding a public API on to of it is not a good idea): https://git.eclipse.org/r/#/c/136091/1/bundles/org.eclipse.swt/Eclipse+SWT/win32/org/eclipse/swt/widgets/Control.java

Hi Alexandr/Conrad,
Quick question to both of you:
With the current patch design, can we have the dynamic functionality in place ?
Comment 98 Alexandr Miloslavskiy CLA 2020-04-03 08:10:41 EDT
> The original patch was actually adding new API(s) to SWT(as we depend on an
> undocumented native API for this feature and adding a public API on to of it
> is not a good idea):

I was just beginning my SWT experience when I made it. Later the patch became a victim of endless discussions.

> With the current patch design, can we have the dynamic functionality in
> place ?

We will obviously need a new API to invoke the effect, either per-Control or something that enumerates all existing Controls and applies theme to them.
Comment 99 Niraj Modi CLA 2020-04-03 08:32:54 EDT
(In reply to Eclipse Genie from comment #97)
> Gerrit change https://git.eclipse.org/r/160426 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=5da1a820915d1242decc61a2a639dcf98a33d25f

With latest commits below are the list of internal APIs in OS platform wise:
GTK:   OS.setDarkThemePreferred(isDark);
Mac:   OS.setTheme(isDarkTheme);
Win32: OS.setTheme (isDarkTheme)

Note: GTK one can also be renamed with changes in SWT followed by dependent fragment code.
Comment 100 Niraj Modi CLA 2020-05-05 03:46:58 EDT
(In reply to Niraj Modi from comment #99)
> (In reply to Eclipse Genie from comment #97)
> > Gerrit change https://git.eclipse.org/r/160426 was merged to [master].
> > Commit:
> > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> > ?id=5da1a820915d1242decc61a2a639dcf98a33d25f
> 
> With latest commits below are the list of internal APIs in OS platform wise:
> GTK:   OS.setDarkThemePreferred(isDark);
> Mac:   OS.setTheme(isDarkTheme);
> Win32: OS.setTheme (isDarkTheme)

Since Windows feature is based on undocumented API on Windows platform and may have unknown side effects like bug 562330 and bug 562783, also this Windows support can be dropped/changed in future releases. If need arises SWT clients can turn this feature 'off' without breaking their code, by setting below system property to false, something as below from the VM arguments:
-Dorg.eclipse.swt.internal.win32.enableCustomTheme=false

I will share a gerrit shortly.
Comment 101 Eclipse Genie CLA 2020-05-05 03:55:13 EDT
New Gerrit change created: https://git.eclipse.org/r/162027
Comment 102 Eclipse Genie CLA 2022-03-10 08:24:00 EST
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/191716
Comment 103 Niraj Modi CLA 2022-03-10 08:34:47 EST
(In reply to Eclipse Genie from comment #102)
> New Gerrit change created:
> https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/191716

With above gerrit we now have a common dark theme API(internal) on all platforms:
OS#setTheme(boolean isDarkTheme)

Note:- This method make redundant below GTK method:
OS#setDarkThemePreferred(boolean preferred)

After merging will create a separate bug for replacing the use of above API with the new common API.
Comment 104 Alexandr Miloslavskiy CLA 2022-03-10 08:47:28 EST
On a relevant note:

On macOS, there is system property 'org.eclipse.swt.display.useSystemTheme'. I remember that it works better than 'OS.setTheme()', because with the latter, 'Display.getSystemColor()' returns wrong colors.

I mean, be careful when replacing old APIs with new ones.
Comment 106 Niraj Modi CLA 2022-04-07 06:38:20 EDT
We now have a common dark theme API(internal) on all platforms:
OS#setTheme(boolean isDarkTheme) -- with updated JavaDocs.

As required, we can replace GTK's OS.setDarkThemePreferred(boolean preferred) method call with OS.setTheme(boolean isDarkTheme) in rest of the platform code-base.

Resolving this bug for 4.24 M1.
Comment 107 Niraj Modi CLA 2022-04-07 06:53:27 EDT
Verified in SWT sources.
Comment 108 Christopher Mindus CLA 2022-07-12 14:07:58 EDT
I do not understand how to retrieve the colors using any kind of reliable Eclipse API's. I have tried the ITheme + ColorRegistry way or the Display.getSystemColor way, also checking perhaps the isDarkTheme return value from OS or Display.

Please find the two listings of colors for "light" and "dark" theme selected under Windows 11 using Dark OS System and Dark Apps theme.

As you will see, the Display.getSystemColor always returns the Windows light colors. The ColorRegistry returns "some" colors that are suitable for the light or dark themes, but this is not reliable. What is the way to do it?

Please help out here, or is there some missing API I haven't seen?
Comment 109 Christopher Mindus CLA 2022-07-12 14:08:50 EDT
Created attachment 288630 [details]
Colors in light theme

Colors in light theme
Comment 110 Christopher Mindus CLA 2022-07-12 14:09:29 EDT
Created attachment 288631 [details]
Colors in dark theme

Colors in dark theme
Comment 111 Christopher Mindus CLA 2022-07-12 14:11:48 EDT
Forgot: Eclipse 2022-06 (4.24)
Comment 112 Alexandr Miloslavskiy CLA 2022-07-12 14:43:43 EDT
I think that there is no way currently. SWT merely uses undocumented and incomplete Windows API to paint some of the controls. I'm not even sure if Windows itself has API for dark theme colors.