mattcasters commented on code in PR #8945:
URL: https://github.com/apache/hop/pull/8945#discussion_r4283669990
##########
ui/src/main/java/org/apache/hop/ui/hopgui/shared/SwtGc.java:
##########
@@ -617,19 +617,27 @@ public boolean drawFileImage(String path, int x, int y,
int width, int height) {
}
try (java.io.InputStream in =
org.apache.hop.core.vfs.HopVfs.getInputStream(path)) {
org.eclipse.swt.graphics.ImageData data = new
org.eclipse.swt.graphics.ImageData(in);
+ // Scale the decoded raster to the requested logical size once. The
DPI-aware provider then
+ // only re-scales this small bitmap by the monitor zoom; scaling the
full native resolution
+ // by zoom (the previous behaviour) could allocate an enormous buffer
for a large source and
+ // fail, silently falling back to the alt text.
+ int w = Math.max(1, width);
+ int h = Math.max(1, height);
+ org.eclipse.swt.graphics.ImageData scaled =
+ (data.width == w && data.height == h) ? data : data.scaledTo(w, h);
Image img =
SwtUniversalImage.createDpiAwareImage(
gc.getDevice(),
zoom -> {
if (zoom == 100) {
- return data;
+ return scaled;
}
- int w = Math.max(1, data.width * zoom / 100);
- int h = Math.max(1, data.height * zoom / 100);
- return data.scaledTo(w, h);
+ int zw = Math.max(1, scaled.width * zoom / 100);
Review Comment:
**[bug]** The DPI provider scales with truncating integer arithmetic
(`scaled.width * zoom / 100`). SWT 3.134 on Windows does not. `GC.drawImage`
(the 9-argument overload) turns the logical source rectangle into device pixels
with `Math.round`, then `Image.getOrCreateImageHandleAtBestFittingSize` asks
this provider for a zoom snapped by `DPIUtil.getZoomForAutoscaleProperty`. On
Windows that default is quarter scaling (25, 50, 75, …) because
monitor-specific scaling is on unless `swt.autoScale.updateOnRuntime=false`.
Below zoom 100 the allowed error is `closestZoom / 100 == 0`, so a 1px mismatch
throws `IllegalArgumentException` (`SWT.ERROR_INVALID_ARGUMENT`).
`drawFileImage` swallows that and returns false, and
`MarkdownNoteRenderer.paintImageBlock` replaces the image with alt text. The
canvas GC transform is `userZoom * PropsUi.getNativeZoomFactor()`
(`PipelinePainter.drawPipelineImage` / `HopGuiPipelineGraph`), and the built-in
levels include 25%, 50%, and 75%. At 100% DPI tho
se transforms are below 1, and a large fraction of widths fail (for example
logical 102×102 at zoom 75: the handle is 76px and the rounded source is 77px).
Resizing the note at those zoom levels still makes the PNG appear and
disappear, which is the #8869 symptom. At transform scale ≥ 1 (normal 100%
zoom) the same simulation does not throw, so the default-zoom fix holds.
Passing `scaled.width` / `scaled.height` as the source rectangle is the correct
logical size (`getBounds()` is the zoom-100 variant); the mismatch is the
device bitmap, not the arguments themselves. The SVG branch in this method is
unchanged.
**Suggestion:** Scale with `Math.round(scaled.width * (zoom / 100.0))` (the
same rounding as `DPIUtil.pointToPixel`), not integer division.
`SwtUniversalImage.pixelSize` has the same truncation, so do not copy it. More
robust on SWT 3.134 is the 4-argument `gc.drawImage(img, x, y, w, h)`, which
uses the handle’s real pixel size as the source and does not apply this bounds
check. Keep a single scale from the decoded raster to that device size so a
large PNG is not expanded to native×zoom.
##########
engine/src/main/java/org/apache/hop/core/gui/markdown/NoteImageSupport.java:
##########
@@ -177,22 +183,43 @@ public static boolean isSvgPath(String path) {
return lower.endsWith(".svg");
}
- /** Fit image into max box preserving aspect ratio. */
+ /**
+ * Fit image into max box preserving aspect ratio, bounded to a drawable
raster range. A 1px
+ * sliver or a native-resolution raster at high zoom makes {@code
drawFileImage} fail and fall
+ * back to the alt text, so keep the result within [{@link
#MIN_RENDERED_SIZE}, {@link
+ * #MAX_RENDERED_SIZE}].
+ */
public static Point fit(int srcW, int srcH, int maxWidth, int maxHeight) {
if (srcW <= 0 || srcH <= 0) {
- return new Point(Math.max(1, maxWidth), Math.max(1, maxHeight));
+ return new Point(bound(maxWidth), bound(maxHeight));
}
- int mw = Math.max(1, maxWidth);
- int mh = Math.max(1, maxHeight);
+ int mw = bound(maxWidth);
+ int mh = bound(maxHeight);
double scale = Math.min((double) mw / srcW, (double) mh / srcH);
if (scale > 1.0) {
scale = 1.0; // do not upscale small icons excessively on canvas
}
int w = Math.max(1, (int) Math.round(srcW * scale));
int h = Math.max(1, (int) Math.round(srcH * scale));
+
+ if (w < MIN_RENDERED_SIZE || h < MIN_RENDERED_SIZE) {
+ double up = Math.max((double) MIN_RENDERED_SIZE / w, (double)
MIN_RENDERED_SIZE / h);
Review Comment:
**[bug]** `MarkdownNoteRenderer.paintImageBlock` uses `fit()` both as the
`drawFileImage` size and as the block’s layout size, and the note is not
clipped. The new minimum upscale ignores `mw` / `mh`. `fit(2000, 100, 20, 200)`
(the new test’s input) is 20×1 after the aspect fit, then ×8, and returns 160×8
— eight times the requested max width, aspect preserved, so the image is
painted outside the note. A 2000×40 banner in a 100px-wide note becomes 400×8.
The following max downscale does not restore the minimum: `fit(100000, 1, 500,
200)` upscales to 4000×8 and then downscales to 2048×4, so one side is below
`MIN_RENDERED_SIZE` again and the layout width is 2048 against a 500px box. The
new test only asserts `>= MIN` and `<= MAX`, so both results pass.
**Suggestion:** Keep the size returned to the renderer inside the caller’s
max box (and at most `MAX_RENDERED_SIZE`). If a side would fall below
`MIN_RENDERED_SIZE`, leave that axis short rather than growing the other axis
past `mw` / `mh`. Apply the min clamp after the max clamp, or skip the upscale
when it cannot be done inside the box. Extend `fitBoundsExtremeDimensions` to
assert the max box and a wide aspect ratio (the 2000×100 / 20×200 case should
stay within 20×200).
##########
ui/src/main/java/org/apache/hop/ui/hopgui/shared/SwtGc.java:
##########
@@ -617,19 +617,27 @@ public boolean drawFileImage(String path, int x, int y,
int width, int height) {
}
try (java.io.InputStream in =
org.apache.hop.core.vfs.HopVfs.getInputStream(path)) {
org.eclipse.swt.graphics.ImageData data = new
org.eclipse.swt.graphics.ImageData(in);
+ // Scale the decoded raster to the requested logical size once. The
DPI-aware provider then
Review Comment:
**[suggestion]** The new comment restates the scale and narrates the
previous native-resolution×zoom failure. The new `fit` javadoc does the same
(1px sliver, alt-text fallback, and a `[MIN, MAX]` range the method does not
actually guarantee).
**Suggestion:** Keep a short note that the raster is scaled to the logical
size before the DPI zoom, and drop the history. State the real `fit` bounds in
the javadoc only if the clamp is fixed to match them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
