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.