From 84b14f92c4a644ef24919ffe96af96c794e568d2 Mon Sep 17 00:00:00 2001 From: Hans Van Akelyen Date: Sat, 5 Sep 2026 12:44:15 +0200 Subject: [PATCH] harden getting the session display, fixes #8248 minor improvements --- .../hop/lint/LintCanvasOverlayRefresh.java | 3 +- .../hop/lint/LintProblemsBarManager.java | 13 +- .../hop/lint/LintStatusFilePainter.java | 221 ++++++++++++++---- .../lint/LintStatusBadgeCompositeTest.java | 155 ++++++++++++ .../hop/testing/gui/TestingGuiPlugin.java | 6 +- .../gui/TestingGuiPluginTweakTest.java | 34 +++ .../ui/hopgui/BackgroundThreadFacadeImpl.java | 13 +- .../BackgroundThreadFacadeImplTest.java | 27 ++- .../apache/hop/core/SwtUniversalImage.java | 2 +- .../apache/hop/ui/core/gui/HopNamespace.java | 5 +- .../apache/hop/ui/core/widget/TreeMemory.java | 3 +- .../apache/hop/ui/hopgui/SessionDisplay.java | 77 ++++++ .../hop/ui/hopgui/shared/SashFormMemory.java | 3 +- .../hop/ui/hopgui/SessionDisplayTest.java | 88 +++++++ 14 files changed, 587 insertions(+), 63 deletions(-) create mode 100644 plugins/misc/lint/src/test/java/org/apache/hop/lint/LintStatusBadgeCompositeTest.java create mode 100644 ui/src/main/java/org/apache/hop/ui/hopgui/SessionDisplay.java create mode 100644 ui/src/test/java/org/apache/hop/ui/hopgui/SessionDisplayTest.java diff --git a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCanvasOverlayRefresh.java b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCanvasOverlayRefresh.java index d54b1e74395..d423ffcf8eb 100644 --- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCanvasOverlayRefresh.java +++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCanvasOverlayRefresh.java @@ -20,6 +20,7 @@ import java.util.Set; import java.util.WeakHashMap; import org.apache.hop.ui.hopgui.HopGui; +import org.apache.hop.ui.hopgui.SessionDisplay; import org.apache.hop.ui.hopgui.file.IHopFileTypeHandler; import org.apache.hop.ui.hopgui.file.shared.HopGuiAbstractGraph; import org.apache.hop.ui.hopgui.perspective.TabItemHandler; @@ -68,7 +69,7 @@ public static void redrawOpenGraphs() { display = hopGui.getShell().getDisplay(); } if (display == null) { - display = Display.getCurrent(); + display = SessionDisplay.current(); } if (display == null || display.isDisposed()) { return; diff --git a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintProblemsBarManager.java b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintProblemsBarManager.java index 640b4012bef..90409d01b51 100644 --- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintProblemsBarManager.java +++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintProblemsBarManager.java @@ -22,6 +22,7 @@ import org.apache.hop.core.logging.ILogChannel; import org.apache.hop.core.logging.LogChannel; import org.apache.hop.ui.hopgui.HopGui; +import org.apache.hop.ui.hopgui.SessionDisplay; import org.apache.hop.ui.hopgui.file.shared.HopGuiAbstractGraph; import org.eclipse.swt.widgets.Display; @@ -138,7 +139,7 @@ public void updateProblemsBar(String filePath) { } // This touches SWT widgets, so make sure it runs on the UI thread regardless of which // thread the caller is on (background lint threads call this too). - if (Display.getCurrent() == null) { + if (SessionDisplay.current() == null) { Display display = sessionDisplay(); if (display == null || display.isDisposed()) { log.logDetailed( @@ -174,7 +175,7 @@ private void updateProblemsBar(String filePath, int attempt) { "Gave up syncing the Problems tab for " + filePath + ": no editor found in time"); return; } - Display display = Display.getCurrent(); + Display display = SessionDisplay.current(); if (display == null || display.isDisposed()) { return; } @@ -184,9 +185,9 @@ private void updateProblemsBar(String filePath, int attempt) { /** * The display to get onto the UI thread with. * - *

An editor we already know about answers first: {@code Display.getDefault()} only knows the - * session bound to the calling thread, which is nothing at all on a thread that was started - * without one - and in Hop Web the wrong session's display would be worse than none. + *

An editor we already know about answers first: the default display only knows the session + * bound to the calling thread, which is nothing at all on a thread that was started without one - + * and in Hop Web the wrong session's display would be worse than none. */ private Display sessionDisplay() { for (HopGuiAbstractGraph graph : graphsById.values()) { @@ -197,7 +198,7 @@ private Display sessionDisplay() { } } } - return Display.getDefault(); + return SessionDisplay.currentOrDefault(); } public void refreshAllOpenEditors() { diff --git a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintStatusFilePainter.java b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintStatusFilePainter.java index 60043a9839b..e018463d2c5 100644 --- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintStatusFilePainter.java +++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintStatusFilePainter.java @@ -20,15 +20,20 @@ import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; +import org.apache.hop.core.SwtUniversalImage; import org.apache.hop.core.logging.ILogChannel; import org.apache.hop.core.logging.LogChannel; import org.apache.hop.core.util.Utils; import org.apache.hop.ui.core.gui.GuiResource; import org.apache.hop.ui.hopgui.HopGui; +import org.apache.hop.ui.hopgui.SessionDisplay; import org.apache.hop.ui.hopgui.perspective.explorer.ExplorerPerspective; import org.apache.hop.ui.hopgui.perspective.explorer.IExplorerFilePaintListener; import org.eclipse.swt.SWT; import org.eclipse.swt.graphics.Image; +import org.eclipse.swt.graphics.ImageData; +import org.eclipse.swt.graphics.PaletteData; +import org.eclipse.swt.graphics.RGB; import org.eclipse.swt.widgets.Display; import org.eclipse.swt.widgets.Tree; import org.eclipse.swt.widgets.TreeItem; @@ -60,8 +65,8 @@ public class LintStatusFilePainter implements IExplorerFilePaintListener { /** * The display of the GUI this painter belongs to, read when it is built on the UI thread. * - *

