search

eclipse-platform / eclipse.platform.ui

Eclipse Platform
https://projects.eclipse.org/projects/eclipse.platform
Eclipse Public License 2.0
81 stars 188 forks source link

[macOS] some UI elements (e.g. list, tree views) seem to have more space between lines now #1674

Closed martinlippert closed 8 months ago

martinlippert commented 9 months ago

Some UI elements seem to have a lot more space between lines on macOS since Eclipse 2024-03 builds, for example the package explorer, dialogs showing lists (switch to different perspective), etc. It looks like a very general issue (maybe even in SWT, not sure). Not sure if this was an intended change, but I would prefer the old behavior to save screen real estate.

Eclipse 2024-03-M2 on macOS Sonoma 14.3.1 Plain vanilla EPP package download, empty workspace.

martinlippert commented 9 months ago

This is how it looks like for 2024-03 M2 on macOS:

package-explorer-2024-03-m2-on-macOS

vs. the overall look using 2023-12 on macOS:

package-explorer-2023-12-on-macOS
merks commented 9 months ago

Have you tried any of the SWT snippets to see if the problem is like that there too?

BeckerWdf commented 9 months ago

I think https://github.com/eclipse-platform/eclipse.platform.swt/pull/771 is the case for this. And I had the impression that is was by intention.

Phillipus commented 9 months ago

The underlying cause is the macOS SDK version that the Eclipse launcher is built with. In older versions of Eclipse the launcher was built using SDK 10. Later versions are built using SDK 11 and 13. This dictates how the UI elements are displayed. Taller tree and table items are part of SDK 11 and greater. https://github.com/eclipse-platform/eclipse.platform.swt/pull/771 is a fix to keep things consistent.

martinlippert commented 9 months ago

The underlying cause is the macOS SDK version that the Eclipse launcher is built with. In older versions of Eclipse the launcher was built using SDK 10. Later versions are built using SDK 11 and 13. This dictates how the UI elements are displayed. Taller tree and table items are part of SDK 11 and greater. eclipse-platform/eclipse.platform.swt#771 is a fix to keep things consistent.

I tested this using I20240219-0600, which has org.eclipse.swt.cocoa.macosx.aarch64_3.125.0.v20240217-1555. This build still has the described issue. I am not sure whether the fix that got merged in https://github.com/eclipse-platform/eclipse.platform.swt/pull/771 is already included in that I-build though.

Phillipus commented 9 months ago

This build still has the described issue.

Not sure what you mean?

To be clear, since Eclipse 4.30 the launcher has been built using SDK 13 so taller row heights are the norm. https://github.com/eclipse-platform/eclipse.platform.swt/pull/771 was merged on Jan 17 2024. All that patch does is to ensure that row heights stay as they should when a font is set.

martinlippert commented 9 months ago

This build still has the described issue.

Not sure what you mean?

To be clear, since Eclipse 4.30 the launcher has been built using SDK 13 so taller row heights are the norm. eclipse-platform/eclipse.platform.swt#771 was merged on Jan 17 2024. All that patch does is to ensure that row heights stay as they should when a font is set.

I got the impression that the mentioned PR fixes the issue that I illustrated in the screenshots in https://github.com/eclipse-platform/eclipse.platform.ui/issues/1674#issuecomment-1940598475, that is why I thought I should re-test the latest I-build and report back here... ;-)

The screenshot above from using Eclipse 2023-12 (4.30) looks like the old behavior though (smaller row height), so it looks like something has changed for 4.31 builds that causes this change.

Phillipus commented 9 months ago

Since Eclipse 4.30 all row heights on Mac are taller by default. Before https://github.com/eclipse-platform/eclipse.platform.swt/pull/771 changing the font of a Table/Tree/List incorrectly added padding of 1 pixel to the row height (wrong). It now adds 8 pixels to match the increased row height. You are seeing smaller row heights in Eclipse 2023-12 because the patch is not present in that version.

The correct row height is the larger one, because of the changes in the SDK.

Phillipus commented 9 months ago

I'll try to make this clearer...

Please take a look at https://github.com/eclipse-platform/eclipse.platform.swt/issues/677

TL;DR - taller row heights is the new normal.

Phillipus commented 9 months ago

If you want smaller row height you could use -Dorg.eclipse.swt.internal.carbon.smallFonts in your eclipse.ini file.

martinlippert commented 9 months ago

-Dorg.eclipse.swt.internal.carbon.smallFonts

