[gwt-contrib] Re: Fix issue 6959. (issue1587803)
http://gwt-code-reviews.appspot.com/1587803/diff/5002/user/src/com/google/gwt/editor/client/adapters/EditorSource.java File user/src/com/google/gwt/editor/client/adapters/EditorSource.java (right): http://gwt-code-reviews.appspot.com/1587803/diff/5002/user/src/com/google/gwt/editor/client/adapters/EditorSource.java#newcode60 user/src/com/google/gwt/editor/client/adapters/EditorSource.java:60: * For backwards compatibility with GWT 2.5.0 and earlier, the default implemtation calls On 2013/01/15 02:55:22, skybrian wrote: sp: implementation Done. http://gwt-code-reviews.appspot.com/1587803/diff/5002/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java File user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java (right): http://gwt-code-reviews.appspot.com/1587803/diff/5002/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java#newcode44 user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java:44: return new IndexedEditorT(-1, null); On 2013/01/15 02:55:22, skybrian wrote: If IndexedEditor's index is -1 then getValue() always returns null and setValue() probably throws an exception eventually in data.setRowData (I didn't trace it). It seems like it would be clearer to return an anonymous subclass of LeafValueEditor that implements getValue() and setValue() to do this directly? I'm assuming setValue() shouldn't be called at all, not sure about getValue. I was assuming getValue/setValue wouldn't be called, but user code can actually call them (from the EditorVisitor passed to EditorContext#traverseSyntheticCompositeEditor). I can't see any use case for that, but still. So I fixed IndexedEditor to no longer throw if data is null. Then dispose() can do an instanceof check and ignore this instance. Or perhaps keep it in a constant and use ==. Actually, dispose() won't be called; that was a leftover from a previous iteration where I only moved the create(0) to createEditorForTraversal; I then moved both create(0) and dispose() so, in the case of HasDataEditor, dispose is no longer called for the synthetic editor. http://gwt-code-reviews.appspot.com/1587803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix issue 6959. (issue1587803)
Seems fine. (I'll take care of these nits.)
http://gwt-code-reviews.appspot.com/1587803/diff/10006/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java
File user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java
(right):
http://gwt-code-reviews.appspot.com/1587803/diff/10006/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java#newcode71
user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java:71:
public Q getValue() {
@Override?
http://gwt-code-reviews.appspot.com/1587803/diff/10006/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java#newcode75
user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java:75:
public void setIndex(int index) {
Doesn't need to be public since we only call it from
HasDataEditorSource.setIndex.
http://gwt-code-reviews.appspot.com/1587803/diff/10006/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java#newcode81
user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java:81:
public void setValue(Q value) {
@Override?
http://gwt-code-reviews.appspot.com/1587803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix issue 6959. (issue1587803)
On 2013/01/13 19:28:16, tbroyer wrote: Ping; would be great to have it in 2.5.1! (IIRC, we initially talked about including it in 2.5.0 and finally decided to postpone it to the next release) This looks like a breaking change but it is hard for me to guess how likely it is going to break things. What do you think? http://gwt-code-reviews.appspot.com/1587803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix issue 6959. (issue1587803)
I'm not all that familiar with the editor framework but perhaps there's
a cleaner way to do this? (See comments.)
http://gwt-code-reviews.appspot.com/1587803/diff/1/user/src/com/google/gwt/editor/client/adapters/EditorSource.java
File user/src/com/google/gwt/editor/client/adapters/EditorSource.java
(right):
http://gwt-code-reviews.appspot.com/1587803/diff/1/user/src/com/google/gwt/editor/client/adapters/EditorSource.java#newcode42
user/src/com/google/gwt/editor/client/adapters/EditorSource.java:42:
public abstract E create(int index);
Instead of using -1 to indicate a synthetic editor, it seems cleaner to
add a new createEditorForTraversal() method. The default implementation
could just call create(0) for backward compatibility, but HasDataEditor
could implement it to either set a flag on IndexedEditor or alternately
use a different subclass altogether.
http://gwt-code-reviews.appspot.com/1587803/diff/1/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java
File user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java
(right):
http://gwt-code-reviews.appspot.com/1587803/diff/1/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java#newcode56
user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java:56:
((IndexedEditorT) editor).setIndex(index);
assert index =0?
http://gwt-code-reviews.appspot.com/1587803/diff/1/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java#newcode60
user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java:60:
static class IndexedEditorQ implements LeafValueEditorQ {
Perhaps IndexedEditor should be private? I see no usages other than in
this class.
http://gwt-code-reviews.appspot.com/1587803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix issue 6959. (issue1587803)
http://gwt-code-reviews.appspot.com/1587803/diff/1/user/src/com/google/gwt/editor/client/adapters/EditorSource.java File user/src/com/google/gwt/editor/client/adapters/EditorSource.java (right): http://gwt-code-reviews.appspot.com/1587803/diff/1/user/src/com/google/gwt/editor/client/adapters/EditorSource.java#newcode42 user/src/com/google/gwt/editor/client/adapters/EditorSource.java:42: public abstract E create(int index); On 2013/01/14 21:16:49, skybrian wrote: Instead of using -1 to indicate a synthetic editor, it seems cleaner to add a new createEditorForTraversal() method. The default implementation could just call create(0) for backward compatibility, but HasDataEditor could implement it to either set a flag on IndexedEditor or alternately use a different subclass altogether. Indeed! Why didn't I think of that?! Thanks Brian, will update the patch soon. http://gwt-code-reviews.appspot.com/1587803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix issue 6959. (issue1587803)
http://gwt-code-reviews.appspot.com/1587803/diff/1/user/src/com/google/gwt/editor/client/adapters/EditorSource.java
File user/src/com/google/gwt/editor/client/adapters/EditorSource.java
(right):
http://gwt-code-reviews.appspot.com/1587803/diff/1/user/src/com/google/gwt/editor/client/adapters/EditorSource.java#newcode42
user/src/com/google/gwt/editor/client/adapters/EditorSource.java:42:
public abstract E create(int index);
On 2013/01/14 21:16:49, skybrian wrote:
Instead of using -1 to indicate a synthetic editor, it seems cleaner
to add a
new createEditorForTraversal() method. The default implementation
could just
call create(0) for backward compatibility, but HasDataEditor could
implement it
to either set a flag on IndexedEditor or alternately use a different
subclass
altogether.
Done.
http://gwt-code-reviews.appspot.com/1587803/diff/1/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java
File user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java
(right):
http://gwt-code-reviews.appspot.com/1587803/diff/1/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java#newcode56
user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java:56:
((IndexedEditorT) editor).setIndex(index);
On 2013/01/14 21:16:49, skybrian wrote:
assert index =0?
Done.
http://gwt-code-reviews.appspot.com/1587803/diff/1/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java#newcode60
user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java:60:
static class IndexedEditorQ implements LeafValueEditorQ {
On 2013/01/14 21:16:49, skybrian wrote:
Perhaps IndexedEditor should be private? I see no usages other than in
this
class.
Done.
http://gwt-code-reviews.appspot.com/1587803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix issue 6959. (issue1587803)
http://gwt-code-reviews.appspot.com/1587803/diff/5002/user/src/com/google/gwt/editor/client/adapters/EditorSource.java File user/src/com/google/gwt/editor/client/adapters/EditorSource.java (right): http://gwt-code-reviews.appspot.com/1587803/diff/5002/user/src/com/google/gwt/editor/client/adapters/EditorSource.java#newcode60 user/src/com/google/gwt/editor/client/adapters/EditorSource.java:60: * For backwards compatibility with GWT 2.5.0 and earlier, the default implemtation calls sp: implementation http://gwt-code-reviews.appspot.com/1587803/diff/5002/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java File user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java (right): http://gwt-code-reviews.appspot.com/1587803/diff/5002/user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java#newcode44 user/src/com/google/gwt/editor/client/adapters/HasDataEditor.java:44: return new IndexedEditorT(-1, null); If IndexedEditor's index is -1 then getValue() always returns null and setValue() probably throws an exception eventually in data.setRowData (I didn't trace it). It seems like it would be clearer to return an anonymous subclass of LeafValueEditor that implements getValue() and setValue() to do this directly? I'm assuming setValue() shouldn't be called at all, not sure about getValue. Then dispose() can do an instanceof check and ignore this instance. Or perhaps keep it in a constant and use ==. http://gwt-code-reviews.appspot.com/1587803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix issue 6959. (issue1587803)
Ping; would be great to have it in 2.5.1! (IIRC, we initially talked about including it in 2.5.0 and finally decided to postpone it to the next release) http://gwt-code-reviews.appspot.com/1587803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix issue 6959. (issue1587803)
Ping http://gwt-code-reviews.appspot.com/1587803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
