Bugzilla – Full Text Bug Listing
| Summary: | Add SingletonUtil method with UISession parameter | ||
|---|---|---|---|
| Product: | [RT] RAP | Reporter: | Ralf Sternberg <rsternberg> |
| Component: | RWT | Assignee: | Project Inbox <rap-inbox> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | tbuschto |
| Version: | 2.3 | ||
| Target Milestone: | 2.3 M2 | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Whiteboard: | |||
I think "getUISessionInstance" could also be confusing. I'm also asking myself if this is overall a useful addition, considering that the code that deals with the Session Singleton is somewhat likely to do more things that require the fake context and therefore might have one anyway. If that is the case more often than not, we would add API with limited use that requires more explenation than before. Not sure this is true, just thinking. (In reply to Tim Buschtoens from comment #1) > I think "getUISessionInstance" could also be confusing. You think the name is confusing? Why? > I'm also asking > myself if this is overall a useful addition, considering that the code that > deals with the Session Singleton is somewhat likely to do more things that > require the fake context and therefore might have one anyway. If that is the > case more often than not, we would add API with limited use that requires > more explenation than before. Not sure this is true, just thinking. I see your concerns but I don't think that your assumption is true. With the introduction of UISession in RAP 2.0, and the possibility to obtain the UISession for a given Display from RWT.getUISession( Display ), SessionSingletons are in fact the only thing in RWT that still requires a fake context. The "fake context" is a problematic concept because it manipulates the environment to act as if it was actually processing a request even though there is none. This has some weird implications such as those discussed in bug 350300. It is needed to support code that relies on accessing its context via static methods. Code that does so is tightly coupled to the environment and is very hard to test. Even though we won't get rid of fake contexts anytime soon (existing applications might rely on it, and it's also needed in the workbench by UIJob), we should support writing new applications in a clean style that does not rely on static methods. (In reply to Ralf Sternberg from comment #2) > (In reply to Tim Buschtoens from comment #1) > > I think "getUISessionInstance" could also be confusing. > > You think the name is confusing? Why? The name could mean "get me an instance of UISession", but the parameter "type" then clarifies that it means another type. > I see your concerns but I don't think that your assumption is true. With the > introduction of UISession in RAP 2.0, and the possibility to obtain the > UISession for a given Display from RWT.getUISession( Display ), > SessionSingletons are in fact the only thing in RWT that still requires a > fake context. I may be wrong, but when accessing widgets you need to use Display.asyncExec not UISession.exec, right? And in that case you already have session context and the current SingletonUtil would work fine. I just don't know how often you need to access a SessionSingleton but no widgets. That's what I meant. (In reply to comment #3) > The name could mean "get me an instance of UISession", but the parameter "type" > then clarifies that it means another type. I see. What about getInstanceFor( Class<T> type, UISession uiSession) and getInstanceFor( Class<T> type, ApplicationContext applicationContext ) I think that SingletonUtil.getInstanceFor( MyType.class, RWT.getUISession() ) is not so much more complicated than SingletonUtil.getSessionInstance( MyType.class ) It might even be clearer. We could also consider a method getSingletonInstance on UISession and ApplicationContext: uiSession().getSingletonInstance( MyType.class ) This would make the functionality easier to find, but it would also add clutter to the UISession. Since it's really just a utility, not a core functionality of the UISession, I'm not fond of this idea. > I may be wrong, but when accessing widgets you need to use Display.asyncExec not > UISession.exec, right? And in that case you already have session context and the > current SingletonUtil would work fine. I just don't know how often you need to > access a SessionSingleton but no widgets. That's what I meant. Background threads have been the main reason for the infamous fake context. When a background thread needs access to a UISession singleton, it should not need to wrap this code in Display.asyncExec() as this would cause additional network traffic and defer code execution. How often this is required is up to the application but we shouldn't rule it out. http://eclipse.org/rap/developers-guide/devguide.php?topic=threads.html&version=2.2 (In reply to Ralf Sternberg from comment #4) > I see. What about > getInstanceFor( Class<T> type, UISession uiSession) > and > getInstanceFor( Class<T> type, ApplicationContext applicationContext ) Crystal clear. Could also be "getInstanceOf"? > > We could also consider a method getSingletonInstance on UISession and > ApplicationContext: > uiSession().getSingletonInstance( MyType.class ) > This would make the functionality easier to find, but it would also add > clutter to the UISession. Since it's really just a utility, not a core > functionality of the UISession, I'm not fond of this idea. We could do it and later remove SingletonUtil completely. What I really like about this is that it the method still has only one parameter. |
When accessing a session singleton from a background thread, the code has to be executed with a “fake context” using `UISession#exec( Runnable )`. If the SingletonUtil provided an additional method to create UI session singletons for a given UISession, this fake context would not be required anymore. A session singleton could then provide a getInstance method with a UISession parameter and/or one without. MySingleton getInstance() { SingletonUtil.getSessionInstance( MySingleton.class, RWT.getUISession ); } MySingleton getInstance( UISession uiSession ) { SingletonUtil.getSessionInstance( MySingleton.class, uiSession ); } The second form allows to create an instance for a given UISession in a background thread directly. This form also improves testability because it does not rely on the UISession of the current context and could also accept a mock object. We should also think about a better method name. With the renaming of "SessionStore" to "UISession" in RAP 2.0, the method name "getSessionInstance" is ambiguous. The method should be named either - getInstance( type, uiSession ) or - getUISessionInstance( type, uiSession )