On Friday, May 18, 2012 10:02:49 PM UTC+2, Brian Slesinsky wrote: > > On Thu, May 17, 2012 at 5:04 PM, <t.bro...@gmail.com> wrote: > > > 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. > > Okay, I did some reading and it seems that the normal lifecycle of an > edit begins with setValue() and ends with flush().
Well, you can call flush() as many times as you want, it doesn't "end" editing. (you can do that to use Bean Validation for instance: call flush() to put the fields' values "back" in the edited object, validate the object, if there are any ConstraintViolation, give them to the EditorDriver for display; also some editors –e.g. ValueBoxEditor– only report errors on flush, so you can only basically only check hasErrors after flush). > So I agree that is > seems logical that a "live" view would only last until the next > flush() or at most until the next setValue(). But this isn't > well-documented in this class or anywhere else that I've found so far. > To me, https://developers.google.com/web-toolkit/doc/latest/DevGuideUiEditors is clear enough. YMMV though. > So maybe we want to do that now? I also wonder if, for consistency, we > want to make sure the live view goes stale on *every* call to > setValue(), which would make our optimization a bit trickier; we > should probably create a new ListValueProvider and copy all the > subeditors into it. > ListEditorWrapper could track this in detach() and from then on act as an immutable list (throwing exceptions if you try to change it). > > 2. there would still need some special treatment in getList to return > > 'null' if 'null' was passed to setValue. > > Normally I like to detect null lists and throw an exception > immediately (it's usually a mistake) but in this case there may be > beans that have null lists, so we should probably not die. But I don't > think we need to preserve null lists or that editors need to display > null lists differently than empty lists, allow users to specify null > versus empty, and so on. We could basically merge OptionalFieldEditor functionality into ListEditor (instead of having to use both) but turning null into empty lists would break amny things (see below). I'm in favor of separation of concerns though. If you have an editor for a sub-object, then setting a 'null' will fail with an NPE when trying to pass values to sub-editors; I believe this is why OptionalFieldEditor was added in the first place (from memory, BobV added it after a discussion with Patrick Julien on the GWT Users group). 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? > (Do existing editors do anything here?) CheckBox turns 'null' into 'Boolean.FALSE' and TextBox turns 'null' into the empty string, and people have complained. See http://code.google.com/p/google-web-toolkit/issues/detail?id=5976#c1 (I'm sure this was discussed, with John or Ray R, and I started working on a patch; can't find this though) If > not, I think we might want to immediately convert nulls into empty > lists on input, so we don't blow up but you never get a null list back > again after an edit. This would be following Postel's law again - "be > liberal in what you accept and conservative in what you send". > It would however generate a "change" when using RequestFactory, because 'null' is different from an empty-list. > 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. > > Sure, makes sense. > > > flush() must not throw in that case, and getList should return 'null' > > (rather than, say an empty list, be it unmodifiable). > > Hmm, why? I think it might be okay to return an empty list. > Only if it was then flushed into the edited property (otherwise your changes to the empty list would be lost), changing it from 'null' to an empty list even if the user didn't do anything: edit(), flush(), and boom, your object has changed! 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) -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
