Skip to content

Conversation

@andrew-tram
Copy link

@andrew-tram andrew-tram commented Aug 13, 2025

Proposed solution for #3069

Currently, the code decides whether to show the onboarding image using a fixed threshold (ONBOARDING_SHOW_IMAGE_HEIGHT_THRESHOLD, 450px content height), regardless of the actual image size.

My idea is to make this more adaptive by retrieve the actual image height from the label.

If the image is small (<= 250px), allow it to be shown with a lower overall height requirement (e.g., 314px total window height).
If the image is large (> 250px), keep using the existing higher threshold.

Also, from the extension point descrption:

The image is shown in the editor area in case no editor is open.
The image shall be grey and not colored and shall have a size of 250 x 250 px.
Plus a second image for high resolution with a size of 500 x 500 px and a name like [image_name]@2x.[image_type].

I dont really see it take into account the @2x size for an image, so when I manually use a 500x500 image, it gets cut off anyway because of the 450 threshold. But maybe I dont know how to trigger the @2x resolution in Eclipse properly.

@andrew-tram
Copy link
Author

Hey all, looking for feedback and review on this PR.

@HeikoKlare
Copy link
Contributor

Sound like a reasonable intermediate solution until we have:

@marcushoepfner this is probably most interesting for you and particularly interesting to have your opinion

@BeckerWdf
Copy link
Member

@marcushoepfner currently is on vacation. So he will not be able to react here in a timely manner.

@HeikoKlare
Copy link
Contributor

Thanks for the info! I guess it's not a problem as this cannot go into the upcoming release anymore anyway, so we basically have time until November to finalize it for the December release.

@BeckerWdf
Copy link
Member

Thanks for the info! I guess it's not a problem as this cannot go into the upcoming release anymore anyway, so we basically have time until November to finalize it for the December release.

Yes I agree that this is not a problem. Just wanted to set expectations.

@andrew-tram
Copy link
Author

Hey all! It sounds like this has potential for post 2025-09?

@github-actions
Copy link
Contributor

github-actions bot commented Aug 18, 2025

Test Results

 3 018 files  ±0   3 018 suites  ±0   2h 16m 29s ⏱️ + 1m 4s
 8 236 tests ±0   7 987 ✅ +1  249 💤 ±0  0 ❌  - 1 
23 628 runs  ±0  22 834 ✅ +1  794 💤 ±0  0 ❌  - 1 

Results for commit 3c7103d. ± Comparison against base commit 2d4334f.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@marcushoepfner marcushoepfner left a comment

Choose a reason for hiding this comment

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

good idea. not really sure though whether it is worth the effort. with svg's (#3098) we could rewrite all the code and even scale the image in case the editor size is changed).

But as we don't know when this will be available it is probably a good idea to merge this PR.

Two minor improvements though.

onBoarding.setSize(width, height);

boolean showImage = height > ONBOARDING_SHOW_IMAGE_HEIGHT_THRESHOLD;
boolean showImage;
Copy link
Contributor

Choose a reason for hiding this comment

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

did you consider putting this in a method and provide some java doc for the method.
With that:

  • very detailed math code is hidden from the method flow (increased readability of the setOnboardingControlSize method)
  • comments can be removed and put into java doc of the new method

WDYT?

Copy link
Contributor

@marcushoepfner marcushoepfner left a comment

Choose a reason for hiding this comment

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

thank you

@eclipse-platform-bot
Copy link
Contributor

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

bundles/org.eclipse.e4.ui.workbench.renderers.swt/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch
From 11885cec2a5b5f3f49000c2a3fc4b00c3b18532c Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Tue, 9 Sep 2025 10:21:19 +0000
Subject: [PATCH] Version bump(s) for 4.38 stream


diff --git a/bundles/org.eclipse.e4.ui.workbench.renderers.swt/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.ui.workbench.renderers.swt/META-INF/MANIFEST.MF
index 9d5e27c11e..249542c521 100644
--- a/bundles/org.eclipse.e4.ui.workbench.renderers.swt/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.e4.ui.workbench.renderers.swt/META-INF/MANIFEST.MF
@@ -1,7 +1,7 @@
 Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-SymbolicName: org.eclipse.e4.ui.workbench.renderers.swt;singleton:=true
-Bundle-Version: 0.16.900.qualifier
+Bundle-Version: 0.16.1000.qualifier
 Bundle-Name: %pluginName
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
-- 
2.51.0

Further information are available in Common Build Issues - Missing version increments.

@andrew-tram
Copy link
Author

What should be done about this?

MINOR_VERSION_CHANGE_NO_NEW_API
Check warning: MINOR_VERSION_CHANGE_NO_NEW_API
ERROR: The service version is increased unnecessarily since either the major or minor or service version is already increased

I figured updating the service portion of the version would be appropriate

@iloveeclipse
Copy link
Member

What should be done about this?

Please rebase (not merge!) your PR on latest master state.

andrew-tram and others added 3 commits December 1, 2025 10:57
> did you consider putting this in a method and provide some java doc for the method. With that:
> 
https://github.com/eclipse-platform/eclipse.platform.ui/pull/3169/files/6841a0ef260ff65316607a1d4d2b6617b8dc637b#r2329939384

> * very detailed math code is hidden from the method flow (increased readability of the setOnboardingControlSize method)
> * comments can be removed and put into java doc of the new method
> 
> WDYT?
@andrew-tram
Copy link
Author

What should be done about this?

Please rebase (not merge!) your PR on latest master state.

Ahh gotcha. Thank you for the heads up.

update version
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.

6 participants