[gwt-contrib] Change in gwt[master]: Add interfaces for widgets.
Stephen Haberman has posted comments on this change.
Change subject: Add interfaces for widgets.
..
Patch Set 7:
Replying to Goktug...
consistent naming convention throughout the SDKs
Agreed, of course. I see what you mean with the asXXX methods.
However, personally, I thought "IsXXX" just meant "we really wanted to name
this interface just 'XXX', but that name was already taken by the concrete
class, which we can't rename, so we're adding the 'Is' prefix instead".
I am open to suggestions, but personally think IsAbsolutePanel/etc. is a
pretty good fit.
Looking at the size of this change and thinking of its implications, we
need a very good justification for it
Well, it is big, yes, but I assert it's actually pretty simple. No
semantics or logic are changing, it's just laying some interfaces over
things.
see a demonstration why mocking options out there [...] and see if we can
fix the problem without this mess
Well, I read the github page for gwt-mockito, and I believe we have
different opinions on what constitutes a "mess" :-).
Personally, I think just adding interfaces is a much more straight forward
approach than hacking around with classloaders/etc.
That said, I would be fine adopting a non-IsXXX variation of my approach if
it was technically viable; however, AFAIK, it is not.
As a distilled/simplified example, what I do is, roughly:
IsElement e;
// if in prod
e = new Element()
// otherwise in tests
e = new StubElement();
// now presenter uses e for business logic
e.setInnerHTML("asdf")
// and the test asserts that it worked
assertThat(e.getInnerHTML(), is("asdf"))
This same pattern applies for all of the widgets, e.g. IsAbsolutePanel/etc.
If I were to use gwt-mockito in the above snippet to replace "IsElement"
with just "Element", I would still need my "class StubElement extends
Element" to be able to override and implement stub logic for all of
Element's methods.
Which, given it's a JSO and has final methods, would not be possible.
So, basically, stubs cannot rely on as much classloader magic as mocks can.
However, I do not believe this means stubs are an unacceptable way of
testing--they just typically require interfaces.
That said, I would assert that the whole notion of having JRE-only tests
(that sometimes talk to real UI widgets and sometimes fake UI widgets)
means that, really, widgets & friends should have had interfaces from the
beginning.
Interfaces are the canonical way in Java to say "program against this API,
and there will be multiple implementations".
I don't think this is very provocative.
--
To view, visit https://gwt-review.googlesource.com/3231
To unsubscribe, visit https://gwt-review.googlesource.com/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd17162d37e367720829bcdaf9a350e446c833b9
Gerrit-PatchSet: 7
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Stephen Haberman
Gerrit-Reviewer: Colin Alworth
Gerrit-Reviewer: Daniel Kurka
Gerrit-Reviewer: Goktug Gokdogan
Gerrit-Reviewer: Leeroy Jenkins
Gerrit-Reviewer: Stephen Haberman
Gerrit-Reviewer: Thomas Broyer
Gerrit-HasComments: No
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups "GWT Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Add interfaces for widgets.
Stephen Haberman has posted comments on this change.
Change subject: Add interfaces for widgets.
..
Patch Set 6:
(9 comments)
File user/src/com/google/gwt/dom/client/HasStyle.java
Line 29: void setStyleName(String styleName);
Done
File user/src/com/google/gwt/dom/client/IsElement.java
Line 61: void appendChild(IsElement element);
The reason only getChild/removeFromParent are included is simply that those
are the only methods I'd needed so far, and I've built all of these
interfaces lazily/as needed over the last few years.
I agree that if some of these methods are going to be here, then they all
should.
I won't do that just yet, but will after the patch is
in/very-close-to-being-2+'d.
File user/src/com/google/gwt/dom/client/Style.java
Line 31: public class Style extends JavaScriptObject implements IsStyle {
True, but that is fine--IsStyle is not a characteristic interface, which
would need to be implemented by many other classes.
That said, I did make a HasStyle, which is a characteristic interface that
both Element (a JSO) and Widget (a regular object/non-JSO) implement.
But that is also kosher, and I wouldn't anticipate other JSOs wanting to
implement it. (As Element should already be a base class of any element-ish
JSO that wanted to implement it.)
File user/src/com/google/gwt/user/client/ui/IndexedPanel.java
Line 34:* Extends this interface with convenience methods to handle
{@link IsWidget}.
Done
File user/src/com/google/gwt/user/client/ui/IsImage.java
Line 34: void setUrl(String url);
Personally, I think the point of these interfaces (and what makes them
handy) is that they expose the widget's API exactly as it is, so that your
code can do anything it would to a real widget, but via the faked interface.
So, in that regard, I don't think the interface itself is the place to
start enforcing SafeUri--instead, I think we would deprecate-then-remove
the non-SafeUris methods in both the interface & widget at the same time.
File user/src/com/google/gwt/user/client/ui/IsUIObject.java
Line 27: IsElement getIsElement();
Done
Line 27: IsElement getIsElement();
Colin, I believe Thomas's suggestion is fine because, as he said, the API
of UIObject stays Element, no breaking changes.
But since Element implements IsElement, it still fulfills the "IsElement
getElement" contract of IsUIObject.
File user/src/com/google/gwt/user/client/ui/IsWidget2.java
Line 28: public interface IsWidget2 extends IsWidget, IsUIObject,
EventListener, HasHandlers,
Np--although feel free to follow up on the mailing list if the thread gets
too long...
RE IsWidget vs. IsWidget2, yes, the "IsWidget2" is for mocking. Although,
AFAIK, this is the same purpose as the original IsWidget, or at least that
is what its javadoc insinuates.
The "2" was just an ugly name that means "the same thing as the original,
but with more methods, except we can't really add them to the original,
because it would break backwards compatibility". E.g. it's just for
versioning the interface.
I've since renamed "IsWidget2" to "IsWidget.Extended", an inner-interface,
which is more like how the existing GWT interfaces, like IndexedPanel, were
extended for the original IsWidget.
File user/src/com/google/gwt/user/client/ui/Panel.java
Line 118: return new IsWidgetIteratorAdaptor(iterator());
Ah, yes, another great catch--I believe that I had to do this in Tessell
since I couldn't change Widget itself.
But now, you're right, the adaptor isn't needed, and the cast seems to work
just fine.
--
To view, visit https://gwt-review.googlesource.com/3231
To unsubscribe, visit https://gwt-review.googlesource.com/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd17162d37e367720829bcdaf9a350e446c833b9
Gerrit-PatchSet: 6
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Stephen Haberman
Gerrit-Reviewer: Colin Alworth
Gerrit-Reviewer: Daniel Kurka
Gerrit-Reviewer: Goktug Gokdogan
Gerrit-Reviewer: Leeroy Jenkins
Gerrit-Reviewer: Stephen Haberman
Gerrit-Reviewer: Thomas Broyer
Gerrit-HasComments: Yes
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups "GWT Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/grou
[gwt-contrib] Change in gwt[master]: Add interfaces for widgets.
Hello Leeroy Jenkins, I'd like you to reexamine a change. Please visit https://gwt-review.googlesource.com/3231 to look at the new patch set (#7). Change subject: Add interfaces for widgets. .. Add interfaces for widgets. Change-Id: Ibd17162d37e367720829bcdaf9a350e446c833b9 --- M tools/api-checker/config/gwt25_26userApi.conf M user/src/com/google/gwt/dom/client/Element.java A user/src/com/google/gwt/dom/client/HasStyle.java A user/src/com/google/gwt/dom/client/IsElement.java A user/src/com/google/gwt/dom/client/IsStyle.java M user/src/com/google/gwt/dom/client/Style.java M user/src/com/google/gwt/user/client/Element.java M user/src/com/google/gwt/user/client/ui/AbsolutePanel.java M user/src/com/google/gwt/user/client/ui/Anchor.java M user/src/com/google/gwt/user/client/ui/Button.java M user/src/com/google/gwt/user/client/ui/ButtonBase.java M user/src/com/google/gwt/user/client/ui/CellPanel.java M user/src/com/google/gwt/user/client/ui/CheckBox.java M user/src/com/google/gwt/user/client/ui/ComplexPanel.java M user/src/com/google/gwt/user/client/ui/DateLabel.java M user/src/com/google/gwt/user/client/ui/DockLayoutPanel.java M user/src/com/google/gwt/user/client/ui/FileUpload.java M user/src/com/google/gwt/user/client/ui/FlowPanel.java M user/src/com/google/gwt/user/client/ui/FocusPanel.java M user/src/com/google/gwt/user/client/ui/FocusWidget.java M user/src/com/google/gwt/user/client/ui/FormPanel.java M user/src/com/google/gwt/user/client/ui/Frame.java M user/src/com/google/gwt/user/client/ui/HTML.java M user/src/com/google/gwt/user/client/ui/HTMLPanel.java M user/src/com/google/gwt/user/client/ui/HasOneWidget.java M user/src/com/google/gwt/user/client/ui/HasWidgets.java M user/src/com/google/gwt/user/client/ui/HorizontalPanel.java M user/src/com/google/gwt/user/client/ui/Hyperlink.java M user/src/com/google/gwt/user/client/ui/Image.java M user/src/com/google/gwt/user/client/ui/IndexedPanel.java M user/src/com/google/gwt/user/client/ui/InlineHTML.java M user/src/com/google/gwt/user/client/ui/InlineHyperlink.java M user/src/com/google/gwt/user/client/ui/InlineLabel.java A user/src/com/google/gwt/user/client/ui/IsAbsolutePanel.java A user/src/com/google/gwt/user/client/ui/IsAnchor.java A user/src/com/google/gwt/user/client/ui/IsButton.java A user/src/com/google/gwt/user/client/ui/IsButtonBase.java A user/src/com/google/gwt/user/client/ui/IsCellPanel.java A user/src/com/google/gwt/user/client/ui/IsCheckBox.java A user/src/com/google/gwt/user/client/ui/IsColumnsPanel.java A user/src/com/google/gwt/user/client/ui/IsComplexPanel.java A user/src/com/google/gwt/user/client/ui/IsDateLabel.java A user/src/com/google/gwt/user/client/ui/IsDialogBox.java A user/src/com/google/gwt/user/client/ui/IsDockLayoutPanel.java A user/src/com/google/gwt/user/client/ui/IsFileUpload.java A user/src/com/google/gwt/user/client/ui/IsFlowPanel.java A user/src/com/google/gwt/user/client/ui/IsFocusPanel.java A user/src/com/google/gwt/user/client/ui/IsFocusWidget.java A user/src/com/google/gwt/user/client/ui/IsFormPanel.java A user/src/com/google/gwt/user/client/ui/IsFrame.java A user/src/com/google/gwt/user/client/ui/IsHTML.java A user/src/com/google/gwt/user/client/ui/IsHTMLPanel.java A user/src/com/google/gwt/user/client/ui/IsHorizontalPanel.java A user/src/com/google/gwt/user/client/ui/IsHyperlink.java A user/src/com/google/gwt/user/client/ui/IsImage.java A user/src/com/google/gwt/user/client/ui/IsInlineHTML.java A user/src/com/google/gwt/user/client/ui/IsInlineHyperlink.java A user/src/com/google/gwt/user/client/ui/IsInlineLabel.java A user/src/com/google/gwt/user/client/ui/IsLabel.java A user/src/com/google/gwt/user/client/ui/IsLabelBase.java A user/src/com/google/gwt/user/client/ui/IsListBox.java A user/src/com/google/gwt/user/client/ui/IsNumberLabel.java A user/src/com/google/gwt/user/client/ui/IsPanel.java A user/src/com/google/gwt/user/client/ui/IsPasswordTextBox.java A user/src/com/google/gwt/user/client/ui/IsPopupPanel.java A user/src/com/google/gwt/user/client/ui/IsPushButton.java A user/src/com/google/gwt/user/client/ui/IsRadioButton.java A user/src/com/google/gwt/user/client/ui/IsResetButton.java A user/src/com/google/gwt/user/client/ui/IsResizeLayoutPanel.java A user/src/com/google/gwt/user/client/ui/IsScrollPanel.java A user/src/com/google/gwt/user/client/ui/IsSimpleCheckBox.java A user/src/com/google/gwt/user/client/ui/IsSimplePanel.java A user/src/com/google/gwt/user/client/ui/IsSimpleRadioButton.java A user/src/com/google/gwt/user/client/ui/IsSubmitButton.java A user/src/com/google/gwt/user/client/ui/IsSuggestBox.java A user/src/com/google/gwt/user/client/ui/IsTabLayoutPanel.java A user/src/com/google/gwt/user/client/ui/IsTextArea.java A user/src/com/google/gwt/user/client/ui/IsTextBox.java A user/src/com/google/gwt/user/client/ui/IsTextBoxBase.java A user/src/com/google/gwt/user/client/ui/IsUIObject.java A user/src/com/google/gwt/user/clien
[gwt-contrib] Change in gwt[master]: making RootPanel.clear(true) respects GWT loader iframe
Manuel Carrasco Moñino has posted comments on this change.
Change subject: making RootPanel.clear(true) respects GWT loader iframe
..
Patch Set 5:
(1 comment)
File user/src/com/google/gwt/user/client/ui/RootPanel.java
Line 340: && GWT.getModuleName().equals(childElement.getId())) {
Forget the first part of my last comment, hasTagName was fixed in the patch
https://gwt-review.googlesource.com/#/c/2975
--
To view, visit https://gwt-review.googlesource.com/3430
To unsubscribe, visit https://gwt-review.googlesource.com/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If876b04c453a1d4e170870e97f3a82d0d86599d5
Gerrit-PatchSet: 5
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Daniel Kurka
Gerrit-Reviewer: Daniel Kurka
Gerrit-Reviewer: Leeroy Jenkins
Gerrit-Reviewer: Manuel Carrasco Moñino
Gerrit-Reviewer: Thomas Broyer
Gerrit-HasComments: Yes
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups "GWT Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: FileUpload: extending FocusWidget instead of Widget so as it...
Hello Leeroy Jenkins, Thomas Broyer, I'd like you to reexamine a change. Please visit https://gwt-review.googlesource.com/3211 to look at the new patch set (#6). Change subject: FileUpload: extending FocusWidget instead of Widget so as it exposes many features which already are in the file-input element: click(), focus(), mouseevents, keyevents, etc. .. FileUpload: extending FocusWidget instead of Widget so as it exposes many features which already are in the file-input element: click(), focus(), mouseevents, keyevents, etc. Now it is possible open the file browser calling fileupload.click(). This allows customize widgets to upload files. It works in almost supported browsers: IE6+, Chrome, Safari, FF (from 4.0) and Opera (from 12). Documented css rules noticing that browsers impose many constrains to input file styling. Change-Id: I4bc3c0991c5025a10a14b2f04ece6d91e11bcddb Bugs: issue 2262, issue 4078, issue 1998 --- M user/src/com/google/gwt/dom/client/DOMImplMozilla.java M user/src/com/google/gwt/user/client/ui/FileUpload.java M user/test/com/google/gwt/user/client/ui/FileUploadTest.java 3 files changed, 81 insertions(+), 31 deletions(-) -- To view, visit https://gwt-review.googlesource.com/3211 To unsubscribe, visit https://gwt-review.googlesource.com/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc3c0991c5025a10a14b2f04ece6d91e11bcddb Gerrit-PatchSet: 6 Gerrit-Project: gwt Gerrit-Branch: master Gerrit-Owner: Manuel Carrasco Moñino Gerrit-Reviewer: Daniel Kurka Gerrit-Reviewer: Leeroy Jenkins Gerrit-Reviewer: Manuel Carrasco Moñino Gerrit-Reviewer: Matthew Dempsky Gerrit-Reviewer: Patrick Tucker Gerrit-Reviewer: Thomas Broyer -- http://groups.google.com/group/Google-Web-Toolkit-Contributors --- You received this message because you are subscribed to the Google Groups "GWT Contributors" group. To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: making RootPanel.clear(true) respects GWT loader iframe
Manuel Carrasco Moñino has posted comments on this change.
Change subject: making RootPanel.clear(true) respects GWT loader iframe
..
Patch Set 5:
(1 comment)
File user/src/com/google/gwt/user/client/ui/RootPanel.java
Line 340: && GWT.getModuleName().equals(childElement.getId())) {
- This code does not work with real browsers unless you change it to the
code below, Because Element.hasTagName() is case-sensitive and compares the
argument against getTagName() which always returns UPPERCASE (see:
http://www.w3schools.com/jsref/prop_element_tagname.asp).
childElement.hasTagName(IFrameElement.TAG.toUpperCase())
A simpler solution could be to get the iframe before the loop and compare it
moduleIframe = DOM.getElementById(GWT.getModuleName()) ;
if (!childElement.equals(moduleIframe)) {} ...
- NOTICE: I think we must preserve the '