The above screenshot when running on Eclipse 2024-03 M2 is with -Dorg.eclipse.swt.internal.carbon.smallFonts on the eclipse.ini file.

BeckerWdf commented 9 months ago

-Dorg.eclipse.swt.internal.carbon.smallFonts

The above screenshot when running on Eclipse 2024-03 M2 is with -Dorg.eclipse.swt.internal.carbon.smallFonts on the eclipse.ini file.

If you remove -Dorg.eclipse.swt.internal.carbon.smallFonts you should see that you even see less entries on the same screen space in project explorer.

martinlippert commented 9 months ago

Okay, here is a comparison (from the users point of view), first with the smallFonts setting:

A) Eclipse 2023-12 (including -Dorg.eclipse.swt.internal.carbon.smallFonts)

eclipse-2023-12-with-small-fonts

B) Eclipse 2024-03 I-Build (including -Dorg.eclipse.swt.internal.carbon.smallFonts)

eclipse-2024-03-i-build-with-small-fonts

The screenshots for both without -Dorg.eclipse.swt.internal.carbon.smallFonts look the same, so posted here just for the reference:

C) Eclipse 2023-12 (excluding -Dorg.eclipse.swt.internal.carbon.smallFonts)

eclipse-2023-12-without-small-fonts

D) Eclipse 2024-03 I-Build (excluding -Dorg.eclipse.swt.internal.carbon.smallFonts)

eclipse-2023-12-without-small-fonts

I would actually prefer to continue to have the more compressed look of A) for all my views, lists, tress, etc., but it looks like even setting -Dorg.eclipse.swt.internal.carbon.smallFonts doesn't bring that back. Your comment sounds like B) is the "new normal", but I highly doubt that users would prefer B) over A). Is there a way to bring back the look of A) for those elements somehow?

Phillipus commented 9 months ago

I'll try to explain the situation again (with reference to https://github.com/eclipse-platform/eclipse.platform.swt/issues/677)

So the new row heights are actually correct for the new macOS SDK.

If -Dorg.eclipse.swt.internal.carbon.smallFonts is set then Tree#setFont() is always called which in turn calls setItemHeight which sets the smaller item height in Eclipse 4.30 but not in 4.31.

Phillipus commented 9 months ago

I would actually prefer to continue to have the more compressed look of A) for all my views, lists, tress, etc., but it looks like even setting -Dorg.eclipse.swt.internal.carbon.smallFonts doesn't bring that back. Your comment sounds like B) is the "new normal", but I highly doubt that users would prefer B) over A). Is there a way to bring back the look of A) for those elements somehow?

Not without creating inconsistent behaviour. If the row height padding in setItemHeight was set back to 1 pixel then any RCP app, or Eclipse itself, that doesn't set -Dorg.eclipse.swt.internal.carbon.smallFonts (which initially forces a call to setItemHeight) would display the larger row height and the next time setFont was called on that control the item height would shrink (as explained here).

To see this, add System.out.println(getItemHeight()); as the first line in Tree#setItemHeight(). This will output 25.

Phillipus commented 9 months ago

The only thing I can think of to maintain backward compatibility would be to use a vertical padding of 1 pixel if -Dorg.eclipse.swt.internal.carbon.smallFonts is set and 8 otherwise.

Phillipus commented 9 months ago

Please comment and test https://github.com/eclipse-platform/eclipse.platform.swt/pull/1055

The caveat with this is that -Dorg.eclipse.swt.internal.carbon.smallFonts forces a call to setItemHeight which sets the row height smaller. The native initial row height is actually larger (25).

martinlippert commented 9 months ago

The only thing I can think of to maintain backward compatibility would be to use a vertical padding of 1 pixel if -Dorg.eclipse.swt.internal.carbon.smallFonts is set and 8 otherwise.

Yeah, I had the same thought, that would be awesome.

Phillipus commented 9 months ago

The only thing I can think of to maintain backward compatibility would be to use a vertical padding of 1 pixel if -Dorg.eclipse.swt.internal.carbon.smallFonts is set and 8 otherwise.

Yeah, I had the same thought, that would be awesome.

@martinlippert Can you test https://github.com/eclipse-platform/eclipse.platform.swt/pull/1055?

martinlippert commented 9 months ago

The only thing I can think of to maintain backward compatibility would be to use a vertical padding of 1 pixel if -Dorg.eclipse.swt.internal.carbon.smallFonts is set and 8 otherwise.

Yeah, I had the same thought, that would be awesome.

@martinlippert Can you test eclipse-platform/eclipse.platform.swt#1055?

