OpenLayers OpenLayers

Ticket #1489 (closed feature: fixed)

Opened 8 months ago

Last modified 3 months ago

Permalink Changes

Reported by: tcoulter Assigned to: ahocevar
Priority: critical Milestone: 2.7 Release
Component: Control.Permalink Version: 2.7 RC1
Keywords: Cc:
State: Complete

Description

I've been working with permalinks at TOPP, and I've made some changes to the permalink control in order to support custom permalinking.

The highlights: 1) Made the required arg parser class configurable, to allow for custom arg parsers. 2) Decoupled the setting of the link (this.element.href = "...") from creating the url. This way, custom controls/applications can easily poll the permalink control for the link without having to know the idiosyncrasies of the DOM element used to store the value.

I've attached a patch with our changes.

Attachments

permalink.patch (3.0 kB) - added by tcoulter on 04/03/08 16:22:49.
1489-r7869-A0.patch (5.9 kB) - added by ahocevar on 08/26/08 18:25:08.
1489-r7874-A1.patch (5.9 kB) - added by ahocevar on 08/27/08 10:49:43.
argParser renamed to argParserClass
permalink.2.patch (7.6 kB) - added by euzuro on 08/27/08 14:35:20.
tiny mod to previous patch -- removing the double-return
permalink.3.patch (6.4 kB) - added by euzuro on 09/09/08 01:32:18.
take three. this time using the parameters passed in. includes full tests.

Change History

04/03/08 16:22:49 changed by tcoulter

  • attachment permalink.patch added.

04/04/08 10:37:58 changed by tcoulter

For an example where a custom ArgParser/Permalink control are used, see here:

http://projects.opengeo.org/vespucci/browser/trunk/Vespucci/Controls/FeaturePermalink.js http://projects.opengeo.org/vespucci/browser/trunk/Vespucci/Controls/SelectionArgParser.js

Basically, the custom permalink control adds a some extra info in the URL -- in this case, a feature to select -- and the selection arg parser parses that extra info.

04/15/08 21:35:14 changed by crschmidt

  • state set to Review.
  • milestone set to 2.7 Release.

07/28/08 15:05:28 changed by euzuro

  • owner set to tcoulter.

07/28/08 15:16:40 changed by euzuro

  • priority changed from minor to critical.

07/30/08 10:47:02 changed by euzuro

  • owner changed from tcoulter to crschmidt.

08/26/08 18:25:08 changed by ahocevar

  • attachment 1489-r7869-A0.patch added.

08/26/08 18:31:54 changed by ahocevar

Created a new patch with the following modifications:

  • added tests
  • use this.argParser instead of this.ARG_PARSER_CLASS_NAME to configure the ArgParser. This is now an OpenLayers.Class, not a CLASS_NAME string.
  • new APIMethod createParams instead of createUrl. Parameters are not just center, but also zoom and layers -- the core components of a permalink. Subclasses or instances are encouraged to override this method to create custom permalink url parameter pairs.

The new patch still meets the requirements of the original idea, but seems more generic.

08/27/08 01:53:22 changed by euzuro

...can we make that property name 'argParserClass'? Just 'argParser' to me means that the variable is of type OpenLayers.Control.ArgParser, when really what we want here is an actual Class. (note that this also goes toward maintaining consistency with the Feature.js's 'popupClass' property)

08/27/08 01:53:46 changed by euzuro

  • state changed from Review to Needs More Work.

08/27/08 10:49:43 changed by ahocevar

  • attachment 1489-r7874-A1.patch added.

argParser renamed to argParserClass

08/27/08 10:50:21 changed by ahocevar

  • state changed from Needs More Work to Review.

renamed argParser to argParserClass, as suggested by euzuro.

08/27/08 14:35:20 changed by euzuro

  • attachment permalink.2.patch added.

tiny mod to previous patch -- removing the double-return

08/27/08 14:37:00 changed by euzuro

  • owner changed from crschmidt to ahocevar.
  • state changed from Review to Commit.

thanks for the mods on the patch, andreas. i think it's good to go now. i like this patch. it's a good fix for this control.

i ran tests in ff2 and ie7 again with the small mod i made to the patch (removing double-return) and everything passes.

if you are cool with this, please commit.

08/27/08 15:33:50 changed by ahocevar

  • status changed from new to closed.
  • state changed from Commit to Complete.
  • resolution set to fixed.

(In [7881]) made permalink control more configurable by adding an argParserClass property and separating the logic for generating the key-value pairs from the div and link generation. Original patch by tcoulter. r=euzuro,me (closes #1489)

09/05/08 02:09:04 changed by euzuro

  • component changed from general to Control.Permalink.

09/08/08 20:59:19 changed by openlayers

You set

zoom = zoom || this.map.getZoom();

layers = layers || this.map.layers;

at the beginning of createParams, but don't use them.

09/09/08 00:19:48 changed by euzuro

  • status changed from closed to reopened.
  • state changed from Complete to Needs More Work.
  • version changed from 2.6 RC1 to 2.7 RC1.
  • resolution deleted.

good catch on this, o anonymous angel. my bad on letting this through in the first place.

i will make a new patch, and i will make tests, so that this doesn't happen again.

09/09/08 00:20:24 changed by euzuro

  • owner changed from ahocevar to euzuro.
  • status changed from reopened to new.

09/09/08 01:32:18 changed by euzuro

  • attachment permalink.3.patch added.

take three. this time using the parameters passed in. includes full tests.

09/09/08 01:35:51 changed by euzuro

  • owner changed from euzuro to ahocevar.
  • state changed from Needs More Work to Review.

ok. blotch fixed. new patch added, includes full test for createParams() function.

tests pass in ff2, ff3, ie7, and chrome (that's right).

Please review... passing it your way, andreas

09/09/08 06:14:12 changed by ahocevar

  • state changed from Review to Commit.

Oops, sorry for that mistake. Thanks euzuro for the comprehensive tests. Let me apologize with a review:

in Permalink.js#L180, it is enough to say 

params.zoom = zoom;

because we already did the fallback to this.map.getZoom() at the top of the createParams method, when we created the zoom variable.

When this is changed, the patch is good to commit.

09/09/08 08:53:14 changed by openlayers

You don't need

Permalink.js#L198

since it's already done at the top of createParams.

09/09/08 08:57:35 changed by openlayers

Sorry, about last comment, ahocevar and me are wrong, because you removed the lines from the top.

patch is ok.

09/09/08 09:28:09 changed by ahocevar

anonymous is right. Sorry, please commit.

09/09/08 11:53:09 changed by euzuro

  • keywords set to pullup.
  • state changed from Commit to Pullup.

(In [7986]) Fix for Permalink's override-able createParams() function, which was designed to allow for input parameter values for center, zoom, and layers... but was actually only using the center value... zoom and layers were being taken from the map and the input parameters ignored. Big thanks to our anonymous poster for spotting this. r=ahocevar (Pullup #1489)

09/12/08 13:24:20 changed by euzuro

  • keywords deleted.
  • status changed from new to closed.
  • state changed from Pullup to Complete.
  • resolution set to fixed.

(In [8012]) Batch merge for rc2 of 2.7. 'svn merge -r7967:HEAD from trunk (Closes #1733) (Closes #1489) (Closes #1639) (Closes #1718) (Closes #1723) (Closes #1732) (Closes #1616) (Closes #1722)