Color-Provider to theme custom controls #130

tmssngr · 2025-03-02T10:14:08Z

Please start ColorProviderTest to test.

Note, that the color provider, the color constants and the renderer are closely linked. In the future there will be exchangeable renderers which then need an own color provider (e.g. because a different renderer needs more/less colors).

Feedback is welcome.

github-actions · 2025-03-02T10:22:05Z

Test Results

341 files ±0 341 suites ±0 2m 25s ⏱️ +2s
3 955 tests ±0 3 671 ✅ ±0 284 💤 ±0 0 ❌ ±0
11 693 runs ±0 10 808 ✅ ±0 885 💤 ±0 0 ❌ ±0

Results for commit b11d41c. ± Comparison against base commit 4f08da9.

♻️ This comment has been updated with latest results.

HeikoKlare

In general, this looks good to me. I only have two comments/thoughts on this.

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Button.java

bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Display.java

tmssngr · 2025-03-07T14:32:29Z

Now also includes (parts of) ToolBar.

DieterMai

I left some comment in the code.

When i tested this, i noticed that the toolBar background is a lot darker then on the master branch. Don't know why.

I general is the following pros of this approach:

All colors defined in a central place
All colors can be easily switch out

The cons:

It is not very friendly for implementing custom widget or a custom renderer. A custom widget would have to either have to define its own colors (like before) or use the color provider with a color-key that is unrelated to their situation.
If a new key is added, a custom color provider would not know about it and would not return a proper color.

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/DefaultColorProvider.java

DieterMai · 2025-03-14T18:36:36Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/ToolBar.java