Yeah, happy to give this a try. Any recommendations about the easiest way to build this locally?

Phillipus commented 9 months ago

Any recommendations about the easiest way to build this locally?

You need to clone https://github.com/eclipse-platform/eclipse.platform.swt and import the org.eclipse.swt and binaries/org.eclipse.swt.cocoa.macosx.aarch64 projects into your workspace. You also need to fetch and checkout the PR.

merks commented 9 months ago

@martinlippert

Using https://github.com/eclipse-platform/.github/blob/main/CONTRIBUTING.md#creating-an-eclipse-development-environment fully automates the setup process.

It's a bit of overkill because it includes everything in the SDK, but it lets you launch a runtime workspace (launcher provide) where you can see the impact in context.

You can also go back:

image

Deselect everything:

image

And select only SWT:

image

Phillipus commented 9 months ago

The only thing I can think of to maintain backward compatibility would be to use a vertical padding of 1 pixel if -Dorg.eclipse.swt.internal.carbon.smallFonts is set and 8 otherwise.

Thinking about this more, this workaround is not really true to the intent of the org.eclipse.swt.internal.carbon.smallFonts setting.

With the workaround patch:

So with this patch we're effectively changing the meaning of the smallFonts setting to mean "use a smaller font and use a small row height". If we are to be true to the native OS, then really the larger row height should be used in all cases.

merks commented 9 months ago

I'm not on a Mac but if this issue were Windows I would be super annoyed to suddenly have a lower density of information content in all my views.

@martinlippert

Are other places (other native mac applications) also showing such less dense tables, tree, and lists?

Phillipus commented 9 months ago

I'm not on a Mac but if this issue were Windows I would be super annoyed to suddenly have a lower density of information content in all my views.

But this was actually the case for all native Mac apps when macOS Big Sur (11) was released. Since then native row heights on Mac are increased (if they target the later Mac SDK), as noted here.

I've spent a lot of time thinking about, and experimenting with, this issue and have come to accept this as the new normal. In our RCP app, Archi, we don't set the org.eclipse.swt.internal.carbon.smallFonts setting. Thus, all row heights are now larger on later macOS. We can't change that, it's native, and SWT creates these controls with that increased row height. The problem for us was that controls had the larger row height but if a user changed the font for a control, it magically shrank the row height (because of the 1 pixel instead of 8 pixels in setItemHeight).

Phillipus commented 9 months ago

Are other places (other native mac applications) also showing such less dense tables, tree, and lists?

Yes. Even the SWT-based app "SmartGit".

Before:

old

After:

new

To be clear, if you don't use the org.eclipse.swt.internal.carbon.smallFonts setting and run Eclipse 4.30 on the latest macOS you'll see the increased row height (except for a few owner-draw tables and trees). This is the native row height. This Snippet proves this.

BeckerWdf commented 9 months ago

If we are to be true to the native OS, then really the larger row height should be used in all cases.

I can only second that. The operating system dictates how the high a line in a tree is. We should not try to circumvent this. I think it's just a matter of what you are used to. When I first saw the differences I also thought that the old one was better. But the longer I see the new one I find the old one wrong - it's just too packed and too less whitespace.

martinlippert commented 9 months ago

Any recommendations about the easiest way to build this locally?

You need to clone https://github.com/eclipse-platform/eclipse.platform.swt and import the org.eclipse.swt and binaries/org.eclipse.swt.cocoa.macosx.aarch64 projects into your workspace. You also need to fetch and checkout the PR.

Did all that, workspace looks okay, but starting up the runtime workbench gives me an error, complaining about the jnilib file

eclipse.platform.swt/binaries/org.eclipse.swt.cocoa.macosx.aarch64/libswt-cocoa-4964r8.jnilib' (not a mach-o file)
Phillipus commented 9 months ago

For me, it's a bit frustrating to explain all of this because the underlying cause is not simple. So I tend to repeat myself and continue to add more comments in the hope of clarifying the issue (even to myself). So...

Phillipus commented 9 months ago

Did all that, workspace looks okay, but starting up the runtime workbench gives me an error, complaining about the jnilib file

eclipse.platform.swt/binaries/org.eclipse.swt.cocoa.macosx.aarch64/libswt-cocoa-4964r8.jnilib' (not a mach-o file)

The SWT git repo uses git LFS so you'll need to configure for LFS. This was changed here.

merks commented 9 months ago

@martinlippert

