Bugzilla – Full Text Bug Listing
|
Description
Simon Scholz
New Gerrit change created: https://git.eclipse.org/r/56927 New Gerrit change created: https://git.eclipse.org/r/59623 Since we still have the hashcode and equals problems with Observables, the safest use of SideEffect will probably be for one-way bindings (initializing labels, the contents of a listbox, etc.) I'd suggest adding such an example. FYI, the SideEffect class was committed to master just now, so we can start committing these as well. Simon, any plans to finish this for M4? Gerrit change https://git.eclipse.org/r/56927 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=05eef427b49209621d83e9fceb34501e5230d6af New Gerrit change created: https://git.eclipse.org/r/67929 New Gerrit change created: https://git.eclipse.org/r/67931 Gerrit change https://git.eclipse.org/r/67931 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=05991be197cd33a29de2e77122f036e4c9bad551 New Gerrit change created: https://git.eclipse.org/r/68082 New Gerrit change created: https://git.eclipse.org/r/68123 Gerrit change https://git.eclipse.org/r/68123 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=b88f960b586ec5842a1d20fbd028f3ddb36431fb Gerrit change https://git.eclipse.org/r/68123 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=b88f960b586ec5842a1d20fbd028f3ddb36431fb I notice in one of the recent samples that the ISideEffect version of the validation code is more lines of code than the DataBindingContext version.
Much of the extra code is due to the need to explicitly dispose the ISideEffects.
I'd suggest we create the following helper method:
ControlSideEffect {
ISideEffect create(Control c, Supplier<T>, Consumer<T>)
}
It would create a new side effect which gets disposed along with the given control. For example:
ControlSideEffect.create(okButton, () -> validationStatus.getValue().isOK(),
okButton::setEnabled);
That would eliminate the need for explicit disposal code, and (in some cases) the need for a local variable to hold the side effect.
The new utility could live in a new utility class, SWTObservables, or WidgetProperties. I'm not sure where the best place for it is.
We could also create a new utility for attaching side effects to workbench parts. It could pause and resume the side-effect when the part is hidden, and dispose the side-effect when the part is closed. I'm not sure what the prototype should be, though, since I'm unsure how E4 parts track their own visibility. (In reply to Stefan Xenos from comment #14) > I notice in one of the recent samples that the ISideEffect version of the > validation code is more lines of code than the DataBindingContext version. > > Much of the extra code is due to the need to explicitly dispose the > ISideEffects. Basically the DataBindingContext is a container for all local bindings. When the DataBindingContext is disposed all Bindings will be disposed too. What do you think of having such a container for ISideEffects as well? public class SideEffectContext implements ISideEffect { private IObservableList<ISideEffect> sideEffects; public SideEffectContext() { this(Realm.getDefault()); } public SideEffectContext(Realm realm) { sideEffects = new WritableList<>(realm); } @Override public void dispose() { sideEffects.forEach(s -> s.dispose()); sideEffects.clear(); } @Override public void pause() { sideEffects.forEach(s -> s.pause()); } @Override public void resume() { sideEffects.forEach(s -> s.resume()); } @Override public void resumeAndRunIfDirty() { sideEffects.forEach(s -> s.resumeAndRunIfDirty()); } @Override public void runIfDirty() { sideEffects.forEach(s -> s.runIfDirty()); } public ISideEffect createPaused(Runnable runnable) { ISideEffect sideEffect = ISideEffect.createPaused(runnable); sideEffects.add(sideEffect); return sideEffect; } public ISideEffect createPaused(Realm realm, Runnable runnable) { ISideEffect sideEffect = ISideEffect.createPaused(realm, runnable); sideEffects.add(sideEffect); return sideEffect; } public ISideEffect create(Runnable runnable) { ISideEffect sideEffect = ISideEffect.create(runnable); sideEffects.add(sideEffect); return sideEffect; } public <T> ISideEffect create(Supplier<T> supplier, Consumer<T> consumer) { ISideEffect sideEffect = ISideEffect.create(supplier, consumer); sideEffects.add(sideEffect); return sideEffect; } public <T> ISideEffect createResumed(Supplier<T> supplier, Consumer<T> consumer) { ISideEffect sideEffect = ISideEffect.createResumed(supplier, consumer); sideEffects.add(sideEffect); return sideEffect; } public <T> ISideEffect consumeOnceAsync(Supplier<T> supplier, Consumer<T> consumer) { ISideEffect sideEffect = ISideEffect.consumeOnceAsync(supplier, consumer); sideEffects.add(sideEffect); return sideEffect; } } With this approach you can use it like this: SideEffectContext sideEffectContext = new SideEffectContext(); sideEffectContext.create(() -> validationStatus.getValue().isOK(), okButton::setEnabled); sideEffectContext.create(person::getFirstName, personFirstNameText::setText); sideEffectContext.create(personFirstNameText::getText, person::setFirstName); // disposed all sideeffects personFirstNameText.addDisposeListener(e -> sideEffectContext.dispose()); Then we could also have a ControlSideEffectContext with a third constructor in JFace Databinding, like this: public class ControlSideEffectContext extends SideEffectContext { public ControlSideEffectContext() { this(Realm.getDefault()); } public ControlSideEffectContext(Realm realm) { this(realm, null); } public ControlSideEffectContext(Realm realm, Control disposeableControl) { super(realm); disposeableControl.addDisposeListener(e -> ControlSideEffectContext.this.dispose()); } } ... so that even the disposal is automatically managed by using ControlSideEffectContext. Concerning E4 Parts, we could then put this SideEffectContext into the context of a part and create a model addon, which is able to pause all SideEffects in case the part is hidden in the stack. @Inject @Optional public void subscribeTopicSelectedElement(@EventTopic (UIEvents.ElementContainer.TOPIC_SELECTEDELEMENT) Event event) { Object newValue = event.getProperty(EventTags.NEW_VALUE); Object oldValue = event.getProperty(EventTags.OLD_VALUE); if (oldValue instanceof MPart) { MPart oldPart = (MPart) oldValue; ISideEffect oldPartsSideEffect = oldPart.getContext().get(ISideEffect.class); if(oldPartsSideEffect != null) { oldPartsSideEffect.pause(); } } if (newValue instanceof MPart) { MPart newPart = (MPart) newValue; ISideEffect newPartsSideEffect = newPart.getContext().get(ISideEffect.class); if(newPartsSideEffect != null) { newPartsSideEffect.resume(); } } } New Gerrit change created: https://git.eclipse.org/r/68283 New Gerrit change created: https://git.eclipse.org/r/72578 Gerrit change https://git.eclipse.org/r/72578 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4b5dc694f17a21124a619b566be2dbe9cf99a909 Gerrit change https://git.eclipse.org/r/68283 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=f9e3ffeb300ec8ab28cacd9d32f4cf05d2c86a0e Please revert those changes. No changes are allowed at this point. Please read https://dev.eclipse.org/mhonarc/lists/platform-ui-dev/msg07082.html " As previously mentioned by David, tonight 20:00 EDT will be our RC1 candidate build. This means, no further changes must be committed or merged after that. " New Gerrit change created: https://git.eclipse.org/r/72613 Gerrit change https://git.eclipse.org/r/72613 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=8c9039bbe6cd389f7a517182efabb43b545aae88 (In reply to Eclipse Genie from comment #23) > Gerrit change https://git.eclipse.org/r/72613 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=8c9039bbe6cd389f7a517182efabb43b545aae88 > Thanks. Please also revert the other change. (In reply to Dani Megert from comment #24) > (In reply to Eclipse Genie from comment #23) > > Gerrit change https://git.eclipse.org/r/72613 was merged to [master]. > > Commit: > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=8c9039bbe6cd389f7a517182efabb43b545aae88 > > > > Thanks. Please also revert the other change. Not good. The rebuild started and now this bug is partially in RC1 and RC2. New Gerrit change created: https://git.eclipse.org/r/72637 Gerrit change https://git.eclipse.org/r/72637 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4a8a502b4698be997f02b1ffc24e4ba18ef4b002 Sorry I was busy almost all afternoon. I reverted it right now. I thought for example projects it would still be ok to merge. (In reply to Simon Scholz from comment #28) > Sorry I was busy almost all afternoon. Next time, please just tell us that here, and someone else takes care of it. That's not a problem. > I reverted it right now. Unfortunately, this makes it even worse. You could have checked http://git.eclipse.org/c/platform/eclipse.platform.ui.git/ to see that your change was already picked up by the 10:00 rebuild, i.e. this is now in RC1 (unless we have to rebuild again), but you've reverted it after that build. > I thought for example projects it would still be ok to merge. Nope, no commit or merge is allowed, except for those that get announced as critical issues on the platform-releng mailing list. We're forgiving commits that do not affect the build, e.g. changes to N&N, webpage, etc. New Gerrit change created: https://git.eclipse.org/r/72852 Gerrit change https://git.eclipse.org/r/72852 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=536b6c53de3708e5c6bb59cbb887f8e60027f180 New Gerrit change created: https://git.eclipse.org/r/72854 Gerrit change https://git.eclipse.org/r/72854 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=ea320b96d23e59e415a80e8125e2c71a24ee8183 Gerrit change https://git.eclipse.org/r/59623 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=748360f919fe26185a2a6a901dc426d6304b103b Done with creating snippets for now. Still one open Gerrit. Gerrit change https://git.eclipse.org/r/67929 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=421d6a1caa64c321dd5a519a412e80436e1572f1 Thanks Simon and Jens. |