It would make sense to rethink the dynamic styling that was introduced in 1.x and introduce it here in a better, more export-and-UI friendly way. This might require a behavior, or just more code in the styling logic, or possibly styles-as-objects akin to layers and behaviors.

Files: 
CommentFileSizeAuthor
#120 style_plugin11.patch33.61 KBstrk
PASSED: [[SimpleTest]]: [MySQL] 221 pass(es).
[ View ]
#117 style_plugin10.patch33.52 KBstrk
FAILED: [[SimpleTest]]: [MySQL] 221 pass(es), 0 fail(s), and 83 exception(es).
[ View ]
#110 style_plugin9.patch32.38 KBstrk
FAILED: [[SimpleTest]]: [MySQL] 221 pass(es), 0 fail(s), and 83 exception(es).
[ View ]
#109 style_plugin8.patch32.25 KBstrk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch style_plugin8_0.patch.
[ View ]
#106 style_plugin8.patch0 bytesstrk
PASSED: [[SimpleTest]]: [MySQL] 221 pass(es).
[ View ]
#104 style_plugin7.patch31.59 KBstrk
FAILED: [[SimpleTest]]: [MySQL] 221 pass(es), 0 fail(s), and 83 exception(es).
[ View ]
#102 style_plugin6.patch32.66 KBstrk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch style_plugin6.patch.
[ View ]
#100 style_plugin5.patch26.67 KBstrk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch style_plugin5.patch.
[ View ]
#97 style_plugin4.patch26.79 KBstrk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch style_plugin4.patch.
[ View ]
#84 style_plugin3.patch13.9 KBstrk
FAILED: [[SimpleTest]]: [MySQL] 221 pass(es), 0 fail(s), and 1 exception(es).
[ View ]
#84 wash_openlayers.tgz1.86 KBstrk
#80 710908_styles_fields.patch18.62 KBtmcw
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 710908_styles_fields.patch.
[ View ]
#78 style_plugin2.tgz1.15 KBstrk
#77 dynamicStyle.patch14.66 KBstrk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch dynamicStyle.patch.
[ View ]
#77 style_plugin.tgz1.17 KBstrk
#72 710908-style_plugin-72.patch17.42 KBzzolo
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 710908-style_plugin-72.patch.
[ View ]
#70 710908-style_plugin-70.patch17 KBzzolo
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 710908-style_plugin-70.patch.
[ View ]
#59 openlayers_ui.styles.inc_.3.patch4.3 KBstrk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch openlayers_ui.styles.inc_.3.patch.
[ View ]
#58 openlayers_ui.styles.inc_.2.patch4.28 KBstrk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch openlayers_ui.styles.inc_.2_0.patch.
[ View ]
#57 openlayers_ui.styles.inc_.2.patch4.56 KBstrk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch openlayers_ui.styles.inc_.2.patch.
[ View ]
#52 openlayers_ui.styles.inc_.patch4.4 KBstrk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch openlayers_ui.styles.inc_.patch.
[ View ]
#47 710908-style_plugin-47.patch14.06 KBzzolo
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 710908-style_plugin-47.patch.
[ View ]
#41 710908-style_handler-41.patch8.77 KBzzolo
PASSED: [[SimpleTest]]: [MySQL] 221 pass(es).
[ View ]
#37 710908-variable-styling-context-37.patch6.14 KBzzolo
PASSED: [[SimpleTest]]: [MySQL] 221 pass(es).
[ View ]
#33 styles_all.patch10.38 KBtmcw
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch styles_all.patch.
[ View ]
#32 callback_pointRadius.patch8.08 KBtmcw
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch callback_pointRadius.patch.
[ View ]
#19 sld_styles.patch3.65 KBtmcw
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sld_styles.patch.
[ View ]

Comments

A few ideas for approaches:

1. PHP based. Layers can declare a 'dynamic style' method that will be called on each feature, and should return a style array. Alternatively it could return FALSE / NULL to use the default.

2. JavaScript based. This would basically be a behavior that would look for a 'dynamic style' property in the layer that references a javascript function, and then would run each feature through that function.

3. Ajax / JSON based. Most flexible but likely to slow in practice. Similar to 2, except it's passed to PHP on the fly, which then return JSON.

Number 3 is obviously the most flexible, but would likely be too slow and cumbersome. I personally prefer implementing number 1, and then if we wanted to implement the others as behaviors, then great. Any other alternative? Ideas?

Another thought: In 1.x we simply allowed features to have a 'style' attribute that took priority of the default style. Other modules then just used whatever methods they wanted to fill this out. This would work to, but I think explicitly having a dynamic style system would be better.

We'll definitely need any kind of dynamic styling / more complex system to be completely exportable, and anything that has a kind of 'loose linkage' (like a reference to javascript that isn't a hard reference in Drupal / ctools).

I think that it might make the most sense to do something that exposes a larger set of the rule-based styling that OpenLayers provides to the UI, and then the full set to the backend. Possibly this would be, like the UI provides basic dynamic styling (scale on X, if a = b, scale as blah), and then output what is needed to construct the filters and symbolizers at runtime.

I think I've said it before, but this kind of thing would really excel because it would make it possible to style any kind of vector points (kml, views) with the same styling logic. Anything beyond what you can do programmatically with the OpenLayers style format (which is the minority of use cases I can imagine) would require a custom stylemap, which just means that the render() method of the style object would add a javascript file instead of rendering its data for the openlayers.js code to interpret.

Also I think that PHP-rule-based styling could be modular enough to work well for this, but there are some problems to be addressed. With 300 points on a map, having a complete stylemap for each of them would make the size of the javascript several times larger.

Given the constraints of being exportable and utilizing OL's native styling rules, here are a couple ideas.

1) Very basically we add a way to write sytle rule in JS that are stored in the style object. This allows for exporting as well as keeping performance optimal. This leaves teh coding simple and the styling still powerful. It does make it difficult for people to write dynamic styles and will mean more manual coding if we want to offer a UI to it.

2) We could abstract that more and start to write a PHP way of representing the OL styling logic. This would allow a lot more flexibility in export/import as well as in the UI, but it means a more complex/abstract system. On the flip side, this will be more coding on the backend to support this method.

In the direction of using existing OpenLayers functionality, we could use the SLD parser (for the Styled Layer Descriptor format) instead of writing any more strings-to-objects de-serialization type stuff, and also allowing for people to write their own SLD stylesheets if the UI is insufficient.

Potentially important to this conversation is that I think previews for layers, styles, and presets, should be high on the roadmap.

Priority:Normal» Critical

So, I just documented the different ways to get variable styling in the 1.x version: http://drupal.org/node/734526

At the moment, we can actually support a very simple but powerful way of styling and that is with feature attributes. If attributes are defined on a features, a style can be defined like so, which will pull values from the attributes:

<?php
  
array(
     
'pointRadius' => '${radius}',
     
'fillColor' => '${color}',
     
'strokeColor' => '${color}',
     
'strokeWidth' => 4,
     
'fillOpacity' => 0.95,
    ),
?>

And this is all exportable. Someone can define these in the interface easily.

As well as views gets to handle all the logic of what the value of that attribute is (so computed fields and the such). Of course, this would require a fair amount of documentation, but all options would as well.

I think if we go supporting this idea more, I would suggest that we provide a set of checkboxes on the views display plugin to choose what fields are put into attributes, that way we are not just dumping everything in JS.

And last, I don't think creating some sort of abstract language for variable styling is the best use of our time. It also sounds very annoying to maintain, but maybe it is simpler than I imagine.

So, I think we should two things to support dynamic styles:

1) The attribute replacement syntax. This just needs some more documentation, and it can be handled through the interface.
2) Add ability to set a context callback with styles. This will be JS based and can easily provide the rest of any flexibility that is not allowed by simple attribute replacement.

This avoids any heavy theme overrides or supporting some rule based syntax, but still gives complete flexibility.

I don't really agree. I'll post more, but we wouldn't be supporting a rule-based syntax - is is what openlayers already has and recommends for complex uses. We're going to need to discuss this further.

And last, I don't think creating some sort of abstract language for variable styling is the best use of our time. It also sounds very annoying to maintain, but maybe it is simpler than I imagine.

I think there's a disconnect here - I pointed to the SLD documentation. We aren't creating or supporting it, it's a standard, and what the OpenLayers javascript library (along with GeoServer and other software) supports. This would support a lot of really basic use cases for which simple variable-based styling falls flat - like, for instance, styling on anything that is not continuously variable, like making a shape a different color if it is above a certain quantity. Yes, attribute-variable styling is the simplest idea here, but I don't think it'll fit even a minority of cases. A callback system may work, but I have my doubts about it - mainly I think that having a stylesheet of sorts will allow us to have styles which are actually exported with presets or layers when they are exported - instead of a loose linkage between Javascript and PHP which will be a hack to export if we try.

I think I was just trying to keep scope down. I understand that SLD is a good standard for this and a good choice. My initial thought is that it will take a fair amount of work to support it, but I don't fully understand it, and I fully back you if you want to put it in.

I think adding a context callback (as supported by OpenLayers) is a good idea. I understand that it is not directly exportable, but neither are behaviors (as far as I understand) in the sense that their JS callbacks and includes are not being exported. I think having a real simple system where someone can do much more than simple replacement and simple logic is important.

I'm not an SLD expert either, but, like developing other parts of this system (like OpenLayers events), we need to start considering reading the documentation and the source of the OpenLayers javascript library as a good thing - or we're likely to rebuild things that exist and build a larger module than needed.

Behaviors aren't exportable because they aren't exportables: layer types and behaviors are both not exportables - this means that exported features with presets and layers which use layer types and behaviors will require the module that provides them, thus ensuring that people won't have a broken map in the end. A naïve implementation of context callbacks probably wouldn't include this and would allow for people to implement context callbacks with random bits of javascript, instead of modules. If we make a style context system on par with behaviors and layer types, then it will fly in this system.

I agree with all points. I think my main thought is to try to make it easier for users to write a quick snippet to get some variable styles. There seems to be a struggle between ease of use and exportable-ness (roughly speaking).

One thing I guess I am not quite clear on: For behaviors, how does whatever mechanism that is exporting know to include the module that provides it? This is not something we support in exporting within out module; does Features know about this?

Just an FYI, the openlayers_styles theme is still in the module. From this discussion, it sounds like we want to remove it. Just to give reference, the thought of adding this as a theme was to be able to give themers a standard Drupal way to change how features were styled. I think this should be removed once we find a standard way to support styles that support some sort of callback and/or rule-based system.

I still don't understand your aversion to JS callbacks, @tmcw. I don't see the difference between having a callback as an option and using behaviors (openlayers, not drupal) as we do. Both are callbacks in a sense, and both require a module (or code) to be required, but no where in our module do we actually have a check to ensure that that module (or code) is around, therefore a hole in the idea of a map truely being exportable in my opinion.

I'd like to get clarity here because there are a number of issues in the queue that have some ideas floating around about callbacks.

I'm not sure about the state of the code that requires all of the component modules that provide callbacks, but there's a strong possibility for maintaining a good system of making sure that code doesn't break, and for properly keeping code modular - something that arbitrary user-input callbacks just doesn't even come close to fitting. As I've said before, there's also this bizarre situation in which having a loose dependency on javascript code that people shove in somewhere - and actually writing a behavior, style plugin, or whatnot - are the same quantity of code, just one's easier to hack together and the other one's more sustainable. So, yes, there may have been a regression in keeping features dependent on the proper modules or some more work to be done there (I think possibly on the implementing-modules side), but I just don't get how it could possibly make sense on any level to have users input the names of Javascript functions in the UI (or in... PHP), and for that to be better than a system that has the possibility of maintaining dependencies. There are also some really crappy interactions between PHP and Javascript that will happen here (inputting namespaced function names will inevitably boil down to some equivalent of eval()), and how can we handle Javascript scope? Or include order? There just aren't any positives and so many potential gotchas.

StatusFileSize
new3.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sld_styles.patch.
[ View ]

