Remove the need to call the PropsUi.setLook() #2638

plugins/transforms/datagrid/src/main/java/org/apache/hop/pipeline/transforms/datagrid/DataGridDialog.java

@@ -287,6 +287,9 @@ private void addDataGrid(boolean refresh) {
     fdData.bottom = new FormAttachment(100, 0);
     wData.setLayoutData(fdData);
 
+    // Set a theme for this control that changes dynamically
+    PropsUi.setTheme(wData);
+
     wTabFolder.layout(true, true);
 
     wFields.nrNonEmpty();

rcp/src/test/java/org/apache/hop/ui/core/PropsUiFixedFontTest.java

@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hop.ui.core;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+
+import org.apache.hop.core.Props;
+import org.apache.hop.core.variables.Variables;
+import org.apache.hop.ui.core.gui.GuiResource;
+import org.apache.hop.ui.core.widget.StyledTextVar;
+import org.apache.hop.ui.testing.SwtBotTestBase;
+import org.eclipse.swt.SWT;
+import org.eclipse.swt.graphics.Font;
+import org.eclipse.swt.graphics.FontData;
+import org.eclipse.swt.widgets.Label;
+import org.eclipse.swt.widgets.Shell;
+import org.eclipse.swt.widgets.Text;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+/**
+ * The shell theme walk used to replace the fixed-width font on script and SQL editors with the
+ * default font. Editors are marked with {@link Props#WIDGET_STYLE_FIXED}; the walk has to keep that
+ * font on the text inside the editor composite.
+ */
+@Tag("uitest")
+class PropsUiFixedFontTest extends SwtBotTestBase {
+
+  @Test
+  @SuppressWarnings("deprecation") // dialogs still call setLook(editor, WIDGET_STYLE_FIXED)
+  void themeWalkKeepsFixedFontOnScriptAndSqlEditors() {
+    Shell shell = new Shell(display);
+    try {
+      Label transformName = new Label(shell, SWT.NONE);
+      transformName.setText("Transform name");
+
+      StyledTextVar script =
+          new StyledTextVar(
+              new Variables(), shell, SWT.MULTI | SWT.H_SCROLL | SWT.V_SCROLL, false, false, false);
+      PropsUi.setLook(script, Props.WIDGET_STYLE_FIXED);
+
+      Text sql = new Text(shell, SWT.MULTI | SWT.H_SCROLL | SWT.V_SCROLL);
+      PropsUi.setLook(sql, Props.WIDGET_STYLE_FIXED);
+
+      // Same order as a dialog: the editors are marked first, then the shell is themed.
+      PropsUi.setTheme(shell);
+
+      Font fixed = GuiResource.getInstance().getFontFixed();
+      Font proportional = GuiResource.getInstance().getFontDefault();
+      assertNotEquals(
+          fontKey(proportional),
+          fontKey(fixed),
+          "the fixed and default fonts have to differ for this check");
+      assertEquals(fontKey(fixed), fontKey(script.getTextWidget().getFont()));
+      assertEquals(fontKey(fixed), fontKey(sql.getFont()));
+      assertEquals(fontKey(proportional), fontKey(transformName.getFont()));
+
+      // A script tab added after the dialog is open, as the JavaScript editor does.
+      StyledTextVar added =
+          new StyledTextVar(
+              new Variables(), shell, SWT.MULTI | SWT.H_SCROLL | SWT.V_SCROLL, false, false, false);
+      PropsUi.setLook(added, Props.WIDGET_STYLE_FIXED);
+      PropsUi.setTheme(shell);
+      assertEquals(fontKey(fixed), fontKey(added.getTextWidget().getFont()));
+      assertEquals(fontKey(fixed), fontKey(script.getTextWidget().getFont()));
+      assertEquals(fontKey(proportional), fontKey(transformName.getFont()));
+    } finally {
+      shell.dispose();
+    }
+  }
+
+  private static String fontKey(Font font) {
+    FontData data = font.getFontData()[0];
+    return data.getName() + "/" + data.getHeight() + "/" + data.getStyle();
+  }
+}

ui/src/main/java/org/apache/hop/ui/core/PropsUi.java

@@ -42,6 +42,7 @@
 import org.apache.hop.ui.util.EnvironmentUtils;
 import org.eclipse.swt.SWT;
 import org.eclipse.swt.custom.CTabFolder;
+import org.eclipse.swt.custom.StyledText;
 import org.eclipse.swt.graphics.Color;
 import org.eclipse.swt.graphics.Font;
 import org.eclipse.swt.graphics.FontData;
@@ -56,6 +57,7 @@
 import org.eclipse.swt.widgets.Layout;
 import org.eclipse.swt.widgets.Shell;
 import org.eclipse.swt.widgets.Table;
+import org.eclipse.swt.widgets.Text;
 import org.eclipse.swt.widgets.ToolBar;
 import org.eclipse.swt.widgets.Tree;
 import org.eclipse.swt.widgets.Widget;
@@ -67,6 +69,12 @@
 public class PropsUi extends Props {
   private static final String OS = System.getProperty("os.name").toLowerCase();
 
+  /**
+   * Style passed to {@link #setTheme(Widget, int)} when it is not the default. The theme walk reads
+   * it back so a fixed-width editor or toolbar field keeps the style it was given.
+   */
+  private static final String WIDGET_STYLE_KEY = PropsUi.class.getName() + ".widgetStyle";
+
   private double nativeZoomFactor;
   private static final String STRING_SHOW_COPY_OR_DISTRIBUTE_WARNING =
       "ShowCopyOrDistributeWarning";
@@ -775,7 +783,84 @@ public void setMetricsPanelDynamicColumnResize(boolean dynamic) {
     setProperty(METRICS_PANEL_DYNAMIC_COLUMN_RESIZE, dynamic ? YES : NO);
   }
 
+  /**
+   * @deprecated Colors and fonts are applied once by {@link #setTheme(Widget)} on the shell. This
+   *     still attaches the keyboard handler, because Hop Web only delivers keys for a widget that
+   *     already has a listener when it is rendered.
+   */
+  @Deprecated(since = "2.20.0", forRemoval = true)
   public static void setLook(Widget widget) {
+    attachKeyHandler(widget);
+  }
+
+  /**
+   * @deprecated Prefer {@link #setTheme(Widget)}. An explicit fixed-width style is remembered and
+   *     applied, so a later theme walk does not replace it with the default font.
+   */
+  @Deprecated(since = "2.20.0", forRemoval = true)
+  public static void setLook(final Widget widget, int style) {
+    // One control, not the tree. Callers pass FIXED, TAB or TOOLBAR for a control that is created
+    // after the shell was themed (a new script tab, a toolbar item). The style is remembered so a
+    // later theme walk does not replace a fixed-width font with the default font.
+    setTheme(widget, style);
+  }
+
+  /** Set theme colors and font on the widget and all its children. */
+  public static void setTheme(final Widget widget) {
+    if (widget == null || widget.isDisposed()) {
+      return;
+    }
+
+    setTheme(widget, styleFor(widget));
+
+    if (widget instanceof Composite composite) {
+      for (Control control : composite.getChildren()) {
+        setTheme(control);
+      }
+    }
+  }
+
+  public static void setTheme(final Widget widget, int style) {
+    attachKeyHandler(widget);
+    if (widget == null || widget.isDisposed()) {
+      return;
+    }
+    if (style != WIDGET_STYLE_DEFAULT) {
+      widget.setData(WIDGET_STYLE_KEY, style);
+    }
+    if (EnvironmentUtils.getInstance().isWeb()) {
+      setThemeOnWeb(widget, style);
+      return;
+    }
+    if (OsHelper.isWindows()) {
+      setThemeOnWindows(widget, style);
+    } else if (OsHelper.isMac()) {
+      setThemeOnMac(widget, style);
+    } else {
+      setThemeOnLinux(widget, style);
+    }
+  }
+
+  /**
+   * Handle the keyboard shortcuts of widgets that are created after their shell was set up, e.g.
+   * when a metadata editor rebuilds a section. In Hop Web the key listener has to be there when the
+   * widget is rendered or RAP never sends its key events to the server.
+   */
+  private static void attachKeyHandler(Widget widget) {
+    if (widget == null || widget.isDisposed()) {
+      return;
+    }
+    HopGuiKeyHandler.getInstance().attachTo(widget);
+  }
+
+  private static int styleFor(Widget widget) {
+    Object explicit = widget.getData(WIDGET_STYLE_KEY);
+    if (explicit instanceof Integer style) {
+      return style;
+    }
+    if (inheritsFixedFont(widget)) {
+      return WIDGET_STYLE_FIXED;
+    }
     int style = WIDGET_STYLE_DEFAULT;
     if (widget instanceof Table) {
       style = WIDGET_STYLE_TABLE;
@@ -796,38 +881,31 @@ public static void setLook(Widget widget) {
         style = WIDGET_STYLE_PUSH_BUTTON;
       }
     }
-
-    setLook(widget, style);
-
-    if (widget instanceof Composite composite) {
-      for (Control child : composite.getChildren()) {
-        setLook(child);
-      }
-    }
+    return style;
   }
 
-  public static void setLook(final Widget widget, int style) {
-    // Handle the keyboard shortcuts of widgets that are created after their shell was set up, e.g.
-    // when a metadata editor rebuilds a section. In Hop Web the key listener has to be there when
-    // the widget is rendered or RAP never sends its key events to the server.
-    //
-    HopGuiKeyHandler.getInstance().attachTo(widget);
-
-    if (EnvironmentUtils.getInstance().isWeb()) {
-      setLookOnWeb(widget, style);
-      return;
+  /**
+   * Script, SQL and the other fixed-width editors are marked on the editor composite. The text
+   * inside that composite is a separate control, and the theme walk would otherwise give it the
+   * default font.
+   */
+  private static boolean inheritsFixedFont(Widget widget) {
+    if (!(widget instanceof Text) && !(widget instanceof StyledText)) {
+      return false;
     }
-    if (OsHelper.isWindows()) {
-      setLookOnWindows(widget, style);
-    } else if (OsHelper.isMac()) {
-      setLookOnMac(widget, style);
-    } else {
-      setLookOnLinux(widget, style);
+    if (!(widget instanceof Control control)) {
+      return false;
     }
+    Composite parent = control.getParent();
+    return parent != null && !parent.isDisposed() && isFixedStyle(parent.getData(WIDGET_STYLE_KEY));
+  }
+
+  private static boolean isFixedStyle(Object style) {
+    return style instanceof Integer value && value == WIDGET_STYLE_FIXED;
   }
 
   /** Hop Web (RAP) specific look. Keeps web theme logic separate from OS-specific setLookOn*. */
-  protected static void setLookOnWeb(final Widget widget, int style) {
+  protected static void setThemeOnWeb(final Widget widget, int style) {
     final GuiResource gui = GuiResource.getInstance();
     Font font = gui.getFontDefault();
     Color background = gui.getWidgetBackGroundColor();
@@ -939,7 +1017,7 @@ private static void ensureSafeRenderer(CTabFolder tabFolder) {
     }
   }
 
-  protected static void setLookOnWindows(final Widget widget, int style) {
+  protected static void setThemeOnWindows(final Widget widget, int style) {
     final GuiResource gui = GuiResource.getInstance();
     Font font = gui.getFontDefault();
     Color background = gui.getWidgetBackGroundColor();
@@ -1016,7 +1094,7 @@ protected static void setLookOnWindows(final Widget widget, int style) {
     }
   }
 
-  protected static void setLookOnMac(final Widget widget, int style) {
+  protected static void setThemeOnMac(final Widget widget, int style) {
     final GuiResource gui = GuiResource.getInstance();
     Font font = gui.getFontDefault();
     Color background = null;
@@ -1078,7 +1156,7 @@ protected static void setLookOnMac(final Widget widget, int style) {
     }
   }
 
-  protected static void setLookOnLinux(final Widget widget, int style) {
+  protected static void setThemeOnLinux(final Widget widget, int style) {
     final GuiResource gui = GuiResource.getInstance();
     Font font = gui.getFontDefault();
     Color background = GuiResource.getInstance().getWidgetBackGroundColor();

ui/src/main/java/org/apache/hop/ui/core/dialog/BaseDialog.java

@@ -908,6 +908,8 @@ public static void defaultShellHandling(
       applyReadOnlyMode(shell);
     }
 
+    PropsUi.setTheme(shell);
+
     if (useStandardMinimumSize) {
       shell.setMinimumSize(650, 250);
     } else {

ui/src/main/java/org/apache/hop/ui/core/dialog/ContextDialog.java

@@ -580,6 +580,8 @@ public GuiAction open() {
       ContextDialogSvgFacade.register(wCanvas, this);
     }
 
+    PropsUi.setTheme(shell);
+
     // Show the dialog now
     //
     shell.open();

ui/src/main/java/org/apache/hop/ui/core/dialog/DialogBoxWithButtons.java

@@ -96,10 +96,11 @@ public int open() {
     BaseTransformDialog.positionBottomButtons(
         shell, buttons.toArray(new Button[0]), formMargin, wLabel);
 
-    BaseTransformDialog.setSize(shell);
     shell.addListener(SWT.Close, e -> dispose());
     BaseTransformDialog.setSize(shell);
 
+    PropsUi.setTheme(shell);
+
     shell.open();
 
     Display display = shell.getDisplay();

ui/src/main/java/org/apache/hop/ui/core/dialog/EnterMappingDialog.java

@@ -36,7 +36,6 @@
 import org.eclipse.swt.layout.FormLayout;
 import org.eclipse.swt.widgets.Button;
 import org.eclipse.swt.widgets.Dialog;
-import org.eclipse.swt.widgets.Display;
 import org.eclipse.swt.widgets.Label;
 import org.eclipse.swt.widgets.List;
 import org.eclipse.swt.widgets.Shell;
@@ -316,26 +315,7 @@ public java.util.List<SourceToTargetMapping> open() {
 
     getData();
 
-    // Set the size as well...
-    //
-    BaseTransformDialog.setSize(shell);
-
-    // Shell closed?
-    //
-    shell.addListener(SWT.Close, e -> cancel());
-
-    // Open the shell
-    //
-    shell.open();
-
-    // Handle the event loop until we're done with this shell...
-    //
-    Display display = shell.getDisplay();
-    while (!shell.isDisposed()) {
-      if (!display.readAndDispatch()) {
-        display.sleep();
-      }
-    }
+    BaseDialog.defaultShellHandling(shell, e -> ok(), e -> cancel());
 
     return mappings;
   }