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

Revert theme changes #2206

Merged
merged 2 commits into from
Aug 19, 2024
Merged

Conversation

BeckerWdf
Copy link
Contributor

As written in #2199
these reverts the changes to the light and dark theme back to the 2024-06 release state.

In a second commit we provide the changes to the light theme as a new theme "Light (Preview)". This enables user to already test these changes and provide feedback.
Providing the changes to the dark theme as a new theme would be a bit more complicated. There are lots of downstream projects enhancing the dark theme (e.g. JDT defines lots of syntax colors for the dark theme). These downstream projects would also need to adapt to a new dark theme (providing their extension also for the new dark theme). This is simply not realistic to happen for the 2024-09 release.

Pls. review and test this PR thoroughly so that we can merge this as soon as possible for the 2024-09 release.

Revert back to the 2024-06 state of the Eclipse Themes so that
no changes are effective for the 2024-09 release.
Once master opens up again we will bring the changes back and continue
to improve them for the 2024-12 release.
Work on the improved light theme could not be finished for the 2024-09
release. To give user the possibility to already use it, this
adds the improved theme as a preview theme. This also enables us to
get early feedback from our users.
@BeckerWdf
Copy link
Contributor Author

@merks, @mvm-sap, @jukzi, @Phillipus : Are you ok with this?

@vogella
Copy link
Contributor

vogella commented Aug 19, 2024

Wasn't the vote to keep the new theme as default and add the old theme as additional one?

One thing that will most likely not work with this approach is that the existing extensions, e.g. JDT syntax highlighting will not be picked up by the additional theme. AFAIR these extensions are based on the theme ID

@HeikoKlare
Copy link
Contributor

I've tested this change (extracting light theme change into a new "preview" theme) on Windows, and for me it works as expected.
I am really looking forward to have the new theme in 2024-12. For me, personally, starting an SDK with this revert to the old light theme felt like a visual step back, but I agree that this is probably the right thing to do, given the open issues.

One thing that will most likely not work with this approach is that the existing extensions, e.g. JDT syntax highlighting will not be picked up by the additional theme. AFAIR these extensions are based on the theme ID

I think the point is that only a preview for the light theme is provided, so theme extensions should be applied like for the default (light) theme. That would not work for the dark theme (which requires matching a specific theme ID), which is why there is no preview for the dark theme. Correct me if I am wrong, @BeckerWdf.

Copy link
Contributor

Test Results

 1 815 files  +  606   1 815 suites  +606   1h 35m 13s ⏱️ + 30m 58s
 7 689 tests ±    0   7 461 ✅ +    3  228 💤  -   3  0 ❌ ±0 
24 228 runs  +8 079  23 479 ✅ +7 843  749 💤 +236  0 ❌ ±0 

Results for commit 803ba4f. ± Comparison against base commit e3628a3.

Copy link
Contributor

@jukzi jukzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks

@@ -51,6 +51,24 @@
label="%theme.win"
os="win32">
</theme>
<theme
basestylesheeturi="css/e4_preview_gtk.css"
id="org.eclipse.e4.ui.css.theme.e4_preview"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it would be worth finding it a new fresh id now that wouldn't change later. More precisely, the id shouldn't mention "preview". So later, when we want it to be default, we just change the default value of the preference and that's all.
People who have switched to this theme already wouldn't see an issue (the theme would still be present), and people who have never changed the theme should also see the update as the default value of the preference would change.
Maybe something like "flatLight" since this themes gets rid of many gradients, lines and constrasts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan is to copy the change to the "org.eclipse.e4.ui.css.theme.e4" and delete the "org.eclipse.e4.ui.css.theme.e4_preview" again. So preview is correct.

Copy link
Contributor

@mickaelistria mickaelistria Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you plan to remove the theme with id org.eclipse.e4.ui.css.theme.e4_preview, then you may introduce issues to people who have enabled it and then update. The selected theme would then be missing.
To be safe, it's either changes in the current theme (but that couldn't be completed), or a new theme with a sustainable new id. Everything in between can be error-prone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this. If I have set the preview theme. Then start an eclipse with that workspace that does not have that theme it defaults to the "standard" light theme.

@merks
Copy link
Contributor

merks commented Aug 19, 2024

@merks, @mvm-sap, @jukzi, @Phillipus : Are you ok with this?

I am okay with what you folks decide is best. I think what @HeikoKlare has outlined is the safest strategy that avoids risk of a backlash and gives sufficient time to iron out the wrinkles.

As we discussed at the community meeting, the poll was intended to be an opinion poll, not a binding vote. The technical experts should make a technical decision that is technically the best in their personal opinion, taking opinions of the community into account...

@BeckerWdf
Copy link
Contributor Author

One thing that will most likely not work with this approach is that the existing extensions, e.g. JDT syntax highlighting will not be picked up by the additional theme. AFAIR these extensions are based on the theme ID

That's why we did not provide a preview version of the dark theme. I think there are no theme extensions for the light theme.

@mvm-sap
Copy link
Contributor

mvm-sap commented Aug 19, 2024

One thing that will most likely not work with this approach is that the existing extensions, e.g. JDT syntax highlighting will not be picked up by the additional theme. AFAIR these extensions are based on the theme ID

That's why we did not provide a preview version of the dark theme. I think there are no theme extensions for the light theme.

JDT already has an extension for light theme

@BeckerWdf
Copy link
Contributor Author

One thing that will most likely not work with this approach is that the existing extensions, e.g. JDT syntax highlighting will not be picked up by the additional theme. AFAIR these extensions are based on the theme ID

That's why we did not provide a preview version of the dark theme. I think there are no theme extensions for the light theme.

JDT already has an extension for light theme

But it's a minor one.

@BeckerWdf
Copy link
Contributor Author

Why is "pr-head" red?
The console says:

09:04:53.520 [INFO] ------------------------------------------------------------------------
09:04:53.520 [INFO] BUILD SUCCESS
09:04:53.520 [INFO] ------------------------------------------------------------------------
09:04:53.520 [INFO] Total time:  37:01 min
09:04:53.521 [INFO] Finished at: 2024-08-19T09:04:53Z
09:04:53.521 [INFO] ------------------------------------------------------------------------

I want to merge this PR today.

@laeubi
Copy link
Contributor

laeubi commented Aug 19, 2024

@HeikoKlare
Copy link
Contributor

Rather looks like a glitch to me. The reference master build from yesterday shows the exact same number of fixed issues than this build shows as new issues, but I do not see how the commits added for that build should have resolved them:
https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/master/603/

image

@BeckerWdf
Copy link
Contributor Author

Rather looks like a glitch to me

For me too...

@BeckerWdf BeckerWdf merged commit e69252a into eclipse-platform:master Aug 19, 2024
13 of 16 checks passed
@BeckerWdf BeckerWdf deleted the revert_theme_changes branch August 19, 2024 12:45
@BeckerWdf BeckerWdf added this to the 4.33 RC1 milestone Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants