Currently 2.x only allows one behavior per type per map. In a lot of cases this is desirable as for many behaviors it only makes sense to ever have one. However, in many behaviors this doesn't make any sense. For example, we may want to assign two different types of tooltips or popups to two different layers in very different ways. There are two general ways to think about this. Either we don't support it and say "It's the behavior's responsibility to handle multiple instances of it'self", or we do support it. If we support it it makes our API a little more complicated, but if we don't support it it could be quite a bit more work for people building behaviors. We had this functionality for 1.x, and it was not that difficult to support.

I could recommend the following way forward:

We add a new option to behaviors called "multiple", which defaults to FALSE. For example:
An implicit "multiple" => FALSE

    'openlayers_behavior_attribution' => array(
       'title' => t('Attribution'),
       'description' => t('Allows layers to provide attribution to the map if it exists.'),
       'type' => 'layer',
       'path' => drupal_get_path('module', 'openlayers') .'/includes/behaviors',
       'file' => 'openlayers_behavior_attribution.inc',
       'behavior' => array(
         'class' => 'openlayers_behavior_attribution',
         'parent' => 'openlayers_behavior',
       ),
     ),

An explicit 'multiple' => TRUE

     'openlayers_behavior_popup' => array(
       'title' => t('Pop Up'),
       'description' => t('Adds clickable info boxes to points or shapes on maps.'),
       'type' => 'layer',
       'path' => drupal_get_path('module', 'openlayers') .'/includes/behaviors',
       'file' => 'openlayers_behavior_popup.inc',
       'multiple' => TRUE,
       'behavior' => array(
         'class' => 'openlayers_behavior_popup',
         'parent' => 'openlayers_behavior',
       ),
     ),

On the implementation side of things things would look a little more like this:

    'behaviors' => array(
      'panzoombar' => array('type' => 'openlayers_behavior_panzoombar'),
      'dragpan' => array('type' => 'openlayers_behavior_dragpan'),
      'popupone' => array(
        'type' => 'openlayers_behavior_popup',
        'layer' => 'layer_one',
        'attribute' => 'some_attribute'
       ),
      'popuptwo' => array(
        'type' => 'openlayers_behavior_popup',
        'layer' => 'layer_two',
        'attribute' => 'some_completely_different_attribute'
       ),
    ),

For the simple behaviors things are a little more verbose, but I think it's worth it. I know this is going slightly back towards what we had in 1.x, but I believe it's the right direction. If everyone is amenable to this general direction, I will start coding a patch that we can work from.

CommentFileSizeAuthor
#5 multi-behaviors.patch31.64 KBphayes
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zzolo’s picture

zzolo CreditAttribution: zzolo commented

I have not really thought about the best way to implement this, but I fully support it. I think between popups, tooltips, and drawing, there are some use cases for it. I do think that most people won't necessarily need it, but anyone needing to do anything complicated with maps and will be using the API more extensively could use this feature.

I think they key is to keep it exportable.

tmcw’s picture

tmcw CreditAttribution: tmcw commented

The behaviors array would probably be integer-indexed in this case. Something like 'popupone' and 'popuptwo' is not going to work via the UI or anything else. Also I'd assume that we want to use a data-> field for any options on behaviors instead of adding them on the same level as the behavior name, etc.

tmcw’s picture

tmcw CreditAttribution: tmcw commented
Component: OpenLayers Behaviors » OpenLayers API
Priority: Normal » Critical
phayes’s picture

phayes CreditAttribution: phayes commented

FYI: I'm coding the first patch for this today -- stay tuned...

phayes’s picture

phayes CreditAttribution: phayes commented
FileSize
multi-behaviors.patch31.64 KB

Okay here's the first patch.

1. I didn't patch openlayersCCK or the install module, let's make sure we are happy with what's here before bringing those up to speed.
2. The UI was patched so that it functions with the new system, but doesn't allow multiple behaviors to be added via the UI. Again, let's tackle that after this gets the go-ahead

Question: Is there a major advantage to using the Drupal-behavior system to handle openlayer-behaviors? Conceptually they are similar, but I don't actually see the advantage in how they are integrated. Currently openlayers behaviors are being called twice per page-load (or more if there is ahah), once for the normal Drupal behavior call, and then the special openlayers call that actually does something. Also, this patch could be improved if we didn't use the Drupal-behavior route - right now I'm having to put the multi-behavior logic inside each behavior JS file. If we didn't use Drupal-behaviors then I could simplify this greatly.

P

zzolo’s picture

zzolo CreditAttribution: zzolo commented

@phayes,

Good work and thanks for the patch. There's a lot of changes there, though it seems that most of it is changing the JS side of the behaviors.

Can you maybe describe what changes, and how a behavior would be implemented with this patch? It would help me understand it more.

Drupal.behaviors is a handy built in way to manage events/behaviors. I like it because it's built into Drupal, but it does have its down side, like being called by every Drupal behavior attach call (even if its not OpenLayers), hence why we ensure that the openlayers data is there. It also helps make things more extensible because when AJAX events happen, Drupal behaviors gets called and so maps can be updated easily, and this way we and users don't have to worry about handling other events happening on the page. I like using it, but having some events (handled through jQuery) may be a good idea as well.

Looking at some of you changes, it looks like you allow for some of the Control-like behaviors to be multiple which doesn't make much sense since these are things that are can only be added to a map once. I liked the idea of setting a "Multiple" flag on the behavior. Maybe there is someway where we can abstract out the multiplcity for be generic for behaviors, and just have that flag dictate how they are handled.

phayes’s picture

phayes CreditAttribution: phayes commented

Indeed, most of the typing was on the JS side, but the substantial changes were actually not very much.

Changes are basically:

1. Changed how behaviors are declared - now need to declare like so:

    'behaviors' => array(
      'panzoombar' => array('behavior' => 'openlayers_behavior_panzoombar'),
      'dragpan' => array('behavior' => 'openlayers_behavior_dragpan'),
    )

2. Updated the behavior rendering to reflect this. Basically just shifting a few things around at _openlayers_behaviors_render in openlayers.render.inc
3. Updated the UI so that it just tags on 'behavior' => 'openlayers_behavior_whatever' on the form submit. This will need to be changed in the future, but it works for now.
4. Updated the JavaScript on each of the behaviors - basically just wrapping them in a foreach so that if there are multiple behaviors, it will get triggered multiple times

Drupal.behaviors : What about not using drupal-behaviors for each individual openlayers-behavior, but just using a single openlayers-attach-behaviors function as a drupal-behavior, and then having that do all the processing on the openlayers-behaviors?

Disallowing multiple openlayers-behaviors on ones that don't allow it: Where is the best place to check for this? I was thinking we would just leave this up to the UI. But I could add in some error checking.

tmcw’s picture

tmcw CreditAttribution: tmcw commented

Why aren't we doing a integer-indexed array (as referred to in #2)? I find this nested structure to be really convoluted. Did you check to see whether the Drupal behaviors system could handle calling a single behavior multiple times (it seems like it would) - this issue is not cause to abandon the Drupal behaviors stack for a homegrown foreach idea. I assume that a final patch will include the UI side of this?

zzolo’s picture

zzolo CreditAttribution: zzolo commented

I don't see the benefit of an integer based array. It just ensures that if we need to actually refer to a specific behavior we are not able to because an integer index will not be consistent.

We could attach the behaviors multiple times, but with every Drupal attach, it will call all the behaviors (even the ones outside of this module).

I don't think we should abandon using Drupal.behaviors, but it is still valuable to look at our use of it critically because it does actually pose some issues.

tmcw’s picture

tmcw CreditAttribution: tmcw commented

Absolutely, I actually think we should abandon Drupal behaviors down the line, just not like this.

