From 7941a065f310ad43a8dbdfca4f0dd0f2bca89c2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Fri, 15 Mar 2024 13:56:12 +0100 Subject: [PATCH] cocoa: Enforce native item height for Table, Tree, List if necessary Newer versions of macOS introduced a new style of table rows with a padding that is larger than previously seen. This broke item height computations (#677). An attempt to fix this had the unexpected side effect that now some UI elements in Eclipse IDE (and other apps with "-Dorg.eclipse.swt.internal.carbon.smallFonts" enabled), eclise.platform.ui #1674, and it also broke compatibility with older versions of macOS that did not have the additional padding. This change reverts the previous fix, and adds logic to enforce the native item height by default, without making assumption about the exact height (which varies with "smallFonts", for example). The setting can be controlled by a new System property, org.eclipse.swt.internal.cocoa.useNativeItemHeight, that, if set to false, overrules the default. This allows apps to bring brack the old macOS behavior. Lastly, update two tests in Test_org_eclipse_swt_widgets_List that made assumptions about the item height being different for two small font sizes -- this is no longer true when the native item height is enforced. We now increase the font size of the second font such that this corner case no longer matters. Fixes https://github.com/eclipse-platform/eclipse.platform.ui/issues/1674 Fixes https://github.com/eclipse-platform/eclipse.platform.swt/issues/677 --- .../cocoa/org/eclipse/swt/widgets/Display.java | 17 +++++++++++++++++ .../cocoa/org/eclipse/swt/widgets/List.java | 14 ++++++++++---- .../cocoa/org/eclipse/swt/widgets/Table.java | 13 +++++++++---- .../cocoa/org/eclipse/swt/widgets/Tree.java | 13 +++++++++---- .../Test_org_eclipse_swt_widgets_List.java | 4 ++-- 5 files changed, 47 insertions(+), 14 deletions(-) diff --git a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java index 1f35bef00a8..c1abd1a31bf 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java @@ -166,6 +166,8 @@ enum APPEARANCE { NSFont textViewFont, tableViewFont, outlineViewFont, datePickerFont; NSFont boxFont, tabViewFont, progressIndicatorFont; + boolean useNativeItemHeight; + Shell [] modalShells; Dialog modalDialog; NSPanel modalPanel; @@ -2404,6 +2406,8 @@ protected void init () { initFonts (); setDeviceZoom (); + useNativeItemHeight = initUseNativeItemHeight(); + /* * Create an application delegate for app-level notifications. The AWT may have already set a delegate; * if so, hold on to it so messages can be forwarded to it. @@ -2502,6 +2506,19 @@ public void run() { isPainting = isPainting.initWithCapacity(12); } +/** + * Checks if the native item height should be enforced as a minimum (which is true by default). + * + * Newer version of macOS may use a default item height in Table, Tree and List + * controls that is larger than what is traditionally expected. + * + * Enforcing the default height as a minimum may break existing assumptions and + * render UI elements with a padding that may be considered too large. + */ +private boolean initUseNativeItemHeight() { + return Boolean.parseBoolean(System.getProperty("org.eclipse.swt.internal.cocoa.useNativeItemHeight", "true")); +} + private static NSString getAwtRunLoopMode() { // Special run loop mode mode used by AWT enters when it only wants related messages processed. // The name of this mode is a defacto contract established by the JavaNativeFoundation (JNF) libary. diff --git a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/List.java b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/List.java index 7d56e8a552a..56a7352a62a 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/List.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/List.java @@ -46,13 +46,12 @@ public class List extends Scrollable { int itemCount; boolean ignoreSelect, didSelect, rowsChanged, mouseIsDown; + final int nativeItemHeight; + static int NEXT_ID; static final int CELL_GAP = 1; - /* Vertical cell padding for list item */ - static final int VERTICAL_CELL_PADDING= 8; - /** * Constructs a new instance of this class given its parent * and a style value describing its behavior and appearance. @@ -84,6 +83,9 @@ public class List extends Scrollable { */ public List (Composite parent, int style) { super (parent, checkStyle (style)); + + this.nativeItemHeight = (int)((NSTableView)view).rowHeight(); + setFont(defaultFont ().handle); // update height } @Override @@ -1182,7 +1184,11 @@ 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) + VERTICAL_CELL_PADDING); + int height = (int)Math.ceil (ascent + descent) + 1; + if (display.useNativeItemHeight) { + height = Math.max (height, nativeItemHeight); + } + ((NSTableView)view).setRowHeight (height); setScrollWidth(); } diff --git a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Table.java b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Table.java index f62d8bc4d42..02a643e1d21 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Table.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Table.java @@ -86,6 +86,8 @@ public class Table extends Composite { boolean shouldScroll = true; boolean keyDown; + final int nativeItemHeight; + static int NEXT_ID; static final int FIRST_COLUMN_MINIMUM_WIDTH = 5; @@ -93,9 +95,6 @@ public class Table extends Composite { static final int TEXT_GAP = 2; static final int CELL_GAP = 1; - /* Vertical cell padding for table item */ - static final int VERTICAL_CELL_PADDING= 8; - /** * Constructs a new instance of this class given its parent * and a style value describing its behavior and appearance. @@ -132,6 +131,9 @@ public class Table extends Composite { */ public Table (Composite parent, int style) { super (parent, checkStyle (style)); + + this.nativeItemHeight = (int)((NSTableView)view).rowHeight(); + setItemHeight(null, null, true); } @Override @@ -2825,7 +2827,10 @@ 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) + VERTICAL_CELL_PADDING; + int height = (int)Math.ceil (ascent + descent) + 1; + if (display.useNativeItemHeight) { + height = Math.max (height, nativeItemHeight); + } Rectangle bounds = image != null ? image.getBounds () : imageBounds; if (bounds != null) { imageBounds = bounds; diff --git a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Tree.java b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Tree.java index 45fd96cba4c..5379958904c 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Tree.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Tree.java @@ -97,6 +97,8 @@ public class Tree extends Composite { /* Used to control drop feedback when DND.FEEDBACK_EXPAND and DND.FEEDBACK_SCROLL is set/not set */ boolean shouldExpand = true, shouldScroll = true; + final int nativeItemHeight; + static int NEXT_ID; /* @@ -109,9 +111,6 @@ public class Tree extends Composite { static final int TEXT_GAP = 2; static final int CELL_GAP = 1; - /* Vertical cell padding for tree item */ - static final int VERTICAL_CELL_PADDING= 8; - /** * Constructs a new instance of this class given its parent * and a style value describing its behavior and appearance. @@ -147,6 +146,9 @@ public class Tree extends Composite { */ public Tree (Composite parent, int style) { super (parent, checkStyle (style)); + + this.nativeItemHeight = (int)((NSTableView)view).rowHeight(); + setItemHeight(null, null, true); } @Override @@ -3173,7 +3175,10 @@ 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) + VERTICAL_CELL_PADDING; + int height = (int)Math.ceil (ascent + descent) + 1; + if (display.useNativeItemHeight) { + height = Math.max (height, nativeItemHeight); + } Rectangle bounds = image != null ? image.getBounds () : imageBounds; if (bounds != null) { imageBounds = bounds; diff --git a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_List.java b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_List.java index 4bffa3d7914..bc6f2480d32 100644 --- a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_List.java +++ b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_List.java @@ -498,7 +498,7 @@ public void test_getItemHeight() { lineHeight = list.getItemHeight(); list.setFont(null); font.dispose(); - font = new Font(list.getDisplay(), fontData.getName(), 12, fontData.getStyle()); + font = new Font(list.getDisplay(), fontData.getName(), 24, fontData.getStyle()); list.setFont(font); int newLineHeight = list.getItemHeight(); assertTrue(":a:", newLineHeight > lineHeight); @@ -1625,7 +1625,7 @@ public void test_setFontLorg_eclipse_swt_graphics_Font() { lineHeight = list.getItemHeight(); list.setFont(null); font.dispose(); - font = new Font(list.getDisplay(), fontData.getName(), 12, fontData.getStyle()); + font = new Font(list.getDisplay(), fontData.getName(), 24, fontData.getStyle()); list.setFont(font); assertEquals(font, list.getFont()); assertTrue("itemHeight=" + list.getItemHeight() + ", lineHeight=" + lineHeight, list.getItemHeight() > lineHeight);