Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#120 | style_plugin11.patch | 33.61 KB | strk |
#117 | style_plugin10.patch | 33.52 KB | strk |
#110 | style_plugin9.patch | 32.38 KB | strk |
#109 | style_plugin8.patch | 32.25 KB | strk |
#106 | style_plugin8.patch | 0 bytes | strk |
Comments
Comment #1
phayes CreditAttribution: phayes commentedA 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?
Comment #2
phayes CreditAttribution: phayes commentedAnother 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.
Comment #3
tmcw CreditAttribution: tmcw commentedWe'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.
Comment #4
tmcw CreditAttribution: tmcw commentedAlso 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.
Comment #5
zzolo CreditAttribution: zzolo commentedGiven 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.
Comment #6
tmcw CreditAttribution: tmcw commentedIn 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.
Comment #7
tmcw CreditAttribution: tmcw commentedComment #8
zzolo CreditAttribution: zzolo commentedSo, 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:
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.
Comment #9
zzolo CreditAttribution: zzolo commentedSee this as well: #736626: Document Variable Styling in Views Display plugin
Comment #10
zzolo CreditAttribution: zzolo commentedSo, 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.
Comment #11
tmcw CreditAttribution: tmcw commentedI 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.
Comment #12
tmcw CreditAttribution: tmcw commentedI 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.
Comment #13
zzolo CreditAttribution: zzolo commentedI 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.
Comment #14
tmcw CreditAttribution: tmcw commentedI'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.
Comment #15
zzolo CreditAttribution: zzolo commentedI 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?
Comment #16
zzolo CreditAttribution: zzolo commentedJust 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.
Comment #17
zzolo CreditAttribution: zzolo commentedI 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.
Comment #18
tmcw CreditAttribution: tmcw commentedI'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.
Comment #19
tmcw CreditAttribution: tmcw commentedHere'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
Comment #20
zzolo CreditAttribution: zzolo commentedI 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.
Comment #21
tmcw CreditAttribution: tmcw commentedYes, 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...
Comment #22
tmcw CreditAttribution: tmcw commentedNote that a 'builder' for SLD is entirely in the works, and would be relatively easy to construct. Hence a 'fundamental implementation' patch.
Comment #23
zzolo CreditAttribution: zzolo commentedThe SLD code you provided basically says this in about 100 times more code:
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.
Comment #24
tmcw CreditAttribution: tmcw commentedHow 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.
Comment #25
tmcw CreditAttribution: tmcw commentedNote, 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.
Comment #26
zzolo CreditAttribution: zzolo commentedSo, 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.
Comment #27
tmcw CreditAttribution: tmcw commentedI'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.
Comment #28
zzolo CreditAttribution: zzolo commentedI 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
Comment #29
tmcw CreditAttribution: tmcw commented#756692: Ensure layer types and behaviors module dependency is handled correctly in Features
Dependencies, scope, encapsulation.
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
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.
Comment #30
zzolo CreditAttribution: zzolo commentedPlease 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.
Comment #31
strk CreditAttribution: strk commentedSorry 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
Comment #32
tmcw CreditAttribution: tmcw commentedAll 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.
Comment #33
tmcw CreditAttribution: tmcw commentedAn 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.
Comment #34
tmcw CreditAttribution: tmcw commentedOn 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.
Comment #35
zzolo CreditAttribution: zzolo commentedThis 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.
Comment #36
strk CreditAttribution: strk commentedAnother 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?
Comment #37
zzolo CreditAttribution: zzolo commentedSo, 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:
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.
Comment #38
tmcw CreditAttribution: tmcw commentedI 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.
Comment #39
zzolo CreditAttribution: zzolo commentedI 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.
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?
Comment #40
tmcw CreditAttribution: tmcw commentedIn specific, how?
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.
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.
Comment #41
zzolo CreditAttribution: zzolo commentedOK, 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.
Comment #42
strk CreditAttribution: strk commentedI'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
Comment #43
zzolo CreditAttribution: zzolo commentedHey @strk, are you putting it in the correct file: tests/js/openlayers_test.context.js
Make sure that that file is getting loaded.
Comment #44
strk CreditAttribution: strk commentedChanging 'Drupal.openlayers.context' to 'Drupal.openlayers.styleContext' in test/js/openlayers_test_context.js fixes the firebug
warning (but still doesn't work).
Comment #45
zzolo CreditAttribution: zzolo commentedI'll re-create the patch with the added file. It was working for me just fine.
Comment #46
tmcw CreditAttribution: tmcw commentedOkay, checking out this patch...
Comment #47
zzolo CreditAttribution: zzolo commentedNew patch:
Comment #48
strk CreditAttribution: strk commentedThe 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
Comment #49
strk CreditAttribution: strk commentedThe '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 :)
Comment #50
strk CreditAttribution: strk commentedAlso, 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.
Comment #51
strk CreditAttribution: strk commentedShould enabling the test module show more options when editing styles ?
As I see none...
Comment #52
strk CreditAttribution: strk commentedThe 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.
Comment #53
strk CreditAttribution: strk commentedNOTE: drush cc all fixed the fact I wasn't seeing more options. will continue testing
Comment #54
strk CreditAttribution: strk commentedSo, 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)
Comment #55
strk CreditAttribution: strk commented$defaults['plugin'] has the value:
The code creating that form array is here:
There's no 'enabled' property in the plugin options, just an empty array...
Comment #56
strk CreditAttribution: strk commentedProblem 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:
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
Comment #57
strk CreditAttribution: strk commentedAttached 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.
Comment #58
strk CreditAttribution: strk commentedSorry, previous patch also included debugging output, this one is w/out them
Comment #59
strk CreditAttribution: strk commentedAnother patch (still from scratch) fixing the call to the plugin-specific form so to pass actual plugin options
Comment #60
strk CreditAttribution: strk commentedAlright, 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.
Comment #61
zzolo CreditAttribution: zzolo commented@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.
Comment #62
strk CreditAttribution: strk commentedNote 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).
Comment #63
zzolo CreditAttribution: zzolo commentedYes, 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.
Comment #64
strk CreditAttribution: strk commentedWhat 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.
Comment #65
zzolo CreditAttribution: zzolo commentedNot 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.
Comment #66
tmcw CreditAttribution: tmcw commentedHuh? 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?
Comment #67
zzolo CreditAttribution: zzolo commentedDefinitely 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.
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.
Comment #68
tmcw CreditAttribution: tmcw commentedYou... can have multiple layers on a map using the same layer type. The concepts of behaviors and layer types are very different.
Comment #69
zzolo CreditAttribution: zzolo commentedtagged as beta blocker
Comment #70
zzolo CreditAttribution: zzolo commentedAlright, new patch:
Comment #71
tmcw CreditAttribution: tmcw commentedCould you remove the parts of this ticket that you've committed along with other code?
Comment #72
zzolo CreditAttribution: zzolo commentedFixed:
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.
Comment #73
zzolo CreditAttribution: zzolo commentedBad link. right one: http://drupal.org/cvs?commit=388222
Comment #74
tmcw CreditAttribution: tmcw commentedOkay, 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?
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?
Comment #75
strk CreditAttribution: strk commented"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.
Comment #76
strk CreditAttribution: strk commentedFor 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
Comment #77
strk CreditAttribution: strk commentedThe 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.
Comment #78
strk CreditAttribution: strk commentedThis 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.
Comment #79
strk CreditAttribution: strk commented@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)
Comment #80
tmcw CreditAttribution: tmcw commentedYeah, 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.
Comment #81
strk CreditAttribution: strk commentedDid 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) ?
Comment #82
tmcw CreditAttribution: tmcw commentedI'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.
Comment #83
zzolo CreditAttribution: zzolo commentedDefinitely agreed, given that what you will write, @tmcw, is significantly different (which is the impression I get).
Comment #84
strk CreditAttribution: strk commentedI 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 ...
Comment #85
strk CreditAttribution: strk commentedTo 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)
Comment #86
strk CreditAttribution: strk commentedCurrent 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).
Comment #87
strk CreditAttribution: strk commentedI 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.
Comment #88
zzolo CreditAttribution: zzolo commentedYeah, 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.
Comment #89
strk CreditAttribution: strk commentedFor 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)
Comment #90
strk CreditAttribution: strk commentedAlright, '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.
Comment #92
strk CreditAttribution: strk commentedSorry, 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.
Comment #93
tmcw CreditAttribution: tmcw commentedI've done a few cleanups on the style_plugins branch and did a pull request.
Mainly I'm thinking about:
Besides those two things, this looks like good work and I think we're close here.
Comment #94
strk CreditAttribution: strk commentedPOST 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!
Comment #95
dominikb1888 CreditAttribution: dominikb1888 commented#84: style_plugin3.patch queued for re-testing.
Comment #97
strk CreditAttribution: strk commentedI attach the current patch (generated from git) so that automatic testing works (altought I dunno how)
Comment #99
zzolo CreditAttribution: zzolo commentedHey @strk, overall, can you explain the general process of how this is going to work. Hear are things that I don't intially understand:
What old plugin system?
What is this for? Seems hackish/bad-wording and does not use t().
You do not actually put in the plugname there.
Please take out debugging code. Also, why are you using AHAH, is this really necessary at this point?
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?
Comment #100
strk CreditAttribution: strk commented* "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
Comment #102
strk CreditAttribution: strk commentedNew patch with additional doc/STYLE_PLUGINS.txt
Comment #104
strk CreditAttribution: strk commentedThe 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 ].
Comment #106
strk CreditAttribution: strk commentedAnd the good one (fixing a bug introduced by previous cleanups).
Comment #107
freelockCurious to see it, but the last patch has size 0 bytes!
Comment #108
ahtih CreditAttribution: ahtih commentedsubscribe
Comment #109
strk CreditAttribution: strk commentedOops. Better now ?
Please note that until this gets into CVS you can always use my dol-6--2 git repository on github.
Comment #110
strk CreditAttribution: strk commentedThis patch applies cleanly to branch after recent commits.
Comment #111
freelockSetting back to needs review...
Comment #112
strk CreditAttribution: strk commentedUhm... 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.
Comment #113
tmcw CreditAttribution: tmcw commentedOkay, I've got a few changes to submit as a pull request on GitHub. As far as larger patterns,
} else {
and indentation problems.if
statements isn't really common in the module.Comment #114
strk CreditAttribution: strk commented> 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
Comment #115
tmcw CreditAttribution: tmcw commentedNo, 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.
Comment #116
strk CreditAttribution: strk commentedAlright, 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).
Comment #117
strk CreditAttribution: strk commentedBtw, 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.
Comment #118
freelockTriggering the testbot... I'll be trying out this patch in the next day...
Comment #120
strk CreditAttribution: strk commentedOk, 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.
Comment #121
strk CreditAttribution: strk commentedBot was finally happy :)
I think this is ready to go prime time.
Comment #122
tmcw CreditAttribution: tmcw commentedIf 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.
Comment #123
strk CreditAttribution: strk commentedThere we are!
http://drupal.org/cvs?commit=435482
Comment #124
strk CreditAttribution: strk commentedand http://drupal.org/cvs?commit=435486 (left over)
Comment #125
zzolo CreditAttribution: zzolo commentedHey, just wanted to say good work on this to all (especially strk); glad to see it come together.
Comment #127
zzolo CreditAttribution: zzolo commentedThis needs to be ported to D7.
Comment #128
zzolo CreditAttribution: zzolo commentedStarted on branch: feature-port-style-plugins
Comment #129
Kaloyan Petrov CreditAttribution: Kaloyan Petrov commentedHey, what's the state of the port to D7. Is there anything I might help with?
Comment #130
zzolo CreditAttribution: zzolo commentedHey @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
Comment #131
zzolo CreditAttribution: zzolo commentedMoving port over to the other issue, as this one is huge.
#1247756: Undefined Label in Clustered Data Points (Port Style Plugins)
Comment #132
batje CreditAttribution: batje commentedThe branch from #130 seems to have been deleted. Zzolo, i almost dont dare to ask if you still have a local branch.
Comment #133
zzolo CreditAttribution: zzolo commentedSure don't. :(
Comment #134
batje CreditAttribution: batje commentedok, reopening this issue
Comment #135
milos.kroulik CreditAttribution: milos.kroulik commentedWhat'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?
Comment #136
milos.kroulik CreditAttribution: milos.kroulik commentedThis is most likely solved by this commit.