+
+	@Override
+	public Color getBackground() {
+		return background != null ? background : getColorProvider().getColor(ToolBarRenderer.COLOR_BACKGROUND);


ToolBar should not have a dependency to a specific renderer

This is the generic ToolBarRenderer.

Not this is the actual Implementation. ToolBarRenderer is not abstract and no other renderer extends from it.

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/ToolBar.java

tmssngr · 2025-03-15T07:28:05Z

Thanks for reviewing.

When i tested this, i noticed that the toolBar background is a lot darker then on the master branch. Don't know why.

I'd say, because the background of composites comes from Windows while the background from controls like Label, Link and ToolBar comes from the specified colors. Maybe it would make sense to only draw the background for such controls if it has been set to a certain color (in contrast to the transparent default).

The cons:

* It is not very friendly for implementing custom widget or a custom renderer. A custom widget would have to either have to define its own colors (like before) or use the color provider with a color-key that is unrelated to their situation.

* If a new key is added, a custom color provider would not know about it and would not return a proper color.

What do you suggest? I also had the idea of directly creating getters for certain colors instead of using a map (which is quite fast, but field access is faster).

Independent of your review, I have the idea of renaming the ColorProvider to something like RendererConfig and also add ways to configure int values (e.g. sizes) and boolean (e.g. to switch some feature on or off, e.g. for configuring what should happen when clicking above/below the thumb of a scrollbar - page-wise scrolling or jump to that location).

Instead of color constants, the custom controls ask the color provider to give them a color by name. This allows easier theming.

DieterMai · 2025-03-15T20:59:28Z

When i tested this, i noticed that the toolBar background is a lot darker then on the master branch. Don't know why.

I'd say, because the background of composites comes from Windows while the background from controls like Label, Link and ToolBar comes from the specified colors. Maybe it would make sense to only draw the background for such controls if it has been set to a certain color (in contrast to the transparent default).

But why is it different between master and PR? adding the ColorProvider should not change the rendering (in Light mode).

What do you suggest? I also had the idea of directly creating getters for certain colors instead of using a map (which is quite fast, but field access is faster).

I think we are in an early state of development so that we are able to try different things. But how i see it this should not be the final solution for color.
Other cons i see:

Most color keys are defined as package private. This means a custom color provider would have no access to them. How is a custom color provider supposed to provide the colors?
Some color keys are public. This means they are now API and can never be changed.
Since the color keys are declared in the Default renderers, the Default color provider has a dependency to those renderers. This causes issues for other renderers of the same widget [1]. As an example:
DefaultScaleRenderer declares the key COLOR_NOTCH. This is used for the notches on the scale. But this notches are a Windows specific thing. There are no notches on Linux. If i would have implemented the rendering of an Linux scale, the COLOR_NOTCH key would be missing.

I think the main problem is that the DefaultColorProvider has dependencies to every renderer. Maybe this could be fixed by moving the color keys into the DefaultColorProvider or maybe separate class.

I'm also wondering why you decided to go against the earlier suggested material design concept of a color scheme [2]

[1] I know the primary goal of I31 is to have less implementations, but a secondary goal i would say is to switch to different renderings

[2] https://developer.android.com/develop/ui/compose/designsystems/material3

tmssngr · 2025-03-16T08:25:51Z

But why is it different between master and PR? adding the ColorProvider should not change the rendering (in Light mode).

So in other words you suggest to use the Windows 11 default background color as default background for the custom drawn widgets?

Most color keys are defined as package private. This means a custom color provider would have no access to them. How is a custom color provider supposed to provide the colors?

They should rather be public. Maybe for some classes without clear renderer hierarchies this is not correctly implemented now.

Some color keys are public. This means they are now API and can never be changed.

What do you suggest as alternative?

Since the color keys are declared in the Default renderers, the Default color provider has a dependency to those renderers. This causes issues for other renderers of the same widget [1].

Which IMHO is correct, because only the specific color provider knows which colors are required. If you want to use a different renderer (which uses completely different colors), you will need to create an own color provider, too.

But this notches are a Windows specific thing. There are no notches on Linux.

Then there needs to be a different LinuxScaleRenderer and a different LinuxRendererFactory and a different LinuxColorFactory (or LinuxRendererConfig as I suggested).

If you have a better suggestion how to implement it, you are welcome to provide a code suggestion.

I'm also wondering why you decided to go against the earlier suggested material design concept of a color scheme [2]

I think, I've didn't. The light and dark default implementations derive from a base color. If you mean something else, again, please feel free to provide a different implementation.

tmssngr · 2025-03-16T12:41:33Z

I'm waiting for #171 to be accepted before this one needs to be modified to fix some of @DieterMai's feedback.

DieterMai · 2025-03-16T18:00:11Z

But why is it different between master and PR? adding the ColorProvider should not change the rendering (in Light mode).

So in other words you suggest to use the Windows 11 default background color as default background for the custom drawn widgets?

No, i am confused why the ToolBar has a different background then before.

Some color keys are public. This means they are now API and can never be changed.

What do you suggest as alternative?

Have more generic color keys.

Since the color keys are declared in the Default renderers, the Default color provider has a dependency to those renderers. This causes issues for other renderers of the same widget [1].

Which IMHO is correct, because only the specific color provider knows which colors are required. If you want to use a different renderer (which uses completely different colors), you will need to create an own color provider, too.

This is not how i imagen it to be used.
Someone who writes a custom color provider should not have to know which renderers are used.
Someone who writes a custom renderer should not have to know which color provider is used.

If you have a better suggestion how to implement it, you are welcome to provide a code suggestion.

I'm hesitant to do so. ATM I'm focused on reaching feature parity with the native implementation.

I'm also wondering why you decided to go against the earlier suggested material design concept of a color scheme [2]

I think, I've didn't. The light and dark default implementations derive from a base color. If you mean something else, again, please feel free to provide a different implementation.

I would have imaged something like colorProvider.get(ColorRole.PRIMARY).

tmssngr · 2025-03-16T18:37:12Z

But why is it different between master and PR? adding the ColorProvider should not change the rendering (in Light mode).

So in other words you suggest to use the Windows 11 default background color as default background for the custom drawn widgets?

No, i am confused why the ToolBar has a different background then before.

IMHO it did not draw any background at all, but used the Windows default. This will not work for a themed application, e.g. with a forced dark theme on a light windows (or visa versa).

But it might be useful to also be able to get nullable colors (with the meaning of don't draw a background, for example).

Some color keys are public. This means they are now API and can never be changed.

What do you suggest as alternative?

Have more generic color keys.

For example?

Since the color keys are declared in the Default renderers, the Default color provider has a dependency to those renderers. This causes issues for other renderers of the same widget [1].

Which IMHO is correct, because only the specific color provider knows which colors are required. If you want to use a different renderer (which uses completely different colors), you will need to create an own color provider, too.

This is not how i imagen it to be used. Someone who writes a custom color provider should not have to know which renderers are used. Someone who writes a custom renderer should not have to know which color provider is used.

IMHO different renderers may need different colors. Example: one (Push)ButtonRenderer mit use 5 colors (background, background-disabled, foreground, foreground-disabled, focus/default-highlight) while another one could use much more colors because it also draws a border with different colors. Maybe each Renderer needs a special belonging Config containing colors?

I think, I've didn't. The light and dark default implementations derive from a base color. If you mean something else, again, please feel free to provide a different implementation.

I would have imaged something like colorProvider.get(ColorRole.PRIMARY).

Yes, me too, but I don't know how I should implement that, e.g. for a button. Hence I've asked for an implementation example. IMHO, the current approach is more generic - it allows any arbitrary number of colors. A MaterialColorProvider could set all these colors (from my PR) from a its material colors.

fedejeanne · 2025-03-17T09:10:30Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Link.java

@@ -169,7 +169,7 @@ private static int checkStyle(int style) {
 		}
 		int mask = SWT.SHADOW_IN | SWT.SHADOW_OUT | SWT.SHADOW_NONE | SWT.LEFT_TO_RIGHT | SWT.RIGHT_TO_LEFT;
 		style = style & mask;
-		style |= SWT.NO_FOCUS | SWT.DOUBLE_BUFFERED;


Why get rid of SWT.DOUBLE_BUFFERED? How is that related to the PR?

fedejeanne · 2025-03-17T09:28:04Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Label.java

 	@Override
 	public void setBackground(Color color) {
 		super.setBackground(color);
 		renderer.setBackground(color);
 		redraw();
 	}

+	@Override
+	public Color getForeground() {
+		return foreground != null ? foreground : getColorProvider().getColor(LabelRenderer.COLOR_FOREGROUND);


Shouldn't the renderer hold this information, like it holds the background color?

fedejeanne · 2025-03-17T09:34:23Z

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Control.java

@@ -5250,4 +5250,7 @@ static double luma (double[] rgbColor) {
 	return 0.2126f * rgbColor[0] + 0.7152f * rgbColor[1] + 0.0722f * rgbColor[2];
 }

+protected ColorProvider getColorProvider() {


It would make sense to make it final so no one gets the idea of overriding it and start instantiating/holding color providers someplace else. WDYT?

fedejeanne · 2025-03-17T09:36:35Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/DefaultColorProvider.java

+
+	private final Map<String, Color> map = new HashMap<>();
+
+	public DefaultColorProvider() {


Suggested change

public DefaultColorProvider() {

private DefaultColorProvider() {

// prevent instantiation from outside

fedejeanne · 2025-03-17T09:42:41Z

bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Control.java

@@ -6875,4 +6875,7 @@ Point getSurfaceOrigin () {
 	return success ? new Point((int)originX[0], (int)originY[0]) : new Point(0, 0);
 }

+protected ColorProvider getColorProvider() {


Suggested change

protected ColorProvider getColorProvider() {

protected final ColorProvider getColorProvider() {

fedejeanne · 2025-03-17T09:45:02Z

bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Display.java

+ * Sets the color provider used for custom-drawn controls.
+ * @param colorProvider a non-null color provider
+ */
+public void setColorProvider(ColorProvider colorProvider) {


Suggested change

public void setColorProvider(ColorProvider colorProvider) {

public final void setColorProvider(ColorProvider colorProvider) {

fedejeanne · 2025-03-17T09:45:51Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java

+ * Returns the color provider used for custom-drawn controls.
+ * @return a non-null instance of the color provider
+ */
+public ColorProvider getColorProvider() {


Suggested change

public ColorProvider getColorProvider() {

public final ColorProvider getColorProvider() {

fedejeanne · 2025-03-17T09:46:00Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java

+ * Sets the color provider used for custom-drawn controls.
+ * @param colorProvider a non-null color provider
+ */
+public void setColorProvider(ColorProvider colorProvider) {


Suggested change

public void setColorProvider(ColorProvider colorProvider) {

public final void setColorProvider(ColorProvider colorProvider) {

fedejeanne · 2025-03-17T09:50:33Z

tests/org.eclipse.swt.tests/ManualTests/org/eclipse/swt/tests/manual/ColorProviderTest.java

+import org.eclipse.swt.layout.*;
+import org.eclipse.swt.widgets.*;
+
+public class ColorProviderTest {


NIT: since this isn't an automatic test class and since we have this kind of showcases already (ControlExample / CustomControlExample), how about calling it:

Suggested change

public class ColorProviderTest {

public class ColorProviderTExample {

?

Also, would it be possible to add the ´darkChkBx´ to the ControlExample and CustomControlExample? Those 2 classes already showcase all of the controls so it should keep up-to-date while we keep implementing the widgets

fedejeanne · 2025-03-17T09:54:25Z

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/CustomControl.java

+	protected Color background;
+	protected Color foreground;


I assume in the future (once all controls have their corresponding renderers) these 2 should also go into the corresponding renderer, right? Otherwise the information about coloring is scattered between the controls and their renderers.

tmssngr force-pushed the feature/color-provider branch from 2c92510 to d65adae Compare March 2, 2025 10:17

tmssngr force-pushed the feature/color-provider branch 11 times, most recently from dc0eaee to 73a24dc Compare March 4, 2025 15:04

HeikoKlare force-pushed the master branch 4 times, most recently from a9f24e5 to d2109d6 Compare March 4, 2025 16:50

tmssngr force-pushed the feature/color-provider branch 3 times, most recently from baf5c63 to 90ef5c8 Compare March 6, 2025 11:02

tmssngr marked this pull request as ready for review March 6, 2025 11:13

tmssngr force-pushed the feature/color-provider branch from 90ef5c8 to 4935aba Compare March 6, 2025 13:06

HeikoKlare reviewed Mar 6, 2025

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/widgets/Button.java Outdated Show resolved Hide resolved

bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Display.java Outdated Show resolved Hide resolved

tmssngr force-pushed the feature/color-provider branch from 4935aba to 55c5182 Compare March 6, 2025 18:08

tmssngr marked this pull request as draft March 7, 2025 11:59

tmssngr force-pushed the feature/color-provider branch 5 times, most recently from c49e0c0 to 232f25a Compare March 7, 2025 14:27

tmssngr marked this pull request as ready for review March 7, 2025 14:32

tmssngr marked this pull request as draft March 7, 2025 15:14

tmssngr mentioned this pull request Mar 12, 2025

Create separate renderer classes #152

Merged

tmssngr force-pushed the feature/color-provider branch 8 times, most recently from bbe6a94 to 07d02a1 Compare March 14, 2025 14:47

tmssngr marked this pull request as ready for review March 14, 2025 14:54

tmssngr requested review from DieterMai and HeikoKlare March 14, 2025 14:54

DieterMai reviewed Mar 14, 2025

View reviewed changes

tmssngr force-pushed the feature/color-provider branch from 07d02a1 to 6e688ba Compare March 15, 2025 07:18

Introduce ColorProvider

b11d41c

Instead of color constants, the custom controls ask the color provider to give them a color by name. This allows easier theming.

tmssngr force-pushed the feature/color-provider branch from 6e688ba to b11d41c Compare March 15, 2025 07:29

tmssngr requested a review from fedejeanne March 15, 2025 08:18

fedejeanne reviewed Mar 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Color-Provider to theme custom controls #130

Color-Provider to theme custom controls #130

tmssngr commented Mar 2, 2025

github-actions bot commented Mar 2, 2025 •

edited

Loading

HeikoKlare left a comment

tmssngr commented Mar 7, 2025

DieterMai left a comment

DieterMai Mar 14, 2025

tmssngr Mar 15, 2025

DieterMai Mar 15, 2025

tmssngr commented Mar 15, 2025

DieterMai commented Mar 15, 2025

tmssngr commented Mar 16, 2025

tmssngr commented Mar 16, 2025

DieterMai commented Mar 16, 2025

tmssngr commented Mar 16, 2025

fedejeanne Mar 17, 2025

fedejeanne Mar 17, 2025

fedejeanne Mar 17, 2025

fedejeanne Mar 17, 2025

fedejeanne Mar 17, 2025

fedejeanne Mar 17, 2025

fedejeanne Mar 17, 2025

fedejeanne Mar 17, 2025

fedejeanne Mar 17, 2025

fedejeanne Mar 17, 2025


		private final Map<String, Color> map = new HashMap<>();

		public DefaultColorProvider() {

	public DefaultColorProvider() {
	private DefaultColorProvider() {
	// prevent instantiation from outside

	protected ColorProvider getColorProvider() {
	protected final ColorProvider getColorProvider() {

	public void setColorProvider(ColorProvider colorProvider) {
	public final void setColorProvider(ColorProvider colorProvider) {

	public ColorProvider getColorProvider() {
	public final ColorProvider getColorProvider() {

	public class ColorProviderTest {
	public class ColorProviderTExample {

Color-Provider to theme custom controls #130

Are you sure you want to change the base?

Color-Provider to theme custom controls #130

Conversation

tmssngr commented Mar 2, 2025

github-actions bot commented Mar 2, 2025 • edited Loading

Test Results

HeikoKlare left a comment

Choose a reason for hiding this comment

tmssngr commented Mar 7, 2025

DieterMai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmssngr commented Mar 15, 2025

DieterMai commented Mar 15, 2025

tmssngr commented Mar 16, 2025

tmssngr commented Mar 16, 2025

DieterMai commented Mar 16, 2025

tmssngr commented Mar 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 2, 2025 •

edited

Loading