PDF printing with the MapFish (GeoServer) print module

Tests pass in FF3.5, IE6,7,8 and Chromium 4. Please review.

Review for lib/GeoExt/data/PrintProvider.js:

    /** api: property[dpi]
     *  ``Ext.data.Record`` the record for the currently used layout.

should be dpi instead of layout.

    /** api: method[setDpi]
     *  :param layout: ``Ext.data.Record`` the dpi record.

param layout should be dpi.

For this to look like in the MapPanel

What is meant here exactly?

Why is map.baseLayer.projection.getCode() used instead of map.getProjectionObject().getCode()?

Should the "loadcapabilities" event not be triggered by the loadCapabilities (after this.capabilities = ...) instead of the loadStores method? Does not make much difference but the position is more logical to me.

Shouldn't getAbsoluteUrl be placed in a more general place in GeoExt? Could be useful for other classes in the future.

Is the encoder for Layer.WMS WMS 1.3 proof? CRS instead of SRS e.g.

Should we "cache" an instance of featureFormat and styleFormat instead of creating them all the time?

Review for PrintPage (I still need to finish this):

/** api: property[handle]

Should be handles.

Can you explain why there is a customParams on both the PrintPage and the PrintProvider? Is it used here at all?

Should the change event not send which property is changed?

*      Optional if a ``layer`` that is added to a map was configured.

I can't find anywhere that there is a config option called layer?

var units = map.baseLayer.units --> why not map.getUnits?

Thanks Bart for your review!

Replying to bartvde:

Review for PrintPage (I still need to finish this): {{{ /** api: property[handle] }}} Should be handles.

Fixed, and made private (along with other handle specific stuff). The rationale behind this is that a generic transformation box would be something to do in OpenLayers, and once we have that, we can remove handles, updateHandles and updateByHandles.

Can you explain why there is a customParams on both the PrintPage and the PrintProvider? Is it used here at all?

The encoding process (see PrintProvider.js) uses it. customParams are hierarchic in the print module. You can have them on the root level, and override on the page level (the print module supports printing of multiple pages).

Should the change event not send which property is changed?

If you update using the handles, scale and rotation can change at the same time, so we would either need to fire the event twice, or return an awkward object or array with multiple changed properties. I did not need this information anywhere else in the printing framework, and since it would mean extra effort to do that, I didn't.

{{{ * Optional if a layer that is added to a map was configured. }}} I can't find anywhere that there is a config option called layer?

Good catch. That was removed. The new description should make things clear.

var units = map.baseLayer.units --> why not map.getUnits?

Right. Changed that.

with some improvements proposed by Bart; made handle specific methods and properties private; added rotationIncrement argument to updateByHandle

Replying to bartvde:

Review for lib/GeoExt/data/PrintProvider.js: {{{ /** api: property[dpi] * Ext.data.Record the record for the currently used layout. }}} should be dpi instead of layout. {{{ /** api: method[setDpi] * :param layout: Ext.data.Record the dpi record. }}} param layout should be dpi.

Changed, thanks.

{{{ For this to look like in the MapPanel }}} What is meant here exactly?

LegendPanel was meant, and I re-worded the sentence to be clearer.

Why is map.baseLayer.projection.getCode() used instead of map.getProjectionObject().getCode()?

Because the API will change here in OpenLayers 3 (from Map::getProjection: "In 3.0, we will remove getProjectionObject, and instead return a Projection object from this function.").

Should the "loadcapabilities" event not be triggered by the loadCapabilities (after this.capabilities = ...) instead of the loadStores method? Does not make much difference but the position is more logical to me.

Populating the stores with data from the capabilities is an important part of capabilities loading, as components accessing the printProvider rely on finding records in the stores. This is why the event is fired after the stores are populated.

Shouldn't getAbsoluteUrl be placed in a more general place in GeoExt? Could be useful for other classes in the future.

I agree. But then, where should it go? It would even fit better into Ext JS, because it is in no way geo* related. So I decided to put it here until we come up with a place where we can put things like this.

Is the encoder for Layer.WMS WMS 1.3 proof? CRS instead of SRS e.g.

No, but I guess this can be done as an improvement in a separate ticket.

Should we "cache" an instance of featureFormat and styleFormat instead of creating them all the time?

Given the small number of layers we usually process, and that these instances are created only once per vector layer, I'd say this is not necessary.

Hi Andreas, thanks for your quick changes, I've got one question left, why is 72 used instead of OpenLayers.DOTS_PER_INCH?

var w = size.width / 72 / unitsRatio * s / 2;

Replying to bartvde:

Hi Andreas, thanks for your quick changes, I've got one question left, why is 72 used instead of OpenLayers.DOTS_PER_INCH? {{{ var w = size.width / 72 / unitsRatio * s / 2; }}}

72 here is not related to the dpi in OpenLayers. It is there to deal with the PostScript points that the page width and height are measured in in the print module.

Thanks for the clarification, I think this is good to go Andreas.

(In [1802]) Added support for basic printing using the MapFish (GeoServer) print module. r=bartvde (closes #199)