[gwt-contrib] Re: Fixed scrollbar width calculation. In Chrome, for a scrollbar styled (issue1756803)
Sorry, I forgot about it. I'll try to submit it this weekend or next week. http://gwt-code-reviews.appspot.com/1756803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixed scrollbar width calculation. In Chrome, for a scrollbar styled (issue1756803)
LGTM Thanks for the patch. I'll try it out and submit it this week. http://gwt-code-reviews.appspot.com/1756803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixed scrollbar width calculation. In Chrome, for a scrollbar styled (issue1756803)
LGTM Thanks for the patch. I'll try it out and submit it this week. http://gwt-code-reviews.appspot.com/1756803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Use Node.contains in IE9/Webkit for isOrHasChild (issue1725808)
committed as r1. Integrated into GWT 2.5 at r8 http://gwt-code-reviews.appspot.com/1725808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Use Node.contains in IE9/Webkit for isOrHasChild (issue1725808)
No problem. Thanks for preparing the patch. This should actually allow the compiler to inline the isOrHasChild call, which could save some bits of compiled code. http://gwt-code-reviews.appspot.com/1725808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Extracted constant strings to the constructor, that allow translation to be provided from the ou... (issue1739803)
committed as r11095 http://gwt-code-reviews.appspot.com/1739803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Use Node.contains in IE9/Webkit for isOrHasChild (issue1725808)
LGTM I'm running tests and committing this now. http://gwt-code-reviews.appspot.com/1725808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Use Node.contains in IE9/Webkit for isOrHasChild (issue1725808)
At this point, the new code is more complicated than the old code. Is it worth switching anymore? Did you performance test to see if element.contains() is faster than the old impl code? http://gwt-code-reviews.appspot.com/1725808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Extracted constant strings to the constructor, that allow translation to be provided from the ou... (issue1739803)
http://gwt-code-reviews.appspot.com/1739803/diff/8001/user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java
File user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java
(right):
http://gwt-code-reviews.appspot.com/1739803/diff/8001/user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java#newcode86
user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java:86: *
Constant for labeling the simple pager navigational {@link ImageButton}s
Add a comment that the default Messages only provide English
translations.
http://gwt-code-reviews.appspot.com/1739803/diff/8001/user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java#newcode705
user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java:705:
private static final Messages MESSAGES =
GWT.Messagescreate(Messages.class);
My GWT.create() foo is failing me. How does one go about adding
translations again?
http://gwt-code-reviews.appspot.com/1739803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Use Node.contains in IE9/Webkit for isOrHasChild (issue1725808)
I missed the changes to the other files. This makes the standard impl simpler and makes all versions of IE use the fixed IE-version. I'll review it closely and submit tomorrow. Thanks for the patch. http://gwt-code-reviews.appspot.com/1725808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Use Node.contains in IE9/Webkit for isOrHasChild (issue1725808)
It was failing on IE9. The Trident implementation has a similar workaround because contains() doesn't work for a non-Element node (ex. the Document). I wonder if the same workaround is still necessary for IE9. http://gwt-code-reviews.appspot.com/1725808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Use Node.contains in IE9/Webkit for isOrHasChild (issue1725808)
LGTM http://gwt-code-reviews.appspot.com/1725808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Use Node.contains in IE9/Webkit for isOrHasChild (issue1725808)
DOMTest.testIsOrHasChild fails consistently with this patch. Rolling it back for now. http://gwt-code-reviews.appspot.com/1725808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding the GWT 2.4_2.5 API Checker configuration file. Many API changes were added to the 2.3_2... (issue1721803)
We don't delete the old files, we just add a new file. http://gwt-code-reviews.appspot.com/1721803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding the GWT 2.4_2.5 API Checker configuration file. Many API changes were added to the 2.3_2... (issue1721803)
That would involve creating 2.3-modified jars, syncing to the 2.4 release, and running API Checker, just to update a file that is no longer used. I'll leave it for the next guy to worry about. It's convenient to have the history of conf files when creating a new config file, even if they aren't exactly correct. Rajeev is right that we could delete them, but its not as if they take up any space. I say leave it as is. http://gwt-code-reviews.appspot.com/1721803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding the GWT 2.4_2.5 API Checker configuration file. Many API changes were added to the 2.3_2... (issue1721803)
committed as r11000. http://gwt-code-reviews.appspot.com/1721803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Adding the GWT 2.4_2.5 API Checker configuration file. Many API changes were added to the 2.3_2... (issue1721803)
Reviewers: rdayal, Description: Adding the GWT 2.4_2.5 API Checker configuration file. Many API changes were added to the 2.3_2.4 file because we never created the 2.4_2.5 config file, so there are a bunch of API change exceptions that had to be copied. Pretty normal, but we should get into the habit of creating a new API checker config file immediately after releasing a version of GWT. I noticed that we have a big API change to the IsRenderable interface between 2.4 and 2.5, which will cause API breaks for anyone using it. I doubt it is used very much (I think its used by UiBinder to render nested elements), so it should be okay to change it with minimal impact to users. But we might want to note it in release notes. Please review this at http://gwt-code-reviews.appspot.com/1721803/ Affected files: M build.xml A tools/api-checker/config/gwt24_25userApi.conf M tools/api-checker/reference/README -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add Integer.TYPE, etc. (issue1710804)
I created http://gwt-code-reviews.appspot.com/1721803/ to update the API checker config file. Its a fairly simple process, but since we only do it 2-3 times per year, we always forget how. I'll submit your change after the API Checker config file is updated. We should just need to add these lines to it: java.lang.Boolean::FALSE FINAL_ADDED java.lang.Boolean::TRUE FINAL_ADDED http://gwt-code-reviews.appspot.com/1710804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixing a Chrome animation bug where animations never stop running. The timestamp that webkitReq... (issue1702803)
committed as r10989 http://gwt-code-reviews.appspot.com/1702803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Removing uses of deprecated Tree code in GWT showcase sample and TreeExample. (issue1712804)
http://gwt-code-reviews.appspot.com/1712804/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwTree.java File samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwTree.java (right): http://gwt-code-reviews.appspot.com/1712804/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwTree.java#newcode194 samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwTree.java:194: item.addTextItem(); Agreed. We should add some API to provide the children the first time the node is opened. I created issue 7387 to track this. http://gwt-code-reviews.appspot.com/1712804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Removing uses of deprecated Tree code in GWT showcase sample and TreeExample. (issue1712804)
committed as r10991 http://gwt-code-reviews.appspot.com/1712804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Using SafeStyles in the ButtonCellBase Templates to avoid the compiler warnings against using St... (issue1707804)
Committed as r10985
http://gwt-code-reviews.appspot.com/1707804/diff/1/user/src/com/google/gwt/cell/client/ButtonCellBase.java
File user/src/com/google/gwt/cell/client/ButtonCellBase.java (right):
http://gwt-code-reviews.appspot.com/1707804/diff/1/user/src/com/google/gwt/cell/client/ButtonCellBase.java#newcode61
user/src/com/google/gwt/cell/client/ButtonCellBase.java:61: public
static interface AppearanceC {
On 2012/05/17 23:34:05, skybrian wrote:
Style: all interfaces are static, and the static keyword should be
omitted.
Done.
http://gwt-code-reviews.appspot.com/1707804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add white-space support to Style (issue1714803)
Looking at it now http://gwt-code-reviews.appspot.com/1714803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Preparing for an upcoming API change to requestAnimationFrame where browsers will pass a sub-mil... (issue1715803)
Reviewers: rdayal,
Description:
Preparing for an upcoming API change to requestAnimationFrame where
browsers will pass a sub-millisecond timer instead of a Date.now()
timestamp. This can cause havoc with animations and has already popped
up in the Chrome dev channel (but was reverted). We now ignore the
native callback argument and just grab the current time from javascript.
http://gwt-code-reviews.appspot.com/1702803/
Please review this at http://gwt-code-reviews.appspot.com/1715803/
Affected files:
M
user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java
M
user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
Index:
user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java
===
---
user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java
(revision 10982)
+++
user/src/com/google/gwt/animation/client/AnimationSchedulerImplMozilla.java
(working copy)
@@ -64,9 +64,12 @@
*/
private native void requestAnimationFrameImpl(AnimationCallback callback,
AnimationHandleImpl handle) /*-{
-var wrapper = $entry(function(time) {
+var wrapper = $entry(function() {
if
(!hand...@com.google.gwt.animation.client.AnimationSchedulerImplMozilla.AnimationHandleImpl::canceled)
{
-
callba...@com.google.gwt.animation.client.AnimationScheduler.AnimationCallback::execute(D)(time);
+// Older versions of firefox pass the current timestamp, but newer
versions pass a
+// high resolution timer. We normalize on the current timestamp.
+var now =
@com.google.gwt.core.client.Duration::currentTimeMillis()();
+
callba...@com.google.gwt.animation.client.AnimationScheduler.AnimationCallback::execute(D)(now);
}
});
$wnd.mozRequestAnimationFrame(wrapper);
Index:
user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
===
---
user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
(revision 10982)
+++
user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
(working copy)
@@ -63,10 +63,11 @@
private native double requestAnimationFrameImpl(AnimationCallback
callback, Element element) /*-{
var _callback = callback;
-var wrapper = $entry(function(time) {
- // Chrome 10 does not pass the 'time' argument, so we fake it.
- time = time ||
@com.google.gwt.core.client.Duration::currentTimeMillis()();
-
_callba...@com.google.gwt.animation.client.AnimationScheduler.AnimationCallback::execute(D)(time);
+var wrapper = $entry(function() {
+ // Older versions of Chrome pass the current timestamp, but newer
versions pass a
+ // high resolution timer. We normalize on the current timestamp.
+ var now =
@com.google.gwt.core.client.Duration::currentTimeMillis()();
+
_callba...@com.google.gwt.animation.client.AnimationScheduler.AnimationCallback::execute(D)(now);
});
return $wnd.webkitRequestAnimationFrame(wrapper, element);
}-*/;
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixing a Chrome animation bug where animations never stop running. The timestamp that webkitReq... (issue1702803)
http://gwt-code-reviews.appspot.com/1702803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixing a Chrome animation bug where animations never stop running. The timestamp that webkitReq... (issue1702803)
Thanks for the heads up Thomas. I've updated the CL to include firefox. http://gwt-code-reviews.appspot.com/1702803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add white-space support to Style (issue1714803)
committed as r10988 http://gwt-code-reviews.appspot.com/1714803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Removing uses of deprecated Tree code in GWT showcase sample and TreeExample. (issue1712804)
Reviewers: skybrian,
Description:
Removing uses of deprecated Tree code in GWT showcase sample and
TreeExample.
Please review this at http://gwt-code-reviews.appspot.com/1712804/
Affected files:
M
samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwStackLayoutPanel.java
M
samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwStackPanel.java
M
samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwTree.java
M user/javadoc/com/google/gwt/examples/TreeExample.java
Index:
samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwStackLayoutPanel.java
===
---
samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwStackLayoutPanel.java
(revision 10982)
+++
samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwStackLayoutPanel.java
(working copy)
@@ -105,6 +105,7 @@
/**
* Use noimage.png, which is a blank 1x1 image.
*/
+@Override
@Source(noimage.png)
ImageResource treeLeaf();
}
@@ -163,10 +164,12 @@
protected void asyncOnInitialize(final AsyncCallbackWidget callback) {
GWT.runAsync(CwStackLayoutPanel.class, new RunAsyncCallback() {
+ @Override
public void onFailure(Throwable caught) {
callback.onFailure(caught);
}
+ @Override
public void onSuccess() {
callback.onSuccess(onInitialize());
}
@@ -219,6 +222,7 @@
// Open the contact info popup when the user clicks a contact
contactLink.addClickHandler(new ClickHandler() {
+@Override
public void onClick(ClickEvent event) {
// Set the info about the contact
SafeHtmlBuilder sb = new SafeHtmlBuilder();
@@ -285,7 +289,7 @@
@ShowcaseSource
private Widget createMailItem(Images images) {
Tree mailPanel = new Tree(images);
-TreeItem mailPanelRoot = mailPanel.addItem(f...@example.com);
+TreeItem mailPanelRoot = mailPanel.addTextItem(f...@example.com);
String[] mailFolders = constants.cwStackLayoutPanelMailFolders();
addItem(mailPanelRoot, images.inbox(), mailFolders[0]);
addItem(mailPanelRoot, images.drafts(), mailFolders[1]);
Index:
samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwStackPanel.java
===
---
samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwStackPanel.java
(revision 10982)
+++
samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwStackPanel.java
(working copy)
@@ -21,6 +21,7 @@
import com.google.gwt.event.dom.client.ClickHandler;
import com.google.gwt.i18n.client.Constants;
import com.google.gwt.resources.client.ImageResource;
+import com.google.gwt.safehtml.shared.SafeHtmlBuilder;
import com.google.gwt.sample.showcase.client.ContentWidget;
import
com.google.gwt.sample.showcase.client.ShowcaseAnnotations.ShowcaseData;
import
com.google.gwt.sample.showcase.client.ShowcaseAnnotations.ShowcaseSource;
@@ -101,6 +102,7 @@
/**
* Use noimage.png, which is a blank 1x1 image.
*/
+@Override
@Source(noimage.png)
ImageResource treeLeaf();
}
@@ -159,10 +161,12 @@
protected void asyncOnInitialize(final AsyncCallbackWidget callback) {
GWT.runAsync(CwStackPanel.class, new RunAsyncCallback() {
+ @Override
public void onFailure(Throwable caught) {
callback.onFailure(caught);
}
+ @Override
public void onSuccess() {
callback.onSuccess(onInitialize());
}
@@ -170,7 +174,10 @@
}
private void addItem(TreeItem root, ImageResource image, String label) {
-root.addItem(AbstractImagePrototype.create(image).getHTML() + +
label);
+SafeHtmlBuilder itemHtml = new SafeHtmlBuilder();
+itemHtml.append(AbstractImagePrototype.create(image).getSafeHtml());
+itemHtml.appendEscaped( ).appendEscaped(label);
+root.addItem(itemHtml.toSafeHtml());
}
/**
@@ -203,6 +210,7 @@
// Open the contact info popup when the user clicks a contact
contactLink.addClickHandler(new ClickHandler() {
+@Override
public void onClick(ClickEvent event) {
// Set the info about the contact
contactInfo.setHTML(contactName + bri + contactEmail
+ /i);
@@ -242,7 +250,7 @@
@ShowcaseSource
private Tree createMailItem(Images images) {
Tree mailPanel = new Tree(images);
-TreeItem mailPanelRoot = mailPanel.addItem(f...@example.com);
+TreeItem mailPanelRoot = mailPanel.addTextItem(f...@example.com);
String[] mailFolders = constants.cwStackPanelMailFolders();
addItem(mailPanelRoot, images.inbox(), mailFolders[0]);
addItem(mailPanelRoot, images.drafts(), mailFolders[1]);
Index:
[gwt-contrib] Re: Removing uses of deprecated Tree code in GWT showcase sample and TreeExample. (issue1712804)
http://gwt-code-reviews.appspot.com/1712804/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwStackPanel.java File samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwStackPanel.java (right): http://gwt-code-reviews.appspot.com/1712804/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwStackPanel.java#newcode179 samples/showcase/src/com/google/gwt/sample/showcase/client/content/lists/CwStackPanel.java:179: itemHtml.appendEscaped( ).appendEscaped(label); Good call. I'll fix it before submitting. http://gwt-code-reviews.appspot.com/1712804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixing a bug in MultiSelectionModel where it does not respect the order of selection when select... (issue1707803)
committed as r10979 http://gwt-code-reviews.appspot.com/1707803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixing a bug where c.g.g.dom.client.Style can throw a JS exception if a style property is set to... (issue1709803)
committed as 10980 http://gwt-code-reviews.appspot.com/1709803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Using SafeStyles in the ButtonCellBase Templates to avoid the compiler warnings against using St... (issue1707804)
Reviewers: skybrian,
Description:
Using SafeStyles in the ButtonCellBase Templates to avoid the compiler
warnings against using Strings in the style portion of templates.
Issue: 6892
Please review this at http://gwt-code-reviews.appspot.com/1707804/
Affected files:
M user/src/com/google/gwt/cell/client/ButtonCellBase.java
Index: user/src/com/google/gwt/cell/client/ButtonCellBase.java
===
--- user/src/com/google/gwt/cell/client/ButtonCellBase.java (revision 10982)
+++ user/src/com/google/gwt/cell/client/ButtonCellBase.java (working copy)
@@ -23,6 +23,7 @@
import com.google.gwt.dom.client.BrowserEvents;
import com.google.gwt.dom.client.Element;
import com.google.gwt.dom.client.NativeEvent;
+import com.google.gwt.dom.client.Style.Unit;
import com.google.gwt.event.shared.HandlerRegistration;
import com.google.gwt.i18n.client.LocaleInfo;
import com.google.gwt.resources.client.ClientBundle;
@@ -32,6 +33,8 @@
import com.google.gwt.resources.client.ImageResource;
import com.google.gwt.resources.client.ImageResource.ImageOptions;
import com.google.gwt.resources.client.ImageResource.RepeatStyle;
+import com.google.gwt.safecss.shared.SafeStyles;
+import com.google.gwt.safecss.shared.SafeStylesBuilder;
import com.google.gwt.safehtml.client.SafeHtmlTemplates;
import com.google.gwt.safehtml.shared.SafeHtml;
import com.google.gwt.safehtml.shared.SafeHtmlBuilder;
@@ -55,7 +58,7 @@
*
* @param C the type that this Cell represents
*/
- public interface AppearanceC {
+ public static interface AppearanceC {
/**
* Called when the user pushes the button down.
@@ -107,7 +110,7 @@
}
/**
- * The default implementation of the {@link Appearance}.
+ * The default implementation of the {@link ButtonCellBase.Appearance}.
*
* @param C the type that this Cell represents
*/
@@ -182,16 +185,16 @@
* wrap even when they do not need to.
*/
@SafeHtmlTemplates.Template(div class=\{0}\
- +
style=\position:relative;padding-{1}:{2}px;zoom:0;\{3}{4}/div)
- SafeHtml iconContentLayout(String classes, String iconDirection, int
iconWidth,
- SafeHtml icon, SafeHtml cellContents);
+ + style=\{1}position:relative;zoom:0;\{2}{3}/div)
+ SafeHtml iconContentLayout(
+ String classes, SafeStyles styles, SafeHtml icon, SafeHtml
cellContents);
/**
* The wrapper around the icon that aligns it vertically with the
text.
*/
- @SafeHtmlTemplates.Template(div
style=\position:absolute;{0}:0px;top:50%;line-height:0px;
- + margin-top:-{1}px;\{2}/div)
- SafeHtml iconWrapper(String direction, int halfHeight, SafeHtml
image);
+ @SafeHtmlTemplates.Template(div
style=\{0}position:absolute;top:50%;line-height:0px;\
+ + {1}/div)
+ SafeHtml iconWrapper(SafeStyles styles, SafeHtml image);
}
private static final int DEFAULT_ICON_PADDING = 3;
@@ -205,14 +208,13 @@
return defaultResources;
}
-private final String iconDirection =
LocaleInfo.getCurrentLocale().isRTL() ? right : left;
private SafeHtml iconSafeHtml = SafeHtmlUtils.EMPTY_SAFE_HTML;
private ImageResource lastIcon;
private final SafeHtmlRendererC renderer;
private final Style style;
/**
- * Construct a new {@link DefaultAppearance} using the default styles.
+ * Construct a new {@link ButtonCellBase.DefaultAppearance} using the
default styles.
*
* @param renderer the {@link SafeHtmlRenderer} used to render the
contents
*/
@@ -221,7 +223,7 @@
}
/**
- * Construct a new {@link DefaultAppearance} using the specified
resources.
+ * Construct a new {@link ButtonCellBase.DefaultAppearance} using the
specified resources.
*
* @param renderer the {@link SafeHtmlRenderer} used to render the
contents
* @param resources the resources and styles to apply to the button
@@ -245,14 +247,17 @@
return renderer;
}
+@Override
public void onPush(Element parent) {
parent.getFirstChildElement().addClassName(style.buttonCellBasePushing());
}
+@Override
public void onUnpush(Element parent) {
parent.getFirstChildElement().removeClassName(style.buttonCellBasePushing());
}
+@Override
public void render(ButtonCellBaseC cell, Context context, C value,
SafeHtmlBuilder sb) {
// Determine the classes from the state of the button.
SafeHtmlBuilder classes = new SafeHtmlBuilder();
@@ -281,7 +286,14 @@
AbstractImagePrototype proto =
AbstractImagePrototype.create(icon);
SafeHtml iconOnly =
SafeHtmlUtils.fromTrustedString(proto.getHTML());
int halfHeight = (int) Math.round(icon.getHeight() / 2.0);
- iconSafeHtml = template.iconWrapper(iconDirection, halfHeight,
iconOnly);
+ SafeStylesBuilder
[gwt-contrib] Re: Fixing a bug in MultiSelectionModel where it does not respect the order of selection when select... (issue1707803)
http://gwt-code-reviews.appspot.com/1707803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixing a bug in MultiSelectionModel where it does not respect the order of selection when select... (issue1707803)
http://gwt-code-reviews.appspot.com/1707803/diff/1/user/src/com/google/gwt/view/client/MultiSelectionModel.java File user/src/com/google/gwt/view/client/MultiSelectionModel.java (right): http://gwt-code-reviews.appspot.com/1707803/diff/1/user/src/com/google/gwt/view/client/MultiSelectionModel.java#newcode28 user/src/com/google/gwt/view/client/MultiSelectionModel.java:28: * @param T the record data type Switched to item in this class and other selection model classes. http://gwt-code-reviews.appspot.com/1707803/diff/1/user/src/com/google/gwt/view/client/MultiSelectionModel.java#newcode33 user/src/com/google/gwt/view/client/MultiSelectionModel.java:33: * Stores an objected and its pending selection state. On 2012/05/10 22:43:42, skybrian wrote: sp: object. Or maybe item? Done. http://gwt-code-reviews.appspot.com/1707803/diff/1/user/test/com/google/gwt/view/client/MultiSelectionModelTest.java File user/test/com/google/gwt/view/client/MultiSelectionModelTest.java (right): http://gwt-code-reviews.appspot.com/1707803/diff/1/user/test/com/google/gwt/view/client/MultiSelectionModelTest.java#newcode221 user/test/com/google/gwt/view/client/MultiSelectionModelTest.java:221: // Change selection before resolving. On 2012/05/10 22:43:42, skybrian wrote: Maybe say something like Verify that the last change wins. Done. http://gwt-code-reviews.appspot.com/1707803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Rendering a blank space in an empty EditTextCell to force the element to have a height. Otherwi... (issue1708803)
http://gwt-code-reviews.appspot.com/1708803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Rendering a blank space in an empty EditTextCell to force the element to have a height. Otherwi... (issue1708803)
http://gwt-code-reviews.appspot.com/1708803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Rendering a blank space in an empty EditTextCell to force the element to have a height. Otherwi... (issue1708803)
http://gwt-code-reviews.appspot.com/1708803/diff/6002/user/src/com/google/gwt/cell/client/EditTextCell.java
File user/src/com/google/gwt/cell/client/EditTextCell.java (right):
http://gwt-code-reviews.appspot.com/1708803/diff/6002/user/src/com/google/gwt/cell/client/EditTextCell.java#newcode226
user/src/com/google/gwt/cell/client/EditTextCell.java:226: if (toRender
!= null toRender.trim().length() 0) {
I went with the trim after all. It's not that heavy weight, and it will
work better.
http://gwt-code-reviews.appspot.com/1708803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixing a bug where c.g.g.dom.client.Style can throw a JS exception if a style property is set to... (issue1709803)
http://gwt-code-reviews.appspot.com/1709803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixing a bug where c.g.g.dom.client.Style can throw a JS exception if a style property is set to... (issue1709803)
http://gwt-code-reviews.appspot.com/1709803/diff/1/user/src/com/google/gwt/dom/client/Style.java
File user/src/com/google/gwt/dom/client/Style.java (right):
http://gwt-code-reviews.appspot.com/1709803/diff/1/user/src/com/google/gwt/dom/client/Style.java#newcode1764
user/src/com/google/gwt/dom/client/Style.java:1764: return typeof(prop)
== number ? + prop : prop;
The type check isn't an optimization per se; we're checking to make sure
prop isn't null or undefined, which would result in the string null or
undefined. We also can't just check this[name]?, because the
numeric 0 == false.
I moved the implementation out to DOMImpl so that we only do the typeof
check in IE. And even then, we now only use the typeof check for
zIndex, because setting the other properties to a numeric value is an
error anyway.
This leaves the possibility that a user could set a non-standard style
property to a numeric value (which cannot be done with the Style API),
then try to read it as a String using Style.getProperty(). That will
still be an error, but its an obscure use case, and it doesn't make
sense to penalize the 99.999% use case.
http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt/dom/client/DOMImpl.java
File user/src/com/google/gwt/dom/client/DOMImpl.java (right):
http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt/dom/client/DOMImpl.java#newcode299
user/src/com/google/gwt/dom/client/DOMImpl.java:299: return style[name];
For non-IE browsers, nothing has changed. FF and Chrome coerce zIndex
to a String automatically.
http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt/dom/client/DOMImplIE9.java
File user/src/com/google/gwt/dom/client/DOMImplIE9.java (right):
http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt/dom/client/DOMImplIE9.java#newcode43
user/src/com/google/gwt/dom/client/DOMImplIE9.java:43: return
typeof(style[name]) == number ? + style[name] : style[name];
I verified that even IE9 can store zIndex as a numeric value.
http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt/dom/client/Style.java
File user/src/com/google/gwt/dom/client/Style.java (right):
http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt/dom/client/Style.java#newcode1354
user/src/com/google/gwt/dom/client/Style.java:1354: public final String
getProperty(String name) {
If the user calls getProperty(String) with a non-standard property name
(ex. myProperty), and that property happens to have a numeric value
that was set through some other native method (because Style does not
have a numeric setter), you'll get the same error. However, that is a
very rare use case.
http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt/dom/client/Style.java#newcode1412
user/src/com/google/gwt/dom/client/Style.java:1412: return
DOMImpl.impl.getNumericStyleProperty(this, STYLE_Z_INDEX);
We only pay the extra cost of the typeof check for zIndex, and only on
IE. I verified that IE does not allow you to set other properties (ex.
top, left, margin) to a numeric value. It just ignores the
command.
If we find other properties that can be set to a numeric value, we can
defer to the same getNumericStyleProperty() method.
http://gwt-code-reviews.appspot.com/1709803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Rendering a blank space in an empty EditTextCell to force the element to have a height. Otherwi... (issue1708803)
committed as r10972 http://gwt-code-reviews.appspot.com/1708803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixing a bug in MultiSelectionModel where it does not respect the order of selection when select... (issue1707803)
Reviewers: skybrian,
Description:
Fixing a bug in MultiSelectionModel where it does not respect the order
of selection when selecting multiple values that share the same key.
MultiSelectionModel currently stores pending selection changes in a Map
keyed by value. If two values share the same key, there is no guarantee
that the Map of pending changes will be processed in the same order that
setSelected() was called. We now use the key from KeyProvider as the
key in the map. Added a unit test to test for this case.
Issue: 7355
Please review this at http://gwt-code-reviews.appspot.com/1707803/
Affected files:
M user/src/com/google/gwt/view/client/MultiSelectionModel.java
M user/src/com/google/gwt/view/client/OrderedMultiSelectionModel.java
M user/test/com/google/gwt/view/client/MultiSelectionModelTest.java
Index: user/src/com/google/gwt/view/client/MultiSelectionModel.java
===
--- user/src/com/google/gwt/view/client/MultiSelectionModel.java (revision
10970)
+++ user/src/com/google/gwt/view/client/MultiSelectionModel.java (working
copy)
@@ -1,12 +1,12 @@
/*
* Copyright 2010 Google Inc.
- *
+ *
* Licensed under the Apache License, Version 2.0 (the License); you may
not
* use this file except in compliance with the License. You may obtain a
copy of
* the License at
- *
+ *
* http://www.apache.org/licenses/LICENSE-2.0
- *
+ *
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an AS IS BASIS,
WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
@@ -24,15 +24,33 @@
/**
* A simple selection model that allows multiple objects to be selected.
- *
+ *
* @param T the record data type
*/
public class MultiSelectionModelT extends AbstractSelectionModelT {
+ /**
+ * Stores an objected and its pending selection state.
+ *
+ * @param T the data type of the object.
+ */
+ static class SelectionChangeT {
+private final T object;
+private final boolean isSelected;
+
+private SelectionChange(T object, boolean isSelected) {
+ this.object = object;
+ this.isSelected = isSelected;
+}
+ }
+
// Ensure one value per key
final MapObject, T selectedSet;
- private final MapT, Boolean selectionChanges;
+ /**
+ * A map of keys to the object and its pending selection state.
+ */
+ private final MapObject, SelectionChangeT selectionChanges;
/**
* Constructs a MultiSelectionModel without a key provider.
@@ -40,29 +58,29 @@
public MultiSelectionModel() {
this(null);
}
-
+
/**
* Constructs a MultiSelectionModel with the given key provider.
- *
+ *
* @param keyProvider an instance of ProvidesKeyT, or null if the
record
- *object should act as its own key
+ * object should act as its own key
*/
public MultiSelectionModel(ProvidesKeyT keyProvider) {
-this(keyProvider, new HashMapObject, T(), new HashMapT, Boolean());
+this(keyProvider, new HashMapObject, T(), new HashMapObject,
SelectionChangeT());
}
/**
* Construct a MultiSelectionModel with the given key provider and
* implementations of selectedSet and selectionChanges. Different
* implementations allow for enforcing order on selection.
- *
+ *
* @param keyProvider an instance of ProvidesKeyT, or null if the
record
- *object should act as its own key
+ * object should act as its own key
* @param selectedSet an instance of Map
* @param selectionChanges an instance of Map
*/
MultiSelectionModel(ProvidesKeyT keyProvider, MapObject, T
selectedSet,
- MapT, Boolean selectionChanges) {
+ MapObject, SelectionChangeT selectionChanges) {
super(keyProvider);
this.selectedSet = selectedSet;
this.selectionChanges = selectionChanges;
@@ -72,7 +90,7 @@
* Deselect all selected values.
*/
public void clear() {
-// Clear the current list of pending changes.
+// Clear the current list of pending changes.
selectionChanges.clear();
/*
@@ -82,14 +100,15 @@
* determine if we should fire an event.
*/
for (T value : selectedSet.values()) {
- selectionChanges.put(value, false);
+ selectionChanges.put(getKey(value), new SelectionChangeT(value,
false));
}
scheduleSelectionChangeEvent();
}
/**
- * Get the set of selected items as a copy.
- *
+ * Get the set of selected items as a copy. If multiple selected values
share
+ * the same key, only the last selected values is included in the set.
+ *
* @return the set of selected items
*/
public SetT getSelectedSet() {
@@ -105,7 +124,7 @@
@Override
public void setSelected(T object, boolean selected) {
-selectionChanges.put(object, selected);
+selectionChanges.put(getKey(object), new SelectionChangeT(object,
selected));
[gwt-contrib] Rendering a blank space in an empty EditTextCell to force the element to have a height. Otherwi... (issue1708803)
Reviewers: rdayal,
Description:
Rendering a blank space in an empty EditTextCell to force the element to
have a height. Otherwise, the EditTextCell is not clickable if it is
empty, and the user cannot select it to switch to edit mode.
Issue: 7247
Please review this at http://gwt-code-reviews.appspot.com/1708803/
Affected files:
M user/src/com/google/gwt/cell/client/EditTextCell.java
Index: user/src/com/google/gwt/cell/client/EditTextCell.java
===
--- user/src/com/google/gwt/cell/client/EditTextCell.java (revision 10970)
+++ user/src/com/google/gwt/cell/client/EditTextCell.java (working copy)
@@ -219,8 +219,14 @@
// The user pressed enter, but view data still exists.
sb.append(renderer.render(text));
}
-} else if (value != null) {
+} else if (value != null value.length() 0) {
sb.append(renderer.render(value));
+} else {
+ /*
+ * Render a blank space to force the rendered element to have a
height.
+ * Otherwise it is not clickable.
+ */
+ sb.appendHtmlConstant(nbsp;);
}
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Rendering a blank space in an empty EditTextCell to force the element to have a height. Otherwi... (issue1708803)
http://gwt-code-reviews.appspot.com/1708803/diff/1/user/src/com/google/gwt/cell/client/EditTextCell.java
File user/src/com/google/gwt/cell/client/EditTextCell.java (right):
http://gwt-code-reviews.appspot.com/1708803/diff/1/user/src/com/google/gwt/cell/client/EditTextCell.java#newcode222
user/src/com/google/gwt/cell/client/EditTextCell.java:222: } else if
(value != null value.length() 0) {
On 2012/05/10 19:14:52, tbroyer wrote:
What if value is made of spaces? Does it work? Doesn't it needs a
nbsp; too?
I don't want everyone to pay the penalty of a regex or trim() for an
relatively obscure case (all whitespace). The real answer is that Cells
should be able to receive events triggered on the TD element. That
could be an option in CellTable, but I don't have time to add it at the
moment.
http://gwt-code-reviews.appspot.com/1708803/diff/1/user/src/com/google/gwt/cell/client/EditTextCell.java#newcode229
user/src/com/google/gwt/cell/client/EditTextCell.java:229:
sb.appendHtmlConstant(nbsp;);
On 2012/05/10 19:14:52, tbroyer wrote:
How about using \u00A0 instead? Would save a few bytes in the compiled
code.
I can switch to \u00A0.
http://gwt-code-reviews.appspot.com/1708803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixing a bug where c.g.g.dom.client.Style can throw a JS exception if a style property is set to... (issue1709803)
Reviewers: skybrian,
Description:
Fixing a bug where c.g.g.dom.client.Style can throw a JS exception if a
style property is set to a numeric type (eg: zIndex = 1). Most browsers
automatically coerce the value to a String, but IE6-7 does not. We now
coerce the returned value to a string if it is a numeric type. This
doesn't come up often because you have to manually set the value to a
numeric type using a native method (our API always sets the value as a
string).
Issue: 5548
Please review this at http://gwt-code-reviews.appspot.com/1709803/
Affected files:
M user/src/com/google/gwt/dom/client/Style.java
M user/test/com/google/gwt/dom/client/StyleTest.java
Index: user/src/com/google/gwt/dom/client/Style.java
===
--- user/src/com/google/gwt/dom/client/Style.java (revision 10970)
+++ user/src/com/google/gwt/dom/client/Style.java (working copy)
@@ -1759,7 +1759,9 @@
* Gets the value of a named property.
*/
private native String getPropertyImpl(String name) /*-{
-return this[name];
+// Coerce numeric values a string. In IE, some values can be stored as
numeric types.
+var prop = this[name];
+return typeof(prop) == number ? + prop : prop;
}-*/;
/**
Index: user/test/com/google/gwt/dom/client/StyleTest.java
===
--- user/test/com/google/gwt/dom/client/StyleTest.java (revision 10970)
+++ user/test/com/google/gwt/dom/client/StyleTest.java (working copy)
@@ -287,6 +287,17 @@
assertEquals(Visibility.HIDDEN, style.getVisibility());
}
+ /**
+ * Test that z-index can be set as an integer and returned as a string.
+ */
+ public void testZIndexInt() {
+DivElement div = Document.get().createDivElement();
+Style style = div.getStyle();
+
+setPropertyInt(style, zIndex, 1);
+assertEquals(1, style.getZIndex());
+ }
+
@DoNotRunWith({Platform.HtmlUnitUnknown})
public void testUnits() {
DivElement div = Document.get().createDivElement();
@@ -315,4 +326,11 @@
private void assertEquals(HasCssName enumValue, String cssValue) {
assertEquals(enumValue.getCssName(), cssValue);
}
+
+ /**
+ * Sets a style property as an int.
+ */
+ private native void setPropertyInt(Style style, String name, int value)
/*-{
+style[name] = value;
+ }-*/;
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Webkit events sometimes target the text node inside of an element instead of the element itself.... (issue1704803)
Reviewers: rdayal,
Description:
Webkit events sometimes target the text node inside of an element
instead of the element itself. This can break GWT code that checks if
the event target is an Element (PopupPanel does this for example). We
now get the parent element if the event target is a text node.
Issue: 6704
Author: dunhamsteve
Reviewer: jlabanca
Please review this at http://gwt-code-reviews.appspot.com/1704803/
Affected files:
M user/src/com/google/gwt/dom/client/DOMImplWebkit.java
Index: user/src/com/google/gwt/dom/client/DOMImplWebkit.java
===
--- user/src/com/google/gwt/dom/client/DOMImplWebkit.java (revision 10958)
+++ user/src/com/google/gwt/dom/client/DOMImplWebkit.java (working copy)
@@ -37,6 +37,19 @@
}-*/;
/**
+ * Webkit events sometimes target the text node inside of the element
instead
+ * of the element itself, so we need to get the parent of the text node.
+ */
+ @Override
+ public native EventTarget eventGetTarget(NativeEvent evt) /*-{
+var target = evt.target;
+if (target target.nodeType == 3) {
+ target = target.parentNode;
+}
+return target;
+ }-*/;
+
+ /**
* Webkit based browsers require that we set the webkit-user-drag style
* attribute to make an element draggable.
*/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Webkit events sometimes target the text node inside of an element instead of the element itself.... (issue1704803)
committed as r10965 http://gwt-code-reviews.appspot.com/1704803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Adding a method to DockLayoutPanel to get the size of a child widget. (issue1705803)
Reviewers: rdayal,
Description:
Adding a method to DockLayoutPanel to get the size of a child widget.
Issue: 4613
Author: tuckerpmt
Reviewer: jlabanca
Please review this at http://gwt-code-reviews.appspot.com/1705803/
Affected files:
M user/src/com/google/gwt/user/client/ui/DockLayoutPanel.java
M user/test/com/google/gwt/user/client/ui/DockLayoutPanelTest.java
Index: user/src/com/google/gwt/user/client/ui/DockLayoutPanel.java
===
--- user/src/com/google/gwt/user/client/ui/DockLayoutPanel.java (revision
10965)
+++ user/src/com/google/gwt/user/client/ui/DockLayoutPanel.java (working
copy)
@@ -283,6 +283,7 @@
* @param child the widget to be queried
* @return the widget's layout direction, or codenull/code if it is
not a
* child of this panel
+ * @throws AssertionError if the widget is not a child and assertions
are enabled
*/
public Direction getWidgetDirection(Widget child) {
assertIsChild(child);
@@ -290,6 +291,22 @@
return null;
}
return ((LayoutData) child.getLayoutData()).direction;
+ }
+
+ /**
+ * Gets the layout size of the given child widget.
+ *
+ * @param child the widget to be queried
+ * @return the widget's layout size, or codenull/code if it is not a
child of
+ * this panel
+ * @throws AssertionError if the widget is not a child and assertions
are enabled
+ */
+ public Double getWidgetSize(Widget child) {
+assertIsChild(child);
+if (child.getParent() != this) {
+ return null;
+}
+return ((LayoutData) child.getLayoutData()).size;
}
/**
Index: user/test/com/google/gwt/user/client/ui/DockLayoutPanelTest.java
===
--- user/test/com/google/gwt/user/client/ui/DockLayoutPanelTest.java
(revision 10965)
+++ user/test/com/google/gwt/user/client/ui/DockLayoutPanelTest.java
(working copy)
@@ -160,6 +160,13 @@
assertPhysicalPaternity(panel, widget);
}
+ public void testGetWidgetSize() {
+DockLayoutPanel panel = createDockLayoutPanel();
+Widget widget = new Label();
+panel.addEast(widget, 123.4);
+assertEquals(123.4, panel.getWidgetSize(widget), 0.1);
+ }
+
public void testInsertLineEnd() {
DockLayoutPanel panel = createDockLayoutPanel();
Widget widget = new Label();
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding a method to DockLayoutPanel to get the size of a child widget. (issue1705803)
committed as r10966 http://gwt-code-reviews.appspot.com/1705803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixing a Chrome animation bug where animations never stop running. The timestamp that webkitReq... (issue1702803)
Reviewers: rdayal,
Description:
Fixing a Chrome animation bug where animations never stop running. The
timestamp that webkitRequestAnimationFrame passes to the callback is not
the number of milliseconds since the epoch, which is what the spec calls
for and what Chrome used to pass. Instead of worrying what the time
argument represents, we now just grab the current time from javascript
and pass it to the animation callback. We already used this workaround
for Chrome 10-, so it is a minor change.
Review by: rda...@google.com
Please review this at http://gwt-code-reviews.appspot.com/1702803/
Affected files:
M
user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
Index:
user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
===
---
user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
(revision 10958)
+++
user/src/com/google/gwt/animation/client/AnimationSchedulerImplWebkit.java
(working copy)
@@ -64,8 +64,9 @@
private native double requestAnimationFrameImpl(AnimationCallback
callback, Element element) /*-{
var _callback = callback;
var wrapper = $entry(function(time) {
- // Chrome 10 does not pass the 'time' argument, so we fake it.
- time = time ||
@com.google.gwt.core.client.Duration::currentTimeMillis()();
+ // The time argument in Chrome does not represent milliseconds since
epoch,
+ // so use the current time instead.
+ time = @com.google.gwt.core.client.Duration::currentTimeMillis()();
_callba...@com.google.gwt.animation.client.AnimationScheduler.AnimationCallback::execute(D)(time);
});
return $wnd.webkitRequestAnimationFrame(wrapper, element);
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Patch for http://code.google.com/p/google-web-toolkit/issues/detail?id=6704 (issue1684803)
On 2012/04/13 00:38:13, Jim Douglas wrote: LGTM Thanks for the patch, I'll submit it tomorrow. http://gwt-code-reviews.appspot.com/1684803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added OrderedMultiSelectionModel that retains the order of selection (issue1674803)
LGTM I recommended a bit of extra javadoc, but code looks good. http://gwt-code-reviews.appspot.com/1674803/diff/4001/user/src/com/google/gwt/view/client/OrderedMultiSelectionModel.java File user/src/com/google/gwt/view/client/OrderedMultiSelectionModel.java (right): http://gwt-code-reviews.appspot.com/1674803/diff/4001/user/src/com/google/gwt/view/client/OrderedMultiSelectionModel.java#newcode26 user/src/com/google/gwt/view/client/OrderedMultiSelectionModel.java:26: * retains order of selection. Add this to the JavaDoc. Normally I don't think its necessary to include implementation details, but in this case its important to understand the cost of using this class. OrderedMultiSelectionModel uses LinkedHashMaps, which may increase the size of your compiled output if you do not use LinkedHashMaps elsewhere in your application. http://gwt-code-reviews.appspot.com/1674803/diff/4001/user/src/com/google/gwt/view/client/OrderedMultiSelectionModel.java#newcode52 user/src/com/google/gwt/view/client/OrderedMultiSelectionModel.java:52: * @return the list of selected items in the order of additions Can you define how setting existing items is handled? It looks like setSelected calls Map.setSelected(), which I assume moves the selected item to the end of the list. http://gwt-code-reviews.appspot.com/1674803/diff/4001/user/test/com/google/gwt/view/client/OrderedMultiSelectionModelTest.java File user/test/com/google/gwt/view/client/OrderedMultiSelectionModelTest.java (right): http://gwt-code-reviews.appspot.com/1674803/diff/4001/user/test/com/google/gwt/view/client/OrderedMultiSelectionModelTest.java#newcode43 user/test/com/google/gwt/view/client/OrderedMultiSelectionModelTest.java:43: Right here, select test0 (which is already selected), and verify that it moves to the end of the list. http://gwt-code-reviews.appspot.com/1674803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added OrderedMultiSelectionModel that retains the order of selection (issue1674803)
LGTM http://gwt-code-reviews.appspot.com/1674803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix PopupPanel.center when content has changed (issue1573803)
LGTM http://gwt-code-reviews.appspot.com/1573803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding a spot for database column name in Column. Allows us to create an updated SQL statement w... (issue1503806)
Leave it in. We've already spent way too long debating a simple string field. I don't feel strongly enough that it should be removed. http://gwt-code-reviews.appspot.com/1503806/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Switched Map implementaion in MultiSelectionModel to LinkedHashMap. This way it retains the (issue1674803)
Can you also add a test case. At the least, select a few values before
calling getSelectedList() and make sure that the values are returned in
the correct order.
http://gwt-code-reviews.appspot.com/1674803/diff/1/user/src/com/google/gwt/view/client/MultiSelectionModel.java
File user/src/com/google/gwt/view/client/MultiSelectionModel.java
(right):
http://gwt-code-reviews.appspot.com/1674803/diff/1/user/src/com/google/gwt/view/client/MultiSelectionModel.java#newcode36
user/src/com/google/gwt/view/client/MultiSelectionModel.java:36: private
final MapObject, T selectedSet = new LinkedHashMapObject, T();
This will include GWT's emulated LinkedHashMap code into the compiled
app, which is a significant amount of code. We generally stick to the
basic HashMap, Hashset, ArrayList collections throughout GWT to avoid
including code associated with the different types of collections.
Can we create an OrderedMultiSelectionModel subclass instead? That way,
only users who need the functionality will pay the cost.
http://gwt-code-reviews.appspot.com/1674803/diff/1/user/src/com/google/gwt/view/client/MultiSelectionModel.java#newcode38
user/src/com/google/gwt/view/client/MultiSelectionModel.java:38: private
final MapT, Boolean selectionChanges = new HashMapT, Boolean();
selectionChanges has to be a LinkedHashMap as well, or multiple pending
changes will be applied in an undefined order when resolveChanges() is
called.
http://gwt-code-reviews.appspot.com/1674803/diff/1/user/src/com/google/gwt/view/client/MultiSelectionModel.java#newcode81
user/src/com/google/gwt/view/client/MultiSelectionModel.java:81: public
ListT getSelectedList() {
I think this would make more sense in an OrderMultiSelectionModel
subclass. Imposing an API restriction that MultiSelectionModel will
retain the order of items would make it tough for a user to subclass and
change the implementation.
http://gwt-code-reviews.appspot.com/1674803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added new SafeImageCell class wich uses SafeUris instead of Strings. (issue1673803)
LGTM http://gwt-code-reviews.appspot.com/1673803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds support for aria-hidden to UIObject.setVisible(). (issue1671803)
http://gwt-code-reviews.appspot.com/1671803/diff/1/user/src/com/google/gwt/user/client/ui/UIObject.java
File user/src/com/google/gwt/user/client/ui/UIObject.java (right):
http://gwt-code-reviews.appspot.com/1671803/diff/1/user/src/com/google/gwt/user/client/ui/UIObject.java#newcode246
user/src/com/google/gwt/user/client/ui/UIObject.java:246: * {@code
setVisible(elem, true)}.
I don't think the last sentence is accurate (but I could be wrong_.
Setting the style to '' should clear the display: none in CSS.
http://gwt-code-reviews.appspot.com/1671803/diff/1/user/test/com/google/gwt/user/client/ui/UIObjectTest.java
File user/test/com/google/gwt/user/client/ui/UIObjectTest.java (right):
http://gwt-code-reviews.appspot.com/1671803/diff/1/user/test/com/google/gwt/user/client/ui/UIObjectTest.java#newcode228
user/test/com/google/gwt/user/client/ui/UIObjectTest.java:228: public
void testIsVisible_displayNone() {
This test is the same as the one above. And its name indicates you
meant to do what you did in testIsVisible_hidden
http://gwt-code-reviews.appspot.com/1671803/diff/1/user/test/com/google/gwt/user/client/ui/UIObjectTest.java#newcode242
user/test/com/google/gwt/user/client/ui/UIObjectTest.java:242:
State.HIDDEN.set(elem, false);
If you are testing that you ignore the aria attribute, shouldn't this be
State.HIDDEN.set(elem, true)
http://gwt-code-reviews.appspot.com/1671803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds support for aria-hidden to UIObject.setVisible(). (issue1671803)
LGTM
http://gwt-code-reviews.appspot.com/1671803/diff/1/user/src/com/google/gwt/user/client/ui/UIObject.java
File user/src/com/google/gwt/user/client/ui/UIObject.java (right):
http://gwt-code-reviews.appspot.com/1671803/diff/1/user/src/com/google/gwt/user/client/ui/UIObject.java#newcode246
user/src/com/google/gwt/user/client/ui/UIObject.java:246: * {@code
setVisible(elem, true)}.
Not what I expected, but thanks for verifying this.
http://gwt-code-reviews.appspot.com/1671803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Deprecating TreeItem.addItem/insertItem(String html) in favor of methods that take SafeHtml. Th... (issue1666803)
committed as r10920 http://gwt-code-reviews.appspot.com/1666803/diff/1/user/test/com/google/gwt/user/client/ui/TreeTest.java File user/test/com/google/gwt/user/client/ui/TreeTest.java (right): http://gwt-code-reviews.appspot.com/1666803/diff/1/user/test/com/google/gwt/user/client/ui/TreeTest.java#newcode187 user/test/com/google/gwt/user/client/ui/TreeTest.java:187: // Normalize the html for ancient safari 3 On 2012/03/20 16:32:36, jat wrote: Could this comment be clearer, like Safari 3 leaves in the HTML. Done. http://gwt-code-reviews.appspot.com/1666803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Deprecating TreeItem.addItem/insertItem(String html) in favor of methods that take SafeHtml. Th... (issue1666803)
Reviewers: skybrian, Description: Deprecating TreeItem.addItem/insertItem(String html) in favor of methods that take SafeHtml. The current versions are misleading because they claim to take itemText but actually set innerHTML. The javadoc of the deprecated methods has been updated to indicate that they take HTML, not text. Please review this at http://gwt-code-reviews.appspot.com/1666803/ Affected files: M user/src/com/google/gwt/user/client/ui/Tree.java M user/src/com/google/gwt/user/client/ui/TreeItem.java M user/test/com/google/gwt/user/client/ui/TreeItemTest.java M user/test/com/google/gwt/user/client/ui/TreeTest.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix left/right mixup in SimplePager (issue1617804)
committed as r10907 http://gwt-code-reviews.appspot.com/1617804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introduced PagerFactory to allow easy swap of pagers for CellBrowser. (issue1659803)
LGTM http://gwt-code-reviews.appspot.com/1659803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introduced PagerFactory to allow easy swap of pagers for CellBrowser. (issue1659803)
http://gwt-code-reviews.appspot.com/1659803/diff/1/user/src/com/google/gwt/user/cellview/client/CellBrowser.java File user/src/com/google/gwt/user/cellview/client/CellBrowser.java (right): http://gwt-code-reviews.appspot.com/1659803/diff/1/user/src/com/google/gwt/user/cellview/client/CellBrowser.java#newcode69 user/src/com/google/gwt/user/cellview/client/CellBrowser.java:69: import javax.annotation.Nullable; Probably not if it isn't part of the standard Java library. @rkj - can you verify that the compiler doesn't warn/error about Nullable, and remove it if it does? http://gwt-code-reviews.appspot.com/1659803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introduced Builder pattern for CellBrowser creation. (issue1658803)
LGTM http://gwt-code-reviews.appspot.com/1658803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add cursor: pointer for Hyperlink (issue1615808)
committed as r10894 http://gwt-code-reviews.appspot.com/1615808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add layout.onAttach/onDetach calls (issue1615804)
Fixed as r10895 http://gwt-code-reviews.appspot.com/1615804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixed issue 1394 : Need a new getSplitter() method in SplitPanel (issue1398801)
committed as r10896 http://gwt-code-reviews.appspot.com/1398801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixing a bug in CellWidget where the cell isn't rendered if the initial value is null. We were ... (issue1655803)
committed as r10897 http://gwt-code-reviews.appspot.com/1655803/diff/1/user/src/com/google/gwt/user/cellview/client/CellWidget.java File user/src/com/google/gwt/user/cellview/client/CellWidget.java (right): http://gwt-code-reviews.appspot.com/1655803/diff/1/user/src/com/google/gwt/user/cellview/client/CellWidget.java#newcode153 user/src/com/google/gwt/user/cellview/client/CellWidget.java:153: setValueWithoutEqualityCheck(initialValue, false, true); On 2012/03/06 18:58:32, skybrian wrote: Nit: I think it would be more straightforward to inline this, since it's just two if statements. Done. http://gwt-code-reviews.appspot.com/1655803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Improving the JavaDoc of ListDataProvider to explain that the wrapped list should not be modifie... (issue1656803)
committed as r10898 http://gwt-code-reviews.appspot.com/1656803/diff/1/user/src/com/google/gwt/view/client/ListDataProvider.java File user/src/com/google/gwt/view/client/ListDataProvider.java (right): http://gwt-code-reviews.appspot.com/1656803/diff/1/user/src/com/google/gwt/view/client/ListDataProvider.java#newcode574 user/src/com/google/gwt/view/client/ListDataProvider.java:574: * more optimal because the data provider knows which rows were modified and On 2012/03/06 19:43:08, skybrian wrote: nit: s/is more optimal/performs better/ (and elsewhere) Done. http://gwt-code-reviews.appspot.com/1656803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Added a method to allow 'snap closed' behavior in SplitLayoutPanel. (issue1657803)
LGTM http://gwt-code-reviews.appspot.com/1657803/diff/1/user/src/com/google/gwt/user/client/ui/SplitLayoutPanel.java File user/src/com/google/gwt/user/client/ui/SplitLayoutPanel.java (right): http://gwt-code-reviews.appspot.com/1657803/diff/1/user/src/com/google/gwt/user/client/ui/SplitLayoutPanel.java#newcode414 user/src/com/google/gwt/user/client/ui/SplitLayoutPanel.java:414: * @param snapClosedSize the width below which the widget will close , or -1 to disable http://gwt-code-reviews.appspot.com/1657803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixing a bug in CellWidget where the cell isn't rendered if the initial value is null. We were ... (issue1655803)
Reviewers: skybrian,
Description:
Fixing a bug in CellWidget where the cell isn't rendered if the initial
value is null. We were checking that the value actually changed in
setValue(), but this.value defaults to null. We no longer do an
equality check when setting the value in the constructor.
Please review this at http://gwt-code-reviews.appspot.com/1655803/
Affected files:
M user/src/com/google/gwt/user/cellview/client/CellWidget.java
M user/test/com/google/gwt/user/cellview/client/CellWidgetTest.java
Index: user/src/com/google/gwt/user/cellview/client/CellWidget.java
===
--- user/src/com/google/gwt/user/cellview/client/CellWidget.java (revision
10862)
+++ user/src/com/google/gwt/user/cellview/client/CellWidget.java (working
copy)
@@ -79,6 +79,7 @@
* The {@link ValueUpdater} used to trigger value update events.
*/
private final ValueUpdaterC valueUpdater = new ValueUpdaterC() {
+@Override
public void update(C value) {
// no need to redraw, the Cell took care of it
setValue(value, true, false);
@@ -143,13 +144,21 @@
this.keyProvider = keyProvider;
setElement(elem);
CellBasedWidgetImpl.get().sinkEvents(this, cell.getConsumedEvents());
-setValue(initialValue);
- }
-
+
+/*
+ * Skip the equality check. If initialValue is null, it will be equal
to
+ * this.value, but that doesn't mean that we don't want to render the
+ * initialValue.
+ */
+setValueWithoutEqualityCheck(initialValue, false, true);
+ }
+
+ @Override
public HandlerRegistration addValueChangeHandler(ValueChangeHandlerC
handler) {
return addHandler(handler, ValueChangeEvent.getType());
}
+ @Override
public LeafValueEditorC asEditor() {
if (editor == null) {
editor = TakesValueEditor.of(this);
@@ -166,10 +175,12 @@
return cell;
}
+ @Override
public ProvidesKeyC getKeyProvider() {
return keyProvider;
}
+ @Override
public C getValue() {
return value;
}
@@ -213,6 +224,7 @@
* existing value.
* /p
*/
+ @Override
public void setValue(C value) {
setValue(value, false, true);
}
@@ -224,6 +236,7 @@
* existing value.
* /p
*/
+ @Override
public void setValue(C value, boolean fireEvents) {
setValue(value, fireEvents, true);
}
@@ -242,13 +255,7 @@
public void setValue(C value, boolean fireEvents, boolean redraw) {
C oldValue = getValue();
if (value != oldValue (oldValue == null |
| !oldValue.equals(value))) {
- this.value = value;
- if (redraw) {
-redraw();
- }
- if (fireEvents) {
-ValueChangeEvent.fire(this, value);
- }
+ setValueWithoutEqualityCheck(value, fireEvents, redraw);
}
}
@@ -268,4 +275,21 @@
private Object getKey(C value) {
return (keyProvider == null || value == null) ? value :
keyProvider.getKey(value);
}
+
+ /**
+ * Sets this object's value without checking for equality.
+ *
+ * @param value the object's new value
+ * @param fireEvents fire events if true and value is new
+ * @param redraw redraw the widget if true and value is new
+ */
+ private void setValueWithoutEqualityCheck(C value, boolean fireEvents,
boolean redraw) {
+this.value = value;
+if (redraw) {
+ redraw();
+}
+if (fireEvents) {
+ ValueChangeEvent.fire(this, value);
+}
+ }
}
\ No newline at end of file
Index: user/test/com/google/gwt/user/cellview/client/CellWidgetTest.java
===
--- user/test/com/google/gwt/user/cellview/client/CellWidgetTest.java
(revision 10862)
+++ user/test/com/google/gwt/user/cellview/client/CellWidgetTest.java
(working copy)
@@ -42,6 +42,7 @@
private String lastEventValue;
private Object lastEventKey;
+private String lastRenderedValue = never_rendered;
public CustomCell() {
super(change);
@@ -55,6 +56,11 @@
public void assertLastEventValue(String expected) {
assertEquals(expected, lastEventValue);
lastEventValue = null;
+}
+
+public void assertLastRenderedValue(String expected) {
+ assertEquals(expected, lastRenderedValue);
+ lastRenderedValue = null;
}
@Override
@@ -69,6 +75,7 @@
@Override
public void render(Context context, String value, SafeHtmlBuilder sb) {
+ lastRenderedValue = value;
if (value != null) {
sb.appendEscaped(value);
}
@@ -96,6 +103,7 @@
onValueChangeCalled = false;
}
+@Override
public void onValueChange(ValueChangeEventC event) {
assertFalse(ValueChangeEvent fired twice, onValueChangeCalled);
onValueChangeCalled = true;
@@ -106,6 +114,16 @@
@Override
public String getModuleName() {
return com.google.gwt.user.cellview.CellView;
+ }
+
+ /**
+ * Tests that the cell widget will render correctly with an initial
[gwt-contrib] Improving the JavaDoc of ListDataProvider to explain that the wrapped list should not be modifie... (issue1656803)
Reviewers: skybrian, Description: Improving the JavaDoc of ListDataProvider to explain that the wrapped list should not be modified, and that mutations to the items within the list cannot be detected without called List#set() or ListDataProvider#refresh(). Issue: 7114 Review by: skybr...@google.com Please review this at http://gwt-code-reviews.appspot.com/1656803/ Affected files: M user/src/com/google/gwt/view/client/ListDataProvider.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Remapping the tabindex ARIA attribute to tabIndex because browsers implement the latter, eve... (issue1651803)
Reviewers: atincheva,
Description:
Remapping the tabindex ARIA attribute to tabIndex because browsers
implement the latter, even though the ARIA spec specifies the former.
Please review this at http://gwt-code-reviews.appspot.com/1651803/
Affected files:
M user/src/com/google/gwt/aria/client/ExtraAttribute.java
M user/test/com/google/gwt/aria/client/RoleTest.java
Index: user/src/com/google/gwt/aria/client/ExtraAttribute.java
===
--- user/src/com/google/gwt/aria/client/ExtraAttribute.java (revision 10862)
+++ user/src/com/google/gwt/aria/client/ExtraAttribute.java (working copy)
@@ -23,7 +23,7 @@
*/
public final class ExtraAttributeT extends AttributeT {
public static final ExtraAttributeInteger TABINDEX =
- new ExtraAttributeInteger(tabindex, );
+ new ExtraAttributeInteger(tabIndex, );
public ExtraAttribute(String name) {
Index: user/test/com/google/gwt/aria/client/RoleTest.java
===
--- user/test/com/google/gwt/aria/client/RoleTest.java (revision 10862)
+++ user/test/com/google/gwt/aria/client/RoleTest.java (working copy)
@@ -17,6 +17,7 @@
import com.google.gwt.aria.client.CommonAttributeTypes.IdReferenceList;
import com.google.gwt.aria.client.PropertyTokenTypes.DropeffectToken;
import com.google.gwt.aria.client.PropertyTokenTypes.DropeffectTokenList;
+import com.google.gwt.dom.client.AnchorElement;
import com.google.gwt.dom.client.Document;
import com.google.gwt.dom.client.Element;
import com.google.gwt.junit.client.GWTTestCase;
@@ -65,15 +66,22 @@
}
public void testSetGetRemoveExtraAttributes() {
+// Older versions of IE do not support tabIndex on divs, so use an
anchor
+// element instead.
+AnchorElement anchor = Document.get().createAnchorElement();
+Document.get().getBody().appendChild(anchor);
+
// Some versions of IE default to 0 instead of
assertTrue(.equals(regionRole.getTabindexExtraAttribute(div))
|| 0.equals(regionRole.getTabindexExtraAttribute(div)));
-regionRole.setTabindexExtraAttribute(div, 1);
-assertEquals(1, regionRole.getTabindexExtraAttribute(div));
-regionRole.removeTabindexExtraAttribute(div);
+regionRole.setTabindexExtraAttribute(anchor, 1);
+assertEquals(1, regionRole.getTabindexExtraAttribute(anchor));
+regionRole.removeTabindexExtraAttribute(anchor);
// Some versions of IE default to 0 instead of
assertTrue(.equals(regionRole.getTabindexExtraAttribute(div))
|| 0.equals(regionRole.getTabindexExtraAttribute(div)));
+
+anchor.removeFromParent();
}
@Override
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixed issue 1394 : Need a new getSplitter() method in SplitPanel (issue1398801)
Sounds reasonable to me. http://gwt-code-reviews.appspot.com/1398801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Prune dead code. (issue1627803)
LGTM http://gwt-code-reviews.appspot.com/1627803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: ColumnSortList can grow indefintely, but some users would like to have only a (issue1625803)
LGTM http://gwt-code-reviews.appspot.com/1625803/diff/1/user/src/com/google/gwt/user/cellview/client/ColumnSortList.java File user/src/com/google/gwt/user/cellview/client/ColumnSortList.java (right): http://gwt-code-reviews.appspot.com/1625803/diff/1/user/src/com/google/gwt/user/cellview/client/ColumnSortList.java#newcode314 user/src/com/google/gwt/user/cellview/client/ColumnSortList.java:314: spaces http://gwt-code-reviews.appspot.com/1625803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Defines API containing definition of ARIA attributes as defined by the W3C ARIA (issue1624803)
http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/AttributeValueType.java
File user/src/com/google/gwt/user/client/ui/aria/AttributeValueType.java
(right):
http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/AttributeValueType.java#newcode67
user/src/com/google/gwt/user/client/ui/aria/AttributeValueType.java:67:
+ value.getClass().getName();
Try compiling with and without this line and see if it makes a
significant difference. Or ask one of the GWT compiler guys (I'm not
sure who though).
http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/Roles.java
File user/src/com/google/gwt/user/client/ui/aria/Roles.java (right):
http://gwt-code-reviews.appspot.com/1624803/diff/1/user/src/com/google/gwt/user/client/ui/aria/Roles.java#newcode61
user/src/com/google/gwt/user/client/ui/aria/Roles.java:61: public final
class Roles {
The benefit of using interfaces is that each interface has
getters/setters only for the properties/states that it supports, so
there aren't any runtime checks. Instead of calling
Roles.BUTTON.setProperty(ACTIVE_DESC, my desc) and possible throwing a
Runtime exception if the property isn't supported, you would call
ButtonRole.setActiveDesc(...).
The interfaces would mirror the hierarchy or roles. Then, you can
generate Impls for the concrete Roles and factory methods to access
them. It would look like this:
Roles.getButtonRole().setActiveDesc(my description);
That would mean some overlap in the impls because setActiveDesc might
be present in more than one concrete Role. But if you generate the code
from the interfaces, it wouldn't matter.
In its current state, this Roles file is very complicated and we can
easily introduce a bug if we try to edit it manually. It would be best
if we could generate a set of interfaces so that when new ARIA features
are added, we could just regenerate them. Or at least we could generate
the code using a GWT generate that builds the concrete classes from the
interfaces.
http://gwt-code-reviews.appspot.com/1624803/diff/6001/user/src/com/google/gwt/aria/Role.java
File user/src/com/google/gwt/aria/Role.java (right):
http://gwt-code-reviews.appspot.com/1624803/diff/6001/user/src/com/google/gwt/aria/Role.java#newcode53
user/src/com/google/gwt/aria/Role.java:53: * @param parentRoles Parent
roles. One role can inherit from one or more patent roles
/more patent roles/more parent roles
http://gwt-code-reviews.appspot.com/1624803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Defines API containing definition of ARIA attributes as defined by the W3C ARIA (issue1624803)
The main reason is to allow the ARIA library to be used independent of the widget library, in case you write an app the uses the low level DOM library but not widgets. We've been moving away from using client.ui as a catch-all for all UI related things in favor or segregated packages for different features. http://gwt-code-reviews.appspot.com/1624803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Add cursor: pointer for Hyperlink (issue1615808)
LGTM http://gwt-code-reviews.appspot.com/1615808/ -- 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)
committed as r10806 http://gwt-code-reviews.appspot.com/1618804/ -- 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)
committed as r10804 http://gwt-code-reviews.appspot.com/1616804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix left/right mixup in SimplePager (issue1617804)
LGTM Yeah, the whole left/right thing. http://gwt-code-reviews.appspot.com/1617804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add layout.onAttach/onDetach calls (issue1615804)
LGTM
Minor nit, but no need to upload another patch. I'll switch to
onAttach/onDetach before submitting.
http://gwt-code-reviews.appspot.com/1615804/diff/1/user/src/com/google/gwt/user/client/ui/DeckLayoutPanel.java
File user/src/com/google/gwt/user/client/ui/DeckLayoutPanel.java
(right):
http://gwt-code-reviews.appspot.com/1615804/diff/1/user/src/com/google/gwt/user/client/ui/DeckLayoutPanel.java#newcode201
user/src/com/google/gwt/user/client/ui/DeckLayoutPanel.java:201: public
void onLoad() {
These should probably go in onAttach/onDetach instead of
onLoad/onUnload. Users may not call super.onLoad/onUnload, but if you
override onAttach/onDetach you must call the super methods.
http://gwt-code-reviews.appspot.com/1615804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Passing the row index to the FieldUpdater for a CompositeCell instead of passing -1 all the time. (issue1618803)
Reviewers: rdayal,
Description:
Passing the row index to the FieldUpdater for a CompositeCell instead of
passing -1 all the time.
Issue: 7050
Please review this at http://gwt-code-reviews.appspot.com/1618803/
Affected files:
M user/src/com/google/gwt/cell/client/CompositeCell.java
Index: user/src/com/google/gwt/cell/client/CompositeCell.java
===
--- user/src/com/google/gwt/cell/client/CompositeCell.java (revision 10802)
+++ user/src/com/google/gwt/cell/client/CompositeCell.java (working copy)
@@ -216,15 +216,16 @@
return hasCell.getCell().isEditing(context, cellParent,
hasCell.getValue(object));
}
- private X void onBrowserEventImpl(Context context, Element parent,
+ private X void onBrowserEventImpl(final Context context, Element
parent,
final C object, NativeEvent event, final ValueUpdaterC
valueUpdater,
final HasCellC, X hasCell) {
ValueUpdaterX tempUpdater = null;
final FieldUpdaterC, X fieldUpdater = hasCell.getFieldUpdater();
if (fieldUpdater != null) {
tempUpdater = new ValueUpdaterX() {
+@Override
public void update(X value) {
- fieldUpdater.update(-1, object, value);
+ fieldUpdater.update(context.getIndex(), object, value);
if (valueUpdater != null) {
valueUpdater.update(object);
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Switch more gwt-user code to use SafeHtml. (issue1614803)
http://gwt-code-reviews.appspot.com/1614803/diff/8/user/src/com/google/gwt/user/client/DOM.java
File user/src/com/google/gwt/user/client/DOM.java (right):
http://gwt-code-reviews.appspot.com/1614803/diff/8/user/src/com/google/gwt/user/client/DOM.java#newcode1192
user/src/com/google/gwt/user/client/DOM.java:1192: public static void
setInnerHTML(com.google.gwt.dom.client.Element elem, SafeHtml html) {
AFAIK, adding this method is a breaking change for anyone who passes
null as the HTML value because the compiler cannot determine which
method to use (ambiguous call). The implementation of
Element.setInnerHTML() specifically supports null to clear the value, so
its reasonable to expect that some users are using it.
I suggest renaming this to setInnerSafeHtml(), and moving the method
from DOM to Element.
http://gwt-code-reviews.appspot.com/1614803/diff/8/user/src/com/google/gwt/user/client/ui/FormPanel.java
File user/src/com/google/gwt/user/client/ui/FormPanel.java (right):
http://gwt-code-reviews.appspot.com/1614803/diff/8/user/src/com/google/gwt/user/client/ui/FormPanel.java#newcode616
user/src/com/google/gwt/user/client/ui/FormPanel.java:616:
DOM.setInnerHTML(dummy, IFrameTemplate.INSTANCE.get(frameName));
I'm not a fan of switching all of the elem.setInnerHTML() calls to
DOM.setInnerHTML() calls. DOM is really an implementation class, and I
don't think we should encourage people to use it.
Instead, can you add Element.setInnerSafeHtml(SafeHtml) to Element, and
delegate to Element.setInnerHTML(String)?
http://gwt-code-reviews.appspot.com/1614803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Switch more gwt-user code to use SafeHtml. (issue1614803)
LGTM http://gwt-code-reviews.appspot.com/1614803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix SafeHtml API misuse (calling SafeHtmlUtils.fromSafeConstant(String) (issue1611804)
LGTM http://gwt-code-reviews.appspot.com/1611804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix typo in SafeStylesUtils.forZIndex. (issue1603803)
committed as r10795 http://gwt-code-reviews.appspot.com/1603803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Don't allow SafeHtml strings to end in a script or style context. (issue1608803)
LGTM http://gwt-code-reviews.appspot.com/1608803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding jscomp to the classpath for gwt user and dev projects. (issue1606803)
committed as r10779 http://gwt-code-reviews.appspot.com/1606803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Change SafeHtmlHostedModeUtils.isCompleteHtml() to public. (issue1606804)
LGTM Just reorder the method alphabetically within the other public methods. In GWT, we sort by visibility first, then alphabetically. http://gwt-code-reviews.appspot.com/1606804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introduce a new method onPreviewColumnSortEvent to Header. An AbstractCellTable checks this meth... (issue1605804)
LGTM http://gwt-code-reviews.appspot.com/1605804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Adding jscomp to the classpath for gwt user and dev projects. (issue1606803)
Reviewers: cromwellian, Description: Adding jscomp to the classpath for gwt user and dev projects. Review by: cromwell...@google.com Please review this at http://gwt-code-reviews.appspot.com/1606803/ Affected files: M eclipse/dev/.classpath M eclipse/user/.classpath Index: eclipse/dev/.classpath === --- eclipse/dev/.classpath (revision 10774) +++ eclipse/dev/.classpath (working copy) @@ -36,9 +36,10 @@ classpathentry kind=var path=GWT_TOOLS/lib/tomcat/tomcat-http11-1.0.jar sourcepath=/GWT_TOOLS/lib/tomcat/tomcat-http11-1.0.jar/ classpathentry kind=var path=GWT_TOOLS/lib/tomcat/tomcat-jk2-2.1.jar/ classpathentry kind=var path=GWT_TOOLS/lib/tomcat/tomcat-util-5.1.jar/ -classpathentry kind=var path=GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-core-js-2.9.jar sourcepath=/GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-core-js-2.9-sources.jar/ -classpathentry kind=var path=GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-2.9.jar sourcepath=/GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-2.9-sources.jar/ + classpathentry kind=var path=GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-core-js-2.9.jar sourcepath=/GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-core-js-2.9-sources.jar/ + classpathentry kind=var path=GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-2.9.jar sourcepath=/GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-2.9-sources.jar/ classpathentry kind=var path=GWT_TOOLS/lib/protobuf/protobuf-2.2.0/protobuf-java-rebased-2.2.0.jar/ classpathentry kind=var path=GWT_TOOLS/lib/guava/guava-r06/guava-r06-rebased.jar/ + classpathentry kind=var path=GWT_TOOLS/lib/jscomp/sourcemap-rebased.jar/ classpathentry kind=output path=bin/ /classpath Index: eclipse/user/.classpath === --- eclipse/user/.classpath (revision 10774) +++ eclipse/user/.classpath (working copy) @@ -33,8 +33,8 @@ classpathentry kind=var path=GWT_TOOLS/lib/easymock/easymock-3.0.jar/ classpathentry kind=var path=GWT_TOOLS/lib/objectweb/asm-3.1.jar/ classpathentry combineaccessrules=false kind=src path=/gwt-dev/ -classpathentry kind=var path=GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-2.9.jar sourcepath=/GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-2.9-sources.jar/ -classpathentry kind=var path=GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-core-js-2.9.jar sourcepath=/GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-core-js-2.9-sources.jar/ + classpathentry kind=var path=GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-2.9.jar sourcepath=/GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-2.9-sources.jar/ + classpathentry kind=var path=GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-core-js-2.9.jar sourcepath=/GWT_TOOLS/lib/htmlunit/htmlunit-2.9/htmlunit-core-js-2.9-sources.jar/ classpathentry kind=var path=GWT_TOOLS/redist/json/r2_20080312/json-1.5.jar sourcepath=/GWT_TOOLS/redist/json/r2_20080312/json-src.jar/ classpathentry exported=true kind=var path=GWT_TOOLS/lib/javax/validation/validation-api-1.0.0.GA.jar sourcepath=/GWT_TOOLS/lib/javax/validation/validation-api-1.0.0.GA-sources.jar/ classpathentry exported=true kind=var path=GWT_TOOLS/lib/javax/validation/validation-api-1.0.0.GA-sources.jar/ @@ -59,5 +59,6 @@ classpathentry kind=var path=GWT_TOOLS/lib/jboss/test-harness/jboss-test-harness-api-1.0.0.jar sourcepath=/GWT_TOOLS/lib/jboss/test-harness/jboss-test-harness-api-1.0.0-sources.jar/ classpathentry kind=var path=GWT_TOOLS/lib/testng/testng-5.14.1-sources.jar/ classpathentry kind=var path=GWT_TOOLS/lib/testng/testng-5.14.1-nojunit.jar sourcepath=/GWT_TOOLS/lib/testng/testng-5.14.1-sources.jar/ + classpathentry kind=var path=GWT_TOOLS/lib/jscomp/sourcemap-rebased.jar/ classpathentry kind=output path=bin/ /classpath -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix typo in SafeStylesUtils.forZIndex. (issue1603803)
LGTM http://gwt-code-reviews.appspot.com/1603803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix com.google.gwt.http.client.UrlBuilder to encode the query string using URL.encodeQueryString(). (issue1586804)
LGTM http://gwt-code-reviews.appspot.com/1586804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixing a bug in HasDataPresenter where changes to the temporary state aren't propagated to the n... (issue1585803)
http://gwt-code-reviews.appspot.com/1585803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixing a bug in HasDataPresenter where changes to the temporary state aren't propagated to the n... (issue1585803)
I updated the patch to actually fix the problem and added a test case that fails without the fix but passes with it. I was getting frustrated on Friday and wanted to get something out, but it didn't fix the underlying problem correctly. After thinking about the CellBrowser bug for the weekend, I realized that CellBrowser has to handle selection itself. We can't control selection from within each individual List because they compete with each other. http://gwt-code-reviews.appspot.com/1585803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fixing a bug in HasDataPresenter where changes to the temporary state aren't propagated to the n... (issue1585803)
http://gwt-code-reviews.appspot.com/1585803/diff/4/user/src/com/google/gwt/user/cellview/client/CellBrowser.java
File user/src/com/google/gwt/user/cellview/client/CellBrowser.java
(right):
http://gwt-code-reviews.appspot.com/1585803/diff/4/user/src/com/google/gwt/user/cellview/client/CellBrowser.java#newcode217
user/src/com/google/gwt/user/cellview/client/CellBrowser.java:217: * The
currently selected value in this list.
On 2011/10/31 17:31:39, skybrian wrote:
s/list/tree/
List is correct in this case. This is the inner BrowserCellList class.
We keep track of the selected value in each list so we can deselect it
as needed.
http://gwt-code-reviews.appspot.com/1585803/diff/4/user/src/com/google/gwt/user/cellview/client/CellBrowser.java#newcode1081
user/src/com/google/gwt/user/cellview/client/CellBrowser.java:1081: //
Always use the ENABLED keyboard selection policy within each list. The
On 2011/10/31 17:31:39, skybrian wrote:
Didn't get it the first time. How about:
A CellBrowser has a single selection policy and multiple lists, so
we're not
using the selection policy in each list. Just leave them on all the
time.
Since we're not using it, does it matter whether it's on or off? Also,
can user
code try to change the list's selection policy? It seems like that
would either
have no effect or cause something to break. Maybe it should throw an
exception
to avoid confusion?
We use it to keep track of the selected/open item at each level in the
browser. Users cannot access the individual CellLists, so they can't
change it.
http://gwt-code-reviews.appspot.com/1585803/diff/4/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
File user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
(right):
http://gwt-code-reviews.appspot.com/1585803/diff/4/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java#newcode1062
user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java:1062:
private boolean resolvePendingState(JsArrayInteger modifiedRows) {
On 2011/10/31 17:31:39, skybrian wrote:
It's unclear to me where the potentially recursive method calls are.
It's
probably a good idea to add comments at those method calls warning
about this.
Done.
Also, it's easy to confuse pending versus pendingState. Maybe
there should
be more distinct names for these?
Switching pending to newState, because it will become the state within
this resolvePendingState method.
http://gwt-code-reviews.appspot.com/1585803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Fixing a bug in HasDataPresenter where changes to the temporary state aren't propagated to the n... (issue1585803)
Reviewers: skybrian,
Description:
Fixing a bug in HasDataPresenter where changes to the temporary state
aren't propagated to the new pending state if a selection event
interupts the SelectionModel part of the state resolution loop. The
problem is that we update the new pending state as we go along, but in
most cases the new pending state won't exist until we're halfway through
the SelectionModel resolution code. The fix is to copy all modified
fields from the temporary state into the new pending state at the end of
the selection resolution logic, when we know the new pending state
exists.
Also fixing a bug where the wrong list gets selected in a CellBrowser if
a shared SelectionModel is used across all lists.
Please review this at http://gwt-code-reviews.appspot.com/1585803/
Affected files:
M user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
Index: user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
===
--- user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
(revision 10729)
+++ user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
(working copy)
@@ -1184,7 +1184,7 @@
// Select the new value.
pending.selectedValue = newValue;
-if (newValue != null !newValueWasSelected) {
+if (newValue != null !newValueWasSelected
pending.keyboardSelectedRowChanged) {
selectionModel.setSelected(newValue, true);
}
} else if (!newValueWasSelected) {
@@ -1192,12 +1192,6 @@
pending.selectedValue = null;
}
}
-
-// User code may have created a new pending state, so we need to
-// propagate this new selected value.
-if (pendingState != null) {
- pendingState.selectedValue = pending.selectedValue;
-}
}
} catch (RuntimeException e) {
// Unlock the rendering loop if the user SelectionModel throw an
error.
@@ -1218,6 +1212,7 @@
* longer have a SelectionModel but had selected rows, we still need to
* update the rows.
*/
+SetInteger newlySelectedRows = new HashSetInteger();
try {
for (int i = pageStart; i pageStart + rowDataCount; i++) {
// Check the new selection state.
@@ -1229,10 +1224,7 @@
boolean wasSelected = oldState.isRowSelected(i);
if (isSelected) {
pending.selectedRows.add(i);
- if (pendingState != null) {
-// Propagate changes if user code created a new pending state.
-pendingState.selectedRows.add(i);
- }
+ newlySelectedRows.add(i);
if (!wasSelected) {
modifiedRows.push(i);
}
@@ -1274,6 +1266,14 @@
if (pendingState != null) {
isResolvingState = false;
// Do not reset pendingStateLoop, or we will not detect the infinite
loop.
+
+ // Propagate modifications to the temporary pending state into the
new
+ // pending state instance.
+ pendingState.selectedValue = pending.selectedValue;
+ pendingState.selectedRows.addAll(newlySelectedRows);
+ if (keyboardRowChanged) {
+pendingState.keyboardSelectedRowChanged = true;
+ }
/*
* Add the keyboard selected rows to the modified rows so they can be
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Mofifying HasDataPresenter to be more resilient to state changes while it is resolving state cha... (issue1583804)
Reviewers: dcavalcanti_google.com, Description: Mofifying HasDataPresenter to be more resilient to state changes while it is resolving state changes. For example, if user code triggers a SelectionChangeEvent while selection is being resolved, we can handle that. Review by: dcavalca...@google.com Please review this at http://gwt-code-reviews.appspot.com/1583804/ Affected files: M user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
