I'm thinking I need to clarify my concern to set it null. That means I'm worried about setting it null b/c it breaks the editor.
On 2012/05/18 01:34:06, Brandon wrote:
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
