[gwt-contrib] Re: Add a "ControllerBinder" in UiBinder so that UiHandlers can be bound on both the (issue923801)
On Tue, Oct 19, 2010 at 10:33 AM, Zach van Schouwen wrote:
> On Tue, Oct 19, 2010 at 10:16 AM, 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
Re: [gwt-contrib] Re: Add a "ControllerBinder" in UiBinder so that UiHandlers can be bound on both the (issue923801)
> How do you test these things?
This "gwt-hack" example project is horribly unmaintained, but the idea is:
http://github.com/stephenh/gwt-hack/blob/master/src/test/java/com/bizo/gwthack/client/presenters/ClientPresenterTest.java
When GwtFooView is generated, so is StubFooView, which has fields like:
StubTextBox name;
public IsTextBox name() { return name; }
So the test can grab the StubTextBox and call methods on it, like
"click" or "type" or "focus", which fire dummy versions of ClickEvent,
ChangeEvent, etc. The presenter handles these just as it would native
events (well, as long as you're not using native event features), and
then the test can observe the side effects in the stub widgets. E.g.
that the textbox's getText changed, or had style=display:none set, or
what not.
Here is StubTextBoxBase, for example:
http://github.com/stephenh/gwt-mpv/blob/master/user/src/main/java/org/gwtmpv/widgets/StubTextBoxBase.java
I have admittedly added stub functionality in an ad hoc manner, so I
can't promise that it acts exactly like the browser does, but the idea is to
get close enough that it's good enough for unit testing.
> How does the next guy know whether his event handler belongs in the
> view or the controller?
Well, the view is generated, so any code he puts there will be
overwritten. :-)
> [snip] but clicking on the header of a disclosure panel does change
> the view state, so it belongs in the view.
Personally, I like putting everything in the presenter so it stays
testable.
> Don't forget, that by mapping each widget to an interface would allow
> you to bind those up with GIN and create providers for those inside
> the presenter, so if for some reasons you wanted to be able to alter
> the view, you could easily do it and still keep the presenter/activity
> JUnit clean.
Yep. :-) I don't use GIN, but I have a WidgetsProvider interface that
allows the presenter to create dynamically create "newTextBox" widgets
and have it be a stub/gwt version as appropriate.
http://github.com/stephenh/gwt-mpv/blob/master/user/src/main/java/org/gwtmpv/widgets/GwtWidgetsProvider.java
> A first awesome step to this would be to make every widget implement
> an interface exposing all their public methods. I really hated
> extending every control and it will become more and more of a
> nightmare to maintain as the gwt codebase changes.
Agreed. I've been doing this lazily, as my apps need things:
http://github.com/stephenh/gwt-mpv/tree/master/user/src/main/java/org/gwtmpv/widgets
> I'd like to look more at the ui:binder changes. My idea was to add an
> annotation/annoatation processor to automatically codegen a new
> interface of every public method. For example
:-)
I started out doing this:
http://github.com/stephenh/interfacegen
But then I wanted stubs too:
http://github.com/stephenh/stubgen
And finally I gave up on APT (for this problem), because I don't want to
have to code the SomeView class at all. All of the XxxView artifacts are
derivable from the `ui.xml` file, so why maintain any of it by hand?
Given that the APT API only supports Java classes, I had to fail over to
a standard Java program that parses all of the `ui.xml` files in a
project.
However, given custom builders in Eclipse (which I need to do a write up
of), I've found this approach to be just as nice (it runs automatically
on save) as APT.
> I would LOVE LOVE LOVE for every gwt object that extends UIObject to
> implement a specific interface and would be more than happy to
> implement this.
Me too. :-)
I'd love to have help maintaining what I've got started. Andrew from
gwt-pectin has expressed interest in them as well--either from within
gwtmpv or extracting them into a gwt-widget-interfaces project.
Obviously it would be best if they were in GWT proper, but I haven't
seen any indication that it is likely to happen.
- Stephen
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a "ControllerBinder" in UiBinder so that UiHandlers can be bound on both the (issue923801)
This is something I'm really interested in as well Stephen.
-
How do you test these things? How does the next guy know
whether his event handler belongs in the view or the controller?
-
If the event doesn't change the view state, it doesn't belong in the
view. I would argue that clicking a save button doesn't change the
view, so it shouldn't be added to the view, but clicking on the header
of a disclosure panel does change the view state, so it belongs in the
view.
As far as how you test them, by coding in the presenter specifically
against interfaces, you can use stubs to run your tests, still getting
all the JUnit testable goodness,
Don't forget, that by mapping each widget to an interface would allow
you to bind those up with GIN and create providers for those inside
the presenter, so if for some reasons you wanted to be able to alter
the view, you could easily do it and still keep the presenter/activity
JUnit clean.
A first awesome step to this would be to make every widget implement
an interface exposing all their public methods. I really hated
extending every control and it will become more and more of a
nightmare to maintain as the gwt codebase changes.
I'd like to look more at the ui:binder changes. My idea was to add an
annotation/annoatation processor to automatically codegen a new
interface of every public method. For example
@GenerateDisplayInterface
class SomeView implements SomeViewDisplay{
@UiField
TextBox box;
@UiField
Label label;
public SomeView(){.}
public IsTextBox getBox(){
return box;
}
@DoNotGenerate
public IsLabel getLabel(){
return label
}
}
would generate
public interface SomeViewDisplay {
IsTextBox getBox();
}
This would then avoid the problems inhernate with having the view be
completely code genned and would allow me to put view state change
handlers inside the view.
I would LOVE LOVE LOVE for every gwt object that extends UIObject to
implement a specific interface and would be more than happy to
implement this.
On Oct 19, 1:35 pm, Stephen Haberman
wrote:
> > 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?
>
> It's not necessarily related to events, but I've been pleased with the
> low-labor aspect of generating views:
>
> http://gwtmpv.org/viewgeneration.html
>
> My workflow:
>
> * In Foo.ui.xml, add a "ui:field=name" line
> * Eclipse project builder sees the change, automatically runs codegen
> * IsFooView is updated with "IsTextBox name()"
> * GwtFooView is updating with @UiField name/etc.
> * StubFooView is updated with "StubTextBox name()"
> * In FooPresenter, which extends Presenter, add:
> * view.name().addClickHandler(...)
>
> Given GwtFooView is entirely generated, I cannot use @UiHandler events,
> hence FooPresenter calling "addClickHandler".
>
> However, I don't find this to be a big deal, as I'm trending towards a
> gwt-pectin-style binding DSL:
>
> // in FooPresenter onBind
> binder.bind(model.nameProperty).to(view.nameTextBox)
>
> So still no inner-classes, and the model/view keep each other up to date
> when one or the other changes. Note that the model part is entirely
> optional, it's only required if you want the handy "binder.bind" syntax.
>
> - Stephen
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Add a "ControllerBinder" in UiBinder so that UiHandlers can be bound on both the (issue923801)
> 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? It's not necessarily related to events, but I've been pleased with the low-labor aspect of generating views: http://gwtmpv.org/viewgeneration.html My workflow: * In Foo.ui.xml, add a "ui:field=name" line * Eclipse project builder sees the change, automatically runs codegen * IsFooView is updated with "IsTextBox name()" * GwtFooView is updating with @UiField name/etc. * StubFooView is updated with "StubTextBox name()" * In FooPresenter, which extends Presenter, add: * view.name().addClickHandler(...) Given GwtFooView is entirely generated, I cannot use @UiHandler events, hence FooPresenter calling "addClickHandler". However, I don't find this to be a big deal, as I'm trending towards a gwt-pectin-style binding DSL: // in FooPresenter onBind binder.bind(model.nameProperty).to(view.nameTextBox) So still no inner-classes, and the model/view keep each other up to date when one or the other changes. Note that the model part is entirely optional, it's only required if you want the handy "binder.bind" syntax. - Stephen -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a "ControllerBinder" in UiBinder so that UiHandlers can be bound on both the (issue923801)
Putting the patch aside for a moment, I think the Wave team solved this
problem best: allow the view to have knowledge of its controller, but in
its own terms, via a delegate interface that the view defines. The
little bit of code you wind up with in most views becomes a semantic
passthrough to the delegate.
interface FooView {
interface Delegate {
void saveClicked();
void cancelClicked();
}
BarView getNestedBarView();
}
A click event on the Save button becomes a call to
delegate.saveClicked(), basically translating browser events to semantic
method calls. Dan Danilatos covered it in detail here:
http://code.google.com/events/io/2010/sessions/gwt-continuous-build-testing.html
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. 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?
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?
http://gwt-code-reviews.appspot.com/923801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a "ControllerBinder" in UiBinder so that UiHandlers can be bound on both the (issue923801)
For context, this is what Zach is implementing:
My team has been struggling a bit to find a way to handle
interactions between controllers and views smoothly. We've ended up
with a lot of code in one of two styles:
class FooController {
void onFirstRender {
view.addClickHandlerToSomeButton(new ClickHandler()
{ handleEvent(ClickEvent e) { /* ... */ } });
}
}
or
class FooView {
private FooController controller;
@UiHandler("fooElement")
void handleClick(ClickEvent e) {
controller.doSomething();
}
}
We do not love either of these styles -- the first one is very
cumbersome, since we have to add methods to the view, and the second
one is nasty because it makes it easy for business logic to leak into
the view. We really don't want our views to store references to their
controllers.
After hashing this out with a few devs, we came up with a compromise
that we like:
class FooController {
@UiHandler("someElementInView")
public void handleClick(ClickEvent e) {
doSomeControllerLogic();
}
}
class FooView {
@UiHandler("someOtherElement")
public void handleClick(ClickEvent e) {
doViewSpecificLogicOnly();
}
}
This requires a little bit of modification to UiBinder. I have a hacky
CL that allows this, by letting the view use a "ControlledUiBinder"
rather than a UiBinder that points the code generator to the
controller and adds hooks for those handlers, then putting a UiBinder
on the controller that places those handlers on the view's binder.
Before I turn this into a 'ready-for-prime-time' CL, does this sound
like a reasonable change? The existing UiBinder syntax remains the
same, but there are two new interfaces extending it that can be used
instead. And there's no more calls to setController(), circular
generics, or business logic in the view (all problems my team has
had).
I can go into more detail if people are interested. I just wanted to
float an idea balloon.
http://gwt-code-reviews.appspot.com/923801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a "ControllerBinder" in UiBinder so that UiHandlers can be bound on both the (issue923801)
It may be a day or two before I can take this review, Zach. Sorry for the delay. On Fri, Sep 24, 2010 at 5:49 PM, wrote: > Reviewers: rjrjr, > > Description: > Add a "ControllerBinder" in UiBinder so that UiHandlers can be bound on > both the > controller and the view. This allows views that are totally ignorant of > their > controllers. > > > Please review this at http://gwt-code-reviews.appspot.com/923801/show > > Affected files: > M user/src/com/google/gwt/uibinder/UiBinder.gwt.xml > A user/src/com/google/gwt/uibinder/client/ControllerBinder.java > A user/src/com/google/gwt/uibinder/rebind/AbstractBinderWriter.java > A user/src/com/google/gwt/uibinder/rebind/ControllerBinderGenerator.java > A user/src/com/google/gwt/uibinder/rebind/ControllerBinderWriter.java > M user/src/com/google/gwt/uibinder/rebind/HandlerEvaluator.java > M user/src/com/google/gwt/uibinder/rebind/UiBinderGenerator.java > M user/src/com/google/gwt/uibinder/rebind/UiBinderWriter.java > A user/src/com/google/gwt/uibinder/rebind/model/HandlerClass.java > M user/src/com/google/gwt/uibinder/rebind/model/OwnerClass.java > M > user/test/com/google/gwt/uibinder/elementparsers/ElementParserTester.java > M user/test/com/google/gwt/uibinder/elementparsers/MockUiBinderWriter.java > M user/test/com/google/gwt/uibinder/rebind/HandlerEvaluatorTest.java > > > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
