On Fri, May 25, 2012 at 1:43 AM, Thomas Broyer <t.bro...@gmail.com> wrote:
> I think the expected use of getEditors() is that users don't keep an
> handle on it.
> Even if they, if they call getEditors() after each setValue(), then it
> doesn't really matter whether the returned value will be the same
> instance or a different one.
> How about documenting that? (users shouldn't hold a reference on the
> returned list, or if they do, they should "refresh" it after any call
> to setValue(); the returned list only guaranteed to be the same
> between calls to setValue, and shouldn't be used after a call to
> setValue).
Yes, that seems fine as a specification. In getEditors: "The returned
list will be live until the next call to setValue() and shouldn't be
used after that."
>
>>> If you ask me, for consistency, I'd
>>> remove the null-check in ListEditor and let it fail for 'null' values, just
>>> like it did originally. It might have been added by mistake, I don't know,
>>> it was added as part of a bigger change and wasn't tracked/documented at
>>> all, so who knows?
>>
>> I'm not sure what you mean. Do you mean that setValue() should just
>> throw an exception for null lists? Or does it blow up later? Or do we
>> want to avoid blowing up?
>
> Throw on 'null' (it will actually blow up with an NPE in
> ListEditorWrapper's constructor, when retrieving its size).
> I would just add an 'assert' in setValue as a hint for developers, and
> let it blow in ListEditorWrapper in prod mode.
>
>>> That's the kind of behavior I'd rather fight against: getValue just after
>>> setValue should try to preserve the value (null vs. empty string)
>>
>> Yes, good point. Except that in this case, there is no getValue()
>> method, since this isn't a LeafValueEditor and doesn't implement
>> HasValue.
>
> Yes, that's what I meant by "Only if it was then flushed into the
> edited property (otherwise your changes to the empty list would be
> lost)" and "We could basically merge OptionalFieldEditor functionality
> into ListEditor": ListEditor would have to be made a LeafValueEditor
> if we wanted to turn nulls into empty lists.
>
>> It looks like all changes get sent to the backing list via flush()?
>
> Yes
>
> This is the general contract of the Editor framework: do not change
> the edited object "in real-time", instead queue changes and only apply
> them on flush(); similarly, LeafValueEditors are not supposed to
> modify the object in-place, but return the "new value" from
> getValue(), otherwise they should be ValueAwareEditors with no
> sub-editor.
> Maybe it should be made clearer in the doc?
>
>> If we call setValue() with a null list, I don't think flush() can do
>> anything, because we don't have any reference to a backing model to
>> update! Therefore, any changes made in the ListEditor cannot be saved?
>> You'd have to use an OptionalFieldEditor if you want to save it.
>>
>> So it seems like at most we can display a read-only, empty list.
>> There's no point in letting the user edit something they can't save.
>
> Exactly. Unless we change ListEditor to be a LeafValueEditor, which
> would be equivalent to "merging OptionalFieldEditor functionality into
> ListEditor".
>
>
> To sum up: ListEditor is currently able to display 'null' lists as if
> they were empty lists, but then you cannot edit the list's content (as
> there's actually no list), and more importantly getEditors() will
> throw a misleading exception, and flush() will throw (so ListEditor
> can really only be used with 'null' lists in read-only use cases,
> where you call edit() on the EditorDriver without ever calling
> flush()).
> This is something we should fix IMO. There are three solutions:
> 1. make getEditors() return an empty list instead of throwing and
> avoid the NPE in flush(). Breaking change: getEditors() no longer
> throw if you call it before calling setValue() for the first time.
> 2. make setValue throw on a 'null' value, mandating the use of an
> OptionalFieldEditor if the list can be 'null'. This is consistent with
> all other editors (most importantly simple Editor-s with sub-editors,
> e.g. all the examples from the doc except LabelDecorator), with the
> exception of LeafValueEditors which are expected to accept nulls (at
> the editor's discretion actually, but all the LeafValueEditors
> provided in gwt-user.jar accept nulls). Breaking change: well, anyone
> using a ListEditor in a read-only scenario where 'null' values are
> expected would have to update his code to add an OptionalFieldEditor
> in from of the ListEditor.
> 3. "merging OptionalFieldEditor functionality into ListEditor",
> turning a 'null' value into an empty list so it can be edited. But
> then one could no longer tell the difference between 'null' and an
> empty list, and we'd have to add code so that getValue returns 'null'
> if 'null' was passed to setValue and getList() is empty (but never
> return 'null' if a non-null value was passed to setValue!).
>
> Patch Set #3 implements #1 above, as it's less likely to break
> anyone's code (quite the contrary), at the expense of being
> inconsistent with most other editors.
> Because ListEditor is the only CompositeEditor provided by GWT
> (OptionalFieldEditor is different, as it's also a LeafValueEditor), we
> could just say that CompositeEditors are supposed to accept 'null's
> without throwing.
>
> My preference would however go to #2 (it if weren't such a breaking change).
Yeah, #3 is out. Option #2 would make sense if we expected that
complaining early means that most people would fix the problem before
it reaches production. I'm not convinced of that; I think people won't
test the corner cases that result in null list fields in their
AutoBeans (particularly if those corner cases come from the server).
So I think #1 might actually be right; accept that the list might be
null and don't break, but don't let the user edit the field either.
Also, I see that having a null is actually the initial state of the
ListEditor so it seems okay to go back to that state.
It seems okay for getEditors to return an empty list when we have no
backing list ("yes, we have no editors").
If that seems okay, here's the javadoc to fix so we don't have to have
this discussion again:
- ListEditor constructor: "The ListEditor will have no backing list
until setValue() is called with a non-null value."
- setValue(): "If a null is passed in, the ListEditor will have no
backing list and edits cannot be made."
- getEditors(): "If there is no backing list, an empty list will be returned."
- getList(): "Returns null if there is no backing list and edits
cannot be made."
- Brian
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
