[gwt-contrib] Re: Change the wrapElement API to receive the id and a parent element. I also ported some of the boo... (issue1446811)

[rjrjr]

LGTM

http://gwt-code-reviews.appspot.com/1446811/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Change the wrapElement API to receive the id and a parent element. I also ported some of the boo... (issue1446811)

[rjrjr]

http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/IsRenderable.java
File user/src/com/google/gwt/user/client/ui/IsRenderable.java (right):

http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/IsRenderable.java#newcode29
user/src/com/google/gwt/user/client/ui/IsRenderable.java:29: public
interface IsRenderable extends SafeHtmlRendererString {
Let's drop the extends SafeHtmlRenderer.

Should document that it's up to the implementation to choose how to use
the id (unless we pass in a helper, see below). Perhaps change its name
to token to drive home the point.

http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/IsRenderable.java#newcode33
user/src/com/google/gwt/user/client/ui/IsRenderable.java:33: * the DOM
tree. The id is assumed to be the same passed in at render time.
I think it's time to implement a helper class, to be used for both
rendering and
lookup. If we don't have one soon retrofitting will be a nightmare.
Document
here that the receiver cannot assume that the element is attached to the
dom,
and @see to the helper.

The helper could be something like:

SafeHtml stampElement(SafeHtml mustBeOpeningTag, String token) {
  String tag = mustBeOpeningTag().asString();
  assert tag.endsWith();
  return SafeHtmlUtil.whatever(tag.substring(tag.length()-1) + ,
id=' +
token + '));
}

Element find(String token, Element parentElement) {
  ensureAttached(parentElement);
  return Document.get().getElementById(token);
}

And I think the thing to do is pass an instance of the helper into the
render() and wrap() calls. The alternative is to make some hacky static
lookup scheme, but I think we'll regret that pretty quickly.

You buying it?

http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java
File user/src/com/google/gwt/user/client/ui/RenderablePanel.java
(right):

http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode61
user/src/com/google/gwt/user/client/ui/RenderablePanel.java:61: BUILT,
RENDERED, WRAPPED, DONE
I'm not liking the phases. I think we can simplify.

http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode65
user/src/com/google/gwt/user/client/ui/RenderablePanel.java:65: //
single callback.
Time to take care of this. But that said, I don't like our plan from the
other day. :-)

We earlier talked about having binder generate subclasses of
RenderablePanel that override no-op default implementations, but I'm
having second thoughts about that. My rational was that it avoids
unnecessary class definitions, but that's silly — a subclass of
RenderablePanel is a new class just like a command object is — and it
will make it trickier to allow people to use their own subclasses of
RenderablePanel (which they will do, trust me).

Instead, let's introduce a null command object and use it here, so that
you can always call the thing without having to think abou it:

public Command wrapInitializationCallback = NullCommand.INSTANCE;
public Command detachedInitializationCallback = NullCommand.INSTANCE;

That said, I think you're right to dislike the magical phase check in
getElement(). Can you go ahead and rework this to give the wrap callback
an argument? So this becomes:

public WrapCallback wrapInitializationCallback = WrapCallback.INSTANCE;
public Command detachedInitializationCallback = NullCommand.INSTANCE;

But *that* said, see the comment on line 182. Maybe we dont need these
commands at all.

http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode70
user/src/com/google/gwt/user/client/ui/RenderablePanel.java:70:
protected SafeHtml html = null;
I dislike protected fields. Can you methods around these? Should also
ensure that the code here uses the methods rather than poking the fields
directly, in case subclasses get tricky.

http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode72
user/src/com/google/gwt/user/client/ui/RenderablePanel.java:72: private
String styleName = null;
TODO need a more general mechanism for caching attributes while
unwrapped

http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode182
user/src/com/google/gwt/user/client/ui/RenderablePanel.java:182: public
void performDetachedInitialization() {
If we go ahead and add the helper object to the wrap and render calls,
can it be in charge of accumulating the callbacks as well, and get these
command objects out of here?

http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/RenderablePanel.java#newcode230
user/src/com/google/gwt/user/client/ui/RenderablePanel.java:230: if
(currentPhase != Phase.RENDERED) {
You're forcing the 

[gwt-contrib] Re: Change the wrapElement API to receive the id and a parent element. I also ported some of the boo... (issue1446811)

[rjrjr]

http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/IsRenderable.java
File user/src/com/google/gwt/user/client/ui/IsRenderable.java (right):

http://gwt-code-reviews.appspot.com/1446811/diff/1/user/src/com/google/gwt/user/client/ui/IsRenderable.java#newcode33
user/src/com/google/gwt/user/client/ui/IsRenderable.java:33: * the DOM
tree. The id is assumed to be the same passed in at render time.
Re-reading this, I see I shifted my stance in midstream. Let me shift if
further.

To be clear, I think we should not put control of the
document.getElementById() call in the hands of the individual panels,
and instead we should give them a helper object.

And now that I think about it, if we're giving them the helper then we
don't need to give them the id at all. It can be built into the helper.
And we can go back to giving them just the element they stamped.

interface Stamper {
  /** Error to call more than once */
  SafeHtml stampRenderedOpenTag(SafeHtml mustBeOpeningTag);
}

interface IsRenderable {
  void render(SafeHtmlBuilder builder, Stamper stamper);

  /** Provides the element that was stamped, not some ancestor */
  void wrapElement(Element);
}

Note that it's entirely up to them to choose what element they stamp.
There is no requirement that it's the root of their rendered product,
and no requirement that the renderer produce a single dom element.

http://gwt-code-reviews.appspot.com/1446811/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Change the wrapElement API to receive the id and a parent element. I also ported some of the boo... (issue1446811)

[rjrjr]

For the record:

Rafa talked some sense into me offline. The Stamper notion really isn't
practical given the limitations of SafeHtmlTemplates, so I'm backing off
of most of this craziness. Instead we'll just delete the extends
SafeHtmlRenderer thing. That will also allow backing away from the phase
stuff, since we won't yet shift responsibility for the getElementById
call.

Instead, when this is done Rafa will look into extracting a superclass
out of UIObject that is able to cache calls made to setStyleName,
setVisible, setWidget, etc., before setElement has been called; maybe
apply them during getElement(); but mainly provide access to them for
smart subclasses to use in their render methods. (Eventually this new
superclass, RenderableObject, should itself implement IsRenderable, but
not in the first pass.)

With that in hand we should be able to make move RenderableComposite
back into Composite, and RenderablePanel back into HTMLPanel.

WOOT!

http://gwt-code-reviews.appspot.com/1446811/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Change the wrapElement API to receive the id and a parent element. I also ported some of the boo... (issue1446811)

[rdcastro]

http://gwt-code-reviews.appspot.com/1446811/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Change the wrapElement API to receive the id and a parent element. I also ported some of the boo... (issue1446811)

[Rafael Castro]

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.InterpreterString {

   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 SafeHtmlRendererString {

   /**
 -   * 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)
 +++