Suppress duplicated key event handling in Linux/GTK3 #4199 by fedejeanne · Pull Request #4200 · eclipse-platform/eclipse.platform.ui

fedejeanne · 2026-07-22T08:44:08Z

Requires

GTK/Cocoa: give synthetic events a fresh timestamp instead of getLast… eclipse.platform.swt#3559

How to test

Navigate the views with Ctrl + PgUp / Ctrl + PgDn like I did in the video in #4199, no view should be skipped.

github-actions · 2026-07-22T09:32:29Z

Test Results

858 files ±0 858 suites ±0 48m 48s ⏱️ - 9m 25s
8 216 tests ±0 7 967 ✅ - 6 243 💤 ±0 6 ❌ +6
20 550 runs ±0 19 872 ✅ - 8 670 💤 ±0 8 ❌ +8

For more details on these failures, see this check.

Results for commit 98b1400. ± Comparison against base commit a680fa8.

♻️ This comment has been updated with latest results.

iloveeclipse

There are some test fails btw...

iloveeclipse · 2026-07-22T09:53:57Z

Beside test failure, the patch seem to work. Tried on RHEL 9.6 / X11

iloveeclipse · 2026-07-23T09:09:00Z

There are some test fails btw...

Reproducible locally, the test was added via 172b704 for https://bugs.eclipse.org/bugs/show_bug.cgi?id=294509, unfortunately bugzilla is down ATM.

expected: <true> but was: <false>
org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:31)
	at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:183)
	at org.eclipse.e4.ui.bindings.tests.KeyDispatcherTest.testExecuteMultiStrokeBinding(KeyDispatcherTest.java:289)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

iloveeclipse · 2026-07-23T09:12:41Z

OK, the test is supposed to test this:

Multi-stroke keybindings like ALT+SHIFT+Q, Q need to recognize a partial key sequence, like ALT+SHIFT+Q.

fedejeanne · 2026-07-23T14:54:27Z

I am looking into it. I suspect something is wrong with the tests because even though they only fail when applying this PR, I also noticed that:

The functionality Alt + Shift + Q, Q (and other similar ones) still work
When commenting out the test testExecuteOneCommand, all other tests pass
When reordering the tests (with help of the JUnit5 annotations org.junit.jupiter.api.TestMethodOrder and org.junit.jupiter.api.Order) and letting testExecuteOneCommand run last, the failing test becomes testExecuteOneCommand

This smells like a bad state after a test ran.

fedejeanne · 2026-09-02T07:49:39Z

It turns out the tests just needed the event.time to be set to something unique so that the new logic didn't consider them to be duplicated.

The tests pass locally, let's see if they pass in the CI.

eclipse-platform-bot · 2026-09-02T07:51:23Z

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

bundles/org.eclipse.e4.ui.bindings/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 0fce4138d7eee1ff67bbe93b927e7107bf380ba7 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <platform-bot@eclipse.org>
Date: Wed, 2 Sep 2026 07:51:05 +0000
Subject: [PATCH] Version bump(s) for 4.42 stream


diff --git a/bundles/org.eclipse.e4.ui.bindings/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.ui.bindings/META-INF/MANIFEST.MF
index 84b518f63d..e03a17509a 100644
--- a/bundles/org.eclipse.e4.ui.bindings/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.e4.ui.bindings/META-INF/MANIFEST.MF
@@ -1,7 +1,7 @@
 Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-SymbolicName: org.eclipse.e4.ui.bindings;singleton:=true
-Bundle-Version: 0.15.200.qualifier
+Bundle-Version: 0.15.300.qualifier
 Bundle-Name: %pluginName
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
-- 
2.55.0

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

fedejeanne · 2026-09-02T12:41:39Z

Failing tests (org.eclipse.ui.internal.findandreplace.overlay.FindReplaceOverlayTest) seem unrelated. Rebasing on master

fedejeanne · 2026-09-02T14:16:16Z

Oh, darn: it turns out this PR does break those tests, just like it broke the previous one. Same reason: event.time == 0 ergo they are "the same" event and get discarded by the new logic.

