Printng reorg

[rmarianski]

@ischneider - can you review this re-organization? The diff is a little overwhelming because I moved a lot around, so I'll try to explain all the different pieces and the reasoning behind it.

I tried to favor composition over inheritance, so things are structured in terms of smaller readers and writers that handle the different necessary cases. I also tried to move away from the stateful rending approach, by having each piece of the process as a separate concern. So the general flow is to obtain a reader that represents the template source, have a parser convert that into a Document, and then have a writer that takes the document and writes it to a stream. My thinking is that having these as separate pieces should also make it easier to re-use them in different scenarios by mixing and matching them in different ways. In particular I tried to make the writers agnostic of the request or restlet, so that generating the output formats can be directly used in other contexts.

The dispatch uses the same PrintFinder class, but instantiates it with a different reader depending on which route matches. It then figures out what the appropriate writer should be based on the request, and instantiates it appropriately. After that the requests go through the same flow with the differences captured by these readers and writers. I was thinking of adding another reader that pulls the template in from a separate uri, which can now just be added by creating another reader for this scenario.

I noticed that you had some logic to clean up bad html. I added in TagSoup when parsing the input xml into a Document, so that should hopefully cover us from invalid html from clients. I'm hoping that this "just works" for our purposes, but if not I added a flag to turn it off if necessary and we can tackle that separately if/when it comes up.

There's also a handler for posting in new freemarker templates. I save these to the location you were using, under printng/templates.

And I added more unit tests for the different scenarios. It's not 100%, but I think it covers the most important cases.

I also moved the HtmlMap and HtmlMapLayer classes that you had into a separate experimental package.

I don't have any comments describing things, but if we decide to go down this path I'd be happy to add them in.

Thanks for reading if you got this far!

[ischneider]

@rmarianski Overall, great work to modularise this and separate concerns. I like the finder changes and thanks for adding tagsoup - the img tag bit was a quick hack.

Nitpicks:

1. PrintngFreemarkerTemplateFacade: I personally do not like having 'Facade' in a class name. It was also confusing to me as I thought that class would be doing more facade-like things, but it appears more like a lamely named 'Utils' type of class
2. package 'iface','reader','writer': I personally prefer an api/spi split for things like this. Put the interfaces in an 'api' package, all the implementations in an 'spi' package.
3. PrintngRestDocumentParser - does not seem to be related to 'Rest' in any way.
4. Prefer lumping various trivial support classes and functions together (PrintRepresentation for example) in a top-level class (PrintngSupport - if a collection of arbitrary support, or Representations if a collection of Representation implementations, for example - either are better than a Utils class...)

More serious discussion:

reader/writer and factory

the reader and writer + factory interfaces seem to be collapsible to just two interfaces (one for reading, one for writing). Since one will always be doing the following:

    Document dom;
    OutputStream out;
    PrintngWriterFactory fac;
    fac.printngWriter(dom).write(out);

Perhaps the new interface looks like:

interface PrintWriter {
  write(Document document, OutputStream out) throws IOException;
}

and the factories are all replaced by these?

PrintRepresentation

It appears too easy to put the wrong media type and writer together - perhaps it is better to wire these together tighter by letting the factory create the representation?

A true facade

For the above two points, a facade that hides the details of stringing these things together - essentially one that does what is in PrintResource.getRepresentation ?

My HTMLToPDF 'main' is gone