Here's my first patch for SLD styles being included here - it's very basic, showing exactly how much code is needed to support the very fundamentals of SLD styling (it's not a ton). There's a built-in TODO list of comments - basically there are some restrictions right now and there should be validation. And for testing, here's the SLD I'm working with: http://gist.github.com/348351

I tried out the patch and a brief test shows everything is fine. Good work.

I am not sure why, but I had the impression that SLD was much simpler than the bulky XML. I think we should put this in here, but this is very much not realistic for most people (even developers) to use (I would never bother writing this), unless there happens to be a really nice online tool for making these.

As far as the callback stuff, I definitely support the idea of things being sustainable and exportable. As it stands, there is no module dependency code in our module for behaviors. I'll start a separate ticket for this.

Specifically for styling, I just think it's extremely important to to give users (site builders) a straightforward way to be able to style a layer with code. Here are my main points as to why a callback system would be good, though I think we can make sure its exportable and sustainable.

* SLD is so unusable (though its a great feature)
* Attribute replacement is limiting
* Someone can utilize something like a computed field or some Views plugin to make some fields/attributes do some crazy things, but I feel this is convoluted.
* A theme layer function is heavy and is PHP centric, though honestly, this may still be a good method.
* Utilizing OpenLayers existing API is a good thing.

All this to say that we should support the OpenLayers ability to use a callback for styling. Now, this does not mean we have to just arbitrarily add a textfield, but it would be cool to think of a way to do this that would be elegant, but also, not be as rough as adding behaviors.

Yes, it somewhat sucks that SLDs are XML, but they are rather equivalent in expression to CSS - despite the initial jolt of seeing closing tags, etc., there's really not much to understand. I don't really see how it's 'so unusable' - SLD is the standard used by GeoServer and MapServer, and Mapnik uses a close cousin. It is the only real, standardized option in web-based mapping, and CSS-syntax is only a syntax-candy addition over it, in the form of Cascadenik. Dismissing SLD as unusable because it requires learning (ala CSS) and some XML syntax just isn't going to fly.

A callback system may be acceptable if it were built very much like behaviors, but... how 'rough' it is to add behaviors? I just don't get it, at all. Like, there must be some disconnect between this being incredibly rough and a typical behavior consisting of under 30 lines of PHP and 20 lines of Javascript...

Note that a 'builder' for SLD is entirely in the works, and would be relatively easy to construct. Hence a 'fundamental implementation' patch.

The SLD code you provided basically says this in about 100 times more code:

strokeColor = (name != "... island") ? '#000000' : '#FF00FF';

I think it's a totally fine standard and that supporting it is a good thing, I just don't think anyone building a site in Drupal, including myself, will ever write this from scratch. It is not unusable, its just unrealistic to write, IMO. Its not difficult to learn, its just not worth learning it for most people. I am not going to stop your efforts in putting it there, but I still feel strongly about supporting the OpenLayers callback styling.

What is rough about behaviors is that it requires 3 functions to be written (hook, callback, and JS). And yes, this is not a whole lot of code, nor is it all that complicated in our eyes, but the idea of adding 3 different functions to support a callback for styling I think is a little much. The behaviors system is powerful, but if a themer or site admin who knows just a little bit of code, wants to make a behavior, they are going to be really stuck. I think for variable styling, this is the majority of our audience, and we should focus on the ability to write as little code as possible to get variable styling up.

Ill write up a patch this weekend for a callback system.

How can a hook, a callback, and a snippet of javascript be too much? The input-random-values method will work for 80% of people, and then 10% will be served by just filtering by views and then applying different styles, and then people who fancy themselves site builders should be able to write three functions, two of which are boilerplate.

Note, as well, that SLD is like CSS - it is *not functional code*, while your example is. There's an extremely significant difference that should be understood here.

So, thinking about this a bit. What about this:

* A hook that collects style_callback implementations. I don't see the need for the CTools plugins for this, but I am open to discuss. This only really needs a callback name and a JS file location.
* A drop-down (like the SLD patch) for styles that assigns that callback to the style.
* As far as data stored in the style, it can be the name of the style_callback and maybe the module as well (to deal with dependencies).
* On map render, its a simple process of including the JS file and passing along the name of the callback. I forget exactly how the style callbacks are formed in the OpenLayers library but we may be able to simply just pass along the correct array in PHP and have it handled correctly by OpenLayers.

This creates the strong connection between styles and callbacks, and also allows for reusing callbacks.

I'll post more later, but concerns: If it's just a callback and a js file location, then we don't get any dependency management for free.

If imperative styles are a necessity, then why we just create a sturdy basis for people to build behavior-styles? There really isn't much code involved: here's how openlayers_plus does scalepoints. This would basically fit every use case you want but would also make it as formal as I would like it to be.

I don't see how we could not easily support module dependency with a hook implementation? A behavior does not actually know what module it came from? Does CTools provide a built-in mechanism for determining what module a hook comes from?

I also don't see how the behavior system is more sturdy in this case? For behaviors, it is the better solution as a behavior encompasses behavior-specific PHP functionality, user settings, and JS functionality. The only goal that I want to achieve is to support OpenLayers' built-in ability to use JS callbacks for styling. Adding that whole system so that a style can use a callback seems like way overkill, while not adding any benefit, IMO. Maybe I am missing something.

I am not sure how openlayers_plus actually supports the flexibility of a callback? It just scales the radius?

I am not saying that this is the best way necessarily. I just want to be able to support style callbacks and I understand your concerns, but I just don't see the benefit of using something as robust like what we do for behaviors

I don't see how we could not easily support module dependency with a hook implementation? A behavior does not actually know what module it came from? Does CTools provide a built-in mechanism for determining what module a hook comes from?

#756692: Ensure layer types and behaviors module dependency is handled correctly in Features

I also don't see how the behavior system is more sturdy in this case? For behaviors, it is the better solution as a behavior encompasses behavior-specific PHP functionality, user settings, and JS functionality. The only goal that I want to achieve is to support OpenLayers' built-in ability to use JS callbacks for styling. Adding that whole system so that a style can use a callback seems like way overkill, while not adding any benefit, IMO. Maybe I am missing something.

Dependencies, scope, encapsulation.

I am not sure how openlayers_plus actually supports the flexibility of a callback? It just scales the radius?

It adds style rules, not a callback. It really seems like that's the more-supported system and that callbacks are a hackish way to do styles in OL

I am not saying that this is the best way necessarily. I just want to be able to support style callbacks and I understand your concerns, but I just don't see the benefit of using something as robust like what we do for behaviors

Because the theoretical problems with having non-exportable, non-dependency-managed, non-universal hacks become very concrete problems when dealing with a real-life workflow that's based on exporting and building features or distributions.

Please be aware that a "real-life workflow that's based on exporting and building features or distributions" is awesome, but it is not the only way to manage and deal with Drupal projects, and that a large amount of people don't have this workflow and don't need it. I agree with supporting this workflow paradigm as much as possible, but to think that we cannot go outside so that we can support other workflows and the ability to have a much more flexible system seems really limited to me and I don't agree.

So, how about this:

1) We create a way to support OpenLayers Rules-Based styling (though I am not a big fan of this, I still think its useful)
2) We create a way to support callbacks. We can easily document the limitations of this method, and there are plenty of methods other people can use that support your workflow. But I still feel this is very useful for a lot of people.

Sorry if this is off-topic but another way I'm thinking I'd find useful (and might implement that next) woudl just be adding
a 'context' textfield to the current style UI.

That would be used for something like this:

var style = new OpenLayers.Style({
pointRadius: "${radius}",
fillColor: "#ffcc66",
fillOpacity: 0.8,
strokeColor: "#cc6633",
strokeWidth: "${width}",
strokeOpacity: 0.8
}, {
context: {
width: function(feature) {
return (feature.cluster) ? 2 : 1;
},
radius: function(feature) {
var pix = 2;
if(feature.cluster) {
pix = Math.min(feature.attributes.count, 7) + 2;
}
return pix;
}
}
});

So you'd basically write the 'context' part in a textarea and use keyword substitution in the options values.
The snippet was taken from here (so you can see the effect): http://openlayers.org/dev/examples/strategy-cluster-threshold.html

StatusFileSize
new8.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch callback_pointRadius.patch.
[ View ]

All right, if we're supporting callbacks, let's support callbacks. Here's an initial patch - it could use more work, obviously - only pointRadius is specified, and the user must input ${pointRadius} as the value of point radius in order for it to be used. Instead it should be a toggle that automatically sets the value to ${pointRadius}. I think that it's necessary to make these functions one-per-attribute for simplicity. The way that this pushes JS around and writes it is a little dingy, but it works in my testing.

