Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fedejeanne wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4636,7 +4636,9 @@ void sendEvent (int eventType, Event event) {
if (event == null) event = new Event ();
event.display = this;
event.type = eventType;
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);
Comment thread
akurtakov marked this conversation as resolved.
sendEvent (eventTable, event);
}

Expand Down
12 changes: 11 additions & 1 deletion bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Widget.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1674,7 +1674,17 @@ void sendEvent (int eventType, Event event, boolean send) {
event.display = display;
event.widget = this;
if (event.time == 0) {
event.time = display.getLastEventTime ();
/*
* display.getLastEventTime() only reflects the time of the last *native*
* NSEvent that was processed; it is not updated by synthetic events
* dispatched directly (e.g. notifyListeners()) without going through the
* native event queue. Relying on it here would make consecutive
* synthetic events (as commonly created by tests) carry the same, stale
* time, which can incorrectly look like a repeated/duplicate event to
* code (e.g. KeyBindingDispatcher) that de-duplicates by event time.
* Use a distinct, always-advancing timestamp instead.
*/
event.time = (int) (System.nanoTime() / 1_000_000L);
Comment thread
fedejeanne marked this conversation as resolved.
}
if (send) {
sendEvent (event);
Expand Down
4 changes: 3 additions & 1 deletion bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Display.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5875,7 +5875,9 @@ void sendEvent (int eventType, Event event) {
if (event == null) event = new Event ();
event.display = this;
event.type = eventType;
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);
Comment thread
fedejeanne marked this conversation as resolved.
if (!filterEvent (event)) {
if (eventTable != null) sendEvent (eventTable, event);
}
Expand Down
13 changes: 12 additions & 1 deletion bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Widget.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1679,7 +1679,18 @@ void sendEvent (int eventType, Event event, boolean send) {
event.display = display;
event.widget = this;
if (event.time == 0) {
event.time = display.getLastEventTime ();
/*
* display.getLastEventTime() only reflects the time of the last *native*
* GDK event that was processed; it is not updated by synthetic events
* dispatched directly (e.g. notifyListeners()) without going through the
* native event queue. Relying on it here would make consecutive
* synthetic events (as commonly created by tests) carry the same, stale
* time, which can incorrectly look like a repeated/duplicate event to
* code (e.g. KeyBindingDispatcher) that de-duplicates by event time.
* 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);
Comment thread
fedejeanne marked this conversation as resolved.

@akurtakov akurtakov Sep 2, 2026

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.

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.

@fedejeanne fedejeanne Sep 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@akurtakov akurtakov Sep 8, 2026

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.

See https://xorg.freedesktop.org/archive/current/doc/xproto/x11protocol.html#glossary:Timestamp "A timestamp is a time value, expressed in milliseconds. It typically is the time since the last server reset. Timestamp values wrap around (after about 49.7 days)." So these timestamps are with different base and using Java timestamp will put some value in the far, far future.

@akurtakov akurtakov Sep 18, 2026

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.

I plan to close this one as it can create major and close to non discoverable issues with event handling. The issue it tries to fix should be totally different IMO - like ensuring that an event is either canceled to not propagate further or to not be fired at all (if it is double fired - native and swt side). Coming up with pure SWT reproducer to make debugging it would be a major part of the work.

@fedejeanne fedejeanne Sep 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

See https://xorg.freedesktop.org/archive/current/doc/xproto/x11protocol.html#glossary:Timestamp "A timestamp is a time value, expressed in milliseconds. It typically is the time since the last server reset. Timestamp values wrap around (after about 49.7 days)." So these timestamps are with different base and using Java timestamp will put some value in the far, far future.

I see, so your fear is that by tampering with the event right before it goes into GTK will have unknown effects because the Java timestamp might be "in the future", correct? How about getting some value that makes sense and putting it in here? I am tempted to say let's use OS.gdk_event_get_time(long) / OS.GDK_CURRENT_TIME but I am not sure how to use those. Would you say that's an appropriate value for the timestamp?

I plan to close this one as it can create major and close to non discoverable issues with event handling. The issue it tries to fix should be totally different IMO - like ensuring that an event is either canceled to not propagate further or to not be fired at all (if it is double fired - native and swt side). Coming up with pure SWT reproducer to make debugging it would be a major part of the work.

Ok.
Funny thing: I was trying to investigate further where does the duplication com from by applying only the changes in eclipse-platform/eclipse.platform.ui#4200) and I couldn't reproduce the error anymore, which tells me that this PR is no longer a pre-condition for eclipse-platform/eclipse.platform.ui#4200.

I am closing it. Feel free to answer my question above anyway (just to quench my thirst for knowledge :D )

@akurtakov akurtakov Sep 24, 2026

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.

Sorry for the delay. I tend to miss quite some notifications lately - it's beyond my processing capabilities :)
About gdk_event_get_time - SWT already does exactly this: Display.eventProc stores gdk_event_get_time(event) in lastEventTime, and getLastEventTime() returns it. Using it would give back the current code.
About getting "proper" time - On X11 it is isn't exactly free - change smth, wait for PropertyNotify, which is what gdk_x11_get_server_time does. On Wayland a client learns the time only from input events, at least I haven't found any other way.
I still believe that there shouldn't be "duplicate" events and the duplication comes either from needless firing of event or somewhere an event not being "canceled" after being processed and this is what should be researched rather than playing with the timestamp.

}
if (send) {
sendEvent (event);
Expand Down
Loading