mattcasters commented on code in PR #9185:
URL: https://github.com/apache/hop/pull/9185#discussion_r4435547221
##########
ui/src/main/java/org/apache/hop/ui/hopgui/terminal/JediTerminalWidget.java:
##########
@@ -205,6 +220,36 @@ private void sendTabToShell() {
}
}
+ /**
+ * On Linux, SWT follows the desktop scale (GTK window scaling) but AWT does
not, so on a HiDPI
+ * screen the terminal font comes out smaller than the rest of Hop. Return
the factor that makes
+ * up the difference: the SWT zoom divided by the AWT scale. On Windows and
macOS AWT scales
+ * itself, so the factor is 1.
+ */
+ private float getHiDpiFontScale() {
+ if (!Const.isLinux()) {
+ return 1.0f;
+ }
+ try {
+ double awtScale =
+ GraphicsEnvironment.getLocalGraphicsEnvironment()
+ .getDefaultScreenDevice()
+ .getDefaultConfiguration()
+ .getDefaultTransform()
+ .getScaleX();
+ int swtZoom = bridgeComposite.getMonitor().getZoom();
+ float scale = (float) Math.max(1.0, swtZoom / (100.0 * awtScale));
Review Comment:
**[bug]** `Monitor.getZoom()` is not the scale Hop's GTK fonts use. On X11,
SWT sets that zoom from `gdk_monitor_get_scale_factor * 100` only
(`Display._getDeviceZoom`). On a 3456×2160 session here the scale factor is 1,
so zoom is 100, while `gdk_screen_get_resolution` is 192 DPI (`Xft.dpi` / GNOME
`text-scaling-factor` 2.0). AWT's default transform `scaleX` is 1.0 on that
same session, so this formula returns 1 and the terminal stays at 13px while
the rest of Hop is drawn at 192 DPI. Integer `GDK_SCALE=2` is the only case
that enlarges the font. The value is also captured once, from the default AWT
device and this composite's monitor, before the tab is necessarily on the
monitor it will be shown on.
**Suggestion:** Use `max(zoom / 100f, display.getDPI().x / 96f) / awtScale`.
GTK3 `Display.getDPI()` is `gdk_screen_get_resolution` (192 here); GTK4
hardcodes 96, so keep zoom in the max. Read the AWT scale from the embedded
frame's graphics configuration once that frame has one, recompute when the
shell is shown or moved, and do not clamp away a factor below 1.
##########
ui/src/main/java/org/apache/hop/ui/hopgui/terminal/JediTerminalWidget.java:
##########
@@ -461,74 +505,82 @@ public String getWorkingDirectory() {
return workingDirectory;
}
- /** Adapter connecting Pty4J to JediTerm's TtyConnector */
- private static class Pty4JTtyConnector implements TtyConnector {
-
- private final PtyProcess ptyProcess;
- private final InputStream inputStream;
- private final OutputStream outputStream;
+ /**
+ * TerminalPanel that falls back to another installed font for characters
the terminal font cannot
+ * draw. On Linux, Java's logical Monospaced font leaves out the fallback
fonts fontconfig would
+ * use, so CJK and similar characters otherwise render as empty boxes.
+ */
+ private static class FallbackFontTerminalPanel extends TerminalPanel {
- public Pty4JTtyConnector(PtyProcess ptyProcess) {
- this.ptyProcess = ptyProcess;
- this.inputStream = ptyProcess.getInputStream();
- this.outputStream = ptyProcess.getOutputStream();
- }
+ /** Fallback per code point; empty when no installed font can draw it.
Shared by all tabs. */
+ private static final Map<Integer, Optional<Font>> FALLBACK_FONTS = new
ConcurrentHashMap<>();
- public boolean init() {
- return true;
+ FallbackFontTerminalPanel(
+ SettingsProvider settingsProvider,
+ TerminalTextBuffer terminalTextBuffer,
+ StyleState styleState) {
+ super(settingsProvider, terminalTextBuffer, styleState);
}
- public void close() {
- try {
- if (ptyProcess != null && ptyProcess.isAlive()) {
- ptyProcess.destroy();
- }
- } catch (Exception e) {
- log.logError("Error closing Pty4J connector", e);
+ @Override
+ protected Font getFontToDisplay(char[] text, int start, int end, TextStyle
style) {
+ Font font = super.getFontToDisplay(text, start, end, style);
+ if (font.canDisplayUpTo(text, start, end) == -1) {
+ return font;
}
+ return FALLBACK_FONTS
+ .computeIfAbsent(
+ Character.codePointAt(text, start), codePoint ->
findFallbackFont(text, start, end))
Review Comment:
**[suggestion]** The map is keyed only by the first code point, but
`findFallbackFont` is asked to cover `text[start, end)`, and a hit is returned
without another `canDisplayUpTo` check. `TerminalPanel.drawChars` calls
`getFontToDisplay` once per grapheme cluster (a CJK ideograph plus a variation
selector, or a ZWJ emoji sequence), not once per code point. The first cluster
that no installed font can cover stores `Optional.empty()` for that base
character, and every later cluster that starts with it, including a plain
character a fallback font can draw, stays a box for the rest of the JVM. The
map is static and shared by every terminal tab.
**Suggestion:** Key the cache by the code points in `[start, end)`, or on a
hit re-check `canDisplayUpTo` and search again when the cached font cannot draw
this slice. Do not cache an empty result for a multi-code-point cluster under
only the first code point.
##########
ui/src/main/java/org/apache/hop/ui/hopgui/terminal/JediTerminalWidget.java:
##########
@@ -461,74 +505,82 @@ public String getWorkingDirectory() {
return workingDirectory;
}
- /** Adapter connecting Pty4J to JediTerm's TtyConnector */
- private static class Pty4JTtyConnector implements TtyConnector {
-
- private final PtyProcess ptyProcess;
- private final InputStream inputStream;
- private final OutputStream outputStream;
+ /**
+ * TerminalPanel that falls back to another installed font for characters
the terminal font cannot
+ * draw. On Linux, Java's logical Monospaced font leaves out the fallback
fonts fontconfig would
+ * use, so CJK and similar characters otherwise render as empty boxes.
+ */
+ private static class FallbackFontTerminalPanel extends TerminalPanel {
- public Pty4JTtyConnector(PtyProcess ptyProcess) {
- this.ptyProcess = ptyProcess;
- this.inputStream = ptyProcess.getInputStream();
- this.outputStream = ptyProcess.getOutputStream();
- }
+ /** Fallback per code point; empty when no installed font can draw it.
Shared by all tabs. */
+ private static final Map<Integer, Optional<Font>> FALLBACK_FONTS = new
ConcurrentHashMap<>();
- public boolean init() {
- return true;
+ FallbackFontTerminalPanel(
+ SettingsProvider settingsProvider,
+ TerminalTextBuffer terminalTextBuffer,
+ StyleState styleState) {
+ super(settingsProvider, terminalTextBuffer, styleState);
}
- public void close() {
- try {
- if (ptyProcess != null && ptyProcess.isAlive()) {
- ptyProcess.destroy();
- }
- } catch (Exception e) {
- log.logError("Error closing Pty4J connector", e);
+ @Override
+ protected Font getFontToDisplay(char[] text, int start, int end, TextStyle
style) {
+ Font font = super.getFontToDisplay(text, start, end, style);
+ if (font.canDisplayUpTo(text, start, end) == -1) {
+ return font;
}
+ return FALLBACK_FONTS
+ .computeIfAbsent(
+ Character.codePointAt(text, start), codePoint ->
findFallbackFont(text, start, end))
+ .map(fallback -> fallback.deriveFont(font.getStyle(),
font.getSize2D()))
+ .orElse(font);
}
- public String getName() {
- return "Pty4J Connector";
- }
-
- public int read(char[] buf, int offset, int length) throws IOException {
- byte[] bytes = new byte[length];
- int bytesRead = inputStream.read(bytes, 0, length);
- if (bytesRead > 0) {
- String str = new String(bytes, 0, bytesRead, StandardCharsets.UTF_8);
- char[] chars = str.toCharArray();
- System.arraycopy(chars, 0, buf, offset, Math.min(chars.length,
length));
- return chars.length;
+ /** First installed font family that can draw the text, preferring a
monospaced one. */
+ private static Optional<Font> findFallbackFont(char[] text, int start, int
end) {
+ Font fallback = null;
+ for (String family :
Review Comment:
**[suggestion]** `getFontToDisplay` runs on the EDT from `drawChars`. Each
distinct missing code point walks every installed family and constructs a
`Font` until a name containing `"mono"` can draw the slice. This machine has
1742 families; a complete miss is about 40ms once the JDK font cache is warm,
and the first pass that loads font files is slower. The winning family is not
remembered, so the next code point starts over. The `"mono"` test also matches
the logical family `Monospaced`, which is the font that already failed, before
a real CJK monospace face.
**Suggestion:** Remember the family that worked (and a single negative
result when nothing installed can draw the slice) and try that font before
scanning. Prefer a monospace face that is not the logical `Monospaced`/`Dialog`
font. Run the scan off the paint thread if it still has to touch every family.
##########
ui/src/main/java/org/apache/hop/ui/hopgui/terminal/JediTerminalWidget.java:
##########
@@ -461,74 +505,82 @@ public String getWorkingDirectory() {
return workingDirectory;
}
- /** Adapter connecting Pty4J to JediTerm's TtyConnector */
- private static class Pty4JTtyConnector implements TtyConnector {
-
- private final PtyProcess ptyProcess;
- private final InputStream inputStream;
- private final OutputStream outputStream;
+ /**
+ * TerminalPanel that falls back to another installed font for characters
the terminal font cannot
+ * draw. On Linux, Java's logical Monospaced font leaves out the fallback
fonts fontconfig would
+ * use, so CJK and similar characters otherwise render as empty boxes.
+ */
+ private static class FallbackFontTerminalPanel extends TerminalPanel {
- public Pty4JTtyConnector(PtyProcess ptyProcess) {
- this.ptyProcess = ptyProcess;
- this.inputStream = ptyProcess.getInputStream();
- this.outputStream = ptyProcess.getOutputStream();
- }
+ /** Fallback per code point; empty when no installed font can draw it.
Shared by all tabs. */
+ private static final Map<Integer, Optional<Font>> FALLBACK_FONTS = new
ConcurrentHashMap<>();
- public boolean init() {
- return true;
+ FallbackFontTerminalPanel(
+ SettingsProvider settingsProvider,
+ TerminalTextBuffer terminalTextBuffer,
+ StyleState styleState) {
+ super(settingsProvider, terminalTextBuffer, styleState);
}
- public void close() {
- try {
- if (ptyProcess != null && ptyProcess.isAlive()) {
- ptyProcess.destroy();
- }
- } catch (Exception e) {
- log.logError("Error closing Pty4J connector", e);
+ @Override
+ protected Font getFontToDisplay(char[] text, int start, int end, TextStyle
style) {
+ Font font = super.getFontToDisplay(text, start, end, style);
+ if (font.canDisplayUpTo(text, start, end) == -1) {
+ return font;
}
+ return FALLBACK_FONTS
+ .computeIfAbsent(
+ Character.codePointAt(text, start), codePoint ->
findFallbackFont(text, start, end))
+ .map(fallback -> fallback.deriveFont(font.getStyle(),
font.getSize2D()))
+ .orElse(font);
}
- public String getName() {
- return "Pty4J Connector";
- }
-
- public int read(char[] buf, int offset, int length) throws IOException {
- byte[] bytes = new byte[length];
- int bytesRead = inputStream.read(bytes, 0, length);
- if (bytesRead > 0) {
- String str = new String(bytes, 0, bytesRead, StandardCharsets.UTF_8);
- char[] chars = str.toCharArray();
- System.arraycopy(chars, 0, buf, offset, Math.min(chars.length,
length));
- return chars.length;
+ /** First installed font family that can draw the text, preferring a
monospaced one. */
+ private static Optional<Font> findFallbackFont(char[] text, int start, int
end) {
+ Font fallback = null;
+ for (String family :
+
GraphicsEnvironment.getLocalGraphicsEnvironment().getAvailableFontFamilyNames())
{
+ Font candidate = new Font(family, Font.PLAIN, 1);
+ if (candidate.canDisplayUpTo(text, start, end) == -1) {
+ if (family.toLowerCase().contains("mono")) {
+ return Optional.of(candidate);
+ }
+ if (fallback == null) {
+ fallback = candidate;
+ }
+ }
}
- return bytesRead;
- }
-
- public void write(byte[] bytes) throws IOException {
- outputStream.write(bytes);
- outputStream.flush();
+ return Optional.ofNullable(fallback);
}
+ }
- public boolean isConnected() {
- return ptyProcess != null && ptyProcess.isAlive();
- }
+ /**
+ * Adapter connecting Pty4J to JediTerm's TtyConnector. ProcessTtyConnector
decodes the output
+ * with a stateful reader, so multi-byte characters split across reads stay
intact.
+ */
+ private static class Pty4JTtyConnector extends ProcessTtyConnector {
- public void write(String string) throws IOException {
- write(string.getBytes(StandardCharsets.UTF_8));
- }
+ private final PtyProcess ptyProcess;
- public int waitFor() throws InterruptedException {
- return ptyProcess.waitFor();
+ public Pty4JTtyConnector(PtyProcess ptyProcess) {
+ super(ptyProcess, StandardCharsets.UTF_8);
+ this.ptyProcess = ptyProcess;
}
- public boolean ready() throws IOException {
- return inputStream.available() > 0;
+ @Override
+ public String getName() {
+ return "Pty4J Connector";
}
- public void resize(int cols, int rows) {
- if (ptyProcess != null && ptyProcess.isAlive()) {
+ /**
+ * Pass the terminal size on to the PTY so the shell wraps lines at the
visible width. JediTerm
+ * only calls this overload; its default implementation throws.
+ */
+ @Override
+ public void resize(TermSize termSize) {
Review Comment:
**[suggestion]** Overriding `resize(TermSize)` is what makes the PTY follow
the grid, and pty4j 0.14's `WinSize` only keeps columns and rows, so dropping
the pixel size is fine. `TerminalStarter` in 3.76 calls this immediately unless
the connector implements `TtyConnectorResizeStrategyProvider`. This one does
not, so Windows ConPTY is resized in the same turn as the screen buffer.
JediTerm postpones that case because a ConPTY screen update can land on the
already-resized buffer and stick lines in the scrollback.
**Suggestion:** Implement `TtyConnectorResizeStrategyProvider` and return
`POSTPONED` on Windows and `IMMEDIATE` elsewhere.
--
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]
