Looks great!

A few notes:

   - You should use DI as most as possible, e.g. inject Provider<>s in the 
   AppActivityMapper, and inject the PlaceController into the activities, 
   instead of using SchoolPlanner.getInjector(). Actually, your injector 
   instance shouldn't be stored in a 'static' field and shouldn't ever be used 
   outside your EntryPoint; simply GWT.create() it within the onModuleLoad, 
   it'll work just as well and will remove the incentive to call it from 
   elsewhere rather than having your dependencies injected. Your Ginjector 
   would then also only have to declare accessors for the PlaceHistoryMapper 
   (or *Handler, depending on how you do it; I prefer creating the Handler and 
   calling register() in my onModuleLoad) and ActivityMapper-s (or *Manager-s).
   - The ActivityManager and PlaceHistoryHandler don't have to be 
   @Singleton: you'll only ever use them once; same for the default place. You 
   should also use binding annotations, this will help when you'll add other 
   ActivityManager-s (and this is why I prefer surfacing my ActivityMapper-s, 
   with their specific classes, that way I don't even need binding annotations 
   to distinguish them). 
   See 
http://www.wanderingcanadian.ca/a-simpler-way-to-use-gin-in-multi-display-gwt
   - Why are you injecting the EventBus in your activities? There's almost 
   no reason to not use the one passed to the start() method.
   - In the AppActivityMapper, why are you "creating an empty student"? Why 
   aren't you simply passing the ID to the activity? (instead of an entity). 
   Your student.getEmail() checked are fragile, they remind me of 
   browser-sniffing vs. feature-detection.
   - In your activities, you should call setPresenter on the view only on 
   start(). Calling it in the constructor means the new activity is "stealing" 
   the view from the previous not-yet-stopped one, which could do things with 
   the view in its onStop(). Well, it's safer to only setPresenter in start(). 
   What we did was to also setPresenter(null) in onStop and onCancel, and in 
   the view's setPresenter add an « assert this.presenter != null && presenter 
   == null : "someone's trying to steal the view from another presenter"; » 
   (or something similar)
   - You should check whether the activity is still "active" before calling 
   PlaceController#goTo from your Receiver: the user might have navigated away 
   from the activity already.
   - You don't want to inject a RequestContext, this is something that 
   should be entirely controlled (including most importantly its lifetime) by 
   the "user code". Inject the RequestFactory or a Provider<> instead 
   (injecting the RequestFactory induces some coupling, but makes it clearer 
   the method will always *create* a new instance, whereas with a provider you 
   defer the scope handling to the injector)
   - IMO, you should only "push" from your presenter to the view, or from 
   the view to the presenter. Your views' render() methods "pull" from their 
   presenter. Also, these render() methods should rather live in the presenter 
   IMO, and/or possibly use the Editor framework (with the EditorDriver 
   "driven" by the presenter)
   - Your entities should IMO live in "server" (no "shared") and your 
   proxies in "shared" (not "client"): the server needs the proxies, and the 
   client doesn't need the entities (it only deals with the proxies)

-- 
You received this message because you are subscribed to the Google Groups 
"Google Web Toolkit" group.
To view this discussion on the web visit 
https://groups.google.com/d/msg/google-web-toolkit/-/s7R3nu-iQtoJ.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to 
[email protected].
For more options, visit this group at 
http://groups.google.com/group/google-web-toolkit?hl=en.

Reply via email to