OpenLayers OpenLayers

Ticket #1019 (new task)

Opened 10 months ago

Last modified 3 weeks ago

Make SphericalMercator less intrusive

Reported by: euzuro Assigned to: euzuro
Priority: minor Milestone: 2.8 Release
Component: Layer.FixedZoomLevels Version: 2.5 RC3
Keywords: Cc:
State: Needs More Work

Description

As it is, adapting a 3rd party class to the spherical mercator "mixin" requires modifications in many areas of the code (switching at all translation functions).

I *think* we could apply this less invasive technique that uses closure to simplify the way the translations occur, making it easier for new 3rd party layers to use the sphericalmercator functionality.

I'm pretty sure but not positive this will work, and the net connection here doesn't allow me to load *any* tiled creature, so it's all rather a crapshoot.

Attachments

spherical.patch (11.9 kB) - added by euzuro on 09/27/07 03:05:42.
spherical.2.patch (9.7 kB) - added by euzuro on 09/27/07 03:12:14.
slightly more readable diff -- instead of breaking the 'forward' and 'inverse' functions up into separate X and Y's, just leave them be. should improve readability
spherical.3.patch (9.7 kB) - added by euzuro on 09/27/07 16:59:22.
fix getExtent() issue

Change History

09/27/07 03:05:42 changed by euzuro

  • attachment spherical.patch added.

09/27/07 03:12:14 changed by euzuro

  • attachment spherical.2.patch added.

slightly more readable diff -- instead of breaking the 'forward' and 'inverse' functions up into separate X and Y's, just leave them be. should improve readability

09/27/07 15:24:05 changed by euzuro

tests pass ff/ie6 and opening the spherical-mercator.html file seems to work.

please review? I think this makes the code more readable.

(follow-up: ↓ 3 ) 09/27/07 15:24:14 changed by euzuro

  • keywords set to review.

(in reply to: ↑ 2 ) 09/27/07 16:11:47 changed by elemoine

Replying to euzuro:

There's something I don't like about this patch: someone that is reading the code of for example the getOLBoundsFromMapObjectBounds() method in Layer/Google.js will probably not figure out right away that the getLongitudeFromMapObjectLonLat() function called from within this method is not the one that's defined below in Layer/Google.js. See my point? This patch hides things, which can be confusing to someone trying to understand what the code does.

09/27/07 16:59:22 changed by euzuro

  • attachment spherical.3.patch added.

fix getExtent() issue

12/13/07 23:35:52 changed by crschmidt

I like the more verbose code, even if it is more invasive -- it's something you only have to do once for each new layer type (*and* it's already done). I think this makes the code harder to understand 'at a glance', and I'd prefer to keep it the way it is.

12/15/07 14:11:39 changed by crschmidt

  • state changed from Review to Needs Discussion.

12/17/07 19:11:30 changed by tschaub

  • state changed from Needs Discussion to Needs More Work.

I think this is a good idea. I agree with crschmidt about documentation (and code readability), but I think we can solve that.

I think two things would improve this:

  • put ND style comments above the functions in SphericalMercator (currently in initMercatorParams)
  • put something about "optionally inherits from SphericalMercator in Google et al. docs
  • perhaps move those function definitions out of initMercatorParams

On this last point, I don't immediately see why getExtent was defined in SphericalMercator with that condidional - that should always be true, right? People aren't toggling between the two as far as I know.

We also have this.CLASS_NAME. So, if you wanted to, you could apply the prototype method instead of defining those temp oldGetLon style variables.

I think having all methods documented - and ideally defined outside of that initMercatorParams closure would be nice.

01/22/08 17:37:31 changed by crschmidt

  • milestone changed from 2.6 Release to 2.7 Release.

Mass ticket move to 2.7 post dev meeting. If you are actively working on this task, please update this ticket with information and change the milestone to 2.6. At the time of the next IRC meeting (most likely 1-31-08), this will mean the ticket can *not* be brought back into 2.6 unless there is further feedback.

07/03/08 18:21:27 changed by euzuro

  • milestone changed from 2.7 Release to 2.8 Release.

Mass ticket move out of 2.7 in preparation for a release plan.

If you are actively working on this task, and think that you can help this ticket to get finished and closed by September 1, please move it back to 2.7.