GTK/Cocoa: give synthetic events a fresh timestamp instead of getLast… - #3559
GTK/Cocoa: give synthetic events a fresh timestamp instead of getLast…#3559fedejeanne wants to merge 1 commit into
Conversation
…EventTime() Widget.sendEvent(int, Event, boolean) and Display.sendEvent(int, Event) default a dispatched event's time to display.getLastEventTime() when the caller left Event.time at 0 (e.g. widget.notifyListeners(type, event) with a freshly-constructed Event, as commonly done by tests and some application code). On GTK, getLastEventTime() returns a field that is only updated while processing a real native GDK event; on Cocoa, it reads the timestamp of NSApplication.currentEvent(). Neither is updated by a synthetic event dispatched directly through notifyListeners()/sendEvent(), so two consecutive synthetic events issued without an intervening native event receive the exact same, stale time. Code that de-duplicates by (command, event time) - such as KeyBindingDispatcher's new duplicate key event guard - then incorrectly treats the second event as a repeat of the first and suppresses it. Win32's getLastEventTime() (OS.GetMessageTime()) is backed by the ever-advancing OS message-queue tick counter and does not exhibit this staleness, so it is left unchanged. Fix GTK and Cocoa by giving these synthetic events a fresh, monotonically-advancing timestamp instead, consistent with how Display.sendJDKInternalEvent() already timestamps its own synthetic events. Event.java itself (shared by all platforms) is untouched, as is any test code - this is purely a platform dispatch fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Millisecond truncation still allows consecutive synthetic events to receive identical timestamps.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates GTK and Cocoa synthetic event timestamps to avoid reusing the last native event time.
Changes:
- Uses
System.nanoTime()-derived timestamps for widget and display events. - Documents why native event timestamps are unsuitable as fallbacks.
File summaries
| File | Description |
|---|---|
gtk/.../Widget.java |
Updates widget event timestamps. |
gtk/.../Display.java |
Updates display event timestamps. |
cocoa/.../Widget.java |
Updates widget event timestamps. |
cocoa/.../Display.java |
Updates display event timestamps. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (event.time == 0) event.time = getLastEventTime (); | ||
| // See comment in Widget.sendEvent(int, Event, boolean) for why | ||
| // getLastEventTime() is not used as a fallback here. | ||
| if (event.time == 0) event.time = (int) (System.nanoTime() / 1_000_000L); |
There was a problem hiding this comment.
This millisecond-resolution clock can return the same value for consecutive synthetic display events, so it does not provide the fresh/distinct fallback described by the referenced Widget.sendEvent rationale.
Agreed, but the chances are very slim that 2 back-to-back events manage to get created and sent in the same millisecond and if they do then the deduplication logic should still notice that they are not the same event (event1 == event2 yields false), they were merely sent at the same time.
Route this through the same per-Display advancing synthetic timestamp allocator rather than reading and truncating the clock independently.
I don't understand that: do you mean the same logic that is being altered by this same PR and contain the exact same fix (event.time = (int) (System.nanoTime() / 1_000_000L);)?
| * Use a distinct, always-advancing timestamp instead, consistent with | ||
| * how other synthetic events (see sendJDKInternalEvent) are timestamped. | ||
| */ | ||
| event.time = (int) (System.nanoTime() / 1_000_000L); |
There was a problem hiding this comment.
So 2 synthetic events fired one after the other will still share the same millisecond (quite likely for a test execution) and de-duplication will not happen , what am I missing?
Also mixing Java and Gtk (X11 and Wayland have different too) clock is something that could create a problem especially when adding remote sessions in the loop too.
There was a problem hiding this comment.
So 2 synthetic events fired one after the other will still share the same millisecond (quite likely for a test execution) and de-duplication will not happen , what am I missing?
Same as above: the deduplication logic still compares event1 == event2 and that check returns false.
Also mixing Java and Gtk (X11 and Wayland have different too) clock is something that could create a problem especially when adding remote sessions in the loop too.
Can you please provide an example? I don't understand it.
The way I see it, the current code in all 4 (actually, all 6) implementations of sendEvent(...) methods in Widget and Display decides to assign the same timestamp as the last real (OS) event to all synthetic events, effectively saying "_I decided that this event you created and triggered by hand without explicitly assigning a timestamp to it (because event.time == 0) _ will have the same timestamp as the last real event the OS triggered". That sounds odd because the synthetic event may not even be related to the last real (OS) event, they could be about 2 totally different things e.g. moving the mouse vs pressing a key. I don't get why one would decide to have them both "happen at the same time" when they might not be related at all.
Pre-condition for: