Currently for vector layer types like KML, OpenLayers Views Vector, WFS, etc., we just have a long list of projections they support. This isn't a good idea, because it isn't true - they support any projection, via reprojection. This should be a property of the layer type and should be taken into account when showing layer lists, etc.

CommentFileSizeAuthor
#1 vector_layer_type_property.patch3.77 KBtmcw

Comments

tmcw’s picture

tmcw commented
StatusFileSize
new vector_layer_type_property.patch3.77 KB

An initial patch

zzolo’s picture

zzolo commented
Status: Active » Needs review
zzolo’s picture

zzolo commented

Overall this seems pretty cool. Basically you are adding a "vector" property to layers, then with some logic, allowing them to show up in any list. I am confused because it does seem like you are providing a projection to the layers as well as denoting vector, is this necessary?

Some small details.

  1. In the future, it would be cool if you provided patches the usual Drupal way, specifically so that Dreditor can parse it correctly and its easier to review. http://drupal.org/patch/create
  2. >     if (
>       // not filtering by projection
>       !isset($projection) || 
>       // this layer has an appropriate projection
>       in_array($projection, $layer->data['projection']) ||
>       // this layer can be reprojected because it is vector
>       (isset($layer->data['vector']) && $layer->data['vector'])
>     ) {

    This is pretty ugly, it is probably better to have a block of comments above the if statement, like so:

        // Determine if layer should be included:
    //   - Not filtering by projection
    //   - Has an appropriate projection
    //   - Can be reprojected because it is vector
    if (!isset($projection) || in_array($projection, $layer->data['projection']) ||
      (isset($layer->data['vector']) && $layer->data['vector'])) {

I'm on crack. Are you, too?

tmcw’s picture

tmcw commented

zzolo: the layers will need a projection, because they do indeed have them. While KML is limited to 4326, more complex vector formats like GML can support multiple projections, so we shouldn't abandon projection entirely for them.

I'll see how it goes with inline comments. I adhere to the 80-chars-per-line rule pretty strictly.

zzolo’s picture

zzolo commented

Sorry, confused about layer projection and display projection. Sounds good.

Sorry, my comment was aimed at the style of commenting, not the length of the line; feel free to change as needed.

zzolo’s picture

zzolo commented
Status: Needs review » Reviewed & tested by the community

I don't think the patch applies anymore, but I would say this looks good to me.

tmcw’s picture

tmcw commented
Status: Reviewed & tested by the community » Fixed

Finished, re-tested, committed: http://drupal.org/cvs?commit=367038

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.