date:20111220


http://gwt-code-reviews.appspot.com/1619803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add ProvidesKey support to ValuePicker (issue1614807)



http://gwt-code-reviews.appspot.com/1614807/diff/6001/user/src/com/google/gwt/user/client/ui/ValuePicker.java
File user/src/com/google/gwt/user/client/ui/ValuePicker.java (right):

http://gwt-code-reviews.appspot.com/1614807/diff/6001/user/src/com/google/gwt/user/client/ui/ValuePicker.java#newcode116
user/src/com/google/gwt/user/client/ui/ValuePicker.java:116: if
((current == value) || (current != null && current.equals(value))) {

Hmm, there might be a need to use the key provider here instead of

equals()

If I fix this to use the keys, then the current test actually fails
because:

setValue(Foo@123[value=able])
setValue(Foo@456[value=able])

Does not accept the new Foo@456 value, but keeps the existing Foo@123
value, as it considers them equal, and so doesn't fire a change.

I suppose this makes sense? I ask only because I copy/pasted the
ValuePickerTest assertions that expect the Foo@456 set to change the
value from ValueListBoxTest.

And ValueListBox#setValue has a similar value-not-key check; do you
think that's a bug?

http://gwt-code-reviews.appspot.com/1614807/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Properly encode request parameters that use Collections when in JSON-RPC mode. (issue1618806)

2011-12-20 Thread Brian Slesinsky

On Tue, Dec 20, 2011 at 2:36 PM,   wrote:

> On second thought, even though List and Set are the two that are
> technically allowed, there's really no problem with JSON-RPC and the
> encoding of a Collection, as its done in the exact same way regardless
> of the collection; I don't think it hurts us to be a bit loose  here.
> What do you think?

I'm leaning towards being more restrictive until someone asks for it,
since it's easier to relax a restriction than to tighten it up. Most
of the time people will be writing AutoBeans interfaces from scratch
and they should probably stick to using maps and lists since that's
what JSON and JavaScript support directly, so it will be the most
efficient in GWT.

But I haven't figured out this code and this isn't a public method. Do
we check for this somewhere else? Maybe some unit tests would make it
more clear.

- Brian

-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Properly encode request parameters that use Collections when in JSON-RPC mode. (issue1618806)

On 2011/12/20 22:31:52, rdayal wrote:

On 2011/12/20 20:21:45, skybrian wrote:
>

http://gwt-code-reviews.appspot.com/1618806/diff/1003/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java

> File
>

user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java

> (right):
>
>

http://gwt-code-reviews.appspot.com/1618806/diff/1003/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode262

>

user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:262:

> } else if (obj instanceof Collection) {
> This seems too broad if we're only going to allow List and Set?

Yes, you're right; I imagine that the encoding of a Map wouldn't be

handled

correctly, even though this is a collection. I'll tighten this up.

Actually wait, that's totally wrong - Map isn't a Collection :).

On second thought, even though List and Set are the two that are
technically allowed, there's really no problem with JSON-RPC and the
encoding of a Collection, as its done in the exact same way regardless
of the collection; I don't think it hurts us to be a bit loose  here.
What do you think?

http://gwt-code-reviews.appspot.com/1618806/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Properly encode request parameters that use Collections when in JSON-RPC mode. (issue1618806)


On 2011/12/20 20:21:45, skybrian wrote:

http://gwt-code-reviews.appspot.com/1618806/diff/1003/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java

File


user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java

(right):



http://gwt-code-reviews.appspot.com/1618806/diff/1003/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode262

user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:262:

} else if (obj instanceof Collection) {
This seems too broad if we're only going to allow List and Set?


Yes, you're right; I imagine that the encoding of a Map wouldn't be
handled correctly, even though this is a collection. I'll tighten this
up.

http://gwt-code-reviews.appspot.com/1618806/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] 2.4 Uibinder code inefficiency

2011-12-20 Thread Paul Stockley

I use a lot of HTML panels so I can use native html for as much layout and 
styling as possible for performance reasons. I try and minimize the number 
of widgets I use which is especially helpful on mobile devices. As a 
result, I use quite a lot of css class references to either local uibinder 
styles or styles defined in a global application css resource. The problem 
is that it results in JS code such as the following:

f_HTMLPanel1 = new HTMLPanel_0((sb_2 = new StringBuilder_0 , 
sb_2.impl.string += "<\/span>  

-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Properly encode request parameters that use Collections when in JSON-RPC mode. (issue1618806)

2011-12-20 Thread skybrian



http://gwt-code-reviews.appspot.com/1618806/diff/1003/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
(right):

http://gwt-code-reviews.appspot.com/1618806/diff/1003/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode262
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:262:
} else if (obj instanceof Collection) {
This seems too broad if we're only going to allow List and Set?

http://gwt-code-reviews.appspot.com/1618806/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Adding an option to change the debug id property and prefix. Some users prefer to use a random a... (issue1618804)

2011-12-20 Thread unnurg


On 2011/12/16 17:38:57, jlabanca wrote:

LGTM

http://gwt-code-reviews.appspot.com/1618804/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add ProvidesKey support to ValuePicker (issue1614807)


Not sure about the test details (documenting the differences with
ValueListBox which are another issue –that might need to be addressed
too, but independently as this one) but overall LGTM.
Be sure to see my comment on getValue() though.


http://gwt-code-reviews.appspot.com/1614807/diff/6001/user/src/com/google/gwt/user/client/ui/ValuePicker.java
File user/src/com/google/gwt/user/client/ui/ValuePicker.java (right):

http://gwt-code-reviews.appspot.com/1614807/diff/6001/user/src/com/google/gwt/user/client/ui/ValuePicker.java#newcode116
user/src/com/google/gwt/user/client/ui/ValuePicker.java:116: if
((current == value) || (current != null && current.equals(value))) {
Hmm, there might be a need to use the key provider here instead of
equals() (which kind-of assumes a SimpleKeyProvider).

We're actually mimicking the SingleSelectionModel, with the exception of
the deferred SelectionChangeEvent.

Note that we could also probably get rid of the 'value' field and use
smodel.getSelectedObject(). But I don't think this change is worth the
possible bugs it could also introduce ;-)

http://gwt-code-reviews.appspot.com/1614807/diff/6001/user/test/com/google/gwt/user/client/ui/ValuePickerTest.java
File user/test/com/google/gwt/user/client/ui/ValuePickerTest.java
(right):

http://gwt-code-reviews.appspot.com/1614807/diff/6001/user/test/com/google/gwt/user/client/ui/ValuePickerTest.java#newcode57
user/test/com/google/gwt/user/client/ui/ValuePickerTest.java:57: return
item.value;
Handle null value?

http://gwt-code-reviews.appspot.com/1614807/diff/6001/user/test/com/google/gwt/user/client/ui/ValuePickerTest.java#newcode173
user/test/com/google/gwt/user/client/ui/ValuePickerTest.java:173:
assertEquals(keyProvider.getKey(value),
keyProvider.getKey(subject.getValue()));
There shouldn't be a need for the KeyProvider here: why not simply
assertEquals(value, subject.getValue())?
(or if you use the key provider, use it for the
smodel.getSelectedObject() too)

http://gwt-code-reviews.appspot.com/1614807/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add ProvidesKey support to ValuePicker (issue1614807)


http://gwt-code-reviews.appspot.com/1614807/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Properly encode request parameters that use Collections when in JSON-RPC mode. (issue1618806)


LGTM


http://gwt-code-reviews.appspot.com/1618806/diff/2001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
(right):

http://gwt-code-reviews.appspot.com/1618806/diff/2001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode274
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:274:
sb.append(nonCollectionEncode(it.next()));
On 2011/12/20 15:55:56, rdayal wrote:

On 2011/12/20 10:51:18, tbroyer wrote:
> Using encode() here would pave the way for supporting collections of
collections
> (see issue 5974).
> You'd then turn the RuntimeException in nonCollectionEncode into a

simple

> 'assert'.
> I mean, in the case of JsonRpc, GWT doesn't provide the backend, so

there's

> theoretically no reason to disallow collections of collections,

except for

> parity with the "standard" RF protocol.
Agreed, but I'd rather put a TODO here and then fix 5974 on both sides

- is that

ok with you?


Yep.

http://gwt-code-reviews.appspot.com/1618806/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add ProvidesKey support to ValuePicker (issue1614807)



http://gwt-code-reviews.appspot.com/1614807/diff/1/user/src/com/google/gwt/user/client/ui/ValuePicker.java
File user/src/com/google/gwt/user/client/ui/ValuePicker.java (right):

http://gwt-code-reviews.appspot.com/1614807/diff/1/user/src/com/google/gwt/user/client/ui/ValuePicker.java#newcode50
user/src/com/google/gwt/user/client/ui/ValuePicker.java:50: public
ValuePicker(CellList cellList, ProvidesKey keyProvider) {

Shouldn't the fix rather be to ask the CellList for its key provider?


Well, that seems logical; note that previously this cstr was already
implicitly creating a separate SingleSelectionModel in the smodel
field's initializer.

So...perhaps we could use the CellList's selection model if it was an
instance of SingleSelectionModel?

But otherwise, yes, I've changed this cstr to not take a ProvidesKey but
to ask the CellList for it.

http://gwt-code-reviews.appspot.com/1614807/diff/1/user/src/com/google/gwt/user/client/ui/ValuePicker.java#newcode70
user/src/com/google/gwt/user/client/ui/ValuePicker.java:70: public
ValuePicker(Renderer renderer, ProvidesKey keyProvider) {

I suppose keyProvider was meant to be passed to the CellList ctor?


Yes, good point.

http://gwt-code-reviews.appspot.com/1614807/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add ProvidesKey support to ValuePicker (issue1614807)


I'm not keen on the proposed changes to setAcceptableValues and

setValue.

Okay, I'll back off of that then; I was thinking, albeit naively, that
behaving as much as like ValueListBox as possible would be a good thing.
But good point on the paging.

Thanks for the review!


http://gwt-code-reviews.appspot.com/1614807/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Properly encode request parameters that use Collections when in JSON-RPC mode. (issue1618806)


(to be clear, the patch with test will be part of a separate issue).

http://gwt-code-reviews.appspot.com/1618806/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Properly encode request parameters that use Collections when in JSON-RPC mode. (issue1618806)

Thanks very much for the review! Your point is taken on tests; I'm
currently in the process of writing some rudimentary tests for JSON-RPC
in RequestFactory.

As you mention, these tests will not be integration-level tests at
first, as there is no RF server side in the testing infrastructure (as
yet), but this will be a start.

Expect another patch with tests shortly.

http://gwt-code-reviews.appspot.com/1618806/diff/2001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
File
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java
(right):

http://gwt-code-reviews.appspot.com/1618806/diff/2001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode269
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:269:
assert (obj instanceof Collection);
On 2011/12/20 10:51:18, tbroyer wrote:

How about casting 'obj' to a Collection at the call site instead? (and

thus

declare the argument here of type Collection)

Good idea, done.

http://gwt-code-reviews.appspot.com/1618806/diff/2001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode274
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:274:
sb.append(nonCollectionEncode(it.next()));
On 2011/12/20 10:51:18, tbroyer wrote:

Using encode() here would pave the way for supporting collections of

collections

(see issue 5974).
You'd then turn the RuntimeException in nonCollectionEncode into a

simple

'assert'.
I mean, in the case of JsonRpc, GWT doesn't provide the backend, so

there's

theoretically no reason to disallow collections of collections, except

for

parity with the "standard" RF protocol.

Agreed, but I'd rather put a TODO here and then fix 5974 on both sides -
is that ok with you?

http://gwt-code-reviews.appspot.com/1618806/diff/2001/user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java#newcode291
user/src/com/google/web/bindery/requestfactory/shared/impl/AbstractRequestContext.java:291:
if (obj.getClass().isEnum() && getAutoBeanFactory() instanceof EnumMap)
{
On 2011/12/20 10:51:18, tbroyer wrote:

Hmm, looks like there's the same issue as was fixed some time ago in

ValueCodex

re. enum values with their own "body", or when the enum has a

constructor with

arguments: in this case, their class is an anonymous subclass of the

enum class,

one for which isEnum() returns false.
Using "obj instanceof Enum" instead of obj.getClass().isEnum() would

fix it.

See issue 6504.

Good catch. Fixed.

http://gwt-code-reviews.appspot.com/1618806/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Properly encode request parameters that use Collections when in JSON-RPC mode. (issue1618806)


http://gwt-code-reviews.appspot.com/1618806/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Reducing the unsafe URI log warning to an info. If you use a String in a URI context, we sanitiz... (issue1616804)

2011-12-20 Thread jlabanca


committed as r10804

http://gwt-code-reviews.appspot.com/1616804/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Add ProvidesKey support to ValuePicker (issue1614807)


I'm not keen on the proposed changes to setAcceptableValues and
setValue. ValuePicker is supposed to be usable as a thin wrapper around
a CellList, without even calling setAcceptableValues.

With your proposed changes, setValue would break the list as soon as you
use pagination (getVisibleItems() wouldn't returned the whole list of
"acceptable values", only the visible ones).

The fix for issue 5478 is limited to the added constructor
(Renderer,ProvidesKey) and constructing the SingleSelectionModel with an
explicit ProvidesKey.

I'm fine with the change using setRowData(List) rather than
setRowData(int,List) though; it was IMO a bug in the original
implementation.


http://gwt-code-reviews.appspot.com/1614807/diff/1/user/src/com/google/gwt/user/client/ui/ValuePicker.java
File user/src/com/google/gwt/user/client/ui/ValuePicker.java (right):

http://gwt-code-reviews.appspot.com/1614807/diff/1/user/src/com/google/gwt/user/client/ui/ValuePicker.java#newcode50
user/src/com/google/gwt/user/client/ui/ValuePicker.java:50: public
ValuePicker(CellList cellList, ProvidesKey keyProvider) {
What if one passes a different key provider as the one used to create
the CellList?
Shouldn't the fix rather be to ask the CellList for its key provider?
I.e. smodel = new SingleSelectionModel(cellList.getKeyProvider());

http://gwt-code-reviews.appspot.com/1614807/diff/1/user/src/com/google/gwt/user/client/ui/ValuePicker.java#newcode70
user/src/com/google/gwt/user/client/ui/ValuePicker.java:70: public
ValuePicker(Renderer renderer, ProvidesKey keyProvider) {
I suppose keyProvider was meant to be passed to the CellList ctor?

http://gwt-code-reviews.appspot.com/1614807/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Properly encode request parameters that use Collections when in JSON-RPC mode. (issue1618806)