Repository navigation
GTK/Cocoa: give synthetic events a fresh timestamp instead of getLast… - #3559
fedejeanne 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.
| * 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
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.
|
Closing it since I can't reproduce the original error (eclipse-platform/eclipse.platform.ui#4199) by applying only the changes in eclipse-platform/eclipse.platform.ui#4200, which tells me that something must have changed in the meantime and this PR is no longer a pre-condition for eclipse-platform/eclipse.platform.ui#4200 |
|
Hm, I actually tested in WSL this time. I need to test again in a real Linux to see if I can reproduce the error and if I really need this PR. Reopening. |
|
I was able to reproduce the error in a real Linux (no WSL). |
|
Can you provide a pure SWT snippet to help reproducing it locally? |
|
@fedejeanne Please test with the following snippet (replicating JFace behavior) with and without this change: package org.eclipse.swt.snippets;
import org.eclipse.swt.*;
import org.eclipse.swt.custom.*;
import org.eclipse.swt.layout.*;
import org.eclipse.swt.widgets.*;
/**
* StyledText example snippet: the Verify events of one keystroke in block
* selection mode share the event.time of the key press.
*
* Type a character: it must appear once on every line. The Verify listener
* works like the one in JFace TextViewer (bug 268044), which applies the
* change on the first event and vetoes the rest by their equal event.time.
*
* For a list of all SWT example snippets see
* http://www.eclipse.org/swt/snippets/
*/
public class Snippet401 {
static boolean applying;
static int lastTime;
public static void main (String [] args) {
Display display = new Display ();
Shell shell = new Shell (display);
shell.setText ("Snippet 401");
shell.setLayout (new FillLayout ());
StyledText text = new StyledText (shell, SWT.V_SCROLL);
text.setText ("line\n".repeat (99) + "line");
text.setBlockSelection (true);
text.setBlockSelectionBounds (0, 0, 0, 100 * text.getLineHeight () - 1);
text.addListener (SWT.Verify, e -> {
if (applying) return;
e.doit = false;
if (e.time == lastTime) return;
lastTime = e.time;
applying = true;
for (int line = 99; line >= 0; line--) {
text.replaceTextRange (text.getOffsetAtLine (line), 0, e.text);
}
applying = false;
});
shell.setSize (300, 400);
shell.open ();
while (!shell.isDisposed ()) {
if (!display.readAndDispatch ()) display.sleep ();
}
display.dispose ();
}
}JFace link (https://github.com/eclipse-platform/eclipse.platform.ui/blob/8764967b381bdcd2e4f2c19e942f55ee7338bd33/bundles/org.eclipse.jface.text/src/org/eclipse/jface/text/TextViewer.java#L3945 ) showing relying on lasttime being as it is now and the change in behavior this PR will create: Any change with event timings will have a very broad effect on the whole ecosystem as one can not predict how many places rely on sometimes not documented (but being like that forever) behavior. It shouldn't be taken light and done only after serious and thorough investigation of many possibilities. |
|
The same effect as in the snippet can easily be replicated in working IDE with this PR, Alt+Shift+A, select multiple rows and type single letter. Cancelation will not work, letter will be typed the number of lines you type on. If you try undo it will have to happen as many types as the edited rows are to get to the pristine state: |
|
@akurtakov as discussed in today's community call, I'll close this PR because it is the wrong fix. I use Ubuntu 24.04 LTS (noble) |
Pre-condition for: