Community
Participate
Working Groups
SafeRunner should be able to return results. Often the "array pattern" is used to overcome final restriction. Object[] obj = new Object[1]; SafeRunner.run(()-> obj[0] = callSomething()); SafeRunner should get a run method which accepts something like a SafeRunnableWithResult. This has a run method which returns something (<T>).
Inspired by https://bugs.eclipse.org/bugs/show_bug.cgi?id=558562
I had the same in mind, when I reviewed your change, therefore +1 for the request. When implementing that, please consider making the class more user friendly than the current ISafeRunnable, which declares 2 methods, where 1 of them is always implemented empty. To get around that noise and to make the SafeRunnable usable as lambda, egit has a slightly different implementation for it: https://git.eclipse.org/r/#/c/131625/4/org.eclipse.egit.core/src/org/eclipse/egit/core/internal/SafeRunnable.java
(In reply to Michael Keppler from comment #2) > When implementing that, please consider making the class more user friendly > than the current ISafeRunnable, which declares 2 methods, where 1 of them is > always implemented empty. To get around that noise and to make the > SafeRunnable usable as lambda, egit has a slightly different implementation > for it: > https://git.eclipse.org/r/#/c/131625/4/org.eclipse.egit.core/src/org/eclipse/ > egit/core/internal/SafeRunnable.java org.eclipse.core.runtime.ISafeRunnable already is a @FunctionalInterface and can be implemented using a lambda.
I think it's a good candidate for next milestone, but too late for the upcoming one.
Gerrit change https://git.eclipse.org/r/155065 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=30247bf8acddfc1a8b89127aeb1caf66dad8ecf8
Thanks Marcus, please add to N&N.
This causes regression on all platforms, see https://download.eclipse.org/eclipse/downloads/drops4/I20200106-1805/testresults/html/org.eclipse.core.tests.runtime_ep415I-unit-cen64-gtk3-java8_linux.gtk.x86_64_8.0.html 2.1 expected:<org.eclipse.[core.tests.runtime]> but was:<org.eclipse.[equinox.common]> junit.framework.ComparisonFailure: 2.1 expected:<org.eclipse.[core.tests.runtime]> but was:<org.eclipse.[equinox.common]> at junit.framework.Assert.assertEquals(Assert.java:100) at junit.framework.TestCase.assertEquals(TestCase.java:261) at org.eclipse.core.tests.runtime.PlatformTest.testRunnable(PlatformTest.java:151) Please evaluate if this is a problem with the change itself or only test that need to be fixed.
(In reply to Andrey Loskutov from comment #7) > This causes regression on all platforms, see Also this causes *another* regression on all platforms: https://download.eclipse.org/eclipse/downloads/drops4/I20200106-1805/testresults/html/org.eclipse.jface.text.tests_ep415I-unit-cen64-gtk3-java8_linux.gtk.x86_64_8.0.html java.lang.AssertionError at org.junit.Assert.fail(Assert.java:86) at org.junit.Assert.assertTrue(Assert.java:41) at org.junit.Assert.assertNotNull(Assert.java:712) at org.junit.Assert.assertNotNull(Assert.java:722) at org.eclipse.jface.text.tests.contentassist.AsyncContentAssistTest.testAsyncFailureStackOverflow(AsyncContentAssistTest.java:69)
I suggest to revert for M1 and revisit for M3. Marcus, OK for you?
(In reply to Lars Vogel from comment #9) > I suggest to revert for M1 and revisit for M3. > > Marcus, OK for you? Yes, I'll revert.
(In reply to Marcus Höpfner from comment #10) > (In reply to Lars Vogel from comment #9) > > I suggest to revert for M1 and revisit for M3. > > > > Marcus, OK for you? > > Yes, I'll revert. Sorry, I'm not allowed. Can someone else revert please.
New Gerrit change created: https://git.eclipse.org/r/155370
New Gerrit change created: https://git.eclipse.org/r/155371
(In reply to Marcus Höpfner from comment #11) > Sorry, I'm not allowed. Can someone else revert please. Revert patch submitted for review at https://git.eclipse.org/r/#/c/155371/ . I'll leave it to other stakeholders to merge it or not.
(In reply to Mickael Istria from comment #14) > (In reply to Marcus Höpfner from comment #11) > > Sorry, I'm not allowed. Can someone else revert please. > > Revert patch submitted for review at https://git.eclipse.org/r/#/c/155371/ . > I'll leave it to other stakeholders to merge it or not. Abandonned and replaced with https://git.eclipse.org/r/155370
Gerrit change https://git.eclipse.org/r/155370 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=4c7d63e115c486d331728fa0396ab2665e29d578
(In reply to Mickael Istria from comment #15) > (In reply to Mickael Istria from comment #14) > > (In reply to Marcus Höpfner from comment #11) > > > Sorry, I'm not allowed. Can someone else revert please. > > > > Revert patch submitted for review at https://git.eclipse.org/r/#/c/155371/ . > > I'll leave it to other stakeholders to merge it or not. > > Abandonned and replaced with https://git.eclipse.org/r/155370 Can someone please revert https://git.eclipse.org/r/c/155370/ again so that we do not forget this for M3. I don't have committer rights and hence cannot do it. Interesting authorization by the ways, I can upload commits by I cannot create revert commits via gerrit.
(In reply to Marcus Höpfner from comment #17) > > Abandonned and replaced with https://git.eclipse.org/r/155370 > > Can someone please revert https://git.eclipse.org/r/c/155370/ again so that > we do not forget this for M3. I don't have committer rights and hence cannot > do it. > Interesting authorization by the ways, I can upload commits by I cannot > create revert commits via gerrit. I would first try to investigate what caused the two regressions. I don't think it will be a full revert then, so you will probably land in a new gerrit anyway.
(In reply to Andrey Loskutov from comment #18) > (In reply to Marcus Höpfner from comment #17) > > > Abandonned and replaced with https://git.eclipse.org/r/155370 > > > > Can someone please revert https://git.eclipse.org/r/c/155370/ again so that > > we do not forget this for M3. I don't have committer rights and hence cannot > > do it. > > Interesting authorization by the ways, I can upload commits by I cannot > > create revert commits via gerrit. > > I would first try to investigate what caused the two regressions. I don't > think it will be a full revert then, so you will probably land in a new > gerrit anyway. Sure. I'll investigate. But I'd like to start with the revert (of the revert). Since the regression occurred due to this change I'd rather start with it and adapt it instead of creating a new from scratch.
(In reply to Marcus Höpfner from comment #19) > Sure. I'll investigate. But I'd like to start with the revert (of the > revert). Since the regression occurred due to this change I'd rather start > with it and adapt it instead of creating a new from scratch. You can revert locally, update the code if needed and push, so where is the problem? If *I* will revert, you will not be able to modify the gerrit, because contributors are unable to update gerrits from commiters (there is a bug in our gerrit somewhere about it).
(In reply to Andrey Loskutov from comment #20) > (In reply to Marcus Höpfner from comment #19) > > Sure. I'll investigate. But I'd like to start with the revert (of the > > revert). Since the regression occurred due to this change I'd rather start > > with it and adapt it instead of creating a new from scratch. > > You can revert locally, update the code if needed and push, so where is the > problem? If *I* will revert, you will not be able to modify the gerrit, > because contributors are unable to update gerrits from commiters (there is a > bug in our gerrit somewhere about it). Ok. I'll do it
New Gerrit change created: https://git.eclipse.org/r/155742
(In reply to Andrey Loskutov from comment #7) > This causes regression on all platforms, see > > https://download.eclipse.org/eclipse/downloads/drops4/I20200106-1805/ > testresults/html/org.eclipse.core.tests.runtime_ep415I-unit-cen64-gtk3- > java8_linux.gtk.x86_64_8.0.html > > 2.1 expected:<org.eclipse.[core.tests.runtime]> but > was:<org.eclipse.[equinox.common]> > > junit.framework.ComparisonFailure: 2.1 > expected:<org.eclipse.[core.tests.runtime]> but > was:<org.eclipse.[equinox.common]> > at junit.framework.Assert.assertEquals(Assert.java:100) > at junit.framework.TestCase.assertEquals(TestCase.java:261) > at > org.eclipse.core.tests.runtime.PlatformTest.testRunnable(PlatformTest.java: > 151) > > Please evaluate if this is a problem with the change itself or only test > that need to be fixed. I have uploaded a new commit. Problem with the original one was, that the run method used a SafeRunnerWithResult<Void> to prevent duplicate code. Unfortunately the bundle of this SafeRunnerWithResult was determined instead of the bundle id of the original runnable. I removed the "composition" code and now it works. I tested only the above unit test. Not the one from comment #8 (https://bugs.eclipse.org/bugs/show_bug.cgi?id=558642#c8) I'm pretty sure it was the same problem.
New Gerrit change created: https://git.eclipse.org/r/155744
For me this is ready for merge. Can someone please submit.
Do you want me to do something more, are there any objections? Or can this be merged? Thanks
Gerrit change https://git.eclipse.org/r/155742 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=c1e824bb2026b5a7ef58698e0bd914ad0d8d0c5a
I merged it on behave of Sebastians review, who AFAIK is not yet an Equinox committer.
Thanks Marcus for the work!
Javadoc build failed with this patch: https://download.eclipse.org/eclipse/downloads/drops4/I20200205-1800/compilelogs/platform.doc.isv.javadoc.txt
New Gerrit change created: https://git.eclipse.org/r/157251
Gerrit change https://git.eclipse.org/r/157251 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=1cf8213ddf6fefa2cf8bd877ea0fbe45f78428c0
As javadoc has been fixed resolved.
The N&N is still in review on gerrit https://git.eclipse.org/r/c/155744/. Should I abandon or merge?
Gerrit change https://git.eclipse.org/r/155744 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=12b76357d5bcb007e3a9315ea7feb764ea155942
