Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure baseURL are absolute when printing #270

Merged
merged 1 commit into from
Aug 10, 2015
Merged

Make sure baseURL are absolute when printing #270

merged 1 commit into from
Aug 10, 2015

Conversation

Jenselme
Copy link
Contributor

No description provided.

@@ -271,7 +271,7 @@ ngeo.Print.prototype.encodeWmsLayer_ = function(arr, opacity, url, params) {
goog.object.remove(customParams, 'FORMAT');

var object = /** @type {MapFishPrintWmsLayer} */ ({
baseURL: url,
baseURL: url.match(/^https?:/) ? url : 'http:' + url,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assume that, if url doesn't start with http or https it will start with //. That doesn't sound correct to me. What if url looks like path/to/wms or /path/to/wms?

GeoExt's print provider includes a getAbsoluteUrl function that we could copy to ngeo. The function would be simpler in ngeo as we don't need to support IE < 9.

@Jenselme
Copy link
Contributor Author

Jenselme commented Aug 7, 2015

PR updated: I now use the getAbsuloteUrl from geoExt.

* @param {string} url
* @return {string} Absolute URL.
*/
ngeo.Print.prototype.getAbsoluteUrl_ = function(url) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make this function static: ngeo.Print.getAbsolute_.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And do not forget the @private annotation.

@Jenselme
Copy link
Contributor Author

Jenselme commented Aug 7, 2015

PR update:

  • ngeo.Print.getAbsolute_ is static
  • add @private

@@ -252,14 +252,16 @@ ngeo.Print.prototype.encodeImageWmsLayer_ = function(arr, layer) {

var url = source.getUrl();
var params = source.getParams();
this.encodeWmsLayer_(arr, layer.getOpacity(), url, params);
if (goog.isDefAndNotNull(url)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just use if (goog.isDef(url)) here no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so yes. Is it problematic to use goog.isDefAndNotNull(url) instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more processing. It's confusing for readers of the code, who may think that source.getUrl() may return null. I've become strict with typing, and I think it's right :)

@elemoine
Copy link
Contributor

This PR's branch needs to be rebased.

- Import the getAbsolute function from GeoExt
- Drop compatibility for IE < 9
@Jenselme
Copy link
Contributor Author

  • Use encodeURI where needed instead of decodeURI
  • More strict about typing
  • Rebased agains master.

elemoine pushed a commit that referenced this pull request Aug 10, 2015
Make sure baseURL are absolute when printing
@elemoine elemoine merged commit 2ea6be5 into camptocamp:master Aug 10, 2015
@elemoine
Copy link
Contributor

Thanks.

@sbrunner sbrunner added this to the Older milestone Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants