eclipse-platform / eclipse.platform.swt

Eclipse SWT
https://www.eclipse.org/swt/
Eclipse Public License 2.0
118 stars 137 forks source link

[macOS Silicon] Table and Tree item height gets smaller when setting the font #677

Closed Phillipus closed 9 months ago

Phillipus commented 1 year ago

~This occurs on Mac Silicon (aarch64) only~, presumably because Eclipse on that platform uses a later Apple SDK. I've tested on Mac Mini M1 with Monterey and Ventura.

Edit - also occurs on Mac Intel. It depends on which macOS SDK is used to create the launcher binary (eclipse or java).

Setting a table or tree font to the same font reduces the item height.

  1. Create a simple table with some rows

  2. The item height of the rows is 25:

    Before
  3. Set the table's font to its current font (table.setFont(table.getFont()))

  4. The item height of the rows is now 18:

    After

Expected behaviour: should use the height of the target SDK. On aarch64 this is the 25 pixel item height. On Intel it is 18.

Phillipus commented 1 year ago

Snippet to reproduce (ensure not to use the argument -Dorg.eclipse.swt.internal.carbon.smallFonts):

import static org.eclipse.swt.events.SelectionListener.widgetSelectedAdapter;

import org.eclipse.jface.layout.GridDataFactory;
import org.eclipse.swt.SWT;
import org.eclipse.swt.layout.GridData;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Button;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Label;
import org.eclipse.swt.widgets.Shell;
import org.eclipse.swt.widgets.Table;
import org.eclipse.swt.widgets.TableItem;

public class TableFont {

    public static void main(String[] args) {
        Display display = new Display();
        Shell shell = new Shell(display);
        shell.setText("Table Font");
        shell.setLayout(new GridLayout());

        Table table = new Table(shell, SWT.BORDER | SWT.V_SCROLL | SWT.H_SCROLL);
        GridDataFactory.create(GridData.FILL_HORIZONTAL).applyTo(table);

        for(int i = 0; i < 11; i++) {
            TableItem item = new TableItem(table, 0);
            item.setText("Item " + i);
        }

        Label label1 = new Label(shell, SWT.NONE);
        Label label2 = new Label(shell, SWT.NONE);
        updateLabels(table, label1, label2);

        Button button = new Button(shell, SWT.PUSH);
        button.setText("Set font");
        button.addSelectionListener(widgetSelectedAdapter(e -> {
            // Set table font to the existing table font
            table.setFont(table.getFont());
            updateLabels(table, label1, label2);
        }));

        shell.setSize(500, 450);
        shell.open();

        while(!shell.isDisposed()) {
            if(!display.readAndDispatch()) display.sleep();
        }

        display.dispose();
    }

    private static void updateLabels(Table table, Label label1, Label label2) {
        label1.setText("Table font: " + table.getFont().getFontData()[0]);
        label2.setText("Table item height: " + table.getItemHeight());
    }
}
elsazac commented 1 year ago

I am able to reproduce the issue in Apple M1 Pro Ventura 13.4

Eclipse SDK Version: 2023-06 (4.28) Build id: I20230605-0440 OS: Mac OS X, v.13.4, aarch64 / cocoa Java vendor: Eclipse Adoptium Java runtime version: 17.0.6+10 Java version: 17.0.6

elsazac commented 1 year ago

@Phillipus Can you also add what should be the expected behaviour?

Phillipus commented 1 year ago

Can you also add what should be the expected behaviour?

Added to OP.

elsazac commented 1 year ago

I am investigating on this issue and these are my findings. https://github.com/eclipse-platform/eclipse.platform.swt/blob/1a939af1dfb0d050f91e5357887fa068f336da23/bundles/org.eclipse.swt/Eclipse%20SWT/cocoa/org/eclipse/swt/widgets/Table.java#L2832-L2833

If I change the above condition either to one of the below , we see the expected behaviour but not sure about the exact purpose of set. Does someone has any idea on when exactly set needs to be evaluated?

