Tapestry Improvements from Review by Howard Lewis Ship

From an e-mail Howard sent me:
---------------------------------------------------------
I'm helping a client upgrade from AppFuse 1.9.0 (Tapestry 3.0) to AppFuse 1.9.3 (Tapestry 4.0.1).

Here's a stream of observations from poking around the code and files. I hope they will be of use to you.

I'm finding a few things that could be simplified a bit in terms of the Tapestry implementation. AppFuse is still largely geared towards Tapestry 3, and so it is working "against the grain" of Tapestry 4 ... that is, it is not making use of the features designed to make Tapestry 4 easier and simpler and more terseand less repetative than Tapestry 3.

Naming conventions:

In Tapestry, the established convention is that page names are CamelCase. Further, the XML specification (.page file), HTML template and Java class should all be named the same. Likewise for components.

File locations:

Page specifications belong in WEB-INF.

HTML Page templates belong in WEB-INF or web context root (this is a user preference) ... i.e., no different than a JSP.

We could hack Tapestry to look for these files in the pages subdirectory.

If the page specifications are in WEB-INF, then there's no need to put <page> elements into the application specification.

Because the servlet name for the Tapestry servlet is configured in web.xml as "tapestry", the framework will actually look in WEB-INF/tapestry first, then WEB-INF, then the tapestry folder, and finally the root folder. You could, as a kludge, change the servlet name to "pages" to force Tapestry to look in this folder first.

Use of the @class attribute in .page files:

Tapestry 4 will attempt to automatically locate Java classes corresponding to page names (this is why they should all have the same name).

Add the following to tapestry.application:

<meta key="org.apache.tapestry.page-class-packages" value="org.appfuse.webapp.action"/>

The value is a comma separated list of package names to search.

Also:

<meta key="org.apache.tapestry.default-page-class" value="org.appfuse.webapp.action.BasePage"/>

This is the page class to use when a page-specific class is not found, or specified with @class.

There is also a component-class-packages.

There is also a bean-class-packages. This allows use of abbreviated class names for <bean>s. I.e. class="Pattern" instead of class="org.apache.tapestry.form.validator.Pattern". This is the configuration entry most likely to be a list of packages.

I never use the @class attribute in page or component specifications.

Package names:

My convention, for some root package (here, org.appfuse.webapp), is:

root.pages – Page classes
root.components – Component classes
root.services – HiveMind services and implementations
root.data – Presentation layer data objects
root.bean – Beans and utilities used on pages

I can certainly see why, for consistency with the other AppFuse variations, not everything is done "the Tapestry way".

Still, some more observations:

BasePage.java:

Make getDelegate() public. Don't rely on a <bean> element to define the delegate, just lazily instantiate and return it from this code. Implement PageDetachListener to clear the instance variable at the end of the request. Remove <bean> elements from (each and every) .page file. Change references from ognl:beans.delegate to just ognl:delegate.

userForm.page:
displayName for PropertySelection now works in 4.0, just as with TextField. In 3.0, it only worked with ValidField (now deprecated).

contrib:MultiplePropertySelection is nice, but contrib:Palette is nicer (but requires client-side JavaScript).

activeUsers.html

I use EvenOdd to zebra-stripe my tables. However, I tend to call the bean "rowClass", so I can say:
rowsClass="ognl:rowsClass.next"
which is more readable.

You could style a link to look like a <button>, and use a PageLink. This is prefereable if you start using client-side state, since the generated link
will include the query parameters that store client side state. Alternately, creating a PageButton component would be very simple. A third option: an ILinkRenderer implementation
that renders the link as a <button> instead of as an <a>.

ognl:assets.foo --> asset:foo

I tend to put my column definitions inside the message catalog, rather than inline.

mainMenu.html:

ognl:listeners.editProfile --> listener:editProfile

selectFile.html:

Use a @FieldLabel for each field. I.e.
<label jwcid="@FieldLabel" field="component:nameField"/>

Tapestry will provide a field's displayName as the body of the <label>, and will generate the @for attribute as well. Under Tapestry 4,
form control elements generate both a @name (unique within the form) and an @id (unique within the page). This really slims down the HTML.

signup.html:

ognl:components.foo --> component:foo

These binding prefixes ("component:", "asset:", "bean:", "listener:", etc.) are more than just improved syntax over "ognl:". The corresponding IBinding implementations use type safe, non reflective calls. The "ognl:"
prefix makes heavy use of reflection. Further, Tapestry is able to identify that the other binding prefixes are "invariant" (i.e., will always return the exact same value) and will cache those values inside component parameter properties aggressively.
Over the course of processing a client request, once both the application and HotSpot are warmed up, this might add up to a measurable amount of time.

Tapestry does generate JavaScript to focus on the first field in a form. The logic has improved in Tapestry 4: its the first field in error, or
the first required field, or just the first field.

userForm.html:

ognl:requestCycle.requestContext.session.xyz --> ognl:session.xyz

It's also possible to write an OGNL PropertyAccessor that would allow attributes of the session to be accessed by name, i.e.:

ognl:session['cookieLogin'] == 'true'

BTW, as a general style note: I tend to keep the HTML pretty simple. The ability to put ognl: into the HTML template is useful in moderation, but makes the pages ugly.
My rule of thumb is any time a component has more than two parameters, or any parameter whose OGNL expression is more than just a property path, I move all the
dynamic parameters into the .page or .jwc file.

