Wanted to ask you guys for specific feedback on the fact that I am
changing the phase right before calling the wrapCallback. As I stated
on the comment, I'm doing that because the UiBinder-generated code
calls getElement(), and it must know it doesn't need to trigger the
slow process. I see 2 other options:
- change the wrapCallback to receive an element;
- change the check on getElement to use something other than the phase;
Please let me know if you think either of these is better (that is,
assuming you're happy with the Phase thing at all).
cheers,
rafa
On Wed, May 25, 2011 at 6:53 PM, <rdcas...@google.com> wrote:
> Reviewers: rjrjr, juliog,
>
> Description:
> Change the wrapElement API to receive the id and a parent element. I
> also ported some of the bookeeping we were doing in our internal Panel
> to RenderablePanel, please let me know what you think.
>
>
> Please review this at http://gwt-code-reviews.appspot.com/1446811/
>
> Affected files:
> M
> user/src/com/google/gwt/uibinder/elementparsers/IsRenderableInterpreter.java
> M user/src/com/google/gwt/user/client/ui/IsRenderable.java
> M user/src/com/google/gwt/user/client/ui/RenderableComposite.java
> M user/src/com/google/gwt/user/client/ui/RenderablePanel.java
>
>
> Index:
> user/src/com/google/gwt/uibinder/elementparsers/IsRenderableInterpreter.java
> ===================================================================
> ---
> user/src/com/google/gwt/uibinder/elementparsers/IsRenderableInterpreter.java
> (revision 10224)
> +++
> user/src/com/google/gwt/uibinder/elementparsers/IsRenderableInterpreter.java
> (working copy)
> @@ -29,7 +29,6 @@
> class IsRenderableInterpreter implements XMLElement.Interpreter<String> {
>
> private final String fieldName;
> -
> private final UiBinderWriter uiWriter;
>
> public IsRenderableInterpreter(String fieldName, UiBinderWriter writer) {
> @@ -50,15 +49,11 @@
>
> FieldWriter childFieldWriter = uiWriter.parseElementToFieldWriter(elem);
>
> - String elementPointer = idHolder + "Element";
> fieldWriter.addAttachStatement(
> - "com.google.gwt.user.client.Element %s = " +
> -
> "com.google.gwt.dom.client.Document.get().getElementById(%s).cast();",
> - elementPointer, fieldManager.convertFieldToGetter(idHolder));
> - fieldWriter.addAttachStatement(
> - "%s.wrapElement(%s);",
> + "%s.wrapElement(%s, %s.getElement());",
> fieldManager.convertFieldToGetter(childFieldWriter.getName()),
> - elementPointer);
> + fieldManager.convertFieldToGetter(idHolder),
> + fieldManager.convertFieldToGetter(fieldName));
>
> // Some operations are more efficient when the Widget isn't attached to
> // the document. Perform them here.
> Index: user/src/com/google/gwt/user/client/ui/IsRenderable.java
> ===================================================================
> --- user/src/com/google/gwt/user/client/ui/IsRenderable.java (revision
> 10224)
> +++ user/src/com/google/gwt/user/client/ui/IsRenderable.java (working
> copy)
> @@ -29,11 +29,10 @@
> public interface IsRenderable extends SafeHtmlRenderer<String> {
>
> /**
> - * Replace the previous contents of the receiver with the given element,
> - * presumed to have been created via a previous call to {@link #render}.
> - * Assumes the element is attached to the document.
> + * Tells the widget to find its element, given an id and a parent element
> in
> + * the DOM tree. The id is assumed to be the same passed in at render
> time.
> */
> - void wrapElement(Element element);
> + void wrapElement(String id, Element parentElement);
>
> /**
> * Perform any initialization needed when the widget is not attached to
> Index: user/src/com/google/gwt/user/client/ui/RenderableComposite.java
> ===================================================================
> --- user/src/com/google/gwt/user/client/ui/RenderableComposite.java
> (revision 10224)
> +++ user/src/com/google/gwt/user/client/ui/RenderableComposite.java
> (working copy)
> @@ -16,6 +16,7 @@
> package com.google.gwt.user.client.ui;
>
> import com.google.gwt.core.client.GWT;
> +import com.google.gwt.dom.client.Document;
> import com.google.gwt.dom.client.Element;
> import com.google.gwt.event.logical.shared.AttachEvent;
> import com.google.gwt.safehtml.client.SafeHtmlTemplates;
> @@ -117,12 +118,12 @@
> }
>
> @Override
> - public void wrapElement(Element element) {
> + public void wrapElement(String id, Element parentElement) {
> if (renderable != null) {
> assert (initFinished == false);
> - renderable.wrapElement(element);
> - } else {
> - this.elementToWrap = element;
> + renderable.wrapElement(id, parentElement);
> + } else {
> + this.elementToWrap = Document.get().getElementById(id).cast();
> }
> }
>
> Index: user/src/com/google/gwt/user/client/ui/RenderablePanel.java
> ===================================================================
> --- user/src/com/google/gwt/user/client/ui/RenderablePanel.java (revision
> 10224)
> +++ user/src/com/google/gwt/user/client/ui/RenderablePanel.java (working
> copy)
> @@ -57,12 +57,18 @@
> RootPanel.getBodyElement().appendChild(hiddenDiv);
> }
>
> + protected static enum Phase {
> + BUILT, RENDERED, WRAPPED, DONE
> + }
> +
> // TODO(rdcastro): Add setters for these, or maybe have a list instead of
> a
> // single callback.
> public Command wrapInitializationCallback = null;
> public Command detachedInitializationCallback = null;
>
> + protected Phase currentPhase = Phase.BUILT;
> protected SafeHtml html = null;
> +
> private String styleName = null;
>
> /**
> @@ -139,7 +145,9 @@
>
> @Override
> public com.google.gwt.user.client.Element getElement() {
> - if (!isFullyInitialized()) {
> + // If neither render nor wrapElement has been called yet,
> + // we have to assume this is the root of the tree and finish
> initialization
> + if (currentPhase == Phase.BUILT) {
> // In case we haven't finished initialization yet, finish it now.
> buildAndInitDivContainer();
> html = null;
> @@ -172,10 +180,15 @@
>
> @Override
> public void performDetachedInitialization() {
> + // this can only be called after wrapElement
> + assert currentPhase == Phase.WRAPPED;
> +
> if (detachedInitializationCallback != null) {
> detachedInitializationCallback.execute();
> detachedInitializationCallback = null;
> }
> +
> + currentPhase = Phase.DONE;
> }
>
> @Override
> @@ -187,28 +200,34 @@
>
> @Override
> public void render(String id, SafeHtmlBuilder builder) {
> + // render() must be the first method called when initializing via the
> + // IsRenderable path.
> + assert currentPhase == Phase.BUILT;
> +
> if (styleName != null) {
> builder.append(TEMPLATE.renderWithIdAndClass(id, styleName,
> getInnerHtml()));
> styleName = null;
> } else {
> builder.append(TEMPLATE.renderWithId(id, getInnerHtml()));
> }
> +
> + currentPhase = Phase.RENDERED;
> }
>
> @Override
> public void setStyleName(String styleName) {
> - if (!isFullyInitialized()) {
> + if (currentPhase == Phase.DONE) {
> + super.setStyleName(styleName);
> + } else {
> // If we haven't built the actual HTML element yet, we save the style
> // to apply later on.
> this.styleName = styleName;
> - } else {
> - super.setStyleName(styleName);
> - }
> - }
> -
> - @Override
> - public void wrapElement(Element element) {
> - if (isFullyInitialized()) {
> + }
> + }
> +
> + @Override
> + public void wrapElement(String id, Element parentElement) {
> + if (currentPhase != Phase.RENDERED) {
> /*
> * If wrapElement is being called after the widget is fully
> initialized,
> * that's probably a programming error, as it's much more efficient to
> @@ -220,7 +239,12 @@
> + "panel)");
> }
>
> - setElement(element);
> + setElement(Document.get().getElementById(id));
> +
> + // This is only here because wrapInitializationCallback may call
> getElement()
> + // and we must know that we don't need to recreate the whole tree.
> + currentPhase = Phase.WRAPPED;
> +
> html = null;
> if (wrapInitializationCallback != null) {
> wrapInitializationCallback.execute();
> @@ -233,14 +257,6 @@
> */
> protected SafeHtml getInnerHtml() {
> return html;
> - }
> -
> - /**
> - * Whether the initilization of the panel is finished (i.e., the
> corresponding
> - * DOM element has been built).
> - */
> - protected boolean isFullyInitialized() {
> - return html == null;
> }
>
> /**
> @@ -253,6 +269,10 @@
> Element element = Document.get().createDivElement();
> element.setInnerHTML(getInnerHtml().asString());
> setElement(element);
> +
> + // This is only here because wrapInitializationCallback may call
> getElement()
> + // and we must know that we don't need to recreate the whole tree.
> + currentPhase = Phase.DONE;
>
> // If there's any wrap callback to call, we have to attach the div
> before
> // calling it, and then detach again.
>
>
>
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