But, on the bright side, this approach does guarantee that exported styles will work, because the Javascript is bound to the style itself.

Status:Active» Needs review
StatusFileSize
new10.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch styles_all.patch.
[ View ]

An updated patch that cleans up the code, removes unnecessary debugging, and covers all of the UI-advertised attributes. This code will also work for other attributes that people define in exported styles.

Status:Needs review» Active

On the other hand, this is a horrible idea - now that I think of it, we can't have users inputting arbitrary Javascript via the UI. It's slightly better to have this be a code-only scenario (which means an exportable or plugin), but still risky and really, really bad technique.

This week is really, really busy, but I would really like to put in a patch for this as well and will do it this weekend.

I agree, arbitrary JS is not a good idea. Then again it does have some benefits.

Another idea, if we don't want arbitrary javascript, could be having the possibility for external modules (and providing some default ones) to add handlers for the different style attributes.

For example, take the current pointRadius style attribute (which is the one I need to hack).
The widget for it is currently a textfield, instead would be a pull down menu to select the handler
and each handler would print its own form. The default handler would be a static radius, and its form
would be the current textfield. Another handler could be an 'attribute-based' value, showing as the form
another select to pick the argument. Another (the one I'd like to do) would be a, say, 'proportional to number of clustered features'
handler.

Does it sound something feasible?

Status:Active» Needs review
StatusFileSize
new6.14 KB
PASSED: [[SimpleTest]]: [MySQL] 221 pass(es).
[ View ]

So, here is a patch to support context styling (callbacks). It follows what is being done on the SLD patch above.

There are three new properties in the style data:
* context_enable: tells openlayers to render style with context
* context_include: what JS file to include
* context_object: what object to use as the context

In the UI, this means that there is a checkbox to enable context for each style, as well as input the other two values as well. Then on the rendering side, a new JS file is included if needed, and the context object is attached to the style.

This patch could use a little work, but mostly in the documentation department. I have included a style in the test module to look at. I am too lazy to get the add thing to work with patches, but here is the context of the test/js/openlayers_test.context.js file:

// $Id$
/**
* @file
* File to hold custom context styling
*/
/**
* Global variable for context styling.
*/
Drupal.openlayers.context.openlayers_test_context_object = {
  'getFillOpacity': function(feature) {
    // Random fill opacity
    return Math.random();
  }
};

Overall, I think this is a pretty good way to go. It keeps things mostly in the existing style structure. I think it might make sense to make a properties column in the styles data structure and keep the style specific values in the "data" column.

If this seems like a good direction, I will clean this up and put back in the SLD patch above, then work on the rules-based stuff.

I really can't support any patch that includes a 'Drupal path' to a Javascript file. The consequences of the problem there aren't theoretical and hard to figure out, they are quite clear and will really compromise the usefulness of this feature in possibly even the majority of its uses.

Why?

Because having a 'drupal path' as a reference to a javascript file completely ignores the whole export system, and, in fact, just site organization in general. If you have a custom feature, like 'mystylething', it would be in sites/all/modules/mystylething, or sites/default/mystypething, or profiles/myprofile/modules/mystylething or profiles/myprofile/modules/features/mystylething.

All of these are totally valid and all of them will break this code. Having a system where it 'detects whether the javascript file exists' only makes this fail more gracefully, it'll still fail for an very, very significant amount of cases.

Including the Javascript code within the export fixes this situation, at the price of putting Javascript code in PHP, but, alas, it's either that, or we need to make these plugins, not exportables.

I think storing JS code (and PHP code) in the database, or embedded in code is dangerous and even more unmaintainable.

I do agree that an arbitrary path in Drupal is difficult, but this a UI problem, not an architectural problem. If you look at the code for a style that uses this, you will see that a drupal_get_path() can be used. This is not currently represented in the UI, but it could easily be.

<?php
function openlayers_test_openlayers_styles() {
 
$styles = array();
 
$style = new stdClass();
 
$style->api_version = 1;
 
$style->name = 'test_context';
 
$style->title = t('Test: Context style');
 
$style->description = t('A style to test context styling.');
 
$style->data = array(
   
'pointRadius' => '6',
   
'fillColor' => '#77777',
   
'strokeColor' => '#222222',
   
'strokeWidth' => '2',
   
'fillOpacity' => '${getFillOpacity}',
   
'strokeOpacity' => '0.8',
   
'context_enable' => TRUE,
   
'context_include' => drupal_get_path('module', 'openlayers_test') . '/js/openlayers_test.context.js',
   
'context_object' => 'openlayers_test_context_object',
  );
 
$styles[$style->name] = $style;
  return
$styles;
}
?>

In my opinion, the UI to this is just a nicety. In reality, someone should be creating the style themselves and thus creating the JS as well in the same module. Yes, this is not perfectly exportable, but behaviors are not exportable either, just a reference to them is, and this is working the same way.

I personally feel this a very acceptable system to deal with context-based styling. To reiterate, we can document the limitations of this method (which I am still confused on what they are) and there is still plenty of "exportable" ways of doing styling. I think putting JS in the database, inputted through the UI, or in a PHP string is a really bad idea.

If you still feel this needs to be a full fledged plugin, could please describe how this would work or a patch?

I think storing JS code (and PHP code) in the database, or embedded in code is dangerous and even more unmaintainable.

In specific, how?

I do agree that an arbitrary path in Drupal is difficult, but this a UI problem, not an architectural problem. If you look at the code for a style that uses this, you will see that a drupal_get_path() can be used. This is not currently represented in the UI, but it could easily be.

No. There's absolutely no connection between UI/API differences and the architectural problem here: it's a problem that it's dangerous and incredibly error-prone to have arbitrary dependencies between a style and a "Drupal path." Using drupal_get_path 'solves' the problem of module placement, but it's a completely unimportant win: most likely the javascript files people will be putting together will not be contained in modules (if they are, they why aren't we just using plugins?), but freerange somewhere else, where they can't possibly be tracked. And the other option is just... 404ing? Checking that the path resolves? This is just not going to cut it, at all.

In my opinion, the UI to this is just a nicety. In reality, someone should be creating the style themselves and thus creating the JS as well in the same module. Yes, this is not perfectly exportable, but behaviors are not exportable either, just a reference to them is, and this is working the same way.

If it's not exportable, it should be a plugin. If the only clean, correct way to implement this is to put it in a module and write all of the code for a pseudo-plugin, then we should just do plugins. What you're suggesting are plugin concepts poorly translated into exports.

StatusFileSize
new8.77 KB
PASSED: [[SimpleTest]]: [MySQL] 221 pass(es).
[ View ]

OK, another patch. This is a plugin system, but I have abstracted it and created a style handler system so that someone could do more with it if needed.

* Basically you can set the handler onto a style (in code and in UI) and it will process the the style through the plugin on rendering in the map.
* This needs a context_object to set for styles so that the styling processing on the JS side can do more with it. Overall, I think we should make a properties field for styles that can be more cleanly defining style properties. (note that properties are for the style object, not the actual style definition) http://dev.openlayers.org/docs/files/OpenLayers/Style-js.html
* There is an example in the test module. Obviously the code would live in its own file, but ease of patch it is in the module file.
* The JS is the same as above.
* The UI part could probably use some help and provide more info with the handler, but not a big deal.
* Technically someone could circumvent the system by just defining a context and adding a JS file whereever they want, but thats their problem.

I'm giving this patch a try and getting this from firebug when looking at a map with a layer on which a style
with the example style handler is used:

Drupal.openlayers.context is undefined

This is using the js found in comment #37

Hey @strk, are you putting it in the correct file: tests/js/openlayers_test.context.js

Make sure that that file is getting loaded.

Changing 'Drupal.openlayers.context' to 'Drupal.openlayers.styleContext' in test/js/openlayers_test_context.js fixes the firebug
warning (but still doesn't work).

I'll re-create the patch with the added file. It was working for me just fine.

Okay, checking out this patch...

  • It seems like style plugins should be able to accept settings. As they stand right now, they're subsets of how useful style behaviors would be
  • Introducing a new term, style_handler, while we already have 'style', 'plugin', 'context', etc., seems like a bad idea. I'm not a huge fan of the handler terminology in the layer system - it's mainly a carry-over from the old system and the proper term is really adapter in this case. But here, it seems like these aren't adapters or in any way similar to the layer handler system. Needs a rename
  • New systems should probably use the new style of CTools plugins - file-based, rather than hook-based. I don't want to switch everything, because that'd kill compatibility within 2.x, but there's no compatibility to be thought of in new features
  • The javascript side to this should be wrapped in try catch, etc., so that it isn't as dangerous to map creation as a whole

StatusFileSize
new14.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 710908-style_plugin-47.patch.
[ View ]

New patch:

  • Includes newly added files so it should work as is.
  • There is ability to have settings now.
  • The new "term" argument is kind of invalid as we currently only use "plugin" as needed by ctools functions, actually use "handler" and "type" for layers and do not use "context" anywhere and occasionally use "process". Overall, these are all general terms for general processes, but I do agree with the idea of consistency, and its not worth the argument. Keeping with the ctools, I have renamed these to "style_plugin"s.
  • Using new ctools plugin system.
  • JS side: Since the whole map rendering is wrapped in a try..catch, I am not sure the benefit here.
  • Overall, the same idea. Still includes example plugin in test module.

The patch doesn't apply cleanly to CVS anymore, do you have another ?

modules/openlayers> patch -p0 < 710908-style_plugin-47.patch
patching file openlayers.module
patching file includes/openlayers.render.inc
patching file js/openlayers.js
patching file modules/openlayers_ui/includes/openlayers_ui.styles.inc
Hunk #1 succeeded at 169 (offset 15 lines).
Hunk #2 succeeded at 192 (offset 15 lines).
Hunk #3 succeeded at 216 (offset 15 lines).
Hunk #4 FAILED at 232.
Hunk #5 succeeded at 286 (offset 35 lines).
1 out of 5 hunks FAILED -- saving rejects to file modules/openlayers_ui/includes/openlayers_ui.styles.inc.rej
patching file tests/openlayers_test.module
patching file tests/plugins/style_plugin/openlayers_test_test_plugin.inc
patching file tests/plugins/style_plugin/openlayers_test_test_plugin.js

The 'Style Properties and Plugins' tab contains this description:

"Plugins are dynamically process the layer at render time"

Dunno if it comes from this patch, but isn't clear :)

Also, a warning (dunno if new): warning: Invalid argument supplied for foreach() in sites/all/modules/openlayers/modules/openlayers_ui/includes/openlayers_ui.styles.inc on line 259.

Should enabling the test module show more options when editing styles ?
As I see none...

StatusFileSize
new4.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch openlayers_ui.styles.inc_.patch.
[ View ]

The attached patch is to be applied to 6--2 branch untouched after reverting openlayers_ui.styles.inc after applying zzolo patch.
Should fix the conflict and the warning.

NOTE: drush cc all fixed the fact I wasn't seeing more options. will continue testing

So, next issue: the 'edit style' form forgets which style plugin is enabled (ie: the checkbox results unchecked, even if the style is active and just-enabled)

$defaults['plugin'] has the value:

array(1) { ["openlayers_test_test_plugin"]=>  array(0) { } }

The code creating that form array is here:

     $form[$key] = array(
        '#tree' => TRUE,
        '#type' => 'fieldset',
        '#title' => $plugin['title'],
        '#description' => $plugin['description'],
        'enabled' => array(
          '#type' => 'checkbox',
          '#title' => t('Enabled'),
          '#default_value' => isset($defaults['plugins'][$key]['enabled'])
            ? $defaults['plugins'][$key]['enabled'] : FALSE,
        ),
      );

There's no 'enabled' property in the plugin options, just an empty array...

Problem is in the openlayers_ui_styles_form_submit function, in openlayers_ui/includes/openlayers_ui.styles.inc,
where the function overrides the plugin option array:

  // Process style plugins.  This allows us to have simpler
  // arrays for plugins.
  if ($data['plugins']) {
    foreach ($data['plugins'] as $plugin => $settings) {
      if ($settings['enabled']) {
        $data['plugins'][$plugin] = isset($data['options']) ? //-------- HERE !
          $data['options'] : array();
      }
      else {
        unset($data['plugins'][$plugin]);
      }
    }
  }

There's no $data['options'] so the whole $data['plugins'][$plugin] become an empty array.
I don't really understand what that block is trying to achieve

StatusFileSize
new4.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch openlayers_ui.styles.inc_.2.patch.
[ View ]

Attached is a patch against 6--2 branch version of openlayers_ui/includes/openlayers_ui.styles.inc
replacing the logic in that block to simply unset disabled plugins and keep enabled ones the way they are.

StatusFileSize
new4.28 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch openlayers_ui.styles.inc_.2_0.patch.
[ View ]

Sorry, previous patch also included debugging output, this one is w/out them

StatusFileSize
new4.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch openlayers_ui.styles.inc_.3.patch.
[ View ]

Another patch (still from scratch) fixing the call to the plugin-specific form so to pass actual plugin options

Alright, for the record: I suggested to allow the context_object defined by style plugins to be a class rather than an object,
and have openlayers.js API instanciate objects of that class by passing plugin-specific options.
That way we should have access to the options from within the JS style plugin.

It has to be confirmed this will work in core OL.

Status:Needs review» Needs work

@strk, thanks for all your work on this. This issue is getting pretty big. But its so close.

Anyway, @strk and I just talked on IRC. The problem currently is that there is no easy way to reference the style plugin options from within the style_context object.

Though he feels there is a need for class. This seems unnecessary in my mind. Either way, as per @strk suggestion and my final realization, we simply need to pass the style data into the style_context object so that it can be referenced easily.

I'll work on cleaning this all up over the weekend.

Note that if you pass the style data into the style_context object you'll need to make sure you're using a clone of the original object, or you won't be able to have the same style plugin used with different configurations in different styles.

Another thing to consider is whether or not to support multiple plugins in the same style.
The current interface seems to allow that. If we do, the final context object will need to be a merge of the context objects defined by the different plugins, and will also need to allow per-plugin options. If we go there, merging conflicts should also be detected and reported to user (say two style plugins both want to provide a callback for the same style property OR want to write a callback with the same name of the callback provided by another plugin).

Yes, a clone should be used.

Yes, we should support multiple plugins per style. But, the feature of trying to manage weights and merging context objects is for future development. As it stands, there would be an override if someone tried to attach two style plugins that defined style contexts (note that style plugins are for more than just style contexts). In this case, the "newest" one would be applied.

What about having style plugins define functions with namespaces for the context object and let user call the functions
from the style properties ?

Similarly to what the views plugin does in the "Attributes and Styles" block: advertise available callbacks.

Not a bad idea, but I would imagine this being fairly complicated. I would like just to focus on getting the basics down first, focus on getting a 2.0 out of OL, and then adding things. The current main use case of this is that people will make their own style plugins per needs of sites and therefore only need one style context plugin at a time.

Huh? I think I'm really losing touch with where this ticket is going or what's the aim at this point. The concepting here seems to be redoing things.

Okay: so object cloning and namespaces? Javascript doesn't have namespaces, and PHP doesn't have good ones, not sure what namespaces refer to here.

If we want instances of behaviors and for behaviors to have forms, and to populate themselves on the map, just copy the layer_type system... it has already thought through these problems. Layer types provide forms, create exportable layers, and support multiple layer instances on the map. Multiple instances of a layer are preserved in javascript. One can easily do the same thing with context objects by just writing constructors that take parameters. Possibly there's some reason that this conversation is getting more complex, and I'm missing it.

Anyway, it seems like you'd want a style form that has either a value or a dropdown of existing handlers, that each provide settings forms that save the settings and the required style plugin within the style's exportable data array, and then on map display, the style plugins call their render methods, adding their javascript to the page and creating instances of their context functions that absorb options as prototype attributes.

Right?

Definitely going for simplicity at this point.

This is very similar to behaviors and layer types, though a little different as we can have multiple styles on a single map that are using the same plugin, as well as having multiple plugins per style. This is not overly complicated, the code just needs a little tweaking to get right.

Anyway, it seems like you'd want a style form that has either a value or a dropdown of existing handlers, that each provide settings forms that save the settings and the required style plugin within the style's exportable data array, and then on map display, the style plugins call their render methods, adding their javascript to the page and creating instances of their context functions that absorb options as prototype attributes.

Yes, this is exactly what is (or close to) happening.

I think we are ont eh same page with this, it just needs some tweaking. I will put this all together this weekend and it'll be awesome.

This is very similar to behaviors and layer types, though a little different as we can have multiple styles on a single map that are using the same plugin, as well as having multiple plugins per style.

You... can have multiple layers on a map using the same layer type. The concepts of behaviors and layer types are very different.

Issue tags:+beta blocker

tagged as beta blocker

StatusFileSize
new17 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 710908-style_plugin-70.patch.
[ View ]

Alright, new patch:

  • Should apply cleanly to HEAD of 2.x branch.
  • Added some documentation.
  • Should save everything fine and represent it correctly in the UI.
  • Updated example to include random point radius with parameters.
  • Had to support another property in the style data, "context_plugin" so that parameters can be referenced. This is due to the fact that the "context_object" is a property of styles and not a property of the plugin. Seems a bit hackish.
  • I went with the class and object route for plugins (or what JS would call a class). This is due to the fact that we want to instantiate a new object and pass it parameters, and this can really only be achieved with defining and a function, then attaching "methods" with the "this" keyword. This seems to work fine, but there is a problem where the parameters that are passed are not available in the new object, though the methods work. This may be how OL handles the object that is passed, I am not sure. So it still needs work

Could you remove the parts of this ticket that you've committed along with other code?

StatusFileSize
new17.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 710908-style_plugin-72.patch.
[ View ]

Fixed:
http://drupal.org/files/issues/710908-style_plugin-70.patch

I forgot to take the files out of the queue to be committed.

New patch added.

Okay, re-rolling this patch to kill this ticket, now that we're back to using plugins.

The code's a little rough, like the fact that the implementation of openlayers_ctools_plugin_directory with

// This should change to the following when converted:

when converted to what from what?

  • contextParams is referenced before it's initialized
  • There's a bunch of logic and configuration related to specifying what javascript is invoked when style plugins are used. I don't get why this doesn't just follow the same pattern as behaviors and avoid that pitfall
  • I think that style plugins supporting multiple style properties are going to be conceptually difficult to manage. What if you use two plugins that provide getters for the same style property? And there's an invisible relationship between enabled style plugins and the fields they affect right now.

I'm working on a version of this that slims the code down a bit and, instead of hardcoding which properties are determined by which behaviors, it codes which properties can be determined by a single behavior. Thus the configuration of whether you use a static value or a plugin that determines that value is at the style property level. Does that sound realistic?

"I'm working on a version of this that slims the code down a bit and, instead of hardcoding which properties are determined by which behaviors, it codes which properties can be determined by a single behavior. Thus the configuration of whether you use a static value or a plugin that determines that value is at the style property level."

I like the idea of not hard-coding which properties are determined by which behavior/plugin.

The "namespacing" I was talking about was meant to allow this, so you construct the context_object
by attaching all methods exposed by all active plugins. By "namespace" a simple string prefix is fine.

Options will need to be per-plugin. So maybe the context object could look like:

context = {
'cluster_getRadius' : function(feature) { ... },
'cluster_options' : { .... },
'test_getFillColor': function(feature) {....},
'test_options': { .... }
};

Where _options would be the an instance of the plugin-specific options
(as compiled in the plugin-specific form).

At that point, plugins don't even need to be explicitly enabled, but their availability would be
determined by wheter or not they have been used for a style property value. Option forms
for them could be collapsed by default.

For the record: Tim Schaub suggested two possible paths to make the "proper object as context" approach:
http://openlayers.org/pipermail/dev/2010-July/006258.html

StatusFileSize
new1.17 KB
new14.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch dynamicStyle.patch.
[ View ]

The attached patch applies to head and is a modification of zzolo's one to make the object-like thing work.
In addition it moves the plugin contextes outside the style instance as they are common to all instances
being classes. The tgz contains the test plugin, taking min-max values for the random radius.

One thing I don't like very much is that the plugin needs to install the style callback name itself and this
name must contain the plugin name as prefix. This part might be better done internally, as tmcw was suggesting.
That is, the plugin should just advertise which style property can reference which methods of which context
object, and the DOL api should take care of setting up the bindings.

Anyway, with this patch it'd be possible to start implementing plugins.

StatusFileSize
new1.15 KB

This new version of the test style plugin shows how the plugin object can be a real class, using the prototype to save some memory.
Also, drops an unused attribute in the options form.

@tmwc: did you start your rework of this ? I'm using my version in #77 in staging w/out functionality problems (but would still be curious to look at your architectural idea)

StatusFileSize
new18.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 710908_styles_fields.patch.
[ View ]

Yeah, it's tricky to get the time required to do a full rework of this right now, but here's a patch of the current state of development. It's not functional yet, but shows some of the things I'm thinking about. Basically one plugin per field, ahah to load applicable plugins for style properties, working on slimming down logic in render methods and save methods and replacing with it more convention as far as naming.

Did anything move forward here ?
Should we make zzolo's approach available in CVS and refine in a second stage (it's still in successful use here) ?

I'd say no. For people who want to use that code, it's only a patch -p0 away, which doesn't require any greater capabilities than cvs checkout. CVS head should be stable and what we want, rather than a place for temporary code. I'll put in a bit more work on the patch, but I don't think it makes sense to commit infrastructure, have people build around it, and then rewrite it in a week.

Definitely agreed, given that what you will write, @tmcw, is significantly different (which is the impression I get).

StatusFileSize
new1.86 KB
new13.9 KB
FAILED: [[SimpleTest]]: [MySQL] 221 pass(es), 0 fail(s), and 1 exception(es).
[ View ]

I attach an updated version of the patch I'm staging with.
This was needed due to changes in modules/openlayers_ui/includes/openlayers_ui.styles.inc

The patch doesn't contain the implementation of the test plugin.
I attach the plugin I'm using for cluster-population-proportional radius ...

To help with testing / improving I've put the version I'm using in production into a branch on github:
http://github.com/strk/dol-6--2/tree/style_plugins

Note that the master branch there is in sync with CVS (6--2 branch)

Current version in github style_plugins branch has the plugin classes expose a function to fetch a list of style properties they are providing callbacks for. This should help adding (also later) UI improvements.

It's to be noted that the current proposed mechanism allows a single style plugin to attach callbacks to multiple properties (when enabled).

I belive we should decide on wheter to allow or not a single style plugin to influence more than a single style property.
This is what zzolo asked on IRC ("should these be style _property_ plugins rather than style plugins ?").

So far all my use cases are fine with a separate plugin for each style property.

Yeah, given the limitations of a style-plugin, and the talk of trying to get around this in the UI, I think it would make more sense, and be easier to code, if we instead wen the route of a style-property plugin.

This will make it very easy to choose a single plugin per style and avoid any conflicts.

For the record: I've created a new _temporary_ branch on github named style_plugins_ahah, taking some ideas from tmwc.
That branch still doesn't work (while 'style_plugins' does) but if you want to help with ahah-based thing I ask you to do from
the ahah branch :)
Feel free to use this tracker for feedback (or also try the one on github but I dunno if I'll be notified)

Status:Needs work» Needs review

Alright, 'style_plugins_ahah' stabilized and merged into 'style_plugins'.
Do you prefer a patch for better review or is git enough ?

You can obtain a patch for CVS by using 'git checkout style_plugins; git diff master'

I find it good enough for commit.

Status:Needs review» Needs work

The last submitted patch, style_plugin3.patch, failed testing.

Status:Needs work» Needs review

Sorry, new commit to fix support of the same plugin possibly giving functions to multiple properties.
Two test plugins show how it works, providing a configurable "random integer" and a non-configurable "factor".

I suggest you stay tuned on the 'style_plugins' branch, as I'll go on some more on cleanups.

I've done a few cleanups on the style_plugins branch and did a pull request.

Mainly I'm thinking about:

  • The AHAH callback uses raw POST values. I think there's a more Drupal-ish/secure way to do this?
  • I haven't been written defensive code in the module. This contains some and I don't know whether that's a good idea. If a style plugin doesn't correctly provide a javascript function, shouldn't the developer get a FireBug or Console error telling her so instead of things failing silently or skipping over it?

Besides those two things, this looks like good work and I think we're close here.

POST values is the only place in which I found the actually-selected plugin (the one selected by the select box triggering the ahah call).
I'd love to see some ahah-specific way to pass arguments to the backend callback otherwise.

I agree on the need for better consistency handling the JS side, but... do we have a standard for it so far ?
Maybe we should add to the map array consistency checker (or is it PHP only?)
Should we throw an exception ?

Waiting for your next pull request!

Issue tags:-beta blocker

#84: style_plugin3.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+beta blocker

The last submitted patch, style_plugin3.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch style_plugin4.patch.
[ View ]

I attach the current patch (generated from git) so that automatic testing works (altought I dunno how)

Status:Needs review» Needs work

The last submitted patch, style_plugin4.patch, failed testing.

Hey @strk, overall, can you explain the general process of how this is going to work. Hear are things that I don't intially understand:

  1. +  // Process with handler if available.
    +  $used_plugins = array();
    +  foreach ($styles_info as $i => $style) {
    +    // Check for property plugins.
    +    foreach ($style->data as $prop => $propval) {
    +      if ( is_array($propval) ) {
    +        $plugname = $propval['plugin'];
    +        if ( ! empty($plugname) ) {
    +          // the above should never happen, except
    +          // for cases in which the old style plugin
    +          // system was used (for prop == 'plugins')
    +          $used_plugins[$plugname] = TRUE;
    +        }
    +      }
    +    }
    +  }

    What old plugin system?
  2. +      $plugin_options = array('' => 'NO');

    What is this for? Seems hackish/bad-wording and does not use t().
  3. +  if ( ! $available[$plugname] ) {
    +    watchdog('openlayers_ui', 'Style plugin !name unknown', array(),
    +      WATCHDOG_ERROR);

    You do not actually put in the plugname there.
  4. +/*
    +  ob_start();
    +  echo "<PRE>";
    +  var_dump($propname);
    +  $output = ob_get_clean();
    +  drupal_json(array('status' => TRUE, 'data' => $output));
    +  return;
    +*/

    Please take out debugging code. Also, why are you using AHAH, is this really necessary at this point?
  5. +/**
    + * Style plugin context class
    + */
    +Drupal.openlayers.style_plugin.openlayers_test_rnd_factor = function (params) {
    +  this.params = params;
    +};
    +
    +/**
    + * Style plugin context class methods
    + */
    +Drupal.openlayers.style_plugin.openlayers_test_rnd_factor.prototype = {
    +
    +  // Fill opacity context.  Sets random fill opacity.
    +  'getFactor' : function(feature) {
    +    // Random factor
    +    return Math.random();
    +  }
    +
    +};

    There should be documentation on how to create style plugins, and specifically why this is necessary as it is kind of a work around given how OL works.

Alos, I cannot apply your patch as it is done with git. Is there an easy way to do this in my CVS repo?

Status:Needs work» Needs review
StatusFileSize
new26.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch style_plugin5.patch.
[ View ]

* "What old plugin system ?" -- the one implemented in your (zzolo's) first patch. That one used to add a 'plugins' property to style exports. I guess we can drop that check here, since no official release of DOL produced those kind of style exports [but I did use them in production, that's why]. In any case, it's good to be tolerant with malformed exports isn't it ?

* "NO" seems good wording to me (Use plugin: NO). Fine to use t(), do you like 'None' better ? Done.

* Done

* Debugging code removed. Dunno what you mean by "using AHAH at this point" as that's an AHAH function definition, not its use.

* Documentation: TODO

About applying to CVS:
cd sites/all/modules/openlayers; patch -p1 < ~/style_pulgin5.patch

Status:Needs review» Needs work

The last submitted patch, style_plugin5.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new32.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch style_plugin6.patch.
[ View ]

New patch with additional doc/STYLE_PLUGINS.txt

Status:Needs review» Needs work

The last submitted patch, style_plugin6.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new31.59 KB
FAILED: [[SimpleTest]]: [MySQL] 221 pass(es), 0 fail(s), and 83 exception(es).
[ View ]

The new patch applies w/out -p1 (hopefully making the patch application both happier here).
Also, it smaller as I found a code snippet which was not used [ legacy from zzolo's original patch ].

Status:Needs review» Needs work

The last submitted patch, style_plugin7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 221 pass(es).
[ View ]

And the good one (fixing a bug introduced by previous cleanups).

Status:Needs review» Needs work

Curious to see it, but the last patch has size 0 bytes!

subscribe

StatusFileSize
new32.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch style_plugin8_0.patch.
[ View ]

Oops. Better now ?
Please note that until this gets into CVS you can always use my dol-6--2 git repository on github.

StatusFileSize
new32.38 KB
FAILED: [[SimpleTest]]: [MySQL] 221 pass(es), 0 fail(s), and 83 exception(es).
[ View ]

This patch applies cleanly to branch after recent commits.

Status:Needs work» Needs review

Setting back to needs review...

Status:Needs review» Needs work

Uhm... doing some debugging in IE8 I found a conceptual bug with the patch used so far.
The bug is in missing information about _which_ method of the JS style property plugin class has to be used for _which_ property.
This information is currently exposed in the PHP side, but doesn't get to the JS side.

Will update ASAP.

Okay, I've got a few changes to submit as a pull request on GitHub. As far as larger patterns,

  • This work doesn't seem to be following the code style of Drupal. Are you using the Coder module to check code style? Mainly it's picking up stuff like } else { and indentation problems.
  • The spacing within if statements isn't really common in the module.
  • I don't think it's a good idea to fail silently when developers do things wrong - the main example would be in the javascript file, in which the required classes and functions don't exist, it fails silently. This has the tendency to make errors just more difficult to detect, and allow a variety of code in contributed modules that's more brittle in future updates. Same thing goes for watchdogging missing plugin classes on the PHP side - I think that's a developer-level problem which shouldn't be treated in that manner
  • The changes to openlayers.js (which seem a little verbose, and somewhat clash with the goal of making openlayers.js minimal enough that it isn't a big deal to maintain across OpenLayers versions) aren't on GitHub
  • Javascript in the module should adhere to either JSLint or Closure Linter - both of which throw a few errors on this code

> I don't think it's a good idea to fail silently when developers do things wrong

Agreed, new version throws from Javascript. Still needs policy work for case in which
a plugin referenced by a stored style isn't found (disabled module for example). I think
the current incarnation of code will trigger a throw from javascript again, might be changed
to always provide a 'missing' function in the context object for just this case.

> the changes to openlayers.js aren't on GitHub

Uh ? I belive I pushed everything.

> JSLint or Closure Linter

Should Coder deal with those ?

... I'll wait for next pull request after a successful merge of my branch into yours ...
NOTE: the new version (commit 4def84288d396b8b0661) is the result of testing against IE8

> JSLint or Closure Linter

Should Coder deal with those ?

No, the Coder module only deals with PHP code - as it probably should. The JSLint and Closure Linter have parsers in JavaScript and Python respectively that the Closure module can't readily take advantage of.

Alright, github has Coder fixes in too, and better handling of missing plugins (map will still be rendered, with style property driven by missing plugins replaced with value '' [empty string] and a watchdog warning is sent).

For JSLint or Closure Linter it'll take more time for me (would avoid that, if you have those handy).

StatusFileSize
new33.52 KB
FAILED: [[SimpleTest]]: [MySQL] 221 pass(es), 0 fail(s), and 83 exception(es).
[ View ]

Btw, another usability improvement for this would be properly handle the "missing plugin" case in the style edit form.
Currently, ahah will just fail and user will get an hard-to-understand message.

Current patch (as per github) attached.

Status:Needs work» Needs review

Triggering the testbot... I'll be trying out this patch in the next day...

Status:Needs review» Needs work

The last submitted patch, style_plugin10.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new33.61 KB
PASSED: [[SimpleTest]]: [MySQL] 221 pass(es).
[ View ]

Ok, finally got around using SimpleTest (which seems to be the cause for the testbot failures).
Attached is a patch which passes tests locally. Fingers crossed.
BTW, I guess we want to add a test for style plugins in OpenLayersStyles test.

Component:OpenLayers Views» OpenLayers API

Bot was finally happy :)
I think this is ready to go prime time.

If this is well-tested on your end and you are quite sure that it won't cause regressions, I think it's okay to commit it, and make improvements in CVS. Let's kill this ticket and get some momentum here.

Status:Needs review» Fixed

Hey, just wanted to say good work on this to all (especially strk); glad to see it come together.

Status:Fixed» Closed (fixed)

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

Status:Closed (fixed)» Patch (to be ported)

This needs to be ported to D7.

Started on branch: feature-port-style-plugins

Component:OpenLayers API» OL API

Hey, what's the state of the port to D7. Is there anything I might help with?

Version:6.x-2.x-dev» 7.x-2.x-dev
Priority:Critical» Major

Hey @Kaloyan Petrov. There is a branch that I started for the porting. Turned into a fair amount of work. Any help would be appreciated.

http://drupal.org/node/177400/git-instructions/feature-port-style-plugins

Status:Patch (to be ported)» Closed (fixed)

Moving port over to the other issue, as this one is huge.

#1247756: Undefined Label in Clustered Data Points (Port Style Plugins)

The branch from #130 seems to have been deleted. Zzolo, i almost dont dare to ask if you still have a local branch.

Sure don't. :(

Assigned:Unassigned» batje
Status:Closed (fixed)» Active

ok, reopening this issue

What's the current state of this issue? I was able to dynamically set some properties (e.g. strokeColor), while others don't work for me (for example, if I try to have dynamic strokeWidth values, I get strokeWidth must be a positive integer error, even, if I use field of type integer. Which is logical, given Pol's code example.

I also don't know, how to set properties controlled by drop-down menus, such as strokeDashstyle. Is there some sort of documentation on this?

Issue summary:View changes
Status:Active» Closed (fixed)

This is most likely solved by this commit.