Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(281)

Issue 1664803: Issue 7038: CompositeEditor and ListEditor optimizations (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 1 month ago by tbroyer
Modified:
2 years, 8 months ago
CC:
google-web-toolkit-contributors_googlegroups.com
Base URL:
http://google-web-toolkit.googlecode.com/svn/trunk
Visibility:
Public.

Description

Issue 7038: CompositeEditor and ListEditor optimizations

Initializer and Refresher visitors cause CompositeEditors' sub-editors
to be visited twice: once by the EditorChain#attach() they should be
calling in their setValue, and then once again by the visitor stepping
into sub-editors.
When a CompositeEditor has a ListEditor as a sub-editor, that ListEditor
then would create a ListEditorWrapper twice in a row, leading in creating
sub-editors to dispose them right away, which is obviously bad for
performance.
Having the Initializer and Refresher avoid visiting CompositeEditors'
sub-editors (because setValue would have already done so) fixes the issue.

Additionally, ListEditor unconditionally recreate its ListEditorWrapper
in setValue even when setting the same value as the one being edited. This
leads to recreating all sub-editors instead of reusing them (even though
the EditorSource should take care of this already for best performance in
all cases).

Patch Set 1

Patch Set 2 : Fix NPE

Patch Set 3 : Add test + fix inconsistencies

Total comments: 1

Patch Set 4 : JavaDoc re. null values (no backing list)

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M user/src/com/google/gwt/editor/client/adapters/ListEditor.java View 1 2 3 4 chunks +36 lines, -8 lines 0 comments Download
M user/src/com/google/gwt/editor/client/adapters/ListEditorWrapper.java View 1 chunk +37 lines, -0 lines 0 comments Download
M user/src/com/google/gwt/editor/client/impl/Refresher.java View 1 chunk +4 lines, -1 line 0 comments Download
M user/test/com/google/gwt/editor/client/SimpleBeanEditorTest.java View 1 2 4 chunks +51 lines, -11 lines 0 comments Download

Messages

Total messages: 20
tbroyer
5 years, 1 month ago #1
skybrian
LGTM
4 years, 11 months ago #2
tbroyer
Hi Brian, I just saw it was rolled back. Patchset #2 fixes the NPE (that ...
4 years, 11 months ago #3
skybrian
Yes please. Sorry about missing this in the review! On May 17, 2012 8:23 AM, ...
4 years, 11 months ago #4
tbroyer
Done. I also fixed two inconsistencies wrt editing a 'null' value (I just discussed about ...
4 years, 11 months ago #5
skybrian
Oops, I had started an email about the revert but apparently never sent it. Sorry ...
4 years, 11 months ago #6
tbroyer
On 2012/05/17 21:38:03, skybrian wrote: > Oops, I had started an email about the revert ...
4 years, 11 months ago #7
Brandon
For me, I'm more concerned about setting it null. Not sure this would work, but ...
4 years, 11 months ago #8
Brandon
I'm thinking I need to clarify my concern to set it null. That means I'm ...
4 years, 11 months ago #9
skybrian
On Thu, May 17, 2012 at 5:04 PM, <t.broyer@gmail.com> wrote: > setValue() is part of ...
4 years, 11 months ago #10
skybrian
Also: are editors typically designed for reuse, or will we break people who have taken ...
4 years, 11 months ago #11
skybrian
On Sat, May 19, 2012 at 9:16 AM, Thomas Broyer <t.broyer@gmail.com> wrote: > >> So ...
4 years, 11 months ago #12
tbroyer
On Fri, May 25, 2012 at 12:48 AM, Brian Slesinsky <skybrian@google.com> wrote: > On Sat, ...
4 years, 11 months ago #13
skybrian
On Fri, May 25, 2012 at 1:43 AM, Thomas Broyer <t.broyer@gmail.com> wrote: > I think ...
4 years, 11 months ago #14
tbroyer
On 2012/05/25 17:58:21, skybrian wrote: > > In getEditors: "The returned > list will be ...
4 years, 11 months ago #15
tbroyer
On 2012/05/25 22:53:18, tbroyer wrote: > On 2012/05/25 17:58:21, skybrian wrote: > > > > ...
4 years, 11 months ago #16
skybrian
I got this feedback from someone who requested a rollback but decided to work around ...
4 years, 10 months ago #17
tbroyer
On 2012/05/31 00:18:43, skybrian wrote: > I got this feedback from someone who requested a ...
4 years, 10 months ago #18
tbroyer
On Sat, May 19, 2012 at 6:16 PM, Thomas Broyer wrote: > > CheckBox turns ...
4 years, 10 months ago #19
ryphoenix
2 years, 8 months ago #20
Not sure if anyone still gets reply from this but I have a scenario that this
patch doesn't address.

I have a DialogBox implements Editor<Parent> which contains a
Editor<List<Child>> that uses ListEditor with ability to add and remove Child. 
Problem comes when user edits Parent and remove some but not all Child, but
clicks on cancel.  User immediately brings up same dialog for Parent, but now
backing and workingCopy are out of sync.

I found it interesting that the javadoc for ListEditorWrapper.refresh() mentions
in case backing list has been modified...but what about when workingCopy was
modified then canceled? 

There are ways to get around this, like calling driver.edit(Parent) or
childEditor.setValue(null) onCancel to force full on reset, but I think
refresh() can be made smarter to account for workingCopy changes.

Hope my description made sense, if not I can elaborate.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld revision f51cb906c4ad+