There must be some serious disconnect on an integer based array, because it seems incredibly simple to me. Like, if you have a list of 

array(
'apples' => 'apples',
'oranges' => 'oranges');

in that form, is it really that hard to think, that if you want to duplicate items, you'll make a list

array(
0 => 'apples',
1 => 'oranges',
2 => 'oranges');

With the code example given above,

    'behaviors' => array(
      'panzoombar' => array('behavior' => 'openlayers_behavior_panzoombar'),
      'dragpan' => array('behavior' => 'openlayers_behavior_dragpan'),
    )

what does this become when there is a duplicate behavior? It seems like if we're making a list of behaviors, in which there can be duplicates, the obvious simple solution is to make it integer indexed so that it's a list.

And I don't mean calling the Drupal behaviors stack twice; I mean jiggering it so that it calls a behavior twice. This may be impossible, it'll take some thought.

zzolo’s picture

zzolo CreditAttribution: zzolo commented

Well, my thought is using integers makes it more difficult to actually find a specific behavior if needed. A string identifier is much more friendly to consistently identifying an item. If integers are used, it does not matter what integer is assigned to what item, just as long as they are different. I don't actually see the need for such explicit keys at the moment, but it seems like it could easily be helpful for something that I am not currently seeing.

tmcw’s picture

tmcw CreditAttribution: tmcw commented

Yeah, this doesn't seem to make sense at all. How will this work in the UI? Will users choose... string identifiers? Will these be exposed in the interface? I think that this is a big case in which we can't just think about the "API," without thinking of any of its uses (or more accurately, its main use). Like, an interface for multiple configurable items would probably look something like this.

Why is it important to be able to refer to behaviors by their 'string identifiers'? This seems like a random request - like, views displays are integer-indexed - you refer to them by choosing their display plugin or by manually adding a name to them. Is there anything lost there that people are begging for, the ability to refer to views that might be identical in settings as different things? Like, there's just no pattern, precedent, or reason for making string identifiers the 'primary keys' of exportables, etc, so I don't know why it is a priority.

tmcw’s picture

tmcw CreditAttribution: tmcw commented
Assigned: Unassigned » tmcw

This ticket is way stale. I'll start working on this after fixing another bug or two.

zzolo’s picture

zzolo CreditAttribution: zzolo commented

Still a bit stale. mmmm. How about we mark as postponed and aim for this feature to be in a 2.1 release or something like that? Also, take this off the critical list?

tmcw’s picture

tmcw CreditAttribution: tmcw commented
Priority: Critical » Normal

Yeah, writing this without introducing bugs would be very difficult and it's not killing most users (who probably have a few tooltip layers max), let's push it back to a post-2.0 release.

zzolo’s picture

zzolo CreditAttribution: zzolo commented
Version: 6.x-2.x-dev » 7.x-2.x-dev
Assigned: tmcw » Unassigned
Category: task » feature
Priority: Normal » Minor

Still a good idea. Updating for D7. Also, the move to Git will allow this to be developed with more stability.

fletch11’s picture

fletch11 CreditAttribution: fletch11 commented

Agree that it's not critical but would be very useful if using a number of layers.

zzolo’s picture

zzolo CreditAttribution: zzolo commented
Component: OpenLayers API » OL API
Status: Active » Closed (won't fix)

@phayes, if you want to still work on this, please re-open.

Closing for now. though I like the idea, I have yet to run into anyone that needs this. With layers and popups and styles, we have the ability to put on many data overlays, and even can use attribute replacement based styles. As far as popups, theres the limitation in OL about only one Feature Select control per layer (feature?).

REgarding popups, it makes sense to have a very lightweight abstraction on top of it, so that can attach any number of behaviors to a popup event. #1285154: Abstract OpenLayers.Control.SelectFeature a bit so that features can have multiple handlers

And then there is the work being done on behaviors to switch to a lightweight module specific behavior system. #1332836: Behavior re-architecting, use map level event system