-
Notifications
You must be signed in to change notification settings - Fork 226
Allowing smaller images to show in the workbench window #3169
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
base: master
Are you sure you want to change the base?
Conversation
Hey all, looking for feedback and review on this PR. |
Sound like a reasonable intermediate solution until we have: @marcushoepfner this is probably most interesting for you and particularly interesting to have your opinion |
@marcushoepfner currently is on vacation. So he will not be able to react here in a timely manner. |
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. |
Hey all! It sounds like this has potential for post 2025-09? |
Test Results 1 179 files - 1 599 1 179 suites - 1 599 20m 31s ⏱️ - 1h 25m 39s Results for commit 6841a0e. ± Comparison against base commit 5922d0c. This pull request removes 4250 tests.
|
There was a problem hiding this 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.
....ui.workbench.renderers.swt/src/org/eclipse/e4/ui/workbench/renderers/swt/StackRenderer.java
Outdated
Show resolved
Hide resolved
onBoarding.setSize(width, height); | ||
|
||
boolean showImage = height > ONBOARDING_SHOW_IMAGE_HEIGHT_THRESHOLD; | ||
boolean showImage; |
There was a problem hiding this comment.
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?
Co-authored-by: marcushoepfner <marcus.hoepfner@sap.com>
> 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you
This pull request changes some projects for the first time in this development cycle.
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
Further information are available in Common Build Issues - Missing version increments. |
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:
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.