[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
On Sat, May 19, 2012 at 6:16 PM, Thomas Broyer wrote: 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) Yay! Just stumbled upon it, so for reference: http://code.google.com/p/google-web-toolkit/issues/detail?id=6713#c8 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
On 2012/05/31 00:18:43, skybrian wrote:
I got this feedback from someone who requested a rollback but decided
to work
around it instead. I wonder how widespread this sort of thing is?
Our use of the editor/driver stuff assumed the editors would be
rebuilt when driver.edit(T) was invoked. This isn't true with this CL,
as the point of the CL was to try and avoid rebuilding the editors
when the same T is put into the same driver/editor framework.
Basically we used the editor/driver thing to show a read-only
version of T and then depended on the sub-editors to be recreated when
the user clicked our Edit button and we pushed the same model object
back into the same top level editor. Our work-around is to push a mock
empty object in first, and then push the real object again, forcing
the sub-editor list to be cleared and recreated.
If you ask me, they have a design issue.
Just like other editors that are not sub-editors of a ListEditor, the
ListEditor sub-editors will have their value updated by the edit() (this
is done by the chain.attach() from ListEditorWrapper#set(), called from
ListEditorWrapper#refresh()); so if the editor needs to be different
between read-only and edit modes (mode which is communicated to the
editors by out-of-band means), their sub-editors should update their
rendering when the mode changes, without even the need for a call to
edit() on the editor driver, or possibly only when their value is
updated.
In other words, it's a mistake to make editors behave differently
depending on out-of-band information IFF they don't update their
behavior themselves when that out-of-band information changes, or when
the value is updated (even if that means implementing ValueAwareEditor
only to be notified of the value changing).
As another workaround, if they're fine recreating their sub-editors,
they could wrap them in simple editors:
class Wrapper implements ValueAwareEditorFoo {
@Path() Real real;
@Override
public void setValue(Foo value) {
// unconditionally create a new Real editor:
Real oldReal = real;
real = new Real();
replaceOldEditorByNewEditorInParentWidget(oldReal, real);
}
// other ValueAwareEditor methods as no-ops.
}
and then use a ListEditorFoo, Wrapper instead of ListEditorFoo,
Real.
Of course, this means the Wrapper here knows how it'll be used (to
implement the replaceOldEditorByNewEditorInParentWidget method); in
other words, there's some cooperation between Wrapper and the
EditorSource (but given Wrapper instances are created by the
EditorSource, that shouldn't really be an issue).
https://gwt-code-reviews.appspot.com/1664803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
I got this feedback from someone who requested a rollback but decided to work around it instead. I wonder how widespread this sort of thing is? Our use of the editor/driver stuff assumed the editors would be rebuilt when driver.edit(T) was invoked. This isn't true with this CL, as the point of the CL was to try and avoid rebuilding the editors when the same T is put into the same driver/editor framework. Basically we used the editor/driver thing to show a read-only version of T and then depended on the sub-editors to be recreated when the user clicked our Edit button and we pushed the same model object back into the same top level editor. Our work-around is to push a mock empty object in first, and then push the real object again, forcing the sub-editor list to be cleared and recreated. https://gwt-code-reviews.appspot.com/1664803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
On 2012/05/25 22:53:18, tbroyer wrote: On 2012/05/25 17:58:21, skybrian wrote: In getEditors: The returned list will be live until the next call to setValue() and shouldn't be used after that. [...] 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). AFAICT, that's already an issue with other editors though: in the DevGuide for editors [1], if the address or manager is null, then you'll have an NPE at runtime. Hey, know what? I was wrong! If the 'address' is 'null', it'll set 'null' to the AddressEditor (if it's a ValueAwareEditor) and all of its sub-editors. So #1 (what we do here) is actually exactly what we *should* do! http://gwt-code-reviews.appspot.com/1664803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
On Fri, May 25, 2012 at 12:48 AM, Brian Slesinsky skybr...@google.com wrote: On Sat, May 19, 2012 at 9:16 AM, Thomas Broyer t.bro...@gmail.com wrote: 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. It does explain a lot of things but I'm missing the part where it explains the lifetime of the view returned by getEditors(). I'd expect that to be explained in the javadoc for getEditors itself. Right. We should add that. 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). Sure, that seems like a separate issue though. Here's my concern: if you call setValue() twice with the same list, this patch will make the live view returned by getEditors() last longer than before. So I think maybe we shouldn't reuse the ListEditorWrapper instance, just the Editors themselves. ListEditorWrapper looks cheap to construct, so we might as well be consistent and avoid changing behavior. On the other hand, maybe you're expecting that the caller *knows* that they're calling setValue() twice with the same List, so they expect the view to last longer as a result, sort of like if we were returning the same list again without wrapping it. If so, maybe this is a change in behavior to be closer to what the user expects? 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). 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
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
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
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
On 2012/05/25 17:58:21, skybrian wrote: In getEditors: The returned list will be live until the next call to setValue() and shouldn't be used after that. [...] 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). AFAICT, that's already an issue with other editors though: in the DevGuide for editors [1], if the address or manager is null, then you'll have an NPE at runtime. (note: not necessarily AutoBeans, any POJO can be edited with the Editor framework ;-) ) [1] https://developers.google.com/web-toolkit/doc/latest/DevGuideUiEditors#Quickstart 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. Done. https://gwt-code-reviews.appspot.com/1664803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
On Sat, May 19, 2012 at 9:16 AM, Thomas Broyer t.bro...@gmail.com wrote: 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. It does explain a lot of things but I'm missing the part where it explains the lifetime of the view returned by getEditors(). I'd expect that to be explained in the javadoc for getEditors itself. 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). Sure, that seems like a separate issue though. Here's my concern: if you call setValue() twice with the same list, this patch will make the live view returned by getEditors() last longer than before. So I think maybe we shouldn't reuse the ListEditorWrapper instance, just the Editors themselves. ListEditorWrapper looks cheap to construct, so we might as well be consistent and avoid changing behavior. On the other hand, maybe you're expecting that the caller *knows* that they're calling setValue() twice with the same List, so they expect the view to last longer as a result, sort of like if we were returning the same list again without wrapping it. If so, maybe this is a change in behavior to be closer to what the user expects? 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? 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. It looks like all changes get sent to the backing list via flush()? 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. Or maybe I missed something - pretty likely, actually. - Brian -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
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
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
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(). 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. 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. 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. (Do existing editors do anything here?) 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. 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. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
Also: are editors typically designed for reuse, or will we break people who have taken shortcuts? Maybe editors need to declare somehow that they can be reused? -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
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
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
Hi Brian, I just saw it was rolled back. Patchset #2 fixes the NPE (that was rather obvious actually). Should I also add a test? https://gwt-code-reviews.appspot.com/1664803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
Yes please. Sorry about missing this in the review! On May 17, 2012 8:23 AM, t.bro...@gmail.com wrote: Hi Brian, I just saw it was rolled back. Patchset #2 fixes the NPE (that was rather obvious actually). Should I also add a test? https://gwt-code-reviews.**appspot.com/1664803/https://gwt-code-reviews.appspot.com/1664803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
Done. I also fixed two inconsistencies wrt editing a 'null' value (I just discussed about it on the GWT users group: https://groups.google.com/d/topic/google-web-toolkit/xQA-TiQUbLA/discussion ); one of them is a breaking change, though I believe it won't hurt anyone (suppressed an exception case). Note that passing 'null' to driver.edit() does not behave the same (flush() will throw), but that's a different issue (and much less likely to happen). On Thu, May 17, 2012 at 6:59 PM, Brian Slesinsky skybr...@google.com wrote: Yes please. Sorry about missing this in the review! On May 17, 2012 8:23 AM, t.bro...@gmail.com wrote: Hi Brian, I just saw it was rolled back. Patchset #2 fixes the NPE (that was rather obvious actually). Should I also add a test? https://gwt-code-reviews.appspot.com/1664803/ -- Thomas Broyer /tɔ.ma.bʁwa.je/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
Oops, I had started an email about the revert but apparently never sent it. Sorry about that! 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. 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. But then again, that's a change in semantics and I know nothing about the editor framework. If you don't want to open this can of worms, I suggest reverting the changes to flush() and getEditors() and leaving it for another CL. https://gwt-code-reviews.appspot.com/1664803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
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. https://gwt-code-reviews.appspot.com/1664803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Issue 7038: CompositeEditor and ListEditor optimizations (issue1664803)
LGTM https://gwt-code-reviews.appspot.com/1664803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
