Can we change cut/copy/paste API from synchronous to asynchronous?

[jonahgraham]

Originally posted by @raghucssit in #2489 (comment)

@raghucssit should we enhance javadoc to mention that calling copy / paste will not immediate update the state and one should possibly use asyncExec wif accessing the control is desired (that would probably also work for the test to use async exec but it of course complicates the test assertions maybe).

I have updated copy/paste doc.

I think this is incorrect to change this API from sync to async. This is probably a breaking change in SWT, but I pose this as a question because perhaps it is an acceptable change, but it is one that may have to extend to many more places (see below).

For example #2491 (comment) still failing is a side effect of this change because the asynchronous method* gtk_text_buffer_paste_clipboard is called just before calling applySegments, but applySegments needs the new value of the text buffer to operate properly.

Another example is this test that does effectively what TextCellEditor.performPaste does, this test passes on GTK3, but fails with current GTK4 implementation.

@Test
public void test_paste_and_selection() {
	text.setText("01234567890");
	text.setSelection(2, 4);
	assertEquals("01234567890", text.getText());
	text.copy();
	text.setSelection(3, 4);
	assertEquals(1, text.getSelectionCount());
	assertEquals(new Point(3, 4), text.getSelection());

	// Simulate what code such as org.eclipse.jface.viewers.TextCellEditor.performPaste() does:
	text.paste();
	assertEquals(0, text.getSelectionCount());
	assertEquals(new Point(5, 5), text.getSelection());
	assertEquals("012234567890", text.getText());
}

* GTK4 changed this (and similar) methods from sync to async.

[laeubi]

I think this is incorrect to change this API from sync to async.

The API was never changed. a change in API would have been for example from public void paste () to public CompletableFuture<?> paste () or public void paste (Runnable completionHandler).

Apart from that the API doc only states

Pastes text from clipboard.
The selected text is deleted from the widget
and new text inserted from the clipboard.

so it actually do not promise when it happens and if the widget is updated immediately. Especially as its hard to tell what "update" mean, does it for example include the control is completely repainted? So one might argue that some code relies on undefined behavior. Also a lot of code simply do not care when it happens a few ms later after the call.

Given that and the ongoing complications with the BrowserWidget that also has be made "synchronous" cause all kind of issues as the UIs these days are async I wonder if we should not simply accept that fact and start considering how we can better reflect that in SWT. So maybe SWT should simply fire a text-change event and code need to take action should listen for that instead of assume everything happens instantly?

[HeikoKlare]

For me, the question in such cases is how likely consumers actually rely on API methods behaving synchronously. I do not see anything in the documentation of Text#copy(), Text#paste() etc. stating that their effects appear synchronously (even though you can argue that particularly for cut and paste you expect the effects to appear immediately) and they do not even return any result. Of course, the behavior is long standing (and even implicitly documented via tests) but that does not mean that anyone really needs it. Compare for the Browser implementation (as indicated by Christoph), where multiple methods changed such that their effects are not visible synchronously anymore. For example, Browser#setText() behaves asynchronously and even returns a boolean result which now is now simply true if the processing of the request is scheduled asynchronously (not being fully consistent with the specification in Javadoc). And it does not seem to produce any severe problems so far.

[jonahgraham]

So one might argue that some code relies on undefined behavior.

But there is a lot of the existing code, including the tests and most every current call from Eclipse Project code relies on this synchronous behaviour.

There are around 6 uses of paste() in the Eclipse Project, these 5 use cases all rely on paste() being synchronous:

• org.eclipse.jdt.internal.ui.dialogs.TableTextCellEditor.performPaste()
• org.eclipse.jface.viewers.TextCellEditor.performPaste()
• org.eclipse.ui.actions.TextActionHandler.PasteActionHandler.runWithEvent(Event)
• org.eclipse.ui.internal.navigator.TextActionHandler.PasteActionHandler.runWithEvent(Event)
• org.eclipse.pde.internal.ui.editor.PDEFormPage.canPerformDirectly(String, Control)

This is made more complicated because (as commented here in JFace):

We really want a selection listener but it is not supported so we
use a key listener and a mouse listener to know when selection changes
may have occurred

Since Text.paste() may change selection we need to know so that all enablements can be updated appropriately.

---

My current feeling is we should investigate implementing synchronous paste. So I will wait for feedback from @raghucssit and/or @iloveeclipse as the author and approver of #2489 to know what may have been considered already in this area.

[jonahgraham]

This relates to #2126 where I am working on full clipboard support and ensuring that copy/paste between different parts of the system work as expected.

[HeikoKlare]

But there is a lot of the existing code, including the tests and most every current call from Eclipse Project code relies on this synchronous behaviour.

The most critical part is of course the "external" consumers, i.e., the ones that we cannot change on our own as they are not part of any Eclipse project. So just as an additional source of empirical data for making an assessment: I checked the quite large code base of our product and I see that we have (only) three places in our code which would need to be adapted in a similar way as, e.g., the TextCellEditor and TableTextCellEditor of the Eclipse codebase. I know it's just a single data point and other products may more heavily rely on the existing behavior, but maybe still interesting to get some impression of potential impacts of such a change.

[laeubi]

I think the most critical part actual is: Can we (reliable) implement it that way? If that do not work, is prone to bugs, deadlocks or other behavioral changes (e.g. we need to drive the event-queue implicitly and unconditionally) then the cure is probably worse than the disease.

We already have many cases where code sometimes work better (either due to bugs or other limitations) if it uses Display.asyncExec and I think this is simply such a case and it is much better be handled on the consumer side then instead of try to emulate a synchron behavior where it is really asynchronous and possibly blocks the UI.

[iloveeclipse]

My current feeling is we should investigate implementing synchronous paste. So I will wait for feedback from @raghucssit and/or @iloveeclipse as the author and approver of #2489 to know what may have been considered already in this area.

Native subsystem behavior is different on different platforms. SWT was always trying to provide some common behavior, but in many places it simply can't be done.

Let assume we would implement synchronous paste and make a bold API statement about that synchronous behavior being "API contract" now. In the next Windows / Mac / Gnome iteration some genious will decide to break previous OS behavior without providing alternative. Especially Gnome devs are known for doing that. What should SWT do? Breaking API?

I agree that it isn't nice if there is code that depends on synchronous operation and this code might fail on some platforms.

Maybe we could try to make it synchronous in the OS specific SWT fragments where we know for sure the native code is asyncronous right now (like GTK), as seen here
https://github.com/raghucssit/eclipse.platform.swt/blob/48e5878df82fbbad0db44b298755377e98c796f4/tests/org.eclipse.swt.tests/JUnit%20Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Combo.java#L239-L244

[laeubi]

Maybe we could try to make it synchronous in the OS specific SWT fragments where we know for sure the native code is asyncronous right now (like GTK), as seen here

I would really strongly highlight that this is test code and it has some serve limitations!

Assume the following SWT code (executed in the Display Thread):

display.asyncExec( () -> {
   text.dispose();
});
if (!text.isDisposed()) {
    combo.paste();
    String str = text.getText();
}

currently what happens is:

1. paste is executed
2. String str = text.getText() return the text
3. after the control flow returns to the event loop events that happens afterwards will be processed and the asyncTask is executed and disposes the text control

if we would do something like in the test and paste() would drive the event queue:

1. paste is executed
2. any events happened after the current processed one are fired and also the asyncTask is executed and disposes the text control
3. text.getText() will throw "Widget disposed"

so this would have all kind of obscure, hard to track behavioral changes, e.g. a key-up event will fire during a key-down event is currently executed and calls a copy / paste operation and then return after that to continue the flow what might result in all kind of inconsistent states as this is really something you need to depend on.

[HeikoKlare]

Just remembered that we quite recently had a discussion on a comparable attempt (spinning the event loop inside Edge browser for similar reasons than here). Here is the PR (that we agreed on not to merge) for reference:

• [Win32] Spin event loop in Edge instead of only processing OS messages #1773

[jonahgraham]

For #2126 to implement some things like Clipboard.getContents we need to be able to run event loop, or else we need new API to get the Clipboard contents in SWT.

Assume the following SWT code (executed in the Display Thread):

To follow along your example, this would fail in GTK4.

display.asyncExec( () -> {
   text.dispose();
});

if (!text.isDisposed()) {
    String s =(String)clipboard.getContents(TextTransfer.getInstance());
    if (s != null)
       text.setText(s);
}

However, my plan is to do what GTK3 does. GTK3 sets DISPATCH_EVENT_KEY on the Display so that we limit events that are processed during the critical section that we need to process event loop. We may need a more sophisticated system of that for GTK4 too. GTK3 alternatively uses SWT.PreExternalEventDispatch/SWT.PostExternalEventDispatch in other places to run the event loop outside of SWT's event loop.

Native subsystem behavior is different on different platforms. SWT was always trying to provide some common behavior, but in many places it simply can't be done.

If here is one of the places that it can't be done, can we change parts of SWT API to be asynchronous? If so, then we should change Clipboard.getContents to be async too. That would probably mean not implementing current API of getContents and adding new method(s) (maybe Clipboard.getContents takes a Consumer<Object> that gets passed the contents when ready?)

It is quite easy to provide an async implementation of a sync method call, the other way around not so much.

Just remembered that we quite recently had a discussion on a comparable attempt (spinning the event loop inside Edge browser for similar reasons than here). Here is the PR (that we agreed on not to merge) for reference:

• [Win32] Spin event loop in Edge instead of only processing OS messages #1773

