Fixing the Item height for Table and Tree when the default font is set #771

elsazac · 2023-08-18T17:17:33Z

Fixes: #677

A new function named as getInset() is created in Table.java. This function calculates the inset value of a cell in a table for aarch64 architecture. The inset value of a row in a table is calculated by subtracting the height of a cell from a frame.This calculated inset value is added to the height in setItemHeight() function.

The inset value of 1 is retained for x86_64 architecture.

/cc @lshanmug

github-actions · 2023-08-18T17:32:46Z

Test Results

299 files ±0 299 suites ±0 5m 49s ⏱️ -8s
4 098 tests ±0 4 090 ✅ +1 8 💤 ±0 0 ❌ - 1
12 206 runs ±0 12 133 ✅ +3 73 💤 ±0 0 ❌ - 3

Results for commit 99d3dc1. ± Comparison against base commit 3e43811.

♻️ This comment has been updated with latest results.

Phillipus · 2023-08-19T09:59:47Z

Hi, I tested the patch. If there are no items the heights are too small:

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.Display;
import org.eclipse.swt.widgets.Shell;
import org.eclipse.swt.widgets.Table;
import org.eclipse.swt.widgets.Tree;

public class FontTest {

    public static void main(String[] args) {
        Display display = new Display();
        Shell shell = new Shell(display);
        shell.setText("Tree & Table Font");
        shell.setLayout(new GridLayout());
        
        Tree tree = new Tree(shell, SWT.BORDER | SWT.V_SCROLL | SWT.H_SCROLL);
        GridDataFactory.create(GridData.FILL_BOTH).applyTo(tree);
        tree.setLinesVisible(true);
        
        Table table = new Table(shell, SWT.BORDER | SWT.V_SCROLL | SWT.H_SCROLL);
        GridDataFactory.create(GridData.FILL_BOTH).applyTo(table);
        table.setLinesVisible(true);
        
        shell.setSize(500, 450);
        shell.open();
        
        tree.setFont(tree.getFont());
        table.setFont(table.getFont());
        
        while(!shell.isDisposed()) {
            if(!display.readAndDispatch()) display.sleep();
        }
        
        display.dispose();
    }
}

elsazac · 2023-10-04T08:56:38Z

@Phillipus

Can you test this patch?

Phillipus · 2023-10-04T12:43:01Z

@Phillipus

Can you test this patch?

Hi @elsazac,

I've tested the patch on Mac M1 Ventura 13.6 and it does fix the row height problem testing with the Snippet in #771 and also in our RCP application.

I remain puzzled, though, how and where the initial row height is set (25 pixels) before setting the font of the table or tree.

Phillipus · 2023-10-04T13:11:28Z

@elsazac The problem also occurs for the List component. I've added a Snippet to the original issue here. This will require a similar fix at line 1182 here:

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/List.java

Lines 1178 to 1184 in 17dcab0

    
           void setFont (NSFont font) { 
        
           	super.setFont (font); 
        
           	double ascent = font.ascender (); 
        
           	double descent = -font.descender () + font.leading (); 
        
           	((NSTableView)view).setRowHeight ((int)Math.ceil (ascent + descent) + 1); 
        
           	setScrollWidth(); 
        
           }

Phillipus · 2023-10-04T13:16:07Z

But why is the row height initially greater (25) on Silicon than on Intel (18)? Which is the correct height on Silicon? Does anyone know?

elsazac · 2023-10-05T11:19:07Z

@elsazac The problem also occurs for the List component. I've added a Snippet to the original issue here. This will require a similar fix at line 1182 here:

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/List.java

Lines 1178 to 1184 in 17dcab0

void setFont (NSFont font) {

super.setFont (font);

double ascent = font.ascender ();

double descent = -font.descender () + font.leading ();

((NSTableView)view).setRowHeight ((int)Math.ceil (ascent + descent) + 1);

setScrollWidth();

}

@Phillipus I have made a similar change in List as well.
Could you test it?

Phillipus · 2023-10-06T09:41:45Z

@Phillipus I have made a similar change in List as well. Could you test it?

Hi @elsazac - I tested and the fix works OK, thank-you.

But before committing this patch I have a doubt concerning what should be the true initial row height. See comment here - #677 (comment)