native key event (Linux/GTK) On GTK (Linux), a single physical key press can, in some situations, be delivered to KeyBindingDispatcher twice for the very same native event: once synchronously via an SWT.Traverse event dispatched from within the native gtk3_key_press_event handling, and once more later via the async message queue. Both deliveries resolve to and execute the exact same bound command, which for page-traversal shortcuts like Ctrl+PageUp / Ctrl+PageDown (Next/Previous Editor) causes an extra tab to be skipped, since the CTabFolder selection is advanced twice per key press. This does not reproduce on Windows or macOS, only on GTK. Fix: track the last executed command together with the native timestamp (Event#time) of its triggering event in executeCommand(...), and skip (without re-executing the handler) a call that matches both the same command and the same event timestamp, since that combination can only happen when the very same native key event is redelivered. Genuinely distinct key presses - including fast auto-repeat - always carry different timestamps, so normal navigation and repeated shortcuts are unaffected. This is reported as the 'bonus' issue in eclipse-platform#4135 (Javadoc View tab cycling two tabs per Ctrl+PageUp/PageDown on Linux). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

fedejeanne · 2026-09-02T14:53:48Z

I created this one so that the event.time is handled similarly in all 3 OSes:

GTK/Cocoa: give synthetic events a fresh timestamp instead of getLast… eclipse.platform.swt#3559

That fixes the problem with the tests so I am reverting the changes to KeyDispatcherTest in this PR.

Notice that the tests in this PR will still fail since they need the changes in SWT to succeed. I am drafting this PR until eclipse-platform/eclipse.platform.swt#3559 is approved and merged.

akurtakov · 2026-09-02T16:43:35Z

Is it investigated where the duplication comes from? IMO the right fix is to not have the duplicated events at all instead of deduplicating in one place and potentially have the duplication elsewhere. The change in SWT to mix Java and X11/Wayland clocks is kind of scary as a start.

Copilot

🟡 Changes recommended

Timestamp collisions can suppress legitimate key events, and the behavior lacks regression coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Suppresses duplicate GTK3 key events to prevent tab navigation from skipping views.

Changes:

Tracks the last executed command and event timestamp.
Suppresses matching duplicate executions and adds trace logging.

File summaries

File	Description
`KeyBindingDispatcher.java`	Adds timestamp-based key-event deduplication.

Review details

Files reviewed: 1/1 changed files
Comments generated: 2
Review effort level: Balanced

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

 			throw new NotEnabledException("Command should have been disabled via activity: " + parameterizedCommand); //$NON-NLS-1$
 		}

+		if (isDuplicateKeyEvent(parameterizedCommand, trigger)) {


+		if (trigger == null) {
+			return false;
+		}
+		boolean duplicate = parameterizedCommand == lastExecutedCommand && trigger.time == lastExecutedEventTime;


fedejeanne · 2026-09-03T07:42:06Z

@akurtakov the duplication happens because Display::readAndDispatch ends up walking 2 paths that reach this if...

eclipse.platform.ui/bundles/org.eclipse.e4.ui.bindings/src/org/eclipse/e4/ui/bindings/keys/KeyBindingDispatcher.java

Lines 318 to 323 in 44ebb5c

    
           if (trigger != null && trigger.widget instanceof Browser) { 
        
           	getDisplay() 
        
           			.asyncExec(() -> handleCommandExecution(parameterizedCommand, handlerService, trigger, obj)); 
        
           } else { 
        
           	commandHandled &= handleCommandExecution(parameterizedCommand, handlerService, trigger, obj); 
        
           }

... and each path execute 1 branch of the if (once if and once else), both reaching KeyBindingDispatcher.handleCommandExecution(...) and following the exact same path from there.

This is the diff between both stack-traces provided in #4199 :

I don't know why the event is dispatched twice though. By looking at the 1st stack-trace though I see this:

 org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:5068)

... and in the 2nd stack-trace I see:

org.eclipse.swt.internal.gtk3.GTK3.gtk_main_iteration_do(Native Method)

Which means that the event must have been triggered by some call to Display.asyncExec(Runnable) and both GTK and Java (Display) ran with it.

iloveeclipse reviewed Jul 22, 2026

View reviewed changes

Comment thread ...les/org.eclipse.e4.ui.bindings/src/org/eclipse/e4/ui/bindings/keys/KeyBindingDispatcher.java Outdated

fedejeanne force-pushed the fix-linux-gtk-double-traversal branch from e66fdd7 to 319c5cd Compare July 23, 2026 08:02

fedejeanne force-pushed the fix-linux-gtk-double-traversal branch from 319c5cd to 176884a Compare September 2, 2026 07:46

fedejeanne force-pushed the fix-linux-gtk-double-traversal branch from ce01588 to 16d280e Compare September 2, 2026 12:41

fedejeanne mentioned this pull request Sep 2, 2026

GTK/Cocoa: give synthetic events a fresh timestamp instead of getLast… eclipse-platform/eclipse.platform.swt#3559

Draft

fedejeanne force-pushed the fix-linux-gtk-double-traversal branch from 16d280e to 98b1400 Compare September 2, 2026 14:52

fedejeanne marked this pull request as draft September 2, 2026 14:53

akurtakov requested a balanced review from Copilot September 2, 2026 16:48

Copilot started reviewing on behalf of akurtakov September 2, 2026 16:49 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

Uh oh!

Conversation

fedejeanne commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Requires

How to test

Uh oh!

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

Uh oh!

Test Results

Uh oh!

iloveeclipse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iloveeclipse commented Jul 22, 2026

Uh oh!

iloveeclipse commented Jul 23, 2026

Uh oh!

iloveeclipse commented Jul 23, 2026

Uh oh!

fedejeanne commented Jul 23, 2026

Uh oh!

fedejeanne commented Sep 2, 2026

Uh oh!

eclipse-platform-bot commented Sep 2, 2026

Uh oh!

fedejeanne commented Sep 2, 2026

Uh oh!

fedejeanne commented Sep 2, 2026

Uh oh!

fedejeanne commented Sep 2, 2026

Uh oh!

akurtakov commented Sep 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

fedejeanne commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fedejeanne commented Jul 22, 2026 •

edited

Loading

github-actions Bot commented Jul 22, 2026 •

edited

Loading