IIUC the Edge browser does do what I am talking about, spinning the event loop during synchronous operations. However what Edge isn't doing is limiting which events get processed (like GTK3's DISPATCH_EVENT_KEY)

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java

Lines 536 to 565 in cbf7f7a

	/**
	* Processes single OS messages using {@link Display#readAndDispatch()}. This
	* is required for processing the OS events during browser initialization, since
	* Edge browser initialization happens asynchronously. Messages are processed
	* until the given condition is fulfilled or a timeout occurs.
	* <p>
	* {@link Display#readAndDispatch()} also processes events scheduled for
	* asynchronous execution via {@link Display#asyncExec(Runnable)}. This may
	* include events such as the disposal of the browser's parent composite, which
	* leads to a failure in browser initialization if processed in between the OS
	* events for initialization. Thus, this method does not implement an ordinary
	* readAndDispatch loop, but waits for an OS event to be processed.
	* @throws Throwable
	*/
	private static void processOSMessagesUntil(Supplier<Boolean> condition, Consumer<SWTException> timeoutHandler, Display display) {
	MSG msg = new MSG();
	AtomicBoolean timeoutOccurred = new AtomicBoolean();
	// The timer call also wakes up the display to avoid being stuck in display.sleep()
	display.timerExec((int) MAXIMUM_OPERATION_TIME.toMillis(), () -> timeoutOccurred.set(true));
	while (!display.isDisposed() && !condition.get() && !timeoutOccurred.get()) {
	if (OS.PeekMessage(msg, 0, 0, 0, OS.PM_NOREMOVE | OS.PM_QS_POSTMESSAGE)) {
	display.readAndDispatch();
	} else {
	display.sleep();
	}
	}
	if (!condition.get()) {
	timeoutHandler.accept(createTimeOutException());
	}
	}

[HeikoKlare]

IIUC the Edge browser does do what I am talking about, spinning the event loop during synchronous operations. However what Edge isn't doing is limiting which events get processed (like GTK3's DISPATCH_EVENT_KEY)

Not limiting which events to process was what the linked PR proposed (and what we decided not to do because of unintended side effects). The current Edge adapter implementation, which you linked, behaves kind of similar to the event limitation you propose: it processes the next OS message but only if it's of specific kind (PM_QS_POSTMESSAGE), to which the Edge initialization response message belongs. To my knowledge, on Windows it's not possible to limit event processing very fine-grained at the level of specific event types but only at the level of event categories, so this was the best we could do here (and we did not experience any problematic side effects so far).

[jonahgraham]

Thanks @HeikoKlare for explaining that to me so I understand better now.

Seeing as Edge does that limited processing, WDYT about parts of SWT on GTK4 taking equivalent approaches?

[HeikoKlare]

Seeing as Edge does that limited processing, WDYT about parts of SWT on GTK4 taking equivalent approaches?

In my opinion, if you really want the implementation to behave synchronous while adapting something that behaves asynchronous, that is probably a good way to go. You should of course try to ensure that no "wrong" event are processed, and I would always add a timeout mechanism to avoid deadlocks. If you do something wrong here (maybe don't expect some very uncommon scenario that is only detected when rolling out such an implenmentation to many consumers), it will likely block the application in an unrecoverable way.

But I would only do this kind of sync-async wrapping/mapping if it's really necessary. In the Edge adaptation there was not reasonable alternative as otherwise every browser instantiation and subsequent usages in every application would have to be rewritten in a potentially not-that-simple way. That was unacceptable, so we decided to got the discussed way. But if you are in a situation in which the effort and complexity to adapt consumers might be acceptable, I would try to avoid the complexity and error-proneness of sync-async wrapping and change the API instead. In the long run it will reduce problems and maintenance efforts. So for me it would always be a difficult and very case-specific decision on where to put the burden for dealing with such a sync to async change.

[jonahgraham]

You should of course try to ensure that no "wrong" event are processed, [...]

So one of the challenges I am having is identifying what must and shouldn't be processed.

One of the really tricky use cases with the clipboard is if a user copies one part of SWT, then opens a file dialog and pastes in there. In this case there are many asynchronous operations live at the same time, all of the following may be active at once.

• dialog
• serializer and deserializer
• input and output stream reading/writing for the serializer/deserializer
• paste clipboard

[...] I would always add a timeout mechanism to avoid deadlocks.

That is a good point - the current code I used as a starting point (GTK3's clipboard get + GTK4's system dialogs) don't have timeouts. I'll make sure some timeouts are in place.

But I would only do this kind of sync-async wrapping/mapping if it's really necessary.

For the methods such as Clipboard.getContents() there is no option, it must be synchronous (or we redesign SWT API).

For methods such as Text.paste we could live with the contract update from #2489 and update all use cases of paste to do post processing asynchronously.