Community
Participate
Working Groups
It feels like half of the callers of Display.syncExec wait for a user feedback. Therefore i ask to provide a public <T> T org.eclipse.swt.widgets.Display.syncExec(Supplier<T>) which returns the user supplied userfeedback. That would simplify the boiler code in the callers. Example from org.eclipse.equinox.internal.security.ui.storage.UICallbackProvider.ask(): return PlatformUI.getWorkbench().getDisplay().syncExec(// () -> MessageDialog.openConfirm(StorageUtils.getShell(), SecUIMessages.generalDialogTitle, msg)// ); instead of final Boolean[] result = new Boolean[1]; PlatformUI.getWorkbench().getDisplay().syncExec(() -> { boolean reply = MessageDialog.openConfirm(StorageUtils.getShell(), SecUIMessages.generalDialogTitle, msg); result[0] = Boolean.valueOf(reply); }); return result[0];
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/184997
I like the idea. Would you please create a gerrit for the equinox example you shown?
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/185027
Great idea Jörg! I would like to propose an OOP alternative that could also help to not increase the volume of Display class that is already 5k LOC. And I think we need the same capability for async exec. ``` public final class SyncExec<T> implements Function<Supplier<T>, T>{ private final Display display; public SyncExec() { this(Optional.ofNullable(Display.getCurrent()).orElseGet(Display::getDefault)); } public SyncExec(Display display) { Objects.nonNull(display); this.display = display; } @Override public T apply(Supplier<T> supplier) { Objects.nonNull(supplier); @SuppressWarnings("unchecked") T[] t = (T[]) new Object[1]; display.syncExec(() -> t[0] = supplier.get()); return t[0]; } } ```
(In reply to Alexander Fedorov from comment #4) > I would like to propose an OOP alternative that could also help to not > increase the volume of Display class that is already 5k LOC. I do not like the code duplication too. But i prefer Display.syncExec over a separate Class because Display.syncExec is very popular and programmers would find the new method automatically by code completion. While a Wrapper won't be found automatically. > And I think we need the same capability for async exec. For completeness i would like a java.util.concurrent.Future<V> style version of asyncExec too. But at a first look i found no usecase for that (do you know any/many?). As far as i see asyncExec is mostly used like fire-and-forget.
(In reply to Jörg Kubitz from comment #5) > (In reply to Alexander Fedorov from comment #4) > > > I would like to propose an OOP alternative that could also help to not > > increase the volume of Display class that is already 5k LOC. > > I do not like the code duplication too. But i prefer Display.syncExec over a > separate Class because Display.syncExec is very popular and programmers > would find the new method automatically by code completion. While a Wrapper > won't be found automatically. This argument is stronger than mine. Until Eclipse IDE whould be smart enough to find the alternative during code completion. > > > And I think we need the same capability for async exec. > > For completeness i would like a java.util.concurrent.Future<V> style version > of asyncExec too. But at a first look i found no usecase for that (do you > know any/many?). As far as i see asyncExec is mostly used like > fire-and-forget. True, the async exec is typically involved to the complex state change, that could be hard to improve without bigger refactoring.
(In reply to Alexander Fedorov from comment #6) > (In reply to Jörg Kubitz from comment #5) > > (In reply to Alexander Fedorov from comment #4) > > > > > I would like to propose an OOP alternative that could also help to not > > > increase the volume of Display class that is already 5k LOC. > > > > I do not like the code duplication too. But i prefer Display.syncExec over a > > separate Class because Display.syncExec is very popular and programmers > > would find the new method automatically by code completion. While a Wrapper > > won't be found automatically. > > This argument is stronger than mine. Until Eclipse IDE whould be smart > enough to find the alternative during code completion. > > > > > > And I think we need the same capability for async exec. > > > > For completeness i would like a java.util.concurrent.Future<V> style version > > of asyncExec too. But at a first look i found no usecase for that (do you > > know any/many?). As far as i see asyncExec is mostly used like > > fire-and-forget. > > True, the async exec is typically involved to the complex state change, that > could be hard to improve without bigger refactoring. Alexander Fedorov, do you have any comments on the patch itself. Please give your vote on gerrit so we can continue with this one .
(In reply to Alexander Kurtakov from comment #7) > > Alexander Fedorov, do you have any comments on the patch itself. Please give > your vote on gerrit so we can continue with this one . added +1
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/184997 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=5a84b03474c8069ced9faed0be467aa755a88c2f
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt.binaries/+/185205
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185221
(In reply to Eclipse Genie from comment #10) > New Gerrit change created: > https://git.eclipse.org/r/c/platform/eclipse.platform.swt.binaries/+/185205 This reverts the proposed change, because it breaks compilation of existing code. Just compile org.eclipse.ui.ide project with the new SWT to see what I mean.
(In reply to Andrey Loskutov from comment #12) > (In reply to Eclipse Genie from comment #10) > > New Gerrit change created: > > https://git.eclipse.org/r/c/platform/eclipse.platform.swt.binaries/+/185205 > > This reverts the proposed change, because it breaks compilation of existing > code. > > Just compile org.eclipse.ui.ide project with the new SWT to see what I mean. I meant of course https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185221
Thanks for catching it Andrey!
(In reply to Alexander Kurtakov from comment #14) > Thanks for catching it Andrey! I'm back from vacation since yesterday, therefore I was not able to review the PR before :-(
So the option is to have it with different method name e.g. syncExecWithValue which is probably not the best name possible so waiting for proposals here.
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185221 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=50956636588471017a5f52baa7ca8a17c54c7e99
(In reply to Alexander Kurtakov from comment #16) > So the option is to have it with different method name e.g. > syncExecWithValue which is probably not the best name possible so waiting > for proposals here. syncGet(Supplier) or maybe syncCall(Callable) if we want to allow checked exceptions.
(In reply to Sebastian Zarnekow from comment #18) > > syncExecWithValue which is probably not the best name possible so waiting > syncGet(Supplier) or maybe syncCall(Callable) if we want to allow checked > exceptions. I would like to keep "syncExec" as prefix such that people used to syncExec find the new possibility by autocomplete. So how about "syncExecGet(Supplier)"? +syncExecCall(Callable)?
(In reply to Jörg Kubitz from comment #19) > (In reply to Sebastian Zarnekow from comment #18) > > > syncExecWithValue which is probably not the best name possible so waiting > > syncGet(Supplier) or maybe syncCall(Callable) if we want to allow checked > > exceptions. > > I would like to keep "syncExec" as prefix such that people used to syncExec > find the new possibility by autocomplete. > > So how about "syncExecGet(Supplier)"? > > +syncExecCall(Callable)? I'm not sure about the reasoning that people will type syncExec to invoke content assist there. syncExec, syncCall, and syncGet will be reasonably close together in the proposal list. Anyhow, I'm fine with both proposals.
(In reply to Sebastian Zarnekow from comment #20) > > I'm not sure about the reasoning that people will type syncExec to invoke > content assist there. syncExec, syncCall, and syncGet will be reasonably > close together in the proposal list. I like this above more compared to syncExec*
(In reply to Sebastian Zarnekow from comment #20) > I'm not sure about the reasoning that people will type syncExec to invoke > content assist there. true. So i will go for syncGet
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185385
I wrote a custom org.eclipse.swt.Callable and syncCall which allows both checked and unchecked exception with out the need to add both syncCall and syncGet See 185385 junit tests for example. OK?
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185385 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=c7e2fb24f0063bf238a3000657aad3a65ec55982
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185531
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt.binaries/+/185532
This caused build failure bug 576048
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185531 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=36c739230440ec5e04d3a407d42d7504178d75cb
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.swt.binaries/+/185532 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.binaries.git/commit/?id=ab65792f17a4727addb310c45f9ace5e9085dc83
(In reply to Eclipse Genie from comment #25) > Gerrit change > https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185385 was > merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=c7e2fb24f0063bf238a3000657aad3a65ec55982 This causes javadoc check fail, see https://download.eclipse.org/eclipse/downloads/drops4/I20210917-0000/testresults/html/org.eclipse.releng.tests_ep422I-unit-cen64-gtk3-java11_linux.gtk.x86_64_11.html https://download.eclipse.org/eclipse/downloads/drops4/I20210917-0000/compilelogs/platform.doc.isv.javadoc.txt ../../../eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java:4777: warning: no description for @param * @param callable ^ ../../../eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/SwtCallable.java:30: error: exception not thrown: java.lang.Exception * @throws Exception of given type ^ 1 error 1 warning @Jörg: please provide patch with a fix.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185545
Gerrit change https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/185027 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=35e8de04b93111d2185a3335fb6119125c50f799
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185545 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=2120ba13d4e1f965e37580d4c11dd18185e7a462
Please make sure a note is added to the platform_isv.html file for 4.22 about this new API.
(In reply to Mickael Istria from comment #35) > Please make sure a note is added to the platform_isv.html file for 4.22 > about this new API. Jörg, in case you are now aware what to change, see this repo: https://git.eclipse.org/r/www.eclipse.org/eclipse/news.git
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/185657
Gerrit change https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/185657 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=31c1edaa872fdc7eefecc275263caa249d3abbe8
This new callAsync method catches exceptions that are thrown from the SWT thread and re-throws them on the calling thread. This is convenient but also problematic... The problem is that the exceptions will keep their original stack trace from the SWT thread. The problem with that is that the callAsync method itself and its caller will not be precent in the stack trace. And the problem with that is that it might be hard to figure out what happened, for example when a stack trace appears in a log file, and there is not indication that callAsync was involved or where it was called. I'm not saying that the current implementation is bad. But it has trade-offs, and I hope you are aware also of the disadvantages! I started on a similar feature for the databinding framework, but I got stuck when it became more implicated that I anticipated due to this kind of problems. https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/169603
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/189170
(In reply to Eclipse Genie from comment #40) > New Gerrit change created: > https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/189170 Please don't reuse bugs for new changes. *This* bug is released already in 4.22 and all new work should go on new bug.
(In reply to Jens Lideström from comment #39) > The problem is that the exceptions will keep their original stack trace from > the SWT thread. I don't see what makes that a problem. When you see an exception in a logfile you will see the Object/method that implements SWTCallable.call() - and in all cases that i know there is a single usage of that Object/method. However if the caller wants a stacktrace of the calling thread he could wrap the syncCall in a try-catch block to add an Exception in the calling thread.
(In reply to Andrey Loskutov from comment #41) > Please don't reuse bugs for new changes. *This* bug is released already in > 4.22 and all new work should go on new bug. Sorry. I'll open a new one for documentation updates.
(In reply to Jörg Kubitz from comment #42) > (In reply to Jens Lideström from comment #39) > > > The problem is that the exceptions will keep their original stack trace from > > the SWT thread. > > I don't see what makes that a problem. To debug a problem you need to know where it happened and how the program got there. That's what the stack trace is for. When you throw away parts of the stack trace it gets harder to find the source of problems. People will see stack traces in logs that show a problem happening in SWTCallable.call(). But stack traces will not show where in the program that Display.asyncCall was called. That will make it harder to locate problems.
