For me, I'm more concerned about setting it null. Not sure this would work, but if setValue(null) create an empty list to use and don't even worry about NPE checks. Shoot if set null and returns empty not such a bad side effect.
On 2012/05/18 00:04:26, tbroyer wrote:
On 2012/05/17 21:38:03, skybrian wrote: > Oops, I had started an email about the revert but apparently never
sent it.
> Sorry about that!
No problem!
> I might need some background about the editor framework... > >
https://gwt-code-reviews.appspot.com/1664803/diff/4002/user/src/com/google/gwt/editor/client/adapters/ListEditor.java
> File user/src/com/google/gwt/editor/client/adapters/ListEditor.java
(right):
> >
https://gwt-code-reviews.appspot.com/1664803/diff/4002/user/src/com/google/gwt/editor/client/adapters/ListEditor.java#newcode84
> user/src/com/google/gwt/editor/client/adapters/ListEditor.java:84:
return
> Collections.emptyList(); > If someone calls setList() then the list returned by this method is
no longer
> live; it will still be empty even though there are new items. Is
that what we
> want? It seems unlikely, but if so it should be documented.
setValue() is part of the ValueAwareEditor contract (inherited through CompositeEditor); it's called by the Editor framework when you EditorDriver#edit() some object (and, to be exact, also in subeditors
of
CompositeEditors), then flush() is called when you
EditorDriver#flush().
ListEditor#getList() can then be used to add/remove/reorder elements
of the
edited list, with direct effect on subeditors, which can be accessed
by
#getEditor(). As you can see, the list returned by either method is stale as soon as
#setValue
is called again. IMO, this is fine, because getList() should return
'null' if
setValue received a 'null' (more on that later); and it's not
illogical to
return a different list instance when the backing list changes.
For completeness: calling setValue yourself on any ValueAwareEditor
would in
many case have no effect, and at worse would simply confuse the editor
and/or
the Editor framework. In any case, it wouldn't change the value of the
edited
property after a flush(), because there's no getValue(). The only exception here is OptionalFieldEditor, because it's also a LeafValueEditor, and this behavior is thus on-purpose.
> It seems like a better idea to create the ListEditorWrapper
immediately in the
> constructor and make it final. When setValue() is called, we should
call a new
> method like ListEditorWrapper.replaceBacking(). Then live views
never expire
and > we don't have to worry about null checks in this class.
There are a few issues with this approach: 1. it would basically only move the null-checks to ListEditorWrapper. 2. there would still need some special treatment in getList to return
'null' if
'null' was passed to setValue.
Let's talk about that last case though: originally,
ListEditor#getValue would
throw an NPE if it received a 'null'. BobV said on the GWT group at
that time
that you should wrapp the ListEditor into an OptionalFieldEditor if
you could
have 'null's. A few weeks/months later, he added the null-check to
setValue, but
didn't change flush() (and to a lesser extent, getEditors). Today, if you want to be able to change the list field from 'null' no
non-null
or vice versa, you still need the OptionalFieldEditor.
If you asked me, I would have kept the original rule; at least the
contract was
clear. Now with the ListEditor accepting 'null's, IMO, flush() must
not throw in
that case, and getList should return 'null' (rather than, say an empty
list, be
it unmodifiable). getEditors() could return either 'null' or an empty
list, it
doesn't really matter given that a) the list is unmodifiable and b) if
the value
changes from 'null' to a non-null value, a new list would have been
returned
anyway (I mean, when change the backing list, a live-view retrieved
for the
first value is no longer valid after the call to setValue, the
behavior here
wouldn't change then). This is what patch set #3 does here.
http://gwt-code-reviews.appspot.com/1664803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