:( - this was very useful for testing/previewing output.

[rmarianski]

Simulating inline comments. Maybe this would be better in a mail thread?

1. PrintngFreemarkerTemplateFacade: I personally do not like having 'Facade' in a class name. It was also confusing to me as I thought that class would be doing more facade-like things, but it appears more like a lamely named 'Utils' type of class

I wanted it to be the "facade" for working with the template directory in the data dir. But you're right, it's a bad name for a utils class. The main reason for its existence is to centralize the location of the "printng/templates" path.

1. package 'iface','reader','writer': I personally prefer an api/spi split for things like this. Put the interfaces in an 'api' package, all the implementations in an 'spi' package.

Can there be multiple subpackages under spi? spi.reader/spi.writer? Or do you prefer to have .reader.api and .writer.api, and have .reader.spi and .writer.spi? I don't want to put all implementations in one bucket because I've found the reader/writer split useful for navigation.

1. PrintngRestDocumentParser - does not seem to be related to 'Rest' in any way.

Good point. When I originally created it I thought that it might, but that turned out not to be the case. I've renamed it.

1. Prefer lumping various trivial support classes and functions together (PrintRepresentation for example) in a top-level class (PrintngSupport - if a collection of arbitrary support, or Representations if a collection of Representation implementations, for example - either are better than a Utils class...)

Related to the above utils class, do you think I should just rename it to something else using this convention? org.geoserver.printng.freemarker.PrintngFreemarkerTemplateFacade -> org.geoserver.printng.PrintngFreemarkerSupport Or should that just go into something like .printng.PrintngSupport? It might potentially grow other methods related to interacting with the data dir, but that's all I expect.

Are you thinking that I should also move the classes that are under .restlet.* to the top level too? I kind of like that the restlet related classes are all bundled together separately from other utilities.

More serious discussion:

reader/writer and factory

the reader and writer + factory interfaces seem to be collapsible to just two interfaces (one for reading, one for writing). Since one will always be doing the following:

    Document dom;
    OutputStream out;
    PrintngWriterFactory fac;
    fac.printngWriter(dom).write(out);

Perhaps the new interface looks like:

interface PrintWriter {
  write(Document document, OutputStream out) throws IOException;
}

and the factories are all replaced by these?

I'll speak to the writer first.

I think it might be possible that we won't have a document handy when generating some pdfs. If we integrate more with mapfish, that will be responsible for generating just the image of the map based on a config spec. Then, that image will be woven into a separate template that will have a document in its context. I was originally thinking that the mapfish writer would also implement the wrtier interface, and it wouldn't have a document handy then. But maybe we just solve that in a different way by having all of that encapsulated in a larger writer. This writer will take in a document and a separate map spec, delegate to mapfish to generate the map image, and then generate the pdf from that. And the mapfish image generating part can be its own thing. If that sounds good, then I'm happy to collapse the writing interfaces into one. You do mention another possible reason to keep this abstraction around below, related to coupling the media type with the writer.

Regarding the readers, it's separate mostly because that's the only way I could think of to integrate it with spring. I liked the idea that along with the rest configuration, if we used different endpoints we knew what kind of request the client was making, so that decision could be encapsulated within a reader. I wanted to actually instantitate the appropriate reader and pass that to the printfinder. The issue is that we don't have access to the request object at that point though. To have a single reader interface I'd need to introduce that into the api:

interface PrintReader {
  reader(Request request);
}

But, that would force that interface to be specific to restlet. I didn't want to do that if I could avoid it, because it could possibly make it tougher to re-use some of those classes in that case.

PrintRepresentation

It appears too easy to put the wrong media type and writer together - perhaps it is better to wire these together tighter by letting the factory create the representation?

Which factory would create these? Were you thinking of introducing a separate factory for this? Or did you want to add a method to the printngwriterfactory interface (which might be going away) to return the media type? Actually, maybe we can add this to the writer interface itself, since the writer certainly knows what media type it's generating, and is already coupled in that sense. Hmm, the problem with that is in order to generate the writer, we already need a document (unless we change that interface). And to instantiate the representation, we need to hand it the variant already. I do like your facade idea though, which brings us to:

A true facade

For the above two points, a facade that hides the details of stringing these things together - essentially one that does what is in PrintResource.getRepresentation ?

This could definitely work. It can be given request/response objects, and handle the whole flow and instantiation of writers. Essentially, it would combine the logic that is in printfinder with the logic in getrepresentation and tie those together. I like this because the higher level flow is in a single place.

My HTMLToPDF 'main' is gone

:( - this was very useful for testing/previewing output.

Whoops, sorry! I'll add it back.