On Tue, Oct 19, 2010 at 10:33 AM, Zach van Schouwen <[email protected]>wrote:
> On Tue, Oct 19, 2010 at 10:16 AM, <[email protected]> wrote: > >> interface FooView { >> interface Delegate { >> void saveClicked(); >> void cancelClicked(); >> } >> BarView getNestedBarView(); >> } >> > > Yeah, we've taken that approach on various occasions, but I don't think > that achieves the appropriate separation of views and controllers. In the > idealized MVC world, I don't want to have to know anything about how my > controller works -- extending even to its API. The Delegate method always > seemed like a kludge to me, particularly since our team injects both > views and controllers pretty aggressively -- meaning the view still needs to > know the full type of its controller in the constructor for us to @Inject it > -- or we have to manually write an @Provides method for every V/C pair with > event handlers. > > >> Back to the patch, if I read it right you're making the fields of a >> uibinder owner define its public API, which seems like a pretty bad >> thing to encourage. > > > I don't expose an API on the view -- the names of the @UiField-annotated > objects are the only thing exposed to the controller. As I understand it, > they already are exposed to allow UiBinder to reflect over the view class. > Typically they're exposed as package private, and so not visible to the controller. But yes, you really are exposing an API. Your controller has knowledge of every @UiField. Unless you're only making some of them public? Are your controllers always in the same package as their view? I'm not trying to prescribe how you set up your app, just trying to make sure the choices are conscious. > > To avoid leaking business logic into your views >> you're flooding your controller with knowledge of its view >> implementation. How do you test these things? How does the next guy know >> whether his event handler belongs in the view or the controller? > > > View event handlers are those that don't need to interact with the > controller (this checkbox triggers that checkbox, etc.). Controller handlers > are the methods that would be defined in the Delegate interface above. > > The testing protocol for these functions is the same as what we have now. > In the current scheme, you test the methods specified by Delegate on the > controller -- it would still be this way. They just aren't defined by an > interface anymore. > > I'm not sure what you mean when you say the controller is "flood[ed] with > knowledge of the view implementation" > > Having methods on the controller named saveClicked() and cancelClicked() is > not very different from having @UiHandler("save") -- you're encoding the > same information with different semantics -- except with my patch there's > less code and less duplication, since you can skip both the Delegate > interface and this inevitable method: > By having no Delegate interface you are drawing no line between the view's public and private concerns. Will you be able to provide a different view implementation? One that has no ui.xml file? One that isn't GWT dependent? For that matter, > > @UiHandler("save") > void handleSaveEvent(ClickEvent event) { > delegate.saveClicked() > } > > instead, you just add this to the controller: > > @UiHandler("save") > void saveClicked(ClickEvent event) { > /*... business logic ...*/ > } > > >> I agree that we need a more general low-labor way to bind events in a >> GWT app, but this doesn't seem to be it. Am I missing something? > > > The current approach produces a lot of boilerplate for us. Moreso, once the > Delegate is in the view, it's much more tempting for future maintainers to > let business logic seep into the view (we've seen this repeated over and > over). Once a few controller methods are exposed in an interface, the rest > often come tumbling in, despite our best efforts. > And my point is that you're going completely in the other direction, encouraging view concerns to tumble into the controller. Putting app design points aside, my concerns with the actual patch at hand are: - doesn't it require the existence of a ui.xml file? That's a bad requirement for a controller. - your controllers now rely upon a binding object generated by GWT. Didn't you just close the door on JRE tests for them? -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
