Thanks for the detailed comments ... a few responses:
> - I would have passed the task's text to addTask rather than having a
> getTaskText() on the view (i.e. make the presenter manage the state, and
> the view only display it and route user events to the presenter)
I like this suggestion - will make this changed.
> - Instead of wrapping everything in a UiBinder, I would have kept part
> of the template in the HTML host page (I'd have wrapped only the <div
> id=todoapp>, keeping the static instructions and credits sections in the
> HTML host page)
I'm not so keen on this based on the goal of this project, to compare
and contrast it with various JavaScript frameworks. Having the markup
in a single place makes this comparison easier.
> - I would have called the ViewEventHandler interface 'Presenter' (as
> many people do it), and use a setter on the view (setPresenter) rather than
> an observer-like method (addViewEventHandler)
I'm not so keen on that naming, having an interface for the Presenter,
which is a restricted view onto the presenter for the purposes of
responding to 'events' from the view seems wrong to me.
> - I wonder if you could have used a CompositeCell to split the "done
> checkbox" and "delete button" handling in their own cells
I thought about that too, but one of the goals of the TodoMVC project
is to use the same HTML markup and CSS for all the implementations.
Mine already deviates due to CellList using divs rather than ul / li
(it's a great shame this cannot be configured), so I wanted to adhere
to the template as much as I could elsewhere.
> - I really don't like having that delete() method in the ToDoItem class,
> that's something that should go in the Presenter (you're doing MVP, not
> MVC). The setDone should IMO also go through the presenter as that one
> needs to be notified (the presenter would then call setDone on the model);
> there's no need to do it for setTitle(). The fact that the model knows the
> presenter is a sign that this is not actually a "model" but rather a "view
> model" (MVVM).
I see your point, this is a little more MVVM-like (considering I have
been working with Silverlight / WPF / Knockout for quite some time,
this is not surprising). I'll think on this some more.
> - I don't understand you 'data-timestamp' hack in the Cell; it looks to
> me like you should have used ViewData and rely on the existing
> AbstractEditableCell behavior, but there might be reasons you didn't do it,
> and/or I might be wrong.
I did try using AbstractEditableCell previously, but hit an issue
which I cannot recall. I'll revisit this.
> - I would have used AutoBeans for the JSON serialization, instead of
> JSONParser/JSONValue and the like (which are a PITA to use)
Good idea - I have been meaning to give AutoBeans a go.
> - I would have used Guava for the various string and list processing
Feels a bit like overkill for such a small sample application.
> - Hiding the footer should be controlled by the presenter, not hidden in
> the view, IMO (easier to unit-test if you ask me; btw, you didn't unit-test
> your presenter)
Personally I am not averse to placing a very small amount of
presentation-oriented logic into the view, specifically the sort of
logic which is so trivial that I would skip unit testing it.
And, yes, I haven't unit tested - this is in common with the other
TodoGWT projects, none have unit tests, where many are of course
perfectly testable.
Colin E.