Community
Participate
Working Groups
Right now one writes a lot of boilerplate code to join the SWT UI thread when called by DI. E.g. @Inject @Optional public void setContent(final ContentService cs, UISynchronize sync) { sync.asyncExec(new Runnable() { public void run() { label.setText(cs.getTitle()); } }); } It would be much cleaner to have a @UIThread-like annotation similar to @UIEventTopic: @Inject @Optional @UIThread public void setContent(final ContentService cs) { label.setText(cs.getTitle()); } Btw. it seems PW touched this idea in the not so distant past as well [1]. [1] http://www.eclipse.org/forums/index.php/mv/msg/488421/1064250/#msg_1064250
I'm not sure where to put this. It's an annotation that effects how a DI method is called as opposed to what a DI method is provided, so it can't be added via ExtendedObjectSupplier, but it is so specific it's not eligible for an extension in the DI mechanism. DI implementation itself can't see the UISynchronize object. Maybe a core.di to ui.di internal interface? PW
(In reply to Paul Webster from comment #1) > I'm not sure where to put this. It's an annotation that effects how a DI > method is called as opposed to what a DI method is provided, so it can't be > added via ExtendedObjectSupplier, but it is so specific it's not eligible > for an extension in the DI mechanism. DI implementation itself can't see > the UISynchronize object. > > Maybe a core.di to ui.di internal interface? Sounds like an ExtendedRealmSupplier that supplies a user provided realm to an invocation call. It could be registered as an OSGi service identical to ExtendedObjectSupplier. org.eclipse.e4.core.internal.di.InjectorImpl.uninject(Object, PrimaryObjectSupplier) and org.eclipse.e4.core.internal.di.InjectorImpl.inject(Object, PrimaryObjectSupplier, PrimaryObjectSupplier) could delegate the call to MethodRequestor to the ERS for it to call org.eclipse.e4.core.di.suppliers.IRequestor.execute(). Btw. I updated the title to reflect that an @UIInject migt be easier to use than @Inject @UIThread from the consumer POV. It's different to ExtendedObjectSupplier anyway.
> Btw. I updated the title to reflect that an @UIInject migt be easier to use > than @Inject @UIThread from the consumer POV. It's different to > ExtendedObjectSupplier anyway. I like the fact that the DI annotations are based on a standard (JSR330). So if possible I would prefer @Inject with an additional qualifier. See https://code.google.com/p/google-guice/wiki/JSR330 http://atinject.googlecode.com/svn/trunk/javadoc/javax/inject/Qualifier.html
<lemmy> paulweb515: What do you think about my last comment on ~421376? If this seems reasonable, I might try to provide a patch. <paulweb515> +lemmy: in comment #1 I was suggesting that this is not an area that we should optimize for extensability ... this is the one specific case where we want @Inject to work differently <lemmy> paulweb515: Why not make it extensible? <lemmy> E.g. other renderer could provide their realm <paulweb515> +We need at least one more example before we add the extra code. It depends on how we plug in the Synchroizer
Having though about this a while I think all DI should run in the main thread. If If I annotate a field or a method with @Inject I cannot make assumption which thread changes the corresponding key in the context. Hence the framework should IMHO always assure that DI is performed in the main thread.
(In reply to Lars Vogel from comment #5) > Having though about this a while I think all DI should run in the main > thread. If If I annotate a field or a method with @Inject I cannot make > assumption which thread changes the corresponding key in the context. Hence > the framework should IMHO always assure that DI is performed in the main > thread. I'd argue that this is an incompatible API change (OSGi versioning aside) as it causes all current implementations that anticipate being called from a backend thread to be obsolete. Additionally, consumers would be required to change the thread context twice (UI > Worker > (back to) UI) if they do a long running operation in one of the @Inject methods.
> Additionally, consumers would be required to change the thread context twice > (UI > Worker > (back to) UI) if they do a long running operation in one of > the @Inject methods. Not sure what you mean, the entry point of the method would be in the main thread by the framework, afterwards the called method can open new threads if desired without the need to switch back to the main thread.
(In reply to Lars Vogel from comment #7) > Not sure what you mean, the entry point of the method would be in the main > thread by the framework, afterwards the called method can open new threads > if desired without the need to switch back to the main thread. Exactly, the callee would have to implement this. Right now, there is no such requirement if the callee knows that the call comes from a backend thread. Btw. your suggestion adds yet another inconsistency to the framework. @EventTopic means you can be called by any thread, @UIEventTopic means you get called by the UI thread. I would strongly suggest to adapt the same pattern for @Inject/@UIInject. (Aside from the backward compatibility issues mentioned in my previous comment which would IMO require a major version increment for core.di).
Here is another thought, lets add @UIThread to generally annotate a method to be called in the UI thread and remove/deprecate @UIEventTopic. Then @UIThread can be used for both use cases (plain inject and event topic based inject). (I'd also assume that it would make the implementation for @UIEventTopic obsolete)
I like the idea of making it a qualifier that we can re-act to. So @Inject @UIThread (or whatever name we decide) @Named("whatever") public void setInfo(String info); Maybe put that in org.eclipse.e4.ui.di. We need to be able to access org.eclipse.e4.ui.di.UISynchronize if it's available to push something marked with @UIThread into a syncExec(*) PW
<lemmy> paulweb515: I might be able to take a stab at ~421376 over the next few days. When you say access to UISynchronize though, I wonder if you really want to create a dep from core.di to ui. I would rather expect UISync to implement an interface provided by core.di like an "IRealm". <paulweb515> +lemmy: core.di should export an interface that we can provide back to it, sure. For now, make it internal. I wouldn't call it IRealm though, since the databinding Realm that platform consumers are familiar with doesn't have the notion of syncExec (only asyncExec)
https://git.eclipse.org/r/19919 (eclipse.platform.runtime) https://git.eclipse.org/r/19920 (eclipse.platform.ui) (Initial design sketch)
(In reply to Markus Kuppe from comment #12) > https://git.eclipse.org/r/19919 (eclipse.platform.runtime) > https://git.eclipse.org/r/19920 (eclipse.platform.ui) Paul could you review these as well? PW
This is really cool Markus — I really like the approach. I'd been having similar thoughts myself: I'm working on a big app where we have both legacy Swing-based code and newer SWT-based code, and potentially Qt- and JavaFX-based code. Managing some of the thread boundaries is becoming painful, and being able to offload some of this work onto the injector was something I was girding up for. (In reply to Markus Kuppe from comment #2) > Btw. I updated the title to reflect that an @UIInject migt be easier to use > than @Inject @UIThread from the consumer POV. It's different to > ExtendedObjectSupplier anyway. I have a slight preference for retaining @Inject and adding a complementary annotation. Retaining @Inject would make introspection tools slightly easier (and means that InjectorImpl doesn't need to be changed); that said, our injector already supports custom annotation for new object suppliers. It would mean that we wouldn't need to worry about @Inject-ed methods being injected "normally" should the domain annotation handler not be found. That said, I'm not sure what "@Inject @UIInject" would mean. I think your patches may be incomplete as I can't actually see how this works since you haven't included a patch for the InjectorImpl#processMethods(), which only looks for Inject.class. My guess is that this was overlooked in the I'll add some feedback on the implementation to the gerrit submissions. High-level comments: * The implementation should support multiple domain annotations. See how org.eclipse.e4.core.internal.di.osgi.ProviderHelper manages this: the supplier service definition specifies the annotation. * I wonder if we want to have annotations for async- and sync-exec? (i.e., @UIAsync and @UISync) * I'd prefer just plain "DomainHandler" since this could be used in situations other than UI threads (e.g., could be transactional?) * Would it make sense for the domain handler to be used for type creation too (IInjector#make)? * It would be useful for the DomainHandler to be provided the annotation and the requestor object too. I've found it useful to be able to introspect on the request site within my custom object suppliers, and I can see it being useful here too. For example, the SWTThreadDomainHandler could check if the requesting object is adaptable to UISynchronize. I've used custom interfaces on the requesting object to obtain further information (e.g., to provide lookup scopes for the object supplier). Alternatively an SWT annotation could provide an optional class name for a class to determine the synchronizer (@UIThread(finder=DisplayFinder.class))
I think it definitely has to be @Inject <modifier annotation> We're not supplanting JSR 330, we're providing a modifier to provide direction to our DI engine. PW
(In reply to Brian de Alwis from comment #14) > I'll add some feedback on the implementation to the gerrit submissions. > High-level comments: > > * The implementation should support multiple domain annotations. See how > org.eclipse.e4.core.internal.di.osgi.ProviderHelper manages this: the > supplier service definition specifies the annotation. So you want to provide something extensible that can modify how the injector calls methods? That would have to be designed very carefully to make sure it's performant, as we make method calls with CIF.invoke(*) a *lot*. It wouldn't really be "domain handler" any more, as it's about modifying inject and invoke calls. > > * I wonder if we want to have annotations for async- and sync-exec? > (i.e., @UIAsync and @UISync) sync or async is only relevant to the caller, not the callee, no? > * I'd prefer just plain "DomainHandler" since this could be used in > situations other than UI threads (e.g., could be transactional?) This is back to making it more modifiable. > * Would it make sense for the domain handler to be used for type creation > too (IInjector#make)? Would you have to annotate the constructor or annotation the class itself? > * It would be useful for the DomainHandler to be provided the annotation > and the requestor object too. If it was extensible and the point was to modify any method calls related to injection, then it would make sense to have the requestor object and any other injection context information that was reasonable. But I'm not 100% convinced that this should be extensible without a couple of usecases and examples to demonstrate how it's going to be used. PW
To me it sounds as if we only have consensus for the current use case. That shouldn't mean though, that the user facing API (annotation name/name ThreadDomainHandler) should just be tailored for the use case at hand. If we rename @UIInject or @UIThread to something more generic like @Context/@Domain and have it accept a parameter that determines the Domain type (@Domain(Domain.UIThread), we can support the current use case and easily extend it later once we have concrete use cases.
As an outside observer let me ask: is the annotation intended only as a shorthand to avoid calls to (a)sync_exec (less coding), or could these be used also for statically checking contracts (fewer bugs)? Please see http://dev.eclipse.org/mhonarc/lists/platform-swt-dev/msg07538.html
(In reply to Markus Kuppe from comment #17) > To me it sounds as if we only have consensus for the current use case. That > shouldn't mean though, that the user facing API (annotation name/name > ThreadDomainHandler) should just be tailored for the use case at hand. That's what we do. We don't add generic framework API until we have a couple of usecases that validate it. We don't here, and I'd prefer a specific for this usecase. Then if there is something that's the same (modifying how injects are called) then we can deprecate this and re-implement it in terms of the extensible API. As for providing an annotation, that's something that we key on (as our extended object supplier pattern). It seems providing one annotation with a bunch of parameters would make it one kind of plug in to the DI engine that would have to switch on one or more parameters and provide multiple consumers. Why introduce a second pattern and push the switching into a sub-module? PW
Markus said: -1 for @SWTThread. It is _not_ SWT specific and can be used by other GUI libararies. Just register a org.eclipse.e4.core.internal.di.ThreadDomainHandler service with a higher rank or simply replace the org.eclipse.e4.ui.workbench.swt.
(In reply to Paul Webster from comment #20) > Markus said: > > -1 for @SWTThread. It is _not_ SWT specific and can be used by other GUI > libararies. Just register a > org.eclipse.e4.core.internal.di.ThreadDomainHandler service with a higher > rank or simply replace the org.eclipse.e4.ui.workbench.swt. I thought Brian mentioned that he's run into SWT and Swing and/or JavaFX in one app. Being specific tells everybody what is needed (SWTThread). Being vague (@UIThread) means making the annotation implementation have to be extensible in some way ... extensible extensions to the engine sound like more complication than is needed. I'm not 100% against the idea of just leaving it @UIThread, but I don't see the value of adding a secondary extension mechanism on top of the extension, which means I prefer more specificity ... but I'm open to more discussion on the topic. PW
(In reply to Paul Webster from comment #16) > So you want to provide something extensible that can modify how the injector > calls methods? That would have to be designed very carefully to make sure > it's performant, as we make method calls with CIF.invoke(*) a *lot*. Agreed. This current implementation is already modifying how the injector calls methods, as Markus points out how the TDH can be replaced. > > * I wonder if we want to have annotations for async- and sync-exec? > > (i.e., @UIAsync and @UISync) > > sync or async is only relevant to the caller, not the callee, no? That seems true, but the callee may not actually require sync. > > * Would it make sense for the domain handler to be used for type creation > > too (IInjector#make)? > > Would you have to annotate the constructor or annotation the class itself? The class. I don't have a use-case for this, however, and I'm happy to drop it. (In reply to Paul Webster from comment #19) > That's what we do. We don't add generic framework API until we have a > couple of usecases that validate it. We don't here, and I'd prefer a > specific for this usecase. Then if there is something that's the same > (modifying how injects are called) then we can deprecate this and > re-implement it in terms of the extensible API. So I have a definite need for handling both SWT and AWT/Swing, right now. I'm dealing with a 15mLOC app suite that's being ported over to Eclipse and much of our time is spent diagnosing and tracing down incorrect threading issues. We have our own @AwtThread and @SwtThread annotations and we use them within some of our supporting infrastructure (e.g., variants of ListenerList). Figuring out how to leverage these annotations for DI has been on my list for a while. Having explicit @SWT and @AWT style annotations allows for easy visual validation. It's easy for developers to understand. Currently I have a number of methods on injected viewparts that look like: @Inject protected void modeChanged(final @Named(InterpretationService.MODE) String mode) { getDisplay().asyncExec(new Runnable() { public void run() { // . . . }}); } In this case I know the implementation is in SWT, and the execution must be performed on the SWT thread. I have some similar code on Swing-based implementations. With only a generic @UI annotation, I now need to reason about the possible runtime injection context. And I likely then spend time to figure out how to influence that runtime context to accomplish what I want. (And I can't easily have mixed-mode injection sites on the same class — that one method should be on SWT and the other on Swing.) I think a generic @UI would be useful for Eclipse-related code that needs to execute on the same UI thread as the workbench. It means we could have code that manipulates the model, etc., seamlessly support E4 on SWT or JavaFX. These annotations are effectively saying: this method needs to be executed in a specific execution context. @UI would mean in the context as specified by the UI framework. So I'd like both @UI and the ability to add my own @SWT/@AWT :-)
(In reply to Stephan Herrmann from comment #18) > As an outside observer let me ask: is the annotation intended only as a > shorthand to avoid calls to (a)sync_exec (less coding), or could these be > used also for statically checking contracts (fewer bugs)? > > Please see > http://dev.eclipse.org/mhonarc/lists/platform-swt-dev/msg07538.html It could do both, couldn't it? (In reply to Brian de Alwis from comment #22) > > > * I wonder if we want to have annotations for async- and sync-exec? > > > (i.e., @UIAsync and @UISync) > > > > sync or async is only relevant to the caller, not the callee, no? > > That seems true, but the callee may not actually require sync. With the inversion of DI, the "caller" will be totally oblivious that this call would be dispatched via SWT, sync or async. The expectation would be something sync (I.e., the recipient has been injected at the end of the invoke/inject), but the recipient may be ok with the injection being delayed. But there are real troubles with deadlock using syncExec from an AWT thread with the SWT_AWT bridge. Apps need some way to plug in their own dispatcher such as per Markus' suggestion in comment 20.
(In reply to Brian de Alwis from comment #22) > I think a generic @UI would be useful for Eclipse-related code that needs to > execute on the same UI thread as the workbench. It means we could have code > that manipulates the model, etc., seamlessly support E4 on SWT or JavaFX. > > These annotations are effectively saying: this method needs to be executed > in a specific execution context. @UI would mean in the context as specified > by the UI framework. > > So I'd like both @UI and the ability to add my own @SWT/@AWT :-) It looks to me as if this is the best way to proceed. It offers consumers to be less specific where it isn't needed, and also leaves the door open to advanced use cases where UI libraries are mixed in a single application. Personally though, I would be in favor of combining everything into a single annotation like @UIThread that takes a (qualifying) parameter (SWT|AWT|JAVAFX|...) when need: @Inject @UIThread(SWT) protected void inSWTThread(final @Named(InterpretationService.MODE) String mode) { // foo } @Inject @UIThread(AWT) protected void inAWTThread(final @Named(InterpretationService.MODE) String mode) { // foo } @Inject @UIThread protected void inGenericThread(final @Named(InterpretationService.MODE) String mode) { // foo } Obviously the lookup in https://git.eclipse.org/r/#/c/19919/3/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/internal/di/osgi/ThreadDomainHelper.java needs an enhancement to take the parameters into account.
Is this something to shoot for Luna or later?
Abondoned due to long inactivitiy, please restore if it is still relevant
