OpenLayers OpenLayers

Ticket #893 (closed feature: fixed)

Opened 9 months ago

Last modified 8 months ago

specify externalGraphic offset

Reported by: fredj Assigned to: fredj
Priority: minor Milestone: 2.5 Release
Component: Layer.Vector Version: SVN
Keywords: Cc:
State:

Description

It would be nice if externalGraphic handle offset just like OpenLayers.Layer.Markers does.

Example:

externalGraphic: "my_icon.png",
graphicWidth: 18,
graphicHeight: 26,
graphicYOffset: 0,
graphicXOffset: 9,
fillOpacity: 1

Attachments

externalGraphicOffset.code.patch (2.3 kB) - added by fredj on 08/08/07 02:56:37.
Renderer/VML.js Renderer/SVG.js patch
patch-893-A0.diff (6.4 kB) - added by elemoine on 09/11/07 15:41:08.
new patch based on fredj's (see comment)
patch-893-A1.diff (6.4 kB) - added by elemoine on 09/11/07 16:05:26.
New patch fixing a stupid error in the tests. Still don't know if the tests pass on IE. I'll give it a try myself once I have IE handy.
patch-893-A2.diff (7.7 kB) - added by elemoine on 09/12/07 16:03:57.
patch-893-A3.diff (8.0 kB) - added by fredj on 09/13/07 05:23:24.
Fix the VML problems found in patch-893-A2

Change History

(follow-up: ↓ 2 ) 08/07/07 08:44:41 changed by fredj

  • status changed from new to assigned.

08/08/07 02:56:37 changed by fredj

  • attachment externalGraphicOffset.code.patch added.

Renderer/VML.js Renderer/SVG.js patch

(in reply to: ↑ 1 ) 09/11/07 15:40:34 changed by elemoine

  • keywords set to review.

Replying to fredj:

New patch based on fredj's. Applies to current SVN. Includes an example of use in vector-features.html. Adds appropriate tests in Layer/test_Vector.html. Tests pass on FF. Could someone run the tests on IE (no w$ handy)?

09/11/07 15:41:08 changed by elemoine

  • attachment patch-893-A0.diff added.

new patch based on fredj's (see comment)

09/11/07 15:44:55 changed by crschmidt

  • milestone set to 2.5 Release.

I'll review this and run tests in IE.

09/11/07 15:53:18 changed by crschmidt

Tests pass in IE.

09/11/07 16:01:37 changed by crschmidt

Actually, they don't. I've let Eric know what the failure is. (Sorry, I had two copies of OL accidentally and didn't know it.)

09/11/07 16:05:26 changed by elemoine

  • attachment patch-893-A1.diff added.

New patch fixing a stupid error in the tests. Still don't know if the tests pass on IE. I'll give it a try myself once I have IE handy.

09/11/07 16:23:45 changed by crschmidt

New tests pass in IE now! (Really, this time.)

(follow-up: ↓ 9 ) 09/12/07 02:28:11 changed by fredj

Thanks for to patch Eric.

The new can patch can cause problem if graphicXOffset or graphicYOffset are set to 0:

var xOffset = style.graphicXOffset || (0.5 * width);

set xOffset to "0.5 * width" because 0 is evaluated as false.

Other problem:

In test_Vector.html, near line 254 the test pass because root.firstChild.getAttributeNS(null, 'x') and (geometryX / renderer.getResolution() + renderer.left) both return NaN.

09/12/07 07:22:33 changed by crschmidt

  • keywords deleted.

Removing review keyword. Eric, could you prepare a new patch?

Fred, thanks for reviewing this one.

(in reply to: ↑ 7 ) 09/12/07 16:03:25 changed by elemoine

Replying to fredj:

Thanks for to patch Eric. The new can patch can cause problem if graphicXOffset or graphicYOffset are set to 0: {{{

var xOffset = style.graphicXOffset (0.5 * width); }}} set xOffset to "0.5 * width" because 0 is evaluated as false.

I fixed this by testing whether graphicX|YOffset is undefined or not, in the same way you did in your patch.

Other problem: In test_Vector.html, near line 254 the test pass because root.firstChild.getAttributeNS(null, 'x') and (geometryX / renderer.getResolution() + renderer.left) both return NaN.

Good catch! The problem came from the fact that renderer.setCenter() was never called resulting in renderer.left and renderer.top being not set. I added a map.zoomToMaxExtent() so that renderer.setCenter() gets called.

To be consistent with the way the original marker offset works, I also did this: graphicXOffset must be negative to offset the marker on the left, and graphicYOffset must be negative to offset the marker to the top (and vice versa).

Again, I haven't run the tests on IE, and they'll probably fail. Will do that as soon as I can. I'm posting the patch anyway, for fredj and others to review...

09/12/07 16:03:57 changed by elemoine

  • attachment patch-893-A2.diff added.

(follow-up: ↓ 11 ) 09/13/07 05:21:03 changed by fredj

I checked to code on IE and found a problem with graphicYOffset.

With the VML renderer, the y coordinates are flipped so the yOffset have to be flipped too.

To have a 'visual' check, apply the patch and load examples/vector-features.html: the bottom of the red marker (the one on the right) have to be exactly on the green point.

(hope you can understand my poor English)

The following patch fix the problem and the units tests.

(hope that my code is not as bad as my English ...)

09/13/07 05:23:24 changed by fredj

  • attachment patch-893-A3.diff added.

Fix the VML problems found in patch-893-A2

(in reply to: ↑ 10 ) 09/13/07 14:54:02 changed by elemoine

Replying to fredj:

I checked to code on IE and found a problem with graphicYOffset. With the VML renderer, the y coordinates are flipped so the yOffset have to be flipped too. To have a 'visual' check, apply the patch and load examples/vector-features.html: the bottom of the red marker (the one on the right) have to be exactly on the green point. (hope you can understand my poor English) The following patch fix the problem and the units tests. (hope that my code is not as bad as my English ...)

Thanks a lot fredj. The patch looks complete and good now.

09/13/07 15:26:50 changed by elemoine

  • keywords set to review.

09/13/07 15:58:47 changed by crschmidt

  • keywords changed from review to commit.

Erik and I discussed this, and I'm willing to call this one ready -- commit when ready.

09/13/07 16:36:38 changed by elemoine

  • keywords deleted.
  • status changed from assigned to closed.
  • resolution set to fixed.

(In [4268]) allow user to specify offsets for externalGraphic (closes #893)