{@code Display.getDefault()} answers for the session bound to the calling thread, and the - * results this repaints for arrive on lint threads that may have none. + *

The default display answers for the session bound to the calling thread, and the results + * this repaints for arrive on lint threads that may have none. */ private final Display display; @@ -77,7 +82,7 @@ public enum LintStatus { } public LintStatusFilePainter() { - this.display = Display.getCurrent() != null ? Display.getCurrent() : Display.getDefault(); + this.display = SessionDisplay.currentOrDefault(); updateFileStatusCache(); LintResultsManager.getInstance() @@ -99,8 +104,11 @@ public LintStatusFilePainter() { * do: RWT only draws on a control, so the very first call failed and the Explorer showed no lint * status at all. Hop ships the three icons as SVG, and {@link GuiResource} loads and caches them * per session, which also settles who disposes them - not us. + * + *

Asked for at the size it will occupy, so the SVG is rasterized at that size rather than a + * bigger bitmap being resampled down into it - resampling is what costs an icon its edges. */ - private Image badgeIcon(LintStatus status) { + private Image badgeIcon(LintStatus status, int size) { String location = switch (status) { case ERROR -> "ui/images/error.svg"; @@ -112,7 +120,7 @@ private Image badgeIcon(LintStatus status) { return null; } try { - return GuiResource.getInstance().getImage(location, BADGE_SIZE, BADGE_SIZE); + return GuiResource.getInstance().getImage(location, size, size); } catch (Exception e) { log.logDetailed("No lint status icon available for " + status + ": " + e.getMessage()); return null; @@ -181,21 +189,17 @@ private void applyStatusStyle( } } - Image badge = badgeIcon(status); - if (badge == null || badge.isDisposed()) { - return; - } switch (status) { case ERROR: - addOverlayIcon(treeItem, badge, status); + addOverlayIcon(treeItem, status); addLintTooltip(treeItem, name, "Linter errors"); break; case WARNING: - addOverlayIcon(treeItem, badge, status); + addOverlayIcon(treeItem, status); addLintTooltip(treeItem, name, "Linter warnings"); break; case CLEAN: - addOverlayIcon(treeItem, badge, status); + addOverlayIcon(treeItem, status); addLintTooltip(treeItem, name, "No linter issues"); break; default: @@ -263,7 +267,7 @@ private LintStatus resolveStatusFromResults(String normalized) { return LintStatus.UNKNOWN; } - private void addOverlayIcon(TreeItem treeItem, Image lintIcon, LintStatus status) { + private void addOverlayIcon(TreeItem treeItem, LintStatus status) { try { // Already showing this exact status for this item -> nothing to do (avoids re-compositing // on every paint, which previously leaked a new Image each time and crashed the GUI). @@ -282,22 +286,31 @@ private void addOverlayIcon(TreeItem treeItem, Image lintIcon, LintStatus status } } - if (base == null || base.isDisposed()) { + boolean noBase = base == null || base.isDisposed(); + org.eclipse.swt.graphics.Rectangle bounds = noBase ? null : base.getBounds(); + // Rasterize the badge at the size it will occupy on this icon, so nothing is resampled. + Image lintIcon = + badgeIcon( + status, bounds == null ? BADGE_SIZE : badgeSizeFor(bounds.width, bounds.height)); + if (lintIcon == null || lintIcon.isDisposed()) { + return; + } + + if (bounds == null) { treeItem.setImage(lintIcon); treeItem.setData(APPLIED_STATUS_KEY, status); return; } - org.eclipse.swt.graphics.Rectangle bounds = base.getBounds(); if (bounds.width > 100 || bounds.height > 100) { treeItem.setImage(lintIcon); treeItem.setData(APPLIED_STATUS_KEY, status); return; } - // No composite (Hop Web cannot draw one): leave the file's own icon alone. The item's - // colour already says what the status is, and replacing the icon with a bare badge would - // cost more than it tells. + // Without a composite, leave the file's own icon alone. The item's colour already says + // what the status is, and replacing the icon with a bare badge would cost more than it + // tells. Image compositeIcon = getOrCreateComposite(base, lintIcon); if (compositeIcon != null) { treeItem.setImage(compositeIcon); @@ -305,7 +318,11 @@ private void addOverlayIcon(TreeItem treeItem, Image lintIcon, LintStatus status treeItem.setData(APPLIED_STATUS_KEY, status); } catch (Exception e) { log.logError("Error creating overlay icon: " + e.getMessage(), e); - treeItem.setImage(lintIcon); + // The bare badge still says what the status is, which beats leaving the item unmarked. + Image fallback = badgeIcon(status, BADGE_SIZE); + if (fallback != null && !fallback.isDisposed()) { + treeItem.setImage(fallback); + } } } @@ -323,45 +340,157 @@ private Image getOrCreateComposite(Image originalIcon, Image lintIcon) { return composite; } + /** + * The file's own icon with the lint badge in its bottom right corner. + * + *

Composited pixel by pixel rather than with a {@code GC}. RWT resolves both the device and + * the drawing delegate of a {@code GC} from its drawable and understands only a Control or a + * Device, so a {@code GC} on an Image is left with neither: the drawing fails with a + * NullPointerException, and disposing it then fails with "A factory-created resource cannot be + * disposed", which is the exception that reaches the log. Working on the {@link ImageData} of + * both icons needs no drawing surface at all, so Hop Web gets the same badges as the desktop. + */ private Image createCompositeIcon(Image originalIcon, Image lintIcon) { try { if (display == null || display.isDisposed()) { return null; } - - org.eclipse.swt.graphics.Rectangle originalBounds = originalIcon.getBounds(); - org.eclipse.swt.graphics.Rectangle lintBounds = lintIcon.getBounds(); - - Image composite = new Image(display, originalBounds.width, originalBounds.height); - org.eclipse.swt.graphics.GC gc = new org.eclipse.swt.graphics.GC(composite); - try { - gc.setBackground(display.getSystemColor(SWT.COLOR_WIDGET_BACKGROUND)); - gc.fillRectangle(0, 0, originalBounds.width, originalBounds.height); - gc.drawImage(originalIcon, 0, 0); - - int badgeSize = Math.min(10, Math.min(originalBounds.width / 2, originalBounds.height / 2)); - int lintX = originalBounds.width - badgeSize - 1; - int lintY = originalBounds.height - badgeSize - 1; - gc.drawImage( - lintIcon, - 0, - 0, - lintBounds.width, - lintBounds.height, - lintX, - lintY, - badgeSize, - badgeSize); - return composite; - } finally { - gc.dispose(); + ImageData baseData = SwtUniversalImage.getImageDataAtZoom(originalIcon, 100); + if (baseData == null) { + return null; } + int badgeSize = badgeSizeFor(baseData.width, baseData.height); + if (badgeSize <= 0) { + return null; + } + // Composited again for every zoom the platform asks for, out of what both icons themselves + // have at that zoom. Handing over the 100% pixels alone and letting SWT raster-scale them up + // is what leaves icons blurry on a HiDPI screen - the very thing createDpiAwareImage exists + // to avoid. + return SwtUniversalImage.createDpiAwareImage( + display, + zoom -> + withBadge( + SwtUniversalImage.getImageDataAtZoom(originalIcon, zoom), + SwtUniversalImage.getImageDataAtZoom(lintIcon, zoom), + SwtUniversalImage.pixelSize(badgeSize, zoom), + SwtUniversalImage.pixelSize(1, zoom))); } catch (Exception e) { log.logError("Error creating composite icon: " + e.getMessage(), e); return null; } } + /** A corner mark on an icon this size: half its width at most, and never more than 10px. */ + private static int badgeSizeFor(int width, int height) { + return Math.min(10, Math.min(width / 2, height / 2)); + } + + /** + * The base icon with the badge scaled into its bottom right corner, blended over whatever the + * base has there rather than punched through it, so a badge with soft edges does not leave a hard + * outline. What the base leaves transparent stays transparent: the tree paints its own background + * behind the icon. + * + *

Sizes are in the pixels of the icons handed in, so that the same badge lands in the same + * place whichever zoom these pixels came from. + */ + static ImageData withBadge(ImageData baseData, ImageData badgeData, int badgeSize, int margin) { + ImageData composite = withPerPixelAlpha(baseData); + ImageData scaled = badgeData.scaledTo(badgeSize, badgeSize); + ImageData scaledMask = transparencyMask(scaled); + int offsetX = composite.width - badgeSize - margin; + int offsetY = composite.height - badgeSize - margin; + + for (int y = 0; y < badgeSize; y++) { + int targetY = offsetY + y; + if (targetY < 0 || targetY >= composite.height) { + continue; + } + for (int x = 0; x < badgeSize; x++) { + int targetX = offsetX + x; + if (targetX < 0 || targetX >= composite.width) { + continue; + } + int overAlpha = alphaAt(scaled, scaledMask, x, y); + if (overAlpha == 0) { + continue; + } + RGB over = scaled.palette.getRGB(scaled.getPixel(x, y)); + if (overAlpha == 255) { + composite.setPixel(targetX, targetY, composite.palette.getPixel(over)); + composite.setAlpha(targetX, targetY, 255); + continue; + } + RGB under = composite.palette.getRGB(composite.getPixel(targetX, targetY)); + int underAlpha = composite.getAlpha(targetX, targetY); + int outAlpha = overAlpha + underAlpha * (255 - overAlpha) / 255; + composite.setPixel( + targetX, + targetY, + composite.palette.getPixel(blend(over, under, overAlpha, underAlpha))); + composite.setAlpha(targetX, targetY, outAlpha); + } + } + return composite; + } + + /** + * The same picture in the one shape we can composite into: direct colour with an alpha value per + * pixel. An icon can express its transparency in any of several ways and only this one can be + * written back to, so the base is read through {@link #alphaAt} and rewritten as this. + */ + private static ImageData withPerPixelAlpha(ImageData source) { + ImageData copy = + new ImageData(source.width, source.height, 24, new PaletteData(0xFF0000, 0xFF00, 0xFF)); + copy.alphaData = new byte[source.width * source.height]; + ImageData mask = transparencyMask(source); + for (int y = 0; y < source.height; y++) { + for (int x = 0; x < source.width; x++) { + RGB rgb = source.palette.getRGB(source.getPixel(x, y)); + copy.setPixel(x, y, copy.palette.getPixel(rgb)); + copy.setAlpha(x, y, alphaAt(source, mask, x, y)); + } + } + return copy; + } + + /** The 1-bit mask of an icon that carries one (ICO, BMP), or null - read once, not per pixel. */ + private static ImageData transparencyMask(ImageData data) { + return data.maskData == null ? null : data.getTransparencyMask(); + } + + /** How opaque one pixel is, whichever of the four ways the icon says so. */ + private static int alphaAt(ImageData data, ImageData mask, int x, int y) { + if (mask != null && mask.getPixel(x, y) == 0) { + return 0; + } + if (data.transparentPixel != -1 && data.getPixel(x, y) == data.transparentPixel) { + return 0; + } + if (data.alphaData != null) { + return data.getAlpha(x, y); + } + return data.alpha == -1 ? 255 : data.alpha; + } + + /** Source-over: the colour left when {@code over} is laid on {@code under}. */ + private static RGB blend(RGB over, RGB under, int overAlpha, int underAlpha) { + return new RGB( + channel(over.red, under.red, overAlpha, underAlpha), + channel(over.green, under.green, overAlpha, underAlpha), + channel(over.blue, under.blue, overAlpha, underAlpha)); + } + + private static int channel(int over, int under, int overAlpha, int underAlpha) { + int outAlpha = overAlpha + underAlpha * (255 - overAlpha) / 255; + if (outAlpha == 0) { + return 0; + } + int weighted = over * overAlpha * 255 + under * underAlpha * (255 - overAlpha); + return Math.min(255, weighted / (outAlpha * 255)); + } + private void addLintTooltip(TreeItem treeItem, String fileName, String lintStatus) { try { treeItem.setData("lintTooltip", fileName + " - " + lintStatus); diff --git a/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintStatusBadgeCompositeTest.java b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintStatusBadgeCompositeTest.java new file mode 100644 index 00000000000..d25fb1290a4 --- /dev/null +++ b/plugins/misc/lint/src/test/java/org/apache/hop/lint/LintStatusBadgeCompositeTest.java @@ -0,0 +1,155 @@ +/* + * 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.lint; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.eclipse.swt.graphics.ImageData; +import org.eclipse.swt.graphics.PaletteData; +import org.eclipse.swt.graphics.RGB; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * The Explorer badges a file's own icon with its lint status. RWT cannot draw onto an Image - a + * {@code GC} built on one has neither a device nor a drawing delegate - so the badge is composited + * from the {@link ImageData} of both icons, which needs no drawing surface and works the same on + * the desktop and in Hop Web. + */ +class LintStatusBadgeCompositeTest { + + private static final RGB BASE_COLOR = new RGB(10, 20, 30); + private static final RGB BADGE_COLOR = new RGB(200, 100, 50); + + /** A 16x16 icon of one colour, opaque, with an alpha channel - what an SVG icon loads as. */ + private static ImageData icon(int size, RGB color, int alpha) { + ImageData data = new ImageData(size, size, 24, new PaletteData(0xFF0000, 0xFF00, 0xFF)); + data.alphaData = new byte[size * size]; + for (int y = 0; y < size; y++) { + for (int x = 0; x < size; x++) { + data.setPixel(x, y, data.palette.getPixel(color)); + data.setAlpha(x, y, alpha); + } + } + return data; + } + + private static RGB colorAt(ImageData data, int x, int y) { + return data.palette.getRGB(data.getPixel(x, y)); + } + + @Test + @DisplayName("the badge lands in the bottom right corner and leaves the rest of the icon alone") + void badgeIsCompositedIntoTheCorner() { + ImageData composite = + LintStatusFilePainter.withBadge(icon(16, BASE_COLOR, 255), icon(8, BADGE_COLOR, 255), 8, 1); + + assertEquals(16, composite.width); + assertEquals(16, composite.height); + // One pixel of margin is kept, so the very last column and row stay the base icon. + assertEquals(BADGE_COLOR, colorAt(composite, 14, 14)); + assertEquals(BASE_COLOR, colorAt(composite, 15, 15)); + assertEquals(BASE_COLOR, colorAt(composite, 0, 0)); + assertEquals(BASE_COLOR, colorAt(composite, 6, 6)); + } + + @Test + @DisplayName("what the badge leaves transparent shows the icon underneath") + void fullyTransparentBadgePixelsChangeNothing() { + ImageData composite = + LintStatusFilePainter.withBadge(icon(16, BASE_COLOR, 255), icon(8, BADGE_COLOR, 0), 8, 1); + + assertEquals(BASE_COLOR, colorAt(composite, 14, 14)); + assertEquals(255, composite.getAlpha(14, 14)); + } + + @Test + @DisplayName("what the icon leaves transparent stays transparent") + void theIconsOwnTransparencySurvives() { + ImageData composite = + LintStatusFilePainter.withBadge(icon(16, BASE_COLOR, 0), icon(8, BADGE_COLOR, 255), 8, 1); + + assertEquals(0, composite.getAlpha(0, 0)); + // Where the badge is opaque it wins outright, transparent base or not. + assertEquals(255, composite.getAlpha(14, 14)); + assertEquals(BADGE_COLOR, colorAt(composite, 14, 14)); + } + + /** Palette icons say "transparent" with a pixel value rather than an alpha channel. */ + @Test + @DisplayName("a palette icon's transparent pixel is read as transparent, not as its colour") + void transparentPixelIsHonoured() { + PaletteData palette = new PaletteData(BASE_COLOR, BADGE_COLOR); + ImageData base = new ImageData(16, 16, 8, palette); + base.transparentPixel = 1; + for (int y = 0; y < 16; y++) { + for (int x = 0; x < 16; x++) { + base.setPixel(x, y, x == 0 && y == 0 ? 1 : 0); + } + } + + ImageData composite = LintStatusFilePainter.withBadge(base, icon(8, BADGE_COLOR, 255), 8, 1); + + assertEquals(0, composite.getAlpha(0, 0)); + assertEquals(255, composite.getAlpha(1, 1)); + assertEquals(BASE_COLOR, colorAt(composite, 1, 1)); + } + + /** + * The platform asks for the composite again at each zoom it paints at, so the same badge is built + * from bigger pixels rather than by scaling up the 100% one - that is what keeps a HiDPI icon + * sharp. Everything scales together, the margin included, so the badge lands in the same place on + * the icon at every zoom. + */ + @Test + @DisplayName("at 200% everything is twice the size and the badge sits in the same place") + void geometryScalesWithTheZoom() { + ImageData at100 = + LintStatusFilePainter.withBadge(icon(16, BASE_COLOR, 255), icon(8, BADGE_COLOR, 255), 8, 1); + ImageData at200 = + LintStatusFilePainter.withBadge( + icon(32, BASE_COLOR, 255), icon(16, BADGE_COLOR, 255), 16, 2); + + assertEquals(32, at200.width); + // The badge corner of the 100% icon, at twice the scale, is still the badge corner. + assertEquals(BADGE_COLOR, colorAt(at100, 7, 7)); + assertEquals(BADGE_COLOR, colorAt(at200, 14, 14)); + // And so is the last pixel before the margin. + assertEquals(BADGE_COLOR, colorAt(at100, 14, 14)); + assertEquals(BADGE_COLOR, colorAt(at200, 29, 29)); + // The margin itself stays the base icon at both zooms. + assertEquals(BASE_COLOR, colorAt(at100, 15, 15)); + assertEquals(BASE_COLOR, colorAt(at200, 30, 30)); + assertEquals(BASE_COLOR, colorAt(at200, 31, 31)); + } + + @Test + @DisplayName("a half transparent badge blends with the icon rather than replacing it") + void partialAlphaBlends() { + ImageData composite = + LintStatusFilePainter.withBadge( + icon(16, new RGB(0, 0, 0), 255), icon(8, new RGB(255, 255, 255), 128), 8, 1); + + // Half of white over black, opaque either way. + RGB blended = colorAt(composite, 14, 14); + assertEquals(128, blended.red); + assertEquals(128, blended.green); + assertEquals(128, blended.blue); + assertEquals(255, composite.getAlpha(14, 14)); + } +} diff --git a/plugins/misc/testing/src/main/java/org/apache/hop/testing/gui/TestingGuiPlugin.java b/plugins/misc/testing/src/main/java/org/apache/hop/testing/gui/TestingGuiPlugin.java index 61437fb05d2..c8d8a3b534c 100644 --- a/plugins/misc/testing/src/main/java/org/apache/hop/testing/gui/TestingGuiPlugin.java +++ b/plugins/misc/testing/src/main/java/org/apache/hop/testing/gui/TestingGuiPlugin.java @@ -78,6 +78,7 @@ import org.apache.hop.ui.core.widget.ComboFilterPopup; import org.apache.hop.ui.core.widget.TableView; import org.apache.hop.ui.hopgui.HopGui; +import org.apache.hop.ui.hopgui.SessionDisplay; import org.apache.hop.ui.hopgui.file.IHopFileTypeHandler; import org.apache.hop.ui.hopgui.file.pipeline.HopGuiPipelineGraph; import org.apache.hop.ui.hopgui.file.pipeline.context.HopGuiPipelineContext; @@ -92,7 +93,6 @@ import org.eclipse.swt.SWTException; import org.eclipse.swt.widgets.Combo; import org.eclipse.swt.widgets.Control; -import org.eclipse.swt.widgets.Display; import org.eclipse.swt.widgets.Shell; import org.eclipse.swt.widgets.TableItem; @@ -1402,7 +1402,7 @@ public static Map getStateMap(PipelineMeta pipelineMeta) { */ public static HopGuiPipelineGraph getPipelineGraph(PipelineMeta pipelineMeta) { // Tab / graph lookup may touch SWT widgets; only safe on the UI thread (issue #7896). - if (Display.getCurrent() == null) { + if (SessionDisplay.current() == null) { return null; } try { @@ -1441,7 +1441,7 @@ public static final PipelineUnitTest getCurrentUnitTest(PipelineMeta pipelineMet return null; } // Same rule as getPipelineGraph: never access HopGui/SWT from a worker thread (issue #7896). - if (Display.getCurrent() == null) { + if (SessionDisplay.current() == null) { return null; } Map stateMap = getStateMap(pipelineMeta); diff --git a/plugins/misc/testing/src/test/java/org/apache/hop/testing/gui/TestingGuiPluginTweakTest.java b/plugins/misc/testing/src/test/java/org/apache/hop/testing/gui/TestingGuiPluginTweakTest.java index 64da9ee26de..697e06a56fc 100644 --- a/plugins/misc/testing/src/test/java/org/apache/hop/testing/gui/TestingGuiPluginTweakTest.java +++ b/plugins/misc/testing/src/test/java/org/apache/hop/testing/gui/TestingGuiPluginTweakTest.java @@ -21,14 +21,18 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mockStatic; import java.util.List; +import org.apache.hop.core.Const; import org.apache.hop.pipeline.PipelineMeta; import org.apache.hop.pipeline.transform.TransformMeta; import org.apache.hop.testing.PipelineTweak; import org.apache.hop.testing.PipelineUnitTest; import org.apache.hop.testing.PipelineUnitTestTweak; +import org.eclipse.swt.widgets.Display; import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; /** * Unit tests for multi-transform unit-test tweak application (issue #2742). Covers pure helpers in @@ -87,6 +91,36 @@ void getCurrentUnitTestReturnsNullOffUiThread() { assertNull(TestingGuiPlugin.getStateMap(new PipelineMeta())); } + /** + * In Hop Web the guard itself used to be the failure (issue #8248): background work carries a RAP + * session over to the thread that runs it, and once that session has been destroyed RWT throws + * from {@code Display.getCurrent()} rather than answering "no display". The exception escaped + * into GetFields, which logged "Error calling extension point 'GetFieldsExtension'" for every + * transform. A thread whose session is gone is a thread with no unit test. + */ + @Test + void getCurrentUnitTestReturnsNullWhenTheSessionIsGone() { + String runtime = System.getProperty(Const.HOP_PLATFORM_RUNTIME); + System.setProperty(Const.HOP_PLATFORM_RUNTIME, "GUI"); + try (MockedStatic display = mockStatic(Display.class)) { + display + .when(Display::getCurrent) + .thenThrow( + new NullPointerException( + "Cannot invoke \"org.eclipse.rap.rwt.service.UISession.getAttribute(String)\"" + + " because \"uiSession\" is null")); + + assertNull(TestingGuiPlugin.getCurrentUnitTest(new PipelineMeta())); + assertNull(TestingGuiPlugin.getStateMap(new PipelineMeta())); + } finally { + if (runtime == null) { + System.clearProperty(Const.HOP_PLATFORM_RUNTIME); + } else { + System.setProperty(Const.HOP_PLATFORM_RUNTIME, runtime); + } + } + } + @Test void applyTweakEnableAddsBypass() { PipelineUnitTest unitTest = new PipelineUnitTest(); diff --git a/rap/src/main/java/org/apache/hop/ui/hopgui/BackgroundThreadFacadeImpl.java b/rap/src/main/java/org/apache/hop/ui/hopgui/BackgroundThreadFacadeImpl.java index 9e4efd74f8a..aa8c96ffb9e 100644 --- a/rap/src/main/java/org/apache/hop/ui/hopgui/BackgroundThreadFacadeImpl.java +++ b/rap/src/main/java/org/apache/hop/ui/hopgui/BackgroundThreadFacadeImpl.java @@ -35,7 +35,18 @@ Runnable bindInternal(Runnable runnable) { } // exec() binds the session to whatever thread calls it, which is the point here: the work // itself runs exactly as before, only now with a session to ask. - return () -> uiSession.exec(runnable); + return () -> { + if (!uiSession.isBound()) { + // The session ended between scheduling this work and running it - the browser closed, the + // user logged out, the session timed out. Binding it anyway leaves the thread with a + // context that resolves to nothing, which is worse than no context at all: RWT reads it + // back unchecked and even asking whether this thread has a display then fails with a + // NullPointerException (issue #8248). The work itself never needed the session. + runnable.run(); + return; + } + uiSession.exec(runnable); + }; } /** The session serving this thread, or null when there is none to carry over. */ diff --git a/rap/src/test/java/org/apache/hop/ui/hopgui/BackgroundThreadFacadeImplTest.java b/rap/src/test/java/org/apache/hop/ui/hopgui/BackgroundThreadFacadeImplTest.java index 853f0b75c88..4552f50094d 100644 --- a/rap/src/test/java/org/apache/hop/ui/hopgui/BackgroundThreadFacadeImplTest.java +++ b/rap/src/test/java/org/apache/hop/ui/hopgui/BackgroundThreadFacadeImplTest.java @@ -20,8 +20,10 @@ import static org.junit.jupiter.api.Assertions.assertSame; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; import org.eclipse.rap.rwt.RWT; import org.eclipse.rap.rwt.service.UISession; @@ -40,6 +42,7 @@ class BackgroundThreadFacadeImplTest { @DisplayName("work started from a session runs with that session bound") void carriesTheSessionOverToTheBackgroundThread() { UISession uiSession = mock(UISession.class); + when(uiSession.isBound()).thenReturn(true); Runnable work = mock(Runnable.class); try (MockedStatic rwt = mockStatic(RWT.class)) { @@ -48,7 +51,7 @@ void carriesTheSessionOverToTheBackgroundThread() { Runnable bound = new BackgroundThreadFacadeImpl().bindInternal(work); // The session is read here, on the thread that starts the work; the work itself waits. - verifyNoInteractions(uiSession, work); + verifyNoInteractions(work); bound.run(); @@ -57,6 +60,28 @@ void carriesTheSessionOverToTheBackgroundThread() { } } + /** + * A session that ended while the work was queued must not be made current: RWT hands out a + * context that resolves to no session, and reading it back fails with a NullPointerException as + * far away as {@code Display.getCurrent()} (issue #8248). The work still runs. + */ + @Test + @DisplayName("a session that ended in the meantime is not carried over") + void runsWithoutASessionThatDiedBeforeTheWorkStarted() { + UISession uiSession = mock(UISession.class); + when(uiSession.isBound()).thenReturn(false); + Runnable work = mock(Runnable.class); + + try (MockedStatic rwt = mockStatic(RWT.class)) { + rwt.when(() -> RWT.getUISession()).thenReturn(uiSession); + + new BackgroundThreadFacadeImpl().bindInternal(work).run(); + + verify(work).run(); + verify(uiSession, never()).exec(work); + } + } + @Test @DisplayName("without a session there is nothing to carry over") void leavesTheWorkAloneOutsideASession() { diff --git a/ui/src/main/java/org/apache/hop/core/SwtUniversalImage.java b/ui/src/main/java/org/apache/hop/core/SwtUniversalImage.java index e2804fe4fe5..750ee54344d 100644 --- a/ui/src/main/java/org/apache/hop/core/SwtUniversalImage.java +++ b/ui/src/main/java/org/apache/hop/core/SwtUniversalImage.java @@ -133,7 +133,7 @@ static boolean isDpiAwareImageProviderSupported() { * Pixel size of a logical extent at an SWT zoom percentage. Must be linear ({@code 200} → {@code * 2 * 100}) to satisfy the desktop ImageDataProvider contract. */ - static int pixelSize(int logical, int zoom) { + public static int pixelSize(int logical, int zoom) { return Math.max(1, logical * zoom / 100); } diff --git a/ui/src/main/java/org/apache/hop/ui/core/gui/HopNamespace.java b/ui/src/main/java/org/apache/hop/ui/core/gui/HopNamespace.java index f3d71258fe8..501b616e16e 100644 --- a/ui/src/main/java/org/apache/hop/ui/core/gui/HopNamespace.java +++ b/ui/src/main/java/org/apache/hop/ui/core/gui/HopNamespace.java @@ -22,6 +22,7 @@ import org.apache.hop.core.Const; import org.apache.hop.core.exception.HopRuntimeException; import org.apache.hop.core.util.Utils; +import org.apache.hop.ui.hopgui.SessionDisplay; import org.eclipse.swt.SWT; import org.eclipse.swt.widgets.Display; @@ -97,7 +98,7 @@ private static boolean hasUserInterface() { /** The namespace of the session on this thread, or null. Only call with a user interface. */ private static String namespaceOfCurrentDisplay() { - Display display = Display.getCurrent(); + Display display = SessionDisplay.current(); if (display == null || display.isDisposed()) { return null; } @@ -106,7 +107,7 @@ private static String namespaceOfCurrentDisplay() { /** Remember the namespace for the session on this thread. Only call with a user interface. */ private static void rememberForCurrentDisplay(String namespace) { - Display display = Display.getCurrent(); + Display display = SessionDisplay.current(); if (display == null || display.isDisposed()) { return; } diff --git a/ui/src/main/java/org/apache/hop/ui/core/widget/TreeMemory.java b/ui/src/main/java/org/apache/hop/ui/core/widget/TreeMemory.java index d762498a406..8fe7bd04e9c 100644 --- a/ui/src/main/java/org/apache/hop/ui/core/widget/TreeMemory.java +++ b/ui/src/main/java/org/apache/hop/ui/core/widget/TreeMemory.java @@ -21,6 +21,7 @@ import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import org.apache.hop.ui.core.ConstUi; +import org.apache.hop.ui.hopgui.SessionDisplay; import org.eclipse.swt.SWT; import org.eclipse.swt.events.TreeEvent; import org.eclipse.swt.events.TreeListener; @@ -40,7 +41,7 @@ public class TreeMemory { private Map map; public static final TreeMemory getInstance() { - Display display = Display.getCurrent(); + Display display = SessionDisplay.current(); if (display != null && !display.isDisposed()) { return BY_DISPLAY.computeIfAbsent( display, diff --git a/ui/src/main/java/org/apache/hop/ui/hopgui/SessionDisplay.java b/ui/src/main/java/org/apache/hop/ui/hopgui/SessionDisplay.java new file mode 100644 index 00000000000..e2f0deb9129 --- /dev/null +++ b/ui/src/main/java/org/apache/hop/ui/hopgui/SessionDisplay.java @@ -0,0 +1,77 @@ +/* + * 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.hopgui; + +import org.eclipse.swt.widgets.Display; + +/** + * Answers the question "does this thread have a display to work with?" without ever throwing. + * + *

{@code Display.getCurrent()} is the usual way to ask, and on the desktop it is a plain + * thread-to-display lookup that cannot fail. In Hop Web it is not: RWT resolves the display through + * the RAP session bound to the calling thread, so asking the question reaches into that session. + * + *

{@link BackgroundThreadFacade} deliberately binds a session to the threads that run background + * work, so those threads do have one to reach into. A session that has since been destroyed - the + * browser closed, the user logged out, the session timed out - is still bound to the thread but no + * longer resolves, and RWT dereferences it unchecked: the guard meant to keep background threads + * away from the UI is where the {@code NullPointerException} comes from instead (issue #8248, + * following #7896 / #7897). + * + *

A thread whose session died is a thread with no display, which is what these methods answer. + */ +public class SessionDisplay { + + private SessionDisplay() { + // Utility class + } + + /** + * The display this thread is the user interface thread for. + * + * @return the display, or null when this thread has none - including when the session that would + * have provided it is gone + */ + public static Display current() { + try { + return Display.getCurrent(); + } catch (RuntimeException e) { + return null; + } + } + + /** + * The display of this thread, falling back to the default display. + * + *

For callers that want a display to schedule work on rather than an answer about the calling + * thread. The fallback resolves through the same session, so it is guarded too. + * + * @return a usable display, or null when there is none + */ + public static Display currentOrDefault() { + Display display = current(); + if (display != null) { + return display; + } + try { + return Display.getDefault(); + } catch (RuntimeException e) { + return null; + } + } +} diff --git a/ui/src/main/java/org/apache/hop/ui/hopgui/shared/SashFormMemory.java b/ui/src/main/java/org/apache/hop/ui/hopgui/shared/SashFormMemory.java index 502f6ad4aab..77e6c7f43b0 100644 --- a/ui/src/main/java/org/apache/hop/ui/hopgui/shared/SashFormMemory.java +++ b/ui/src/main/java/org/apache/hop/ui/hopgui/shared/SashFormMemory.java @@ -27,6 +27,7 @@ import org.apache.hop.history.AuditList; import org.apache.hop.history.AuditManager; import org.apache.hop.ui.hopgui.HopGui; +import org.apache.hop.ui.hopgui.SessionDisplay; import org.eclipse.swt.SWT; import org.eclipse.swt.custom.SashForm; import org.eclipse.swt.widgets.Control; @@ -155,7 +156,7 @@ private static void save(SashForm sashForm, String key) { * startup). */ public static void resetAll() { - Display display = Display.getCurrent(); + Display display = SessionDisplay.current(); if (display == null || display.isDisposed()) { return; } diff --git a/ui/src/test/java/org/apache/hop/ui/hopgui/SessionDisplayTest.java b/ui/src/test/java/org/apache/hop/ui/hopgui/SessionDisplayTest.java new file mode 100644 index 00000000000..79ef812863a --- /dev/null +++ b/ui/src/test/java/org/apache/hop/ui/hopgui/SessionDisplayTest.java @@ -0,0 +1,88 @@ +/* + * 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.hopgui; + +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; + +import org.eclipse.swt.widgets.Display; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; + +/** + * In Hop Web, asking whether this thread has a display goes through the RAP session bound to it. + * When that session has been destroyed - the browser closed, the user logged out, the session timed + * out - RWT dereferences it unchecked and the question itself throws (issue #8248). The answer for + * such a thread is "no display", not an exception thrown out of a guard clause. + */ +class SessionDisplayTest { + + /** What RWT throws out of {@code LifeCycleUtil.getSessionDisplay} for a destroyed session. */ + private static NullPointerException deadSession() { + return new NullPointerException( + "Cannot invoke \"org.eclipse.rap.rwt.service.UISession.getAttribute(String)\"" + + " because \"uiSession\" is null"); + } + + @Test + @DisplayName("a thread whose session is gone has no display, rather than an exception") + void currentAnswersNullWhenTheSessionIsGone() { + try (MockedStatic display = mockStatic(Display.class)) { + display.when(Display::getCurrent).thenThrow(deadSession()); + + assertNull(SessionDisplay.current()); + } + } + + @Test + @DisplayName("the display of a live session is handed back unchanged") + void currentAnswersTheDisplayOfTheThread() { + Display sessionDisplay = mock(Display.class); + try (MockedStatic display = mockStatic(Display.class)) { + display.when(Display::getCurrent).thenReturn(sessionDisplay); + + assertSame(sessionDisplay, SessionDisplay.current()); + } + } + + @Test + @DisplayName("the fallback to the default display is guarded the same way") + void currentOrDefaultAnswersNullWhenNeitherResolves() { + try (MockedStatic display = mockStatic(Display.class)) { + display.when(Display::getCurrent).thenThrow(deadSession()); + display.when(Display::getDefault).thenThrow(deadSession()); + + assertNull(SessionDisplay.currentOrDefault()); + } + } + + @Test + @DisplayName("a thread without a display of its own falls back to the default one") + void currentOrDefaultFallsBackToTheDefaultDisplay() { + Display defaultDisplay = mock(Display.class); + try (MockedStatic display = mockStatic(Display.class)) { + display.when(Display::getCurrent).thenReturn(null); + display.when(Display::getDefault).thenReturn(defaultDisplay); + + assertSame(defaultDisplay, SessionDisplay.currentOrDefault()); + } + } +}