Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Light (Preview) Theme: Issues in Progress View #2313

Open
2 tasks done
Tracked by #2114
BeckerWdf opened this issue Sep 23, 2024 · 8 comments · May be fixed by #2485
Open
2 tasks done
Tracked by #2114

Light (Preview) Theme: Issues in Progress View #2313

BeckerWdf opened this issue Sep 23, 2024 · 8 comments · May be fixed by #2485
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@BeckerWdf
Copy link
Contributor

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: Screenshot 2024-09-23 at 08 34 57

With the "Light" Theme this looks like this:
Screenshot 2024-09-23 at 08 35 41

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

  • 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?

@BeckerWdf BeckerWdf added the bug Something isn't working label Sep 23, 2024
@iloveeclipse iloveeclipse changed the title Light (Preview) Theme: Issues in Progress Voew Light (Preview) Theme: Issues in Progress View Sep 23, 2024
@opcoach opcoach added the good first issue Good for newcomers label Sep 30, 2024
@mai-tran-03
Copy link

I'm working on this.

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

@HannesWell
Copy link
Member

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
Copy link

mai-tran-03 commented Nov 3, 2024

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?

@mai-tran-03 mai-tran-03 linked a pull request Nov 4, 2024 that will close this issue
@BeckerWdf
Copy link
Contributor Author

BeckerWdf commented Nov 4, 2024

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 #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
Copy link

Yes, I tested these out by the following criteria.
image
image

@BeckerWdf
Copy link
Contributor Author

Yes, I tested these out by the following criteria. image

This means isThemed is true for these screenshots?

@mai-tran-03
Copy link

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
Copy link
Member

opcoach commented Nov 16, 2024

See the discussion summarizing the work done : eclipse-ide/contributing#3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants