Terminal: fix moving a terminal tab to another terminals view by insjang · Pull Request #2979 · eclipse-platform/eclipse.platform

insjang · 2026-09-30T08:10:29Z

Dragging a terminal tab to another terminals view (the view's "New Terminal View" button, then drag a tab across) currently fails:

The tab stays in the source view, and both terminals go blank.
The error log shows SWTException: Widget is disposed, thrown from StyleMap.updateFont via VT100TerminalControl.setupTerminal.

Cause

setupTerminal(Composite) recreates the controls in the new parent. The TextLineRenderer of the new canvas measures the font on fCtlText, but at that point fCtlText is still the canvas that was just disposed.

A second problem shows once that is fixed: the preferences (colors, font, buffer size) were only applied to freshly created terminals, so a moved terminal fell back to the default colors and font.

Fix

The renderer measures on the display while the new canvas does not exist yet, as it already does for the first canvas.
setupTerminal applies the preferences to recreated controls too. The property change listener is still added only once.

Test

TerminalControlRecreateUITest recreates a terminal in a new parent, as the drag and drop does:

Without the first change it fails with "Widget is disposed".
Without the second change the background color preference is lost.

The existing terminal tests pass.

Copilot

Copilot review overview

🟡 Changes recommended

Test teardown leaks terminal resources, and both modified plug-ins require bundle version increments.

Review effort: Balanced
Findings: 1 · 2

Open (3)

Dispose terminal control during test teardown · New
Increment changed control bundle service version · New
Increment changed test bundle service version · New

What changed in this PR

Fixes terminal tab migration between terminal views by safely recreating controls and preserving preferences.

Changes:

Avoids measuring fonts against a disposed canvas.
Reapplies terminal preferences after control recreation.
Adds UI regression coverage for recreation and preference retention.

File	Description
`AutomatedTestSuite.java`	Registers the new UI test.
`TerminalControlRecreateUITest.java`	Tests control recreation and preference retention.
`VT100TerminalControl.java`	Fixes recreation and reapplies preferences.

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

eclipse-platform-bot · 2026-09-30T08:21:16Z

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

terminal/bundles/org.eclipse.terminal.control/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From cafa0e6a979729754a22c0bfe208fbcf85e855b8 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <platform-bot@eclipse.org>
Date: Wed, 30 Sep 2026 08:20:56 +0000
Subject: [PATCH] Version bump(s) for 4.42 stream


diff --git a/terminal/bundles/org.eclipse.terminal.control/META-INF/MANIFEST.MF b/terminal/bundles/org.eclipse.terminal.control/META-INF/MANIFEST.MF
index ff5a3fb72b..0c618bbd15 100644
--- a/terminal/bundles/org.eclipse.terminal.control/META-INF/MANIFEST.MF
+++ b/terminal/bundles/org.eclipse.terminal.control/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.terminal.control; singleton:=true
-Bundle-Version: 1.1.200.qualifier
+Bundle-Version: 1.1.300.qualifier
 Bundle-Activator: org.eclipse.terminal.internal.control.impl.TerminalPlugin
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
-- 
2.55.0

Further information are available in Common Build Issues - Missing version increments.

Copilot

🟢 Approval recommended

No active review issues remain for the terminal-control recreation and preference-preservation changes.

No open issues 🎉

3 resolved since last review

_{Review effort: Balanced}

akurtakov

Thanks, this fixes the drag and drop: I reproduced Widget is disposed on master and the move works with this PR. One gap: inverted colors set on a terminal are reset to the preference when it is moved (details inline). Please also rebase on master and squash into one commit; the version bump commit is no longer needed, master is at 1.1.300 already.

akurtakov · 2026-11-04T07:34:05Z

 		setupListeners();
-		if (fPreferenceStore != null && wasDisposed) {
+		if (fPreferenceStore != null) {
+			// new controls after a drag and drop need the colors and font as well


Inverted colors set on one terminal (right-click → Inverted colors, InvertColorsAction) are lost when the tab is moved: updatePreferences() resets them to PREF_INVERT_COLORS. To reproduce, invert a terminal with the global preference off, drag it to another Terminals view, and it arrives not inverted. The flag lives on the canvas (fCtlText.isInvertedColors()), so reading it before fWndParent.dispose() and setting it on the new canvas after updatePreferences() would keep it.

A smaller point on the same line: re-running updatePreferences() on a move also calls setBufferLineLimit(pref), which trims history if a client set a different limit through ITerminalViewControl. The Terminals view never does, so this is only for other users of the control; skipping the buffer limit on a move would avoid it.

Fixed in c069116. On a move, setupTerminal() no longer runs updatePreferences(): it reads the inverted colors from the old canvas before disposing its parent, applies only the colors and the font from the preferences to the new canvas, and sets the inverted colors back. The buffer line limit is left as it was, so a limit set through ITerminalViewControl survives the move too.

akurtakov · 2026-11-04T07:34:05Z

-				new TextLineRenderer(() -> fCtlText, fPollingTextCanvasModel));
+				// While the canvas is being created, fCtlText may still be the canvas disposed by a
+				// drag and drop to another tab folder: measure on the display then
+				new TextLineRenderer(() -> fCtlText != null && !fCtlText.isDisposed() ? fCtlText : null,


Optional: rather than checking isDisposed() on every font measurement, setupTerminal() could set fCtlText = null once it disposes the old parent, and the supplier could stay () -> fCtlText. That keeps the invariant in one place and drops the need for the comment.

Done: setupTerminal() sets fCtlText to null once the old parent is disposed, and the renderer gets () -> fCtlText. StyleMap already measures on the display when the supplier gives null, so the comment is gone.

akurtakov · 2026-11-04T07:34:05Z

+	}
+
+	@Test
+	public void recreatedCanvasKeepsThePreferences() {


The test only checks the background color. Could it also check the font (the part that threw Widget is disposed), and, with the fix above, that setInvertedColors(true) survives setupTerminal()?

The test now also checks the font (the part that threw "Widget is disposed"), and a new test sets the inverted colors and a buffer line limit of 123 on the terminal, recreates it and checks both stay. On master all three tests fail with "Widget is disposed"; on the previous head of this PR the new one failed on the inverted colors.

Copilot

🟡 Changes recommended

The saved color inversion state is restored only when a preference store exists, so terminal setup still resets inversion for controls created without one.

1 open issue

Restore inversion state even without a preference store · New

_{Review effort: Balanced}

akurtakov · 2026-11-05T06:50:53Z

@insjang If you have access to copilot please request new review from it after you push new version, that would shorten the loop as you would be able to handle multiple reviews till I look into it again.

github-actions · 2026-11-05T07:03:24Z

Test Results

54 files ± 0 54 suites ±0 35m 7s ⏱️ - 1m 31s
4 982 tests + 4 4 960 ✅ + 4 22 💤 ±0 0 ❌ ±0
12 837 runs +12 12 683 ✅ +12 154 💤 ±0 0 ❌ ±0

Results for commit 783a360. ± Comparison against base commit 019989b.

♻️ This comment has been updated with latest results.

Dragging a terminal tab to another terminals view recreates the terminal controls in the new view. This failed in two ways: - The text line renderer of the new canvas measured the font on fCtlText, which at that point still is the canvas that was just disposed. The move stopped with "Widget is disposed", leaving the tab behind and both terminals blank. - The preferences were only applied to freshly created terminals, so a moved terminal fell back to the default colors and font. setupTerminal() now drops fCtlText with the old parent, so the renderer measures on the display until the new canvas exists. A moved terminal gets the colors and the font from the preferences (the defaults when it has no preference store), and keeps what was set on it alone: the inverted colors of its context menu, and the buffer line limit a client may have set through ITerminalViewControl. TerminalControlRecreateUITest recreates the controls in a new parent and checks the background color, the font, the inverted colors and the buffer line limit, and the inverted colors of a terminal made without a preference store; on master the tests fail with "Widget is disposed". It disposes the terminal afterwards, as the terminals view does when a tab is closed, and reads the background color through the renderer, whose getter is public.

Copilot

🟢 Approval recommended

No active review issues were identified for the terminal control recreation and settings-preservation changes.

No open issues 🎉

1 resolved since last review

Restore inversion state even without a preference store

_{Review effort: Balanced}

insjang · 2026-11-05T07:22:05Z

Thanks, will do. I re-requested a Copilot review on 783a360 and will do so after each push from now on.

akurtakov · 2026-11-05T07:25:05Z

Thanks, will do. I re-requested a Copilot review on 783a360 and will do so after each push from now on.

In that case it might be best to even run copilot review in a PR in your own repo so the PRs you open here are truly "ready".

akurtakov

Thanks, all addressed: a moved terminal keeps its inverted colors (also without a preference store) and its buffer line limit, gets the colors and font from the preferences, and the renderer no longer touches the disposed canvas. The new tests cover each of these and fail without the fix.
Tested manually: inverted the colors of a terminal from its context menu, with the global preference off, and dragged its tab to a second Terminals view; it arrived inverted, with the right font and colors and no "Widget is disposed".

insjang · 2026-11-05T07:31:56Z

Good idea, thanks. I'll set up Copilot review on my fork and run it on each branch there before opening or updating a PR here.

akurtakov requested a balanced review from Copilot September 30, 2026 08:13

Copilot started reviewing on behalf of akurtakov September 30, 2026 08:14

View session

Copilot AI reviewed Sep 30, 2026

View reviewed changes

akurtakov requested a balanced review from Copilot November 4, 2026 06:42

Copilot started reviewing on behalf of akurtakov November 4, 2026 06:43

View session

Copilot AI reviewed Nov 4, 2026

View reviewed changes

akurtakov requested changes Nov 4, 2026

View reviewed changes

insjang force-pushed the terminal-dnd-recreate branch from d17d8a4 to c069116 Compare November 5, 2026 06:38

akurtakov requested a balanced review from Copilot November 5, 2026 06:47

Copilot started reviewing on behalf of akurtakov November 5, 2026 06:47

View session

Copilot AI reviewed Nov 5, 2026

View reviewed changes

Comment thread ...clipse.terminal.control/src/org/eclipse/terminal/internal/emulator/VT100TerminalControl.java Resolved

insjang force-pushed the terminal-dnd-recreate branch from c069116 to 783a360 Compare November 5, 2026 07:15

akurtakov requested a balanced review from Copilot November 5, 2026 07:17

Copilot started reviewing on behalf of akurtakov November 5, 2026 07:17

View session

Copilot AI reviewed Nov 5, 2026

View reviewed changes

akurtakov approved these changes Nov 5, 2026

View reviewed changes

akurtakov merged commit e85e581 into eclipse-platform:master Nov 5, 2026
18 checks passed

Conversation

insjang commented Sep 30, 2026

Cause

Fix

Test

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eclipse-platform-bot commented Sep 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

akurtakov left a comment

Choose a reason for hiding this comment

Uh oh!

akurtakov Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

insjang Nov 5, 2026

Choose a reason for hiding this comment

Uh oh!

akurtakov Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

insjang Nov 5, 2026

Choose a reason for hiding this comment

Uh oh!

akurtakov Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

insjang Nov 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

akurtakov commented Nov 5, 2026

Uh oh!

github-actions Bot commented Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

insjang commented Nov 5, 2026

Uh oh!

akurtakov commented Nov 5, 2026

Uh oh!

akurtakov left a comment

Choose a reason for hiding this comment

Uh oh!

insjang commented Nov 5, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Nov 5, 2026 •

edited

Loading