OpenLayers OpenLayers

Ticket #995 (closed feature: fixed)

Opened 1 year ago

Last modified 9 months ago

add support for MapGuide OS layer type

Reported by: madair Assigned to: madair
Priority: minor Milestone: 2.6 Release
Component: Layer Version: 2.5
Keywords: Cc:
State: Complete

Description

add a new MapGuide layer type.
Single tile version requires a proxy script if the maguide service is not on the same host.
Includes an override of initGriddedTiles for the tiled version since MapGuide tiles are initialized starting at the top-left, rather than bottom-left corner.

Attachments

mapguide.patch (30.3 kB) - added by madair on 01/29/08 14:51:38.
mapguide.2.patch (34.3 kB) - added by crschmidt on 02/09/08 14:02:57.
mapguide.3.patch (29.2 kB) - added by crschmidt on 02/09/08 14:36:01.
mapguide_grid.patch (28.8 kB) - added by crschmidt on 02/09/08 23:57:12.
mapguide.5.patch (29.4 kB) - added by madair on 02/13/08 16:29:48.
clipParam.patch (0.5 kB) - added by madair on 03/05/08 10:58:59.
clipParam.2.patch (378 bytes) - added by madair on 03/05/08 11:06:32.

Change History

12/18/07 15:19:17 changed by tschaub

  • state changed.

I think a patch for this sounds good.

01/18/08 11:35:10 changed by madair

  • state set to Review.
  • version changed from 2.5 RC1 to 2.5.

proxy script no longer required for baselayers. It is still required for transparent overlays though.

01/21/08 13:58:25 changed by madair

  • milestone set to 2.6 Release.

01/21/08 19:34:03 changed by crschmidt

  • state changed from Review to Needs More Work.
  1. Old style docstrings. Need to be upgraded to naturaldocs format. I know this patch is old enough that these things arne't entirely the fault the patch, but they need fixing.
  2. no docstrings at all for 3 parameters.
  3. docstrings for DEFAULT_PARAMS which doesn't exist -- needs to be changed to TILE_PARAMs, etc.
  4. Remove the unused Basic Auth hash.
  5. Why are there _getExtentFoo no-ops?
  6. updateExtents and getURL seem to have duplication of code -- one uses getParamString, one doesn't. They both need to use getParamString, and either the code duplication needs to be removed or explained.
  7. tileSize should be set in options on creation -- the setTileSize shouldn't be neccesary? if it is neccesary to have the default be 300,300, then set it on 'this' in the initialize before the Grid.initialize.apply, so that it can be overridden in the options, unless it should never be overridden by the user (unlikely).
  8. Synchronous requests are bad, because they lock up the browser. I don't see evidence that we have a Very Strong Need for this -- either explain it, or refactor it away.
  9. addTile uses a 'url' variable which is not defined in the function. Where is it coming from?
  10. default for reproject is false. You can remove this from your code, since it's not overriding a default.
  11. example uses HTML comments in <script> tag -- we dropped those a thousand revisions ago or so. Apparetly they're only needed in Netscape 2. If you're using netscape 2 with OL, I'd love to hear about it.
  12. Basic tests are required for any new layer type: at least to test whether the layer succeeds at constructor-ing, etc. There are plenty of examples to go from.
  13. Update copyright dates to 2008.

01/29/08 14:51:38 changed by madair

  • attachment mapguide.patch added.

01/29/08 14:59:35 changed by madair

  • state changed from Needs More Work to Review.

new patch to address the review comments. Replies to specific items below

7. not sure what this comment is trying to get at. For MapGuide, the tile size is fixed at 300x300, with no way to change that (at least that I know of)

8. for the transparent overlays, the MGOS architecture requires a call to GETVISIBLEMAPEXTENT (which sets the extent of the map) before calling GETDYNAMICOVERLAYIMAGE. There is no way around that. As discussed in an email thread this morning (29/1/08), to set the image URL asynchronously would require some work. Therefore the synchronous call must be made here so that getUrl returns a valid URL.

12 basic tests added, no doubt these can be improved

01/31/08 17:31:41 changed by crschmidt

  • state changed from Review to Needs More Work.

For XML requests required for tiles, there is a clear path forward: a tile can be created, with an async XML request, that will set the img.src on the tile once that request returns.

Based on IRC discussion, it is my personal opinion (backed up by some low-level agreement) that we should do one of two things:

  • Actually implement a tile.mapguide which does this requesting, making the request asynch instead of synchronous
  • Pull out the getdynamicoverlayimage support, and let the patch go in without that particular functionality.

I understand that the 2.6 deadline is growing near, and that the former may be difficult, so I'm willing to support the latter (MapGuide layer without GETDYNAMICOVERLAYIMAGE) if that's useful at all. (As I understand it, the difference is single tile vs. tiled -- I could be wrong on this.)

Bumping to needs mor ework at this time.

02/07/08 13:36:21 changed by madair

I had a quick look into changing the synchronous request to async, however it looks like it is more work than I'm prepared to do at this time.

My preference would be to leave it in as is with a "Developer beware!" warning attached, not only because it's synchronous, but also because for that particular case you need to pass a valid session ID to it. This means that the untiled transparent overlay can only be used with some framework code to create the MapGuide sessions (e.g. Fusion).

However, if synchronous requests are banned from the OL codebase, then I would be happy to remove this piece until the Tile.Image.draw method can load the img.src attribute asynchronously.

02/09/08 14:02:57 changed by crschmidt

  • attachment mapguide.2.patch added.

02/09/08 14:36:01 changed by crschmidt

  • attachment mapguide.3.patch added.

02/09/08 14:40:32 changed by crschmidt

There are two outstanding issues here:

  • A number of things which may be *just* parameters (not used in the API) are defined as 'options' on the layer. If something is only designed to be sent to the server, then it should be a parameter. Options are used for internal knowledge of the library, params are directed to the server.
  • The Layer.Grid change is halfhearted at best. Instead of this, the grid initialization work should be moved into a seperate function which layers can override. I got partway into this before realizing that I had no idea what the dividing line on that should be, so it still needs to be done.

This patch cleans up the example and some of the layer documentation, creates a 'params' parameter on the layer, and moves most of the documentation of things which are clearly parameters out of being options on the layer. The other properties should get a similar review: for example, to me, 'session' feels like a parameter, not a property.

02/09/08 19:58:32 changed by crschmidt

#1349 is a blocker for this patch, and will implement the replacement for the grid origin change in this patch.

02/09/08 23:57:12 changed by crschmidt

  • attachment mapguide_grid.patch added.

02/09/08 23:58:15 changed by crschmidt

Latest patch depends on #1349

02/10/08 09:09:23 changed by crschmidt

Having read through the code several more times, I believe that I'm of the opinion that all of the properties which are currently listed as Properties should be moved into params (minus inherited properties like isBaseLayer and singleTile), with documentation in that section. It does not appear that these properties are used at the application level in any important way. The only possible exception is the 'if' statements that only assign things to params based on whether the layer is single-tiled or not: I believe that this is not hugely important though, because sending the additional properties doesn't hurt the request, so far as I can tell. (MapGuide, like all sane servers, doesn't object to additional parameters being sent.)

Mike, can you correct me if I'm wrong here? Is there any reason why these things are needed at the application level, or can they all move into the params hash with appropriate documentation?

02/10/08 09:38:38 changed by crschmidt

Untiled overlays on a different server depend on #1350.

02/13/08 14:34:29 changed by madair

it wasn't clear to me when something should be an option or a parameter but with the explanation above it makes more sense. And given the explanation, most of those things should be params. I set some of those params as options (session, mapname, groupname, mapdefinition) because they will not be changed once the layer gets initialized. I will move these to request params

02/13/08 16:29:48 changed by madair

  • attachment mapguide.5.patch added.

02/13/08 16:30:57 changed by madair

  • state changed from Needs More Work to Review.

remaining request parameters moved to be params rather than layer options. updated example and tests.

02/25/08 21:56:40 changed by crschmidt

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

(In [6368]) Add support for MapGuide Open Source layers in OpenLayers. Great work by Mike Adair on following this one through. Includes example, tests, code, etc. r=me (Closes #995)

03/05/08 10:58:59 changed by madair

  • attachment clipParam.patch added.

03/05/08 11:00:48 changed by crschmidt

If the param is always 1, probably should go in the DEFAULT_*_PARAMS?

03/05/08 11:06:09 changed by madair

  • status changed from closed to reopened.
  • state changed from Complete to Review.
  • resolution deleted.

MGOS 2.0 has added a new parameter to improve performance with non-tiled layers: http://trac.osgeo.org/mapguide/ticket/458

(also should add a Layer.MapGuide to the trac components since this is in trunk now)

Good point Chris. Updated patch to follow.

03/05/08 11:06:32 changed by madair

  • attachment clipParam.2.patch added.

03/05/08 16:06:42 changed by crschmidt

  • status changed from reopened to closed.
  • state changed from Review to Complete.
  • resolution set to fixed.

(In [6438]) MapGuide 2.0 (just released) has support for a 'clip' parameter, which is designed to help performance with non-tiled layers. Patch from madair to take advantage of this in OpenLayers. Thanks Mike. r=me, (Closes #995)

03/05/08 16:07:08 changed by crschmidt

Mike:

In the future, please make new tickets for things like this, thx