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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

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 of Drupal.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 :þ

Status: Needs review » Needs work

The last submitted patch, openlayers-behavior-register-1332836-1-whitespace.patch, failed testing.

zzolo’s picture

Hey @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().

nod_’s picture

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.

nod_’s picture

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.

nod_’s picture

Status: Needs work » Needs review
FileSize
73.11 KB

This should be it.

Changes to openlayers.js

  • The temporary Drupal.openlayers.addBehavior() is gone \o/.
  • ol.js doesn't call Drupal.attachBehaviors() anymore. We manage our own list of behaviors in Drupal.openlayers.behaviors. They are called for each processed map with additional parameters.
  • A couple of things got shortened inside the main behavior. No loss of functionnality, just a better use of OL API.
  • We now properly destroy the map and all handlers on 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)

Drupal.behaviors.openlayers_behavior_dragpan = {
  'attach': function(context, settings) {
    var data = $(context).data('openlayers');
    if (data && data.map.behaviors['openlayers_behavior_dragpan']) {
      // function code
    }
  }
};

Now

Drupal.openlayers.behaviors['openlayers_behavior_dragpan'] = {
  attach: function (context, map, behavior, openlayers) {
    if (!behavior) {return;}
    Drupal.openlayers.addControl(openlayers, 'DragPan');
  }
};

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.

  • context: a jquery object of the div containing the map (yes, jquery object, not the raw object like in drupal behaviors)
  • map: the current map settings. On thing is added, map[0] refers to the OL object representing the map, à la jQuery.
  • behavior: the current behaviors settings it's really a shortcut to map.behaviors['openlayers_behavior_dragpan']
  • openlayers: exactly the same as map[0], the OL object. Need to use the API for some advanced stuff ? use this.
  • to check if the behavior apply to the current map, we check that the settings for this behavior is not empty.

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 the context 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:

Drupal.openlayers.behaviors['openlayers_behavior_savemydrawings'] = {
  attach: function (context, map, behavior, openlayers) {
    if (map.behaviors['openlayers_behavior_drawfeatures']) {
      var $myField = $("#my-awesome-wkt-field');
      context.bind('change', function (event, type, data, olEvent) {
        if (type !== 'moveend') {
          // data contains a serialized WKT string
          $myField.val(data);
        }
      });
    }
  }
};

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.

nod_’s picture

Same as before, with a couple of things changed :)

  1. delayed the map rendering to the page load event, on IE to avoid a redraw. That needs some testing Removed the redrawVector method
  2. Huge cleanup of popup and tooltip behaviors. It's readable now. And we can even share a couple of methods between popup and tooltips (but i was too lazy today).
  3. popup and tooltip have now a restricted context passed to D.attachbehaviors and D.detachBehaviors is run on destroy too.

A little beauty fix too : Drupal.openlayers.behaviors['ol_behavior'] = function () {}; is changed back to Drupal.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.

zzolo’s picture

Title: OL needs to manage his own register of attach function » Behavior re-architecting, use map level event system

Hey @nod_. Thanks for all the awesome work so far. Most of this is looking good. Here are my thoughts so far:

  • I agree with making our own map behaviors event system. Good job with this.
  • Scope creep! It seems like you are doing a lot more than re-architecting behaviors. I understand that there is a lot of code to cleanup and improve, but lets do one thing at a time. 1) This makes sure changes are discreet and dont break things as easily and we can get the patch in sooner, and 2) allows me (and others) to review things much more easily. I know its tough rerolling patches and I don't want to keep you from developing, but I can't just put all this in at once. Hopefully you are working on a new branch that has multiple commits.
  • That said, let's remove any projection changes and popup/tooltip changes.
  • Also the addFeatures changes and the draw behavior are awesome, but really should be in a different issue.
  • If it makes it easier, you don't have to update all the behaviors for each patch, just a couple so that I can see what the changes entail.
  • Is there anyway you can not do the whitespace changes? I am all about getting rid of that, but there's a lot of it in this patch and it makes it really hard to see the changes that I need to look at.
  • Drupal.openlayers.behaviors['openlayers_behavior_dragpan'] = {
      attach: function (context, map, behavior, openlayers) {
        if (!behavior) {return;}
        Drupal.openlayers.addControl(openlayers, 'DragPan');
      }
    };
    

    Can't we just put the line that checks the layer into the OL.attach function?

  • Can we change how we reference the OL map object from map[0] to something like map.openlayers?
  • I think we should still do a Drupal.attachBehaviors() at the end of rendering, since the page element has significantly changed.
nod_’s picture

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

zzolo’s picture

Version: 7.x-2.x-dev » 7.x-3.x-dev
Status: Needs review » Active

Hey @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.

nod_’s picture

yeah i've managed to remove the if (!behavior) {return;} and added a return to help with testing. I'll update this issue when I have a bit more time this weekend. With code too :)

nod_’s picture

Assigned: Unassigned » 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.

nod_’s picture

Status: Active » Fixed

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 :)

zzolo’s picture

Component: OL Behaviors » Documentation
Status: Fixed » Needs work
Issue tags: +Needs documentation

Awesome 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.

nod_’s picture

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.

zzolo’s picture

Hey @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

nod_’s picture

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 :)

ekes’s picture

Status: Needs work » Postponed

3.x postponed.

Postponed as part of the issue queue cleanup - DrupalCon Munich sprint.