Ticket #199 (closed enhancement: fixed)

Opened 4 years ago

Last modified 4 years ago

PDF printing with the MapFish (GeoServer) print module

Reported by: ahocevar Assigned to:
Priority: major Milestone: 0.7
Component: GeoExt.data Version: trunk
Keywords: Cc:
State: Commit

Description

The MapFish print module has recently been packaged into a GeoServer extension, making it a popular solution for PDF-based printing of web maps.

The attached patch provides the basic components (GeoExt.data.PrintProvider and GeoExt.data.PrintPage) to access the print module. It comes with tests and an example to show how basic printing works.

More components for richer user interfaces (preview map, form elements) are already prepared in http://dev.geoext.org/sandbox/ahocevar/playground, and will be provided in separate tickets. Fully featured printing dialogs will also be provided as user extension?.

Attachments

geoext-199.patch (61.4 kB) - added by ahocevar on 01/19/10 22:14:05.
geoext-199.2.patch (61.3 kB) - added by ahocevar on 01/21/10 20:56:27.
with some improvements proposed by Bart; made handle specific methods and properties private; added rotationIncrement argument to updateByHandle

Change History

01/19/10 16:51:58 changed by ahocevar

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

01/19/10 22:14:05 changed by ahocevar

  • attachment geoext-199.patch added.

(follow-up: ↓ 5 ) 01/21/10 15:41:53 changed by 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.

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?

(follow-up: ↓ 4 ) 01/21/10 16:10:26 changed by bartvde

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?

(in reply to: ↑ 3 ) 01/21/10 20:55:42 changed by ahocevar

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.

01/21/10 20:56:27 changed by ahocevar

  • attachment geoext-199.2.patch added.

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

(in reply to: ↑ 2 ) 01/21/10 21:05:08 changed by ahocevar

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.

(follow-up: ↓ 7 ) 01/22/10 08:13:37 changed by 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;

(in reply to: ↑ 6 ) 01/22/10 08:20:09 changed by ahocevar

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.

01/22/10 08:30:44 changed by bartvde

  • state changed from Review to Commit.

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

01/22/10 09:08:01 changed by ahocevar

  • status changed from new to closed.
  • resolution set to fixed.

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