elsazac · 2023-12-12T11:28:44Z

@Phillipus I have made a similar change in List as well. Could you test it?

Hi @elsazac - I tested and the fix works OK, thank-you.

But before committing this patch I have a doubt concerning what should be the true initial row height. See comment here - #677 (comment)

@Phillipus I have responded in the linked issue. Can you review and help progress this PR?

merks · 2024-01-10T08:01:15Z

It looks like this current incarnation sprinkles the appropriate magic numbers to fix the problem. I think #677 (comment) answers the outstanding problem.

@Phillipus Are there any outstanding concerns with this PR?

Phillipus · 2024-01-10T11:02:31Z

@Phillipus Are there any outstanding concerns with this PR?

@merks Yes.

The larger item height is seen on macOS aarch64 but not on x86_64 when Eclipse or an RCP app is run from its launcher. As explained by @sravanlakkimsetti in 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.

However, I recently installed a new JDK on an Intel Mac (x86_64) that has been compiled against Mac SDK 11.1 and when launching a child Eclipse, or our RCP app, the item height is also bigger. This is because when running a child Eclipse it uses the java binary, not the launcher, and this one has been compiled using 11.1 SDK.

So the inconsistency is due not to OS architecture but on the linked SDK version of the launcher or the java binary and perhaps we should consider this needs to be addressed by rebuilding the x86_64 launcher against MacOS 11 SDK:

But the way forward is to use latest SDK and update SWT to MacOS 11 SDK.

I don't know how that is done (is it the equinox binaries project?)

If this is done, however, note that Eclipse and RCP apps will not be able to run on macOS < 11 (before Big Sur)

merks · 2024-01-10T11:13:55Z

I see! Thank you for the detailed information!! Magic numbers never do feel quite right. 😱

sravanlakkimsetti · 2024-01-10T11:38:03Z

I don't know how that is done (is it the equinox binaries project?)

I prefer not to have different code for x86 and arm implementation. My suggestion would be to use same masOS sdk for both versions of launcher.

Here is the section from equinox project where we switch between macos 11 and macos 10.14 sdk for arm and intel architectures

Removing this if condition and and rebuilding launcher should make make both versions of launcher work in the same way.

If you need to upgrade to latest version of macos sdk you'll need to work with foundation to get it upgraded on the macos build machine

Phillipus · 2024-01-10T12:11:23Z

@sravanlakkimsetti Thanks for that information!

I notice that you did already do this here:

eclipse-equinox/equinox@0dad463

but reverted this:

eclipse-equinox/equinox@4db53c0

Did you revert because it was no longer needed due to #452 fixing itself?

Phillipus · 2024-01-10T12:15:44Z

Opened eclipse-equinox/equinox#473

sravanlakkimsetti · 2024-01-10T12:32:24Z

@sravanlakkimsetti Thanks for that information!

I notice that you did already do this here:

eclipse-equinox/equinox@0dad463

but reverted this:

eclipse-equinox/equinox@4db53c0

Did you revert because it was no longer needed due to #452 fixing itself?

Not exactly. The problem was slightly complicated. At that time the supported java version was built with 10.14. When we moved to Macos 11, Eclipse had one behavior and the child, eclipse launched using run menu, had different behavior. For this reason we wanted to wait till java moved to Macos 11 before we move.

Now that java also moved to Macos 11 we can move mow. Lets reinstate eclipse-equinox/equinox@0dad463

Phillipus · 2024-01-10T12:39:04Z

Lets reinstate eclipse-equinox/equinox@0dad463

Could you do that?

sravanlakkimsetti · 2024-01-10T13:08:43Z

Lets reinstate eclipse-equinox/equinox@0dad463

Could you do that?

Sure will take it up

Phillipus · 2024-01-10T13:20:51Z

Now that java also moved to Macos 11

For the Temurin version of Java this was a very recent move.

Temurin Java 17 moved to SDK 11.1 with version jdk-17.0.9+9 - October 19 2023

Temurin Java 21 moved to SDK 11.1 with version jdk-21.0.1+12 - October 24 2023

Phillipus · 2024-01-10T13:57:04Z

Something to be aware of regarding using SDK 11 on the macOS Intel launcher in the future:

This patch will need to change from:

int height = (int)Math.ceil (ascent + descent) + ((OS.IS_X86_64) ? 1 : 8);

to

int height = (int)Math.ceil (ascent + descent) + 8;

And this means that an end user will also see the increased item height if they are using an older JDK that was compiled with 10.14.

Phillipus · 2024-01-12T14:24:54Z

It turns out that the Mac x86_64 launcher has been built with SDK 13 since around Eclipse 4.30. So we don't need to wait.

@elsazac Could you update your patch by removing the OS.IS_X86_64 condition so it applies to both x86_64 and aarch64, please?

elsazac · 2024-01-12T15:17:21Z

@elsazac Could you update your patch by removing the OS.IS_X86_64 condition so it applies to both x86_64 and aarch64, please?

done.

Phillipus · 2024-01-12T15:52:02Z

done.

Thanks. I tested this on both x86_64 and aarch64 and it works OK.

However, a user will see discrepancies in Item height if they are launching a child Eclipse/RCP app using an older Java version (one compiled with Mac SDK 10.14). I guess in this case they should update to latest version.

I'm happy with this PR now. I don't know if it needs more approval or who should commit it.

Phillipus · 2024-01-15T18:03:57Z

Is this ready to be merged now?

Phillipus · 2024-01-15T18:11:10Z

Be aware that because of the increased item height in Lists, Tables and Trees, text is not vertically centered in the List component (run this Snippet to see) nor in Cell Editors (TableEditor and TreeEditor). It looks kind of ugly but I guess that would need further investigation (I tried but failed)

Phillipus · 2024-01-16T13:16:46Z

@elsazac In three places a value of 8 is added (it was 1). Do you think it would be a good idea to create a static constant in each class for this? Something like the existing static final int CELL_GAP = 1 but with a new name maybe CELL_PADDING)? And add a comment to explain what it is for?

elsazac · 2024-01-17T07:01:44Z

@elsazac In three places a value of 8 is added (it was 1). Do you think it would be a good idea to create a static constant in each class for this? Something like the existing static final int CELL_GAP = 1 but with a new name maybe CELL_PADDING)? And add a comment to explain what it is for?

I have added the changes and squashed the commits. Please have a look.

merks · 2024-01-17T07:18:23Z

Nit picky detail... The comment Row height for xxx item is not really a correct/accurate description. This is padding that is added to the height as determined by the font metrics, so Row height padding for ...

X_86_64 the hardcoded inset value was 1.It is retained as such.Since aarch64 was introduced at a very later time the new table format was introduced with extra paddings.So the appropriate value to get the same font would be 8.

elsazac · 2024-01-17T09:02:00Z

Nit picky detail... The comment Row height for xxx item is not really a correct/accurate description. This is padding that is added to the height as determined by the font metrics, so Row height padding for ...

I have updated the comments.

merks · 2024-01-17T09:05:36Z

Yes, that's a better more descriptive name too! 😄

Phillipus · 2024-01-17T09:33:42Z

Thanks @elsazac for your work on this, and thanks @merks for the feedback and merge. :-)

elsazac requested a review from lshanmug as a code owner August 18, 2023 17:17

elsazac force-pushed the TableFont branch from 29535b9 to e7914d4 Compare August 18, 2023 17:20

elsazac changed the title ~~Fixing the Item height for Table when the default font is set~~ Fixing the Item height for Table and Tree when the default font is set Aug 18, 2023

elsazac force-pushed the TableFont branch from e7914d4 to ac27afb Compare August 31, 2023 16:28

elsazac force-pushed the TableFont branch from e9160cf to 6514629 Compare January 10, 2024 04:43

Phillipus mentioned this pull request Jan 10, 2024

Switch to macOS sdk version 11 eclipse-equinox/equinox#473

Closed

sravanlakkimsetti force-pushed the TableFont branch from 526a37e to 5fcb679 Compare January 12, 2024 15:56

sravanlakkimsetti approved these changes Jan 12, 2024

View reviewed changes

Phillipus mentioned this pull request Jan 16, 2024

Not enough contributors #961

Open

elsazac force-pushed the TableFont branch from 5fcb679 to 2d1da63 Compare January 17, 2024 06:43

elsazac force-pushed the TableFont branch from 2d1da63 to 670c578 Compare January 17, 2024 08:58

