Light (Preview) Theme: Issues in Progress View

[BeckerWdf]

• [x] I verified I can reproduce this issue against latest Integration Build of Eclipse SDK

Steps to reproduce

From a fresh installation and clean workspace:

• Choose "Light (Preview)"

I tried

• Open the Progress View
• Build your complete workspace

But got:

With the "Light" Theme this looks like this:

I expected that the texts like "Building" don't have a white background but simply use the same background as the parent (the light grey color)

Tested under this environment:

• OS & version: macOS

Community

• [x] I understand reporting an issue to this OSS project does not mandate anyone to fix it. Other contributors may consider the issue, or not, at their own convenience. The most efficient way to get it fixed is that I fix it myself and contribute it back as a good quality patch to the project.

@mvm-sap: Can you pls. have a look?

[mai-tran-03]

I'm working on this.

• Team Mai Tran/Andy Espinoza/Mahdi Alsalami (students), and Akshatha Anantharamu (mentor)

[HannesWell]

Great. You are some of the students from CodeDays, aren't you?

Much success on this task and don't hesitate to ask for help if you need any.

[mai-tran-03]

To get started, I replicated the issue and observed how the background color of this progress bar is rendered in different themes. The color is consistent for all other themes, except the light preview. Based on the keywords, using the global search tool, I searched for progressBar and setBackground. Then, I looked at any relevant packages and files where the progress bar could be created and the background could be called. Then, I followed this path /bundles/org.eclipse.ui.workbench/Eclipse UI/org.eclipse.ui.internal.progress/ to the key locations of DetailedProgressView.java and ProgressInfoItem.java.

Although I could not develop a full solution with my team, I suspect the problem is that not all relevant components of the progress item are synced in the light preview theme. Upon closer inspection of the function setColor, it checks a boolean flag isThemed and applies a default color setting if the variable isThemed is false. This means if there is no custom theme, then the program proceeds to set the background color in alternating order of darker and lighter color to help with readability and foreground color of the text.

My hypothesis is to remove the condition isThemed to see if it affects the light preview theme. First, I checked all the themes, and the progress item with the condition isThemed, surprisingly, most themes use the default color for their progress components. Then, I checked all the themes again without the condition; the light preview works fine. However, the dark theme had inherited the default color. This means the dark theme is a custom theme so the condition isThemed is needed but in a different way.

My next step is to check how the condition isThemed is being checked. The function getCustomThemeFlag returns isThemed by checking whether the active theme is equal to the default theme; if the current theme is not default, then it returns true, otherwise false. Since the dark theme is the only one affected by the condition isThemed, I modify the function getCustomThemeFlag to return true only when the active theme is equal to the dark theme.

The following commit seems to fix the issue, however, I have yet to submit a pull request. Do you have any suggestions?

[BeckerWdf]

My next step is to check how the condition isThemed is being checked. The function getCustomThemeFlag returns isThemed by checking whether the active theme is equal to the default theme; if the current theme is not default, then it returns true, otherwise false. Since the dark theme is the only one affected by the condition isThemed, I modify the function getCustomThemeFlag to return true only when the active theme is equal to the dark theme.

So this basically boils down to: "Do that extras stuff if the currently selected theme is the dark theme (as the only theme != default used to be the dark theme)". So the issue only happens because the "Light (Preview)" theme is not the "Light" theme (the have a different key). Once we copy the content of the "Light (Preview)" theme to the "Light" theme (and remove the "Light (Preview) theme the issue no longer appears.

But anyway the current implementation assumes that there are basically two themes - the light and the dark theme. As this might not be true any more in the future it would be better to change this to a more robust solution.

So in the current implementation (without https://github.com/eclipse-platform/eclipse.platform.ui/pull/2485 applied) the block

if (!isThemed) {
	if (row % 2 == 0) {
		setAllBackgrounds(JFaceResources.getColorRegistry().get(DARK_COLOR_KEY));
	} else {
		setAllBackgrounds(getDisplay().getSystemColor(SWT.COLOR_LIST_BACKGROUND));
	}
	setAllForegrounds(getDisplay().getSystemColor(SWT.COLOR_LIST_FOREGROUND));
}

is only executed when the light theme is selected. Correct?

[mai-tran-03]

Yes, I tested these out by the following criteria.

[BeckerWdf]

Yes, I tested these out by the following criteria.

This means isThemed is true for these screenshots?

[mai-tran-03]

Without the fixes, getCustomThemeFlag() does the following:

1. if the current theme is not the default theme (which is only the light theme), return isThemed == true
2. else, return isThemed == false

then setColor() does the following`:

1. if isThemed == false (this means the current theme is the light theme), display alternating colors for progress items (this is seen in the upper left corner of the first screenshot)
2. else, display one color for progress items (this is seen in the rest of the first screenshot)

So the getCustomThemeFlag() did not create alternating row colors for any other themes besides the light theme.

When I removed isThemed condition entirely, there are alternating row colors for all themes (shown in the second screenshot). However, the dark theme could not use its color scheme.

When I just removed the ! in the !isThemed condition, there are alternating row colors for all other themes except for the light theme. But the dark theme also could not use its color scheme.

1. if isThemed == true, display alternating colors for progress items
2. else, display one color for progress items

I'm not sure how to create a more robust solution that does not explicitly check for the dark theme in getCustomThemeFlag() and would apply for any future custom themes.

[opcoach]

See the discussion summarizing the work done : https://github.com/eclipse-ide/contributing/discussions/3

[BeckerWdf]

Closing as the issue can no longer be reproduced. Re-open if this is still an issue.