I haven't tried this, but there must be some way to replace loginForm.jsp with a Tapestry page. Given friendly URLs, there shouldn't be a problem to get the security filter to forward to an HTML page that happens to be a Tapestry page.

Ideally, everything should be Tapestry pages. I'm not sure to what degree the servlet container will support this.

FileUpload.java:

Listener methods are improved in Tapestry 4. IRequestCycle is no longer necessary (it is optional). The return type can be void, String, IPage or ILink.Thus:

public String cancel()

{ log.debug(...); return "mainMenu"; }

You can use the @InjectPage (<inject type="page">) annotation to gain access to another page within the app.

The goal here is to reduce or eliminate the IRequestCycle from page and component code, or at least, from listener methods.

FileDisplay.java:

The abstract getter methods are not referenced in Java code and may be removed (just the setter methods are used). For transient and persistent properties, Tapestry always generates both a concrete getter and setter (so that property may be read and updated via OGNL), but you should only define the ones you actually need in Java code.

RoleModel.java:

RoleModel seems a bit limiting, as it is really a IPropertySelectionModel around a list of LabelValues, not a list of Roles.

SignupForm.java:

public void pageBeginRender(PageEvent event) {
if (getUser() == null && !event.getRequestCycle().isRewinding())

{ setUser(new User()); } else if (event.getRequestCycle().isRewinding()) { setUser(new User()); }

}

This looks tortured to me. How about:

if (getUser() == null || getRequestCycle().isRewinding())
setUser(new User() );

However, I think this will work at least as well:

if (getUser() == null)
setUser(new User() );

BasePage.java:

The addError() method doesn't need the ValidationDelegate passed in; it can invoke getDelegate() to get it instead.

FieldLabel.java:

I believe the new validation delegate method writeLabelAttributes() can be used for this purpose. That is, use standard FieldLabel, and
cand ValidationDelegate writeLabelSuffix to writeLabelAttributes().

BasePage.java:

Another thought about BasePage: The cost of inheriting injections when not using Annotations is high: the cost of the duplication of the <inject> element in the XML of each page. One option would be to use an XML external entity to store the common XML snippet ... but the setup for that is as long as the snippet itself.

Another option that occurred to me would be to limit the necessary injections ... limit it to a single injection. So instead of injecting the HttpServletRequest and HttpServletResponse and the page IEngineService, instead do the following:

1) Define a new interface, AppfuseResources, that has getters for the request, response, page service and anything else you think of.
2) Setup this interface in hivemodule.xml, with an implementation, and inject the request, response, page service and so forth into it.
3) Inject just this service into BasePage:

public abstract AppfuseResources getAppfuseResources(); <inject property="appfuseResources" object="service:org.appfuse.tapestry.AppfuseResources"/>

4) Change the getRequest(), etc., methods on BasePage to concrete methods that return the corresponding value from appfuseResources.

Once this is in place, you can handle Spring services the same way, and also move some of the code that generates common IPropertySelectionModel and IPropertySelectionRenderer instances into the AppfuseResources service as well. Anything that gets code out of page and component classes is a good thing ... its always easier to test POJOs than to test Tapestry classes. The service hivemind.ThreadLocale can be used to determine what the current locale is (when creating certain models). This can be injected into AppfuseResources as well.

Other conventions:

I tend to name all my listener methods "doSomething".

I often have a method (or methods) on my pages named "activate" whose job is to set up state for the page and make it the active page to render the response. This is common for "single purpose" pages, common in CRUD apps, such as "EditUser" or "ShowRoles". The pattern tends to look like:

MainMenu.java:

@InjectPage("ShowUser")
public abstract ShowUser getShowUser();

public void doShowUser(long userId)

{ getShowUser().activate(userId); }

ShowUser.java:

public void activate(long userId)

{ setUserId(userId); getRequestCycle().activate(this); }

hivemodule.xml

HiveMind 1.1 supports the idea of a "conditional contribution".

<contribution configuration-id="tapestry.InfrastructureOverrides" if="not(property developer-mode)">
<property name="exceptionPageName" value="error"/>
</contribution>

With this in place, the contribution occurs any time the JVM system property -Ddeveloper-mode=true is not set a JVM startup. I forget how to set this up under Tomcat but I know you can.

Wrap up

Are you using a hacked version of the Tapestry JAR? I can't for the life of me figure out how Tapestry is finding messages inside ApplicationResources.properties.

In general, I found that the conversion of AppFuse from Tapestry 3 to Tapestry 4 was hit or miss in terms of taking advantage of Tapestry 4's features. We need to enlist the AppFuse and/or Tapestry community to "finish the job". I do like AppFuse's general approach, especially the fact that more advanced project features (the kind of features too many teams are forced to implement towards the end of a project, and with great difficulty) are wired in from day one, and just need to be enabled or configured.

My Response:
----------------------------------------------
RE: ApplicationResources.properties >>

Yes. It's definitely a hack, and an ugly one too. I entered a bug for it and posted what I did to the mailing list, but I can't seem to find the diff I posted now.

https://issues.apache.org/jira/browse/TAPESTRY-881