if (widget.rowHeight () < height) {
        widget.setRowHeight (height);
if (set && widget.rowHeight () < height) {
        widget.setRowHeight (height);
Phillipus commented 1 year ago

The problem also occurs for the SWT List widget:

import static org.eclipse.swt.events.SelectionListener.widgetSelectedAdapter;

import org.eclipse.jface.layout.GridDataFactory;
import org.eclipse.swt.SWT;
import org.eclipse.swt.layout.GridData;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Button;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Label;
import org.eclipse.swt.widgets.List;
import org.eclipse.swt.widgets.Shell;

public class ListRowHeight {

    public static void main(String[] args) {
        Display display = new Display();
        Shell shell = new Shell(display);
        shell.setText("List Row Height");
        shell.setLayout(new GridLayout());

        List list = new List(shell, SWT.BORDER | SWT.V_SCROLL | SWT.H_SCROLL);
        GridDataFactory.create(GridData.FILL_HORIZONTAL).applyTo(list);

        String[] items = new String[10];
        for(int i = 0; i < 10; i++) {
            items[i] = "Item " + i;
        }
        list.setItems(items);

        Label label1 = new Label(shell, SWT.NONE);
        Label label2 = new Label(shell, SWT.NONE);
        updateLabels(list, label1, label2);

        Button button = new Button(shell, SWT.PUSH);
        button.setText("Set font");
        button.addSelectionListener(widgetSelectedAdapter(e -> {
            // Set list font to the existing table font
            list.setFont(list.getFont());
            updateLabels(list, label1, label2);
        }));

        shell.setSize(500, 450);
        shell.open();

        while(!shell.isDisposed()) {
            if(!display.readAndDispatch()) display.sleep();
        }

        display.dispose();
    }

    private static void updateLabels(List list, Label label1, Label label2) {
        label1.setText("List font: " + list.getFont().getFontData()[0]);
        label2.setText("List item height: " + list.getItemHeight());
    }
}
Phillipus commented 1 year ago

Table and Tree row heights are initially set to 25 on Mac aarch64 and then set to 18 when resetting the font. With a List widget the row height is initially 24 and then 17 when resetting the font.

I have a doubt about whether the initial row height is wrong and should be as it is on intel Mac. But there are these earlier bug reports:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=575696

https://bugs.eclipse.org/bugs/show_bug.cgi?id=574932

and this comment from @sravanlakkimsetti:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=574932#c2

This is because eclipse launcher SDK version On x86_64 SDK used is 10.14 On aarch64 SDK used is 11 But the way forward is to use latest SDK and update SWT to MacOS 11 SDK.

So I guess the higher row height values are correct for aarch64?

elsazac commented 11 months ago

@Phillipus

I have verified some tables in Mac and I can see the table height in Mac being aligned with table in the snippet(with height 25)

Screenshot 2023-12-06 at 1 11 26 PM

I have gone through the bugzilla links and I see that the increased item height is the new native behaviour on BigSur. row height is 24pt in BigSur and 17pt in macOS 10.15. https://bugs.eclipse.org/bugs/show_bug.cgi?id=574932#c40

the snippet is initially set to item height 25 because the default item height in aarch is 25 unless we override and change it to 18

Also can you please try some tables in x86_64?

tmssngr commented 7 months ago

What is the expected result of the last commit 611accb8? For our applications users complain that the line height is much higher with the latest SWT build than with previous versions. However, I don't see any change with this commit.

merks commented 7 months ago

I assume that you need to ensure that this returns false:

Boolean.parseBoolean(System.getProperty("org.eclipse.swt.internal.cocoa.useNativeItemHeight", "true"));
kohlschuetter commented 7 months ago

@tmssngr For apps that use -Dorg.eclipse.swt.internal.carbon.smallFonts (like Eclipse IDE), the change in https://github.com/eclipse-platform/eclipse.platform.swt/commit/611accb844fb525efb3727f7fe8e2e7a3472debf brings back the old behavior, as an immediate remedy to the problem without the need of further configuration.

For apps that don't specify smallFonts, the appearance depends on their macOS version. Newer versions of macOS have a taller native height that matters here. To consistently bring back the old behavior, specify -Dorg.eclipse.swt.internal.cocoa.useNativeItemHeight=false when launching your app.

tmssngr commented 7 months ago

I'm not sure I fully understand all. Older versions of SWT (e.g. used in SmartGit 23.1.2) show table and tree rows quite narrow on my M1 Mac Mini with macOS 14.4. Newer versions of SWT (e.g. used in SmartGit 24.1 preview) show a much larger row height. We don't have the mentioned properties set. Do you mean, that the larger row height in newer SWT builds is now the correct behavior for macOS 14.4, and if a user wants to have a narrower row-height, he needs to set the mentioned properties?

kohlschuetter commented 7 months ago

@tmssngr Yes, I'm afraid that's the case, but setting a property will do the trick.

Anyhow, I'm just a Eclipse IDE user whose eyes couldn't bear looking at these oversized list items, so I'm glad I could contribute to resolving this issue.

Phillipus commented 7 months ago

Older versions of SWT (e.g. used in SmartGit 23.1.2) show table and tree rows quite narrow on my M1 Mac Mini with macOS 14.4.

@tmssngr Take a look at SmartGit 23.1.2 Settings tree. You'll see bigger row heights. See https://github.com/eclipse-platform/eclipse.platform.ui/issues/1674#issuecomment-1954167516

There always has been bigger row heights in SWT since the later macOS versions. However, when a call to setItemHeight is made (by setting the control's font or image or something else), the row heights shrink, as the snippet here demonstrates. The commit of @kohlschuetter corrects this anomaly.

tmssngr commented 7 months ago

@Phillipus Ah, so there was even another difference between plain and owner-drawn trees. On MacOS we were setting the display's system font (a slightly different one) to have all digits the same width. So it looks like our problem of the increased row-heights is a slightly different problem than this one covered in this ticket. Sorry for the noise.

Phillipus commented 7 months ago

@tmssngr

It depends on how a Tree/Table/List is used.

As the snippet in this issue shows, when a Tree/Table/List control is first created it has the native row height (larger on later macOS). Thereafter, a change to the control's font/image/whatever triggers a re-calculation of the row height. This is where the row height shrinks because it doesn't take into account the initial native row height when calculating the row height based upon the current font.

Eclipse uses the -Dorg.eclipse.swt.internal.carbon.smallFonts property and this triggers the recalculation of row heights immediately after the control is created. This is why Eclipse users always saw smaller row heights.

Now that the recalculation anomaly has been fixed there is consistency between the initial row heights and the re-calculated ones, so no more unexpected row height shrinkage. However, this means that row heights are bigger by default because that's how they are initially created.

I think SmartGit Preview is seeing bigger row heights because of the erroneous padding added in this commit.

You could try using the latest SWT build with the new fix in and add the -Dorg.eclipse.swt.internal.cocoa.useNativeItemHeight=false setting to SmartGit's Info.plist file (I think that's where you set these properties?).

Phillipus commented 7 months ago

@tmssngr My assumptions are correct.

  1. I downloaded SmartGit Preview 2 for Mac aarch64 and saw the increased row heights (except for the owner draw ones)
  2. I replaced SmartGit.app/Contents/Resources/Java/org.eclipse.swt.cocoa.macosx.aarch64.jar with the latest SWT build
  3. I added -Dorg.eclipse.swt.internal.cocoa.useNativeItemHeight=false to the Info.plist file in the JVMOptions section
  4. Bingo! smaller row heights (and consistent ones, too, so no more anomalous large rows in the Settings tree in SmartGit 23.1.2)

Before:

Screenshot 2024-03-21 at 15 53 37

After:

Screenshot 2024-03-21 at 15 54 13
tmssngr commented 7 months ago

Thanks, @Phillipus, this helped to fix it. :)

odrotbohm commented 4 months ago

I might be the odd one out here, but is there a way to get both small fonts, but the bigger spacing back after the fix? 😇 I see my ini file having -Dorg.eclipse.swt.internal.carbon.smallFonts set. I've tried adding -Dorg.eclipse.swt.internal.cocoa.useNativeItemHeight=true (I also tried false) to get the bigger spacing back, but don't see any effect.

Phillipus commented 4 months ago

is there a way to get both small fonts, but the bigger spacing back after the fix?

No, because the native item height is smaller when small fonts are set.

odrotbohm commented 4 months ago

That's a pity, but thanks! 😕

marcleidner commented 4 months ago

Issue is fixed in 2024-06 (4.32.0), for me at least.