Skip to content

[GTK] Change the zoom only on scale factor changes of the Shell window - #3679

Open
vogella wants to merge 1 commit into
eclipse-platform:masterfrom
vogella:styledtext-skin-3458
Open

vogella wants to merge 1 commit into
eclipse-platform:masterfrom
vogella:styledtext-skin-3458

Conversation

@vogella

@vogella vogella commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

GTK notifies scale-factor on every widget, including a child that is attached to its parent inside the Control constructor. SWT took this for a DPI change, so the half-built child changed the global zoom, received SWT.ZoomChanged and laid out all shells, which sent SWT.Skin to custom widgets like StyledText before their constructor had finished and caused a NullPointerException. Now only the Shell window's notification changes the zoom and sends SWT.ZoomChanged, while image widgets still refresh their images. Regression tests cover both the early Skin event and the Shell path.

Fixes #3458

@vogella
vogella requested a balanced review from Copilot October 6, 2026 11:33
@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

  224 files  ± 0    224 suites  ±0   26m 52s ⏱️ + 1m 41s
5 033 tests + 2  5 005 ✅ +2   28 💤 ±0  0 ❌ ±0 
7 482 runs  +12  7 261 ✅ +6  221 💤 +6  0 ❌ ±0 

Results for commit 4c1306b. ± Comparison against base commit 18fe3cd.

♻️ This comment has been updated with latest results.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The focused fix addresses the reported construction-time event ordering and includes appropriate regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Defers GTK DPI-change layouts to prevent SWT.Skin events during widget construction.

Changes:

  • Schedules shell relayout asynchronously after scale-factor changes.
  • Adds a GTK regression test covering StyledText construction.
File Description
Display.java Defers DPI-triggered shell layouts.
Test_org_eclipse_swt_custom_StyledText.java Verifies skin events occur after construction.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@vogella
vogella force-pushed the styledtext-skin-3458 branch from 15653c2 to 52ec5b7 Compare October 7, 2026 06:59
@akurtakov

akurtakov commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

Some analysis follow:
Cause: notify::scale-factor is connected on every widget. When a child is attached to its parent, GTK corrects that child's scale, and SWT mistakes this for a DPI change. So the half-built child sets the global zoom and lays out all shells, and that sends the Skin events. With this PR the Skin event is no longer early, but the zoom still flips, and ZoomChanged still goes to the half-built widget.

Suggestion: only the Shell's own window notification should change the global zoom and I don't see a reason why/when non-Shells should send zoom-changed on Gtk. The per-widget hook can stay for refreshing images:

// Widget.dpiChanged: return 0;

// Shell
@OverRide
long dpiChanged (long object, long arg0) {
if (object == shellHandle) {
int scale = GTK.gtk_widget_get_scale_factor (shellHandle);
if (DPIUtil.getDeviceZoom () / 100 != scale) {
display.dpiChanged (scale);
Event event = new Event ();
event.detail = scale;
event.doit = true;
notifyListeners (SWT.ZoomChanged, event);
}
}
return super.dpiChanged (object, arg0);
}

On both GTK3 and GTK4 that should have the effect of no early Skin event and the zoom doesn't change, while real scale changes still work. That should make the async call not needed and make things even more predictable. Please try it.

@vogella
vogella force-pushed the styledtext-skin-3458 branch from 52ec5b7 to a795fce Compare October 9, 2026 04:52
@vogella vogella changed the title [GTK] Defer the layout after a scale factor change to avoid early SWT.Skin events [GTK] Change the zoom only on scale factor changes of the Shell window Oct 9, 2026
@vogella

vogella commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, applied.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The Shell event reports a native scale factor instead of the documented zoom percentage.

1 open finding

🧠 Review effort: Balanced

Comment thread bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Shell.java Outdated
@vogella
vogella force-pushed the styledtext-skin-3458 branch 2 times, most recently from 7cb0836 to d4dba30 Compare October 9, 2026 13:41
GTK notifies scale-factor on every widget, including a child that is being attached to its parent inside the Control constructor. SWT took this for a DPI change, so the half-built child changed the global zoom, received SWT.ZoomChanged and laid out all shells, which sent SWT.Skin to custom widgets like StyledText before their constructor had initialized them. Only the notification of the Shell window now changes the zoom and sends SWT.ZoomChanged; other widgets still refresh their images.

Fixes eclipse-platform#3458

Assisted-by: multiple AI agents and layers of automated tooling 🤖

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Shell DPI handling mishandles fixed autoscaling and GTK4 popover-backed child Shells.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

@Override
long dpiChanged (long object, long arg0) {
// Only the window changes the global zoom, children report their scale while being attached
if (object == shellHandle) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a must have to not regress on Gtk 4.

@akurtakov

Copy link
Copy Markdown
Member

The tests are really complicated and non-generic enough. It would be nice to have them in org.eclipse.swt.tests.gtk and use Gtk specific methods directly so it's easier to understand what it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[WSL] StyledText can send SWT.Skin event too early

3 participants