Fixes: eclipse-platform#677 For

99d3dc1

X_86_64 the hardcoded inset value was 1.It is retained as such.Since aarch64 was introduced at a very later time the new table format was introduced with extra paddings.So the appropriate value to get the same font would be 8.

elsazac force-pushed the TableFont branch from 670c578 to 99d3dc1 Compare January 17, 2024 09:00

merks merged commit 0032340 into eclipse-platform:master Jan 17, 2024
13 checks passed

BeckerWdf mentioned this pull request Feb 13, 2024

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

Closed

Phillipus mentioned this pull request Mar 20, 2024

Test PR base on PR #1117 #1126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing the Item height for Table and Tree when the default font is set #771

Fixing the Item height for Table and Tree when the default font is set #771

elsazac commented Aug 18, 2023 •

edited

Loading

github-actions bot commented Aug 18, 2023 •

edited

Loading

Phillipus commented Aug 19, 2023

elsazac commented Oct 4, 2023

Phillipus commented Oct 4, 2023

Phillipus commented Oct 4, 2023

Phillipus commented Oct 4, 2023

elsazac commented Oct 5, 2023

Phillipus commented Oct 6, 2023

elsazac commented Dec 12, 2023

merks commented Jan 10, 2024

Phillipus commented Jan 10, 2024

merks commented Jan 10, 2024

sravanlakkimsetti commented Jan 10, 2024

Phillipus commented Jan 10, 2024 •

edited

Loading

Phillipus commented Jan 10, 2024

sravanlakkimsetti commented Jan 10, 2024

Phillipus commented Jan 10, 2024

sravanlakkimsetti commented Jan 10, 2024

Phillipus commented Jan 10, 2024

Phillipus commented Jan 10, 2024

Phillipus commented Jan 12, 2024

elsazac commented Jan 12, 2024

Phillipus commented Jan 12, 2024 •

edited

Loading

Phillipus commented Jan 15, 2024

Phillipus commented Jan 15, 2024 •

edited

Loading

Phillipus commented Jan 16, 2024 •

edited

Loading

elsazac commented Jan 17, 2024

merks commented Jan 17, 2024

elsazac commented Jan 17, 2024

merks commented Jan 17, 2024

Phillipus commented Jan 17, 2024

Fixing the Item height for Table and Tree when the default font is set #771

Fixing the Item height for Table and Tree when the default font is set #771

Conversation

elsazac commented Aug 18, 2023 • edited Loading

github-actions bot commented Aug 18, 2023 • edited Loading

Test Results

Phillipus commented Aug 19, 2023

elsazac commented Oct 4, 2023

Phillipus commented Oct 4, 2023

Phillipus commented Oct 4, 2023

Phillipus commented Oct 4, 2023

elsazac commented Oct 5, 2023

Phillipus commented Oct 6, 2023

elsazac commented Dec 12, 2023

merks commented Jan 10, 2024

Phillipus commented Jan 10, 2024

merks commented Jan 10, 2024

sravanlakkimsetti commented Jan 10, 2024

Phillipus commented Jan 10, 2024 • edited Loading

Phillipus commented Jan 10, 2024

sravanlakkimsetti commented Jan 10, 2024

Phillipus commented Jan 10, 2024

sravanlakkimsetti commented Jan 10, 2024

Phillipus commented Jan 10, 2024

Phillipus commented Jan 10, 2024

Phillipus commented Jan 12, 2024

elsazac commented Jan 12, 2024

Phillipus commented Jan 12, 2024 • edited Loading

Phillipus commented Jan 15, 2024

Phillipus commented Jan 15, 2024 • edited Loading

Phillipus commented Jan 16, 2024 • edited Loading

elsazac commented Jan 17, 2024

merks commented Jan 17, 2024

elsazac commented Jan 17, 2024

merks commented Jan 17, 2024

Phillipus commented Jan 17, 2024

elsazac commented Aug 18, 2023 •

edited

Loading

github-actions bot commented Aug 18, 2023 •

edited

Loading

Phillipus commented Jan 10, 2024 •

edited

Loading

Phillipus commented Jan 12, 2024 •

edited

Loading

Phillipus commented Jan 15, 2024 •

edited

Loading

Phillipus commented Jan 16, 2024 •

edited

Loading