All behaviors attached to a page are run twice on every page/ajax load because of OL. It's not a problem if the other behaviors uses $.once()
, but it's a lot of overhead for no good reason.
We have a function for each behavior in Drupal.behaviors
and one main function to bootstrap OL, display the map, etc. This bootstrap function calls Drupal.attachBehaviors(this);
making all the unrelated behaviors bound to the page run again.
We need to manage a register of events to run on the maps, like Drupal.behaviors
does for pages. It would simplify a lot of things in the JS, get rid of the horrible Drupal.settings.openlayers.maps[map_id].stop_render != true
, we won't have to keep around $(context).data({map, openlayers});
and it would clear up Drupal.behaviors
a lot.
There is also the problem of random map id wich makes it impossible to keep local settings bewteen ajax refresh but that's for another task.
Patch comming.
Comment | File | Size | Author |
---|---|---|---|
#9 | openlayers-behavior-register-1332836-9.patch | 5.83 KB | nod_ |
#7 | openlayers-behavior-register-1332836-7.patch | 98.57 KB | nod_ |
#6 | openlayers-behavior-register-1332836-6.patch | 73.11 KB | nod_ |
#5 | openlayers-behavior-register-1332836-5.patch | 72.1 KB | nod_ |
#1 | openlayers-behavior-register-1332836-1.patch | 6.74 KB | nod_ |
Comments
Comment #1
nod_here it is.
First patch is the refactor. Second is whitespace fixing.
It's pretty straightforward. I changed addBehavior to populate
Drupal.openlayers.behaviors
instead ofDrupal.behaviors
and in the main openlayer behavior I called the appropriate behaviors for the current map.I changed 'id' to 'name' in addBehavior and remove 'openlayers_auto' since I need the real name and it's not added to drupal.behaviors anymore so no chances of confilct, we could event drop 'openlayers_behavior_' but that's not really important.
Since I didn't rewrite drawfeatures to use addBehavior, that doesn't work. It'll be my next patch :þ
Comment #3
zzolo CreditAttribution: zzolo commentedHey @nod_. Thanks for following through with this. But I am weary of adding this complexity to the module. I know that Drupal behaviors re not very robust, but I don't really want to rewrite it either.
I like the simplicity of .once() and want to make sure that developers have as much control as they want over the behaviors. I don't want to have to require the use of addBehaviors().
Comment #4
nod_well sure, i can rework it so that you only have to add it to
Drupal.openlayers.behaviors
and not use addBehaviors. addBehavior was a monkey patch around the wired way map behaviors were triggered and the painful check it introduced (and you'll notice it's all gone now, I still keep it to avoid rewriting all the behaviors to change parameters again).Now that it's all clean we can add a
Drupal.openlayers.attachBehaviors()
that other modules can call to do whatever they want with it. And actually with a little$(context).trigger('load', [map, openlayers]);
we give back the power to any module that catch the load event on a map to access underlaying OL object and Drupal OL's config. and as you can see the.once()
paradigm is still used and should be used. If you look back, the way it was done,.once()
was not enough, they had to add a.stop_render
to be sure. So really, we're using the.once()
way more than ever.Now If you look at it from an architectural point, there is no justifications as to have a map behavior together with a page behavior. I mean we have to do something better than that because of the module's nature. It's a wrapper around a JS library, not some small js powder around a back-end code.
My point is, with this we will allow much easier and cleaner way of interacting with the map object and config from the outside. I'm still working on the draw feature, it's around 130 lines of JS. Without the sugar for the other modules it's 90 LOC. and the JS required from the geofield module to do it's thing is like 40 lines against a new behavior class and 100+LOC with less features. I'm not even talking about my leaking FF after a couple of hour's work on maps.
My point is, Drupal behaviors are fine, you just need to know how to use them. Right now we're abusing them. And we are not adding complexity, we're actually simplifying. With a few lines (ok, a paragraph or two at most) of documentation, bam! it's all better. Please don't forsake this patch just because it kinda works now.
Comment #5
nod_Ok here it is. It's very much a work in progress i didn't have time to test on IE yet. I'll post explainations and a bit of doc later after i better test that.
Comment #6
nod_This should be it.
Changes to openlayers.js
Drupal.openlayers.addBehavior()
is gone \o/.Drupal.attachBehaviors()
anymore. We manage our own list of behaviors inDrupal.openlayers.behaviors
. They are called for each processed map with additional parameters.detach
. Meaning things shouldn't leak even after working for a while, ajaxing all the time.The main work is there, apart from drawfeatures, the rest is pretty much a refactor to use the new parameters, nothing ground breaking.
JS Behaviors
Change of how to register behaviors in JS :
Before (before the first refactor)
Now
We add behaviors to our own behavior register, the name has to match what's in the map settings otherwise behavior is going to be empty.
map[0]
, the OL object. Need to use the API for some advanced stuff ? use this.There is no
.once()
because it's done while calling the behaviors, there is no reason to let people handle this, if you need something that execute multiple time, you're probably doing something wrong. Drupal should have done that too. And there is always the.removeOnce()
function if that's really what you want. Core got it wrong, not a reason we should too.Draw features
Huges changes in this. It's actually useful now. The behaviors settings have changed, you can choose what to draw and if you want to allow edit or not. The feature number is something that has to be handled by a custom behavior, not this one. A example will follow soon for geofield to take advantage of the new DF.
How it works now is very simple. When you draw, edit or delete a feature (a shape really), a
change
event is triggered on the div containing the map (it's what thecontext
parameter refers to in our behaviors). The event contains the original OL event (featureadded, featuredeleted, afterfeaturemodify), a WKT serialisation of the shapes drawn and finally the original OL event.So to do something with the thing:
And here you are, you have bound your field to the map. You can see i checked
type
, it's because we are also triggering a change event when the map moves, if you need to know the center, bounds or zoomlevel of the current map it's easy.Now there are a couple of events that you can trigger on the map to initialize the drawings, center and zoomlevel. I'll have to ask to wait for an implementation for geofield to have a clear code of how to do what. Know that it's there for now.
Comment #7
nod_Same as before, with a couple of things changed :)
D.attachbehaviors
andD.detachBehaviors
is run on destroy too.A little beauty fix too :
Drupal.openlayers.behaviors['ol_behavior'] = function () {};
is changed back toDrupal.openlayers.behaviors.ol_behavior = function () {};
, JSLINT was complaining.But i've run into a few more issues.
On tooltip, the marker flicker when the popup shows up above. It's because we need to add an
Openlayers.Icon
object to offset the pointy bit a little from the marker. There should be all the informations we need to set size and offset from the marker style, i'm just too tired for now :þBecause of the change, on the map admin, the map used to set center and restrict extent, we can't have the dragpan behavior or else drawing the rectangle doesn't work. It's a real pain and i haven't found a solution yet. Speaking of that there is a lot of dead code in
ol_ui.maps.js
haven't touched it, yet.The big bump in size is mainly because of tooltip and popup (which still work fine) and there's been some whitespace caught in the middle of the fight too.
Comment #8
zzolo CreditAttribution: zzolo commentedHey @nod_. Thanks for all the awesome work so far. Most of this is looking good. Here are my thoughts so far:
Can't we just put the line that checks the layer into the OL.attach function?
map[0]
to something likemap.openlayers
?Comment #9
nod_Here is the small patch requested. you were right I got really really carried away…
Minimum changes to ol.js and rewrite of navigation and cluster behaviors to use the new API, changed a couple of things in addBehavior to make all behaviors work. So nothing is actually broken \o/
i kept, map[0] modules are not supposed to use it. It get explicitly passed as an argument in the attach behavior function. It's there only for the main detach function (which is not in this patch).
I don't think calling D.attachBehaviors is necessary, all that changed is some html/svg got added inside the already existing div for the map, and a lot of javascript wizardry happened trough OL.js
Comment #10
zzolo CreditAttribution: zzolo commentedHey @nod_. I have given you access, and updated the co-maintainer issue with advice on general contributions. I have also started a goals issue for 3.x: #1353122: 3.x Goals
Let's start making these changes in the 3.x version. Please keep this issue up to date with your progress, as I won't have so much time to look over all the code.
Comment #11
nod_yeah i've managed to remove the
if (!behavior) {return;}
and added areturn
to help with testing. I'll update this issue when I have a bit more time this weekend. With code too :)Comment #12
nod_commited #9 and added a detach function that clean up the map destroying all controls and map behaviors. It's the first step towards testing as we need to be able to create and destroy cleanly a map to run tests.
With the new detach function using maps sould feels faster, in forms especially.
updated JS doc to reflect API change too.
Comment #13
nod_Maps level events are in 7.x-3.x, so i'm closing this. I'll be gradually updating behaviors to get rid of addBehavior() in the main openlayers.js, it was introduced to remove a lot of bothersome data-getting that is now directly passed as parameters.
Drawfeatures behaviors that was in the last monster patch will be discussed somewhere else #1330658: Drawing widget redesign.
Enjoy :)
Comment #14
zzolo CreditAttribution: zzolo commentedAwesome work, @nod_. Havent looked at the code yet. But can we keep this open until we have some initial docs up. I think it'll be good to make documentation as part of issues/changes in 3.x.
Comment #15
nod_we have some, i updated a couple of docs at the same time: Doc update for behaviors API change :) but yes, it could use more work.
Comment #16
zzolo CreditAttribution: zzolo commentedHey @nod_. That's awesome!
Actually, though, what I was thinking was having documentation on Drupal.org. It's a tough call of whether to have documentation in the module itself, with Advanced Help, or on Drupal.org. There's no right answer, but I don't really want to duplicate things, and I really am leaning towards docs on Drupal.org because it allows a place for anyone to contribute to.
I have update #1353122: 3.x Goals
Comment #17
nod_oh yeah much better indeed, i personally hate advanced help so i'm more than happy to contribute docs on d.o I'll get to it soon :)
Comment #18
ekes CreditAttribution: ekes commented3.x postponed.
Postponed as part of the issue queue cleanup - DrupalCon Munich sprint.