Anti-patterns in Renderer

[Joshua-Barclay]

See closed issue #631 for more information on how this issue was discovered.

WEditableImageRenderer is clearly supposed to be a subclass of WImageRenderer, however due to the Renderer classes being final the relationship has instead been implementing using a static method.

I can think of no reason why these classes should be final. If applications randomly extend these Renderers and add unknown attributes then the XSLT will fail. However, surely we can give applications more credit than that. If they attempt to extend the Renderers they probably have a good reason for it.

Assuming that we go with the subclassing approach, we will need a way of inserting attributes into the 'middle' of the XML output. WEditableImageRenderer is a clear example of this - we want all of the super class's tags, but also something else.

I recommend we remove doRender in AbstractWebXmlRenderer and replace it with the following methods:

    @Override
    public void render(final WComponent component, final RenderContext renderContext) {
        if (renderContext instanceof WebXmlRenderContext) {
            renderOpen(component, (WebXmlRenderContext) renderContext);
            renderClose(component, (WebXmlRenderContext) renderContext);
        } else {
            throw new SystemException("Unable to render web xml output to " + renderContext);
        }
    }

//Pretty much replaces the current doRender method, but without the end tag.
protected abstract void renderOpen(WComponent component, WebXmlRenderContext renderContext);

//Provide a default implementation so it only needs to be overridden for special cases.
protected void renderClose(WComponent component, WebXmlRenderContext renderContext)
{
    renderContext.getWriter().appendEnd();
}

It will now be trivial to add a custom attribute to a parent renderer. WEditableImageRenderer will now simply be:

class WEditableImageRenderer extends WImageRenderer {

    @Override
    public void renderClose(final WComponent component, final WebXmlRenderContext renderContext) {
        WEditableImage editableImage = (WEditableImage) component;
        WComponent uploader = editableImage.getEditUploader();
        if (uploader != null) {
            renderContext.getWriter().appendAttribute("editor", uploader.getId());
        }
        super.renderClose();
    }
}

[jonathanaustin]

Totally agree these renders should not be final.

As we are going to rework the renderers very soon, we should keep in mind the ability for renderers to be extended. As renderers can be very different from each other, splitting doRender into open/close might not be the answer as a lot of renderers do not follow that pattern (eg rendering extra components outside their tag). It might be best for each renderer to make sure they are written to allow themselves to be "extended" and provide their own "protected" methods.

Also worth remembering, that extending a renderer would be a very last resort for a project to modify a component.

Rescheduling this as we will be rewriting all of the renderers in the next major release.

See #572, #781