There is a reason why I suggested using the setup. There are a bunch of "magical things" that need to be done that some folks know how to do, but it's poorly document, so mostly humans will make mistakes.

martinlippert commented 9 months ago

I'm not on a Mac but if this issue were Windows I would be super annoyed to suddenly have a lower density of information content in all my views.

@martinlippert

Are other places (other native mac applications) also showing such less dense tables, tree, and lists?

Here is a screenshot from XCode, running on the same machine. You will notice that the font is slightly larger than in Eclipse (with the small font property)

xcode-file-explorer-view

Interesting outcome:

Phillipus commented 9 months ago

If we are to be true to the native OS, then really the larger row height should be used in all cases.

I can only second that. The operating system dictates how the high a line in a tree is. We should not try to circumvent this. I think it's just a matter of what you are used to. When I first saw the differences I also thought that the old one was better. But the longer I see the new one I find the old one wrong - it's just too packed and too less whitespace.

I agree. My vote is to not merge the draft PR I created.

BeckerWdf commented 9 months ago

You will notice that the font is slightly larger

I think that this is true but to be honest I cannot see that. To see that really I would like to see screenshots next to each other.

Phillipus commented 9 months ago

Checking the row heights on native Mac apps is not a good indicator. We don't know what an app is doing and whether they have custom drawing for controls, or if it targets a newer or old Mac SDK. In fact, even in Eclipse itself, you'll see some controls in Preferences that have smaller row heights because they do some custom drawing. What we can say with certainty is that when SWT creates a new tree/table/list the row height is natively larger (the smallFonts setting clobbered that in setItemHeight)

Phillipus commented 9 months ago

I'm running out of steam with this issue. ;-). Can we have a consensus?

Thoughts?

martinlippert commented 9 months ago

You will notice that the font is slightly larger

I think that this is true but to be honest I cannot see that. To see that really I would like to see screenshots next to each other.

Since you asked for it, here we are. I shot a few side-by-side screenshots. Each one is comparing 2024-03 CI with small fonts on the left, without small fonts in the middle, and with some other macOS app on the right.

Compared to Eclipse 2023-12 (where 2023-12 has smallfonts enabled):

side-by-side-eclipse

Compared to VSCode (default settings in VSCode, no zoom):

side-by-side-vscode

Compared to Xcode:

side-by-side-xcode

Compared to macOS native finder app:

side-by-side-finder
martinlippert commented 9 months ago

I don't know a lot about the implementation details and I am grateful for all the technical details provided here and the discussion around that, very very much appreciated. I am just looking at this from the point of view of the developers using Eclipse as their IDE on macOS and this change that is coming to them, having the quote from Ed in mind:

I'm not on a Mac but if this issue were Windows I would be super annoyed to suddenly have a lower density of information content in all my views.

Phillipus commented 9 months ago

As I said earlier, comparing with XCode or Finder is meaningless.

Please take another look at the first comment and snippet here. When a SWT Tree/Table/List is created its row height is 25 pixels (assuming the standard default macOS font). That is correct for macOS SDK creating a new control for SWT. Now in order to maintain the correct padding in a row when setItemHeight is called (triggered by setFont) the padding has to be 8 pixels, not 1 pixel as it was before.

I don't know how I can say it any other way - the old value of 1 pixel in setItemHeight is wrong for later macOS versions.

Phillipus commented 9 months ago

I don't know a lot about the implementation details and I am grateful for all the technical details provided here and the discussion around that, very very much appreciated. I am just looking at this from the point of view of the developers using Eclipse as their IDE on macOS and this change that is coming to them, having the quote from Ed in mind:

I'm not on a Mac but if this issue were Windows I would be super annoyed to suddenly have a lower density of information content in all my views.

Then the blame has to go to Apple. ;-)

martinlippert commented 9 months ago

The only thing I can think of to maintain backward compatibility would be to use a vertical padding of 1 pixel if -Dorg.eclipse.swt.internal.carbon.smallFonts is set and 8 otherwise.

Yeah, I had the same thought, that would be awesome.

@martinlippert Can you test eclipse-platform/eclipse.platform.swt#1055?

Thanks a lot again for helping me setting up an environment to test this, I finally succeeded... :-) Many many thanks again to you all, much appreciated.

And yes, I can confirm that this brings back the look of Eclipse 2023-12 when running with the smallfonts setting enabled (as all the Eclipse IDE packages do, as far as I am aware of).

I know there are different opinions about what the right approach here is and I do understand the points being made. My personal vote would be to merge this PR in [eclipse-platform/eclipse.platform.swt#1055](https://github.com/eclipse-platform/eclipse.platform.swt/pull/1055.

martinlippert commented 9 months ago

(and I promise, I will not post any screenshots anymore) 😂

Phillipus commented 9 months ago

I recognise that the increased row height might be unwelcome for some Mac users. But consider those users who don't use the smallFonts setting either in Eclipse or an SWT based app. Our RCP app has never used that setting and so our users are going to get a surprise in our next version when row heights are bigger. There's nothing I can do about that.

merks commented 9 months ago

It's rather late in the development cycle to suggest yet another system property, and if we don't do that this release cycle, everyone will just get used to it such that changing it again would be kind of strange.

I have no strong opinion of what is right or best for the general public. There are good arguments made in favor of doing what is natively intended...

Phillipus commented 9 months ago

to suggest yet another system property

I already thought of, and tested, that idea.

In the Display class add:

boolean smallItems;

// Initialise it somewhere
smallItems = System.getProperty("org.eclipse.swt.smallItems") != null;

In Table:


// Have to initialise item height in constructor
public Table (Composite parent, int style) {
    super (parent, checkStyle (style));
    setItemHeight(null, null, true);
}

// Check display.smallItems here...
void setItemHeight (Image image, NSFont font, boolean set) {
    if (font == null) font = getFont ().handle;
    double ascent = font.ascender ();
    double descent = -font.descender () + font.leading ();
    int height = (int)Math.ceil (ascent + descent) + (display.smallItems ? 1 : VERTICAL_CELL_PADDING);
...
Phillipus commented 9 months ago

Are there other Mac devs we can ping to get an opinion? @HannesWell @HeikoKlare do you have Mac access?

akurtakov commented 9 months ago

@lshanmug wdyt?

HeikoKlare commented 9 months ago

Even though I have Mac access, I am not a "real Mac user", as I only use it for Eclipse development validation purposes. So probably my opinion on this is not that relevant and I actually do not have a strong opinion on the specific issue here.

I see a more general issue here: there is no specification and no common agreement on what is the "baseline" for how the Eclipse/SWT UI should look like on each OS. I understand every point of view or preference, be it that someone is used to a specific look & feel, be it what the underlying framework/OS etc. gives you, or be it whatever makes "most sense" for some other reasons (like a cleaner look with larger spaces or more information with smaller spaces). I don't see that one can be objectively better than the other, but it will just be about personal preference. Mine is as follows: I do not really see anything feeling "native" anymore (which once was the baseline for SWT look & feel). In particular on Windows, Eclipse, even though using native graphics, feels like the only application looking that way (as the native API is 20+ years old). I am not sure how the feeling is for Linux or Mac users, but there are probably more applications using GTK and Cocoa looking kind of similiar. I would definitely prefer if we had an "Eclipse look & feel" rather than a "native look & feel". But that would require users of some OS leave behind what they are used to, since the look & feel is quite different between the OS now, in particular regarding spaces that are small on Windows, large on GTK and obviously changing on Mac.

Sorry for only being able to make such a general comment. Trying to apply it this issue I would say that sticking to existing behavior (and maybe making it more configurable) is more valuable than adopting something "native".

HannesWell commented 9 months ago

Are there other Mac devs we can ping to get an opinion? @HannesWell @HeikoKlare do you have Mac access?

Sorry I don't have Mac access, I'm on Windows at home and on Linux at work.

Phillipus commented 9 months ago

@HeikoKlare Thanks for your feedback.

The underlying issue here is not necessarily to change things but to be consistent. Again, I refer to this comment and the Snippet there. When a Table/Tree/List is created in SWT it has the larger row height (because it is created natively by the OS). Previously, calling setFont() (which in turn calls setItemHeight()) on the control would reduce the row height because of using 1 pixel as vertical padding. Now, to be consistent with the larger initial row height, we use a value of 8 pixels.

Even before the change to 8 pixels in setItemHeight() if you create an RCP application that does not call setFont on the tree/table/list you get the bigger row height whether you like it or not. What you don't want is the row height magically shrinking when and if you do call setFont (as is the case if -Dorg.eclipse.swt.internal.carbon.smallFonts is set).

What we can't do is to go back to using 1 pixel as the vertical padding for the reasons just stated. Either ensure these controls are created with a smaller initial height (I don't know how you'd do that as the control is created natively) or leave this as it is.

And see https://wiki.lazarus.freepascal.org/macOS_Big_Sur_changes_for_developers#Table_Views