Breaking Change: Use ValueBoxBase<String> instead of TextBox in SuggestBox

Breaking Change: Use ValueBoxBase<String> instead of TextBox in SuggestBox

[t.broyer, 2012-09-10 10:42:04]

My preferred option, with one small adjustment to make it a non-breaking change.

https://codereview.appspot.com/6492092/diff/1/user/src/com/google/gwt/user/cl...
File user/src/com/google/gwt/user/client/ui/SuggestBox.java (right):

https://codereview.appspot.com/6492092/diff/1/user/src/com/google/gwt/user/cl...
user/src/com/google/gwt/user/client/ui/SuggestBox.java:877: public
ValueBoxBase<String> getTextBox() {
How about adding a getValueBox() instead, and let this method throw a
ClassCastException in case 'box' is not a TextBoxBase?
(and deprecate it in favor of getValueBox)

That way, we wouldn't even break anyone's code.

[jtamplin, 2012-09-10 12:03:20]

BTW: you should create code reviews at http://gwt-code-reviews.appspot.com/
instead.

There is a also a TODO to support SafeHtml here.  I'm not entirely sure what
that means in this case (Doesn't TextBox already ensure the string is
uninterpreted?  Does changing to a ValueBoxBase mean you might be displaying
HTML in a value?), but we should either make sure this change moves us closer to
that or remove the TODO.

https://codereview.appspot.com/6492092/diff/1/user/src/com/google/gwt/user/cl...
File user/src/com/google/gwt/user/client/ui/SuggestBox.java (right):

https://codereview.appspot.com/6492092/diff/1/user/src/com/google/gwt/user/cl...
user/src/com/google/gwt/user/client/ui/SuggestBox.java:877: public
ValueBoxBase<String> getTextBox() {
On 2012/09/10 10:42:04, t.broyer wrote:
> How about adding a getValueBox() instead, and let this method throw a
> ClassCastException in case 'box' is not a TextBoxBase?
> (and deprecate it in favor of getValueBox)
> 
> That way, we wouldn't even break anyone's code.

True, but you leave a runtime landmine that may fail in production.  I think if
you do that, you need to deprecate this method and retire it pretty soon.

[t.broyer, 2012-09-10 12:35:50]

On 2012/09/10 12:03:20, jtamplin wrote:
> BTW: you should create code reviews at http://gwt-code-reviews.appspot.com/
> instead.
> 
> There is a also a TODO to support SafeHtml here.  I'm not entirely sure what
> that means in this case (Doesn't TextBox already ensure the string is
> uninterpreted?  Does changing to a ValueBoxBase mean you might be displaying
> HTML in a value?), but we should either make sure this change moves us closer
to
> that or remove the TODO.

Could it have rather been about the SuggestionDisplay?
Sounds hard to add (would require a breaking-change in SuggestOracle.Suggestion,
or possibly a new interface, and a breaking change to
SuggestBox.SuggestionDisplay).
At this point, we'd probably better rebuild SuggestBox from scratch with better
modularity (possibly even build it as an extender/behavior, similar to ASP.NET
AJAX's AutoComplete http://www.asp.net/ajaxlibrary/act_AutoComplete.ashx) and
allowing for auto-completing parts of the valuebox's value (similar to Closure
Library's AutoComplete
http://closure-library.googlecode.com/svn/docs/class_goog_ui_ac_AutoComplete....)

In other words, we should probably remove this TODO, or change it to a comment
suggesting the above.

[seanross, 2012-09-10 14:56:45]

On 2012/09/10 10:42:04, t.broyer wrote:
> My preferred option, with one small adjustment to make it a non-breaking
change.
> 
>
https://codereview.appspot.com/6492092/diff/1/user/src/com/google/gwt/user/cl...
> File user/src/com/google/gwt/user/client/ui/SuggestBox.java (right):
> 
>
https://codereview.appspot.com/6492092/diff/1/user/src/com/google/gwt/user/cl...
> user/src/com/google/gwt/user/client/ui/SuggestBox.java:877: public
> ValueBoxBase<String> getTextBox() {
> How about adding a getValueBox() instead, and let this method throw a
> ClassCastException in case 'box' is not a TextBoxBase?
> (and deprecate it in favor of getValueBox)
> 
> That way, we wouldn't even break anyone's code.

Nice idea! New patch set(s) uploaded

[jtamplin, 2012-09-10 15:44:43]

LGTM

[jtamplin, 2012-09-10 15:46:14]

https://codereview.appspot.com/6492092/diff/5001/user/src/com/google/gwt/user...
File user/src/com/google/gwt/user/client/ui/SuggestBox.java (right):

https://codereview.appspot.com/6492092/diff/5001/user/src/com/google/gwt/user...
user/src/com/google/gwt/user/client/ui/SuggestBox.java:878: * @deprecated in
favour of getValueBox
If there is going to be an official policy of removing deprecated items, should
list a date or a release version where it will be removed here?

[t.broyer, 2012-09-10 16:21:53]

LGTM

https://codereview.appspot.com/6492092/diff/5001/user/src/com/google/gwt/user...
File user/src/com/google/gwt/user/client/ui/SuggestBox.java (right):

https://codereview.appspot.com/6492092/diff/5001/user/src/com/google/gwt/user...
user/src/com/google/gwt/user/client/ui/SuggestBox.java:878: * @deprecated in
favour of getValueBox
On 2012/09/10 15:46:14, jtamplin wrote:
> If there is going to be an official policy of removing
> deprecated items,

I really hope so!
Proposed it a couple times already but received no feedback so far :-(

> should list a date or a release version where it will be
> removed here?

In the mean time, maybe list the first release version where it'll be marked
deprecated (i.e. 2.6 here).

[skybrian1, 2012-09-10 21:10:25]

LGTM. I'll commit this soon.