Add ability to toggle included plugins on/off?

[indigoxela]

This might need more feedback...

Currently the included plugins are on by default:

• Control.Coordinates (shows clicked position lat/lon and allows copy)
• Control.Zoomslider (the enhanced zoomslider)
• Leaflet.fullscreen (toggle fullscreen display)
• leaflet.viewcenter (allows to reset the map to initial display (coords and zoom)

Maybe a setting to switch those on/off. Not sure yet, if that's worth the effort.

[AlexHoebart-ICPDR]

Picking up this old topic, as I just worked on something related. I'm also using the ip_geoloc module, which is making use of the Leaflet module. These modules go well together in Drupal7 and I worked on this in the Backdrop port, you can see this PR https://github.com/backdrop-contrib/ip_geoloc/pull/9. In that module there are switches for Fullscreen, and also the Scale, additionally a mini-map. So, it can be configured that these controls are not duplicated. However, it's not possible to switch off the controls set already in the Leaflet module. If there is a better way to integrate these modules, it would be an improvement. So, in summary, it would be nice to have these option switches, but I don't think they are necessary. I'm fine with the default controls by Leaflet.

[indigoxela]

I'm also using the ip_geoloc module, which is making use of the Leaflet module.

Interesting... what does that module combo do?

I never used ip_geoloc, but use the lightweight geoip_tokens module to center (empty) maps.

[indigoxela]

it would be nice to have these option switches, but I don't think they are necessary.

I'd say, it's not urgent. :wink: And given, that a bigger rewrite just happened (fix code duplication), I'd rather wait a bit to see, if there's any regression with the recent changes. Not that I'd expect regressions, but the rewrite was quite "juicy".

[AlexHoebart-ICPDR]

Interesting... what does that module combo do?

It does a lot, for me it was it important that it can take coordinates from database fields (instead of GeoField), can display different markers based on field values and it can synchronize map markers with table rows. The Backdrop port can do that already, but it's not complete. See https://www.drupal.org/project/ip_geoloc and https://github.com/backdrop-contrib/ip_geoloc/pulls

[AlexHoebart-ICPDR]

Hi, I'm coming back to this. If not a switch in the UI, is there a way to override some map settings in a custom module? For example, I would like to remove the Zoomslider and use the default zoomControl instead.

[indigoxela]

If not a switch in the UI, is there a way to override some map settings in a custom module?

Currently ... not really. There's still a lot to do in Backdrop.behaviors.leaflet.attach, I admit. But currently I don't have the time for a rewrite.

However, I could expose the controls for custom code. You seem to be the only one requesting it - and I know you can handle that code. :wink:

In My PR these controls now get exposed, so in a custom module (custom js) you can remove some/all of them.

Given, your custom module's name is "foobar", you'd need the following (with the patch applied):

foobar.module

<?php
/**
 * Implements hook_preprocess_HOOK().
 */
function foobar_preprocess_page($variables) {
  $path = backdrop_get_path('module', 'foobar') . '/js/foobar.js';
  backdrop_add_js($path, array(
    'group' => JS_LIBRARY,// Make sure this loads before leaflet.backdrop.js.
    'every_page' => TRUE,
  ));
}

And js/foobar.js

(function($) {
  "use strict";
  Backdrop.behaviors.foobar = {
    attach: function (context, settings) {
      // Add event listener to custom leaflet map creation event.
      $(document).on('leaflet.map', function (event, map, lMap) {
        // Remove all existing controls.
        lMap.fullscreenControl.remove();
        lMap.viewcenterControl.remove();
        lMap.zoomsliderControl.remove();
        lMap.controlCoordinates.remove();
        // Add simple default zoom buttons.
        let simpleZoom = L.control.zoom({ position: 'topleft' });
        lMap.addControl(simpleZoom);
      });
    }
  };
})(jQuery);

It turned out, there's already a custom event, other code can listen to, but the controls weren't exposed, yet.

[AlexHoebart-ICPDR]

Thank you for this quick reaction and solution. This works well and I am able to use it in my custom module.

I'm learning and getting to understand a bit better how this all works... And I'm now also co-maintaining the ip_geoloc module. That module has switches in the UI to enable the fullscreen and scale controls (metric and imperial). Currently, the scale controls appears also when switched off, because is always enabled by the leaflet module. To make switching off work, I noticed this solution: I made a PR https://github.com/backdrop-contrib/leaflet/pull/62 which slightly changes the javascript in leaflet module to allow settings to be overridden (like fullscreen) and only adds the controls if they are enabled. With this, the switches in ip_geoloc work in both ways and it is now also possible to change these settings via Implements hook_leaflet_map_prebuild_alter.

I also added this for zoomControl, so it is possible to enable this the same way (and also in the ip_geoloc module) and add ZoomSlider only if no zoomControl is set - both don't make sense.

I'm not sure about the viewcenterControl. In ip_geoloc, the L.control.reset is used which behaves very similarly. Probably, only one makes sense.

With this PR, the default behaviour of leaflet module is the same of course.

[indigoxela]

I'm learning and getting to understand a bit better how this all works...

Hey, welcome to the club. There's still corners in this module's code, I don't fully understand, yet. :laughing:

I made a PR https://github.com/backdrop-contrib/leaflet/pull/62 which slightly changes the javascript...

I see. That's a slightly different approach, but absolutely worth to consider. However, besides the "trailing whitespace" nagging when applying the patch, there's also some problem with the zoom vs. zoomslider logic.

By default the setting zoomControl is set to true (in PHP), but the current module JS (oddly) changes that to a zoomslider. Unfortunately we can't use the setting for checking, whether this should be zoom or zoomslider. The check if (!this.map.settings.zoomControl) will always end up with zoom buttons, unless one sets the value to false in the alter hook in php.

The setting 'scaleControl' : {metric: true, imperial: false}, makes assumptions, that work for you and me, but change the defaults, by turning imperial off.

My current suspicion is, that if I touch the settings object in JS, this will be the rewrite, I currently don't have the time for. With all the pitfalls a rewrite ships with. :wink: But I'll take a closer look at the weekend.

Just to make sure: the approach to expose the controls would work for you?

[AlexHoebart-ICPDR]

Thanks for your response and sorry that I did not test my changes well, particularly not with the Leaflet Views plugin as I'm not using it. I updated the PR accordingly:

• set the zoomControl to false by default in PHP code (Why would that be not possible?)
• used the merged settings instead of this.map.settings (I think this could be also adapted further down in the code, but I did not check that)
• Setting imperial: false for the scale in JS was not my idea, it was in the old code. I agree that showing both scales by default makes sense.

The approach to expose controls in Javascript also works for me in my custom module. It's kind of end-of-pipe solution, but I think it is good for some cases, e.g. if somebody wants to reorder the controls (I think they appear just in the order added, right?). That's why I kept it in my PR. My PR makes it even easier for other modules (like ip_geoloc) to change the map controls. I did not check if the ip_geoloc module could use the Javascript approach to force its settings, but it would make it even more complicated.

Sorry for going also beyond this: I added a small change which uses the small zoomControl when the map is too small (and the ZoomSlider does not fit into the window height) - just as illustration how this approach could make sense. It's not needed, but would make the module more adaptable.

[indigoxela]

... and sorry that I did not test my changes well

No need to excuse. It's my job to review and consider stuff like that. :wink:

I updated the PR accordingly... set the zoomControl to false by default in PHP

I'm not convinced, that's a good idea. How would you turn it off, then? I know, this currently also doesn't behave properly, but I'd not want to make things even weirder. :wink:

The approach to expose controls in Javascript also works for me in my custom module. It's kind of end-of-pipe solution...

Yes, a straw (sort of). It doesn't interfere with the current defaults and doesn't prevent future improvements.

My PR makes it even easier for other modules (like ip_geoloc) to change the map controls.

I see. But, sorry,... your current PR is a bit too much.

Changing a default value is sort of a red flag. Additionally, using 'zoomControl' that way is IMO quite confusing, as this is a Leaflet (library) setting to turn the default zoom buttons on/off. There's a reason, why I hesitate to rework Backdrop.behaviors.leaflet.attach. :wink:

Probably you'll have to live with a compromise for now, we'll see. Particularly re the zoom vs. zoomslider problem. I'll try to provide as much flexibility as possible, but you'll have to be a little more patient re the rewrite of that functionality. That rewrite won't happen this weekend. :grin:

[indigoxela]

Another idea (without even testing anything): have you considered, to override Backdrop.behaviors.leaflet.attach, yet?

In theory you can. "Backdrop" is the global space, so you should be able to simply overwrite it with a (later loaded) js file, which also implements Backdrop.behaviors.leaflet.

[AlexHoebart-ICPDR]

Changing a default value is sort of a red flag. Additionally, using 'zoomControl' that way is IMO quite confusing, as this is a Leaflet (library) setting to turn the default zoom buttons on/off.

That default value from PHP was anyway overridden in the JS which builds the map.

        for (let setting in this.map.settings) {
          settings[setting] = this.map.settings[setting];
        }
        settings.zoomControl = false; // replaced by L.Control.Zoomslider

Where would that default from PHP have any implications?

OK, I understand that zoomControl setting would bee a bit misused and it would not be possible have no zoom control. But I was just following the current behaviour of the module JS (not the leaflet library) which does the same. I could change the PR to introduce a new setting for Zoomslider independently and set that to TRUE by default. That would be a more acceptable approach?

I have no problem if that PR is put on hold for a while due to further review and testing needed. Still I would like to make it "right".

[indigoxela]

That default value from PHP was anyway overridden in the JS which builds the map.

I know, and I also mentioned that. :wink:

Still I would like to make it "right".

And that's highly appreciated. :+1:

But... I take the liberty to be a bit more cautious. I'd like the change to be both, backwards compatible and future proof. :stuck_out_tongue_winking_eye:

I'd suggest to stop working on your PR until I find the time to give it the attention it deserves. Parts of it are cool.

[AlexHoebart-ICPDR]

Another idea (without even testing anything): have you considered, to override Backdrop.behaviors.leaflet.attach, yet?

In theory you can. "Backdrop" is the global space, so you should be able to simply overwrite it with a (later loaded) js file, which also implements Backdrop.behaviors.leaflet.

Well, I'm not so much familiar with these Javascript concepts, honestly. :man_shrugging: Actually, the Backdrop.behaviors.foobar approach you suggested above did not work for me. I just started with $(document).on('leaflet.map', function (event, map, lMap) { and I was happy that it worked like that - maybe has something to do with the ip_geoloc module which uses that approach.

But I see what you mean. But then I would copy quite a big chunk of code and would miss any future improvements there... :worried: :wink:

[indigoxela]

I'm not familiar with ip_geoloc, so I can't suggest anything with it. :shrug: I'm relying on you (as maintainer) to report back any glitches with the module combo. :wink:

But then I would copy quite a big chunk of code and would miss any future improvements there...

That's a clear disadvantage, yes.

[indigoxela]

Thinking this over: the JS-only approach might not work for many people. We have a php API, so why not using that?

Here's now a totally different approach: #63

An example module, that removes all controls besides the attribution (and layer toggler, if any):

<?php

/**
 * Implements hook_leaflet_map_prebuild_alter().
 */
function customleafletmap_leaflet_map_prebuild_alter(&$settings) {
  // Remove all controls.
  $settings['mapControls'] = array(
    'ControlFullscreen' => FALSE,
    'ControlScale' => FALSE,
    'ControlZoomslider' => FALSE,
    'ControlCoordinates' => FALSE,
    'ControlViewCenter' => FALSE,
  );
  // Also the default zoom buttons.
  $settings['map']['settings']['zoomControl'] = FALSE;
}

As you can see, there's a new array "mapControls". To get rid of the zoomslider, but use the zoom buttons instead, you'd skip the last override.

Or, a little simpler, just turn off two of them (zoomslider and viewcenter):

<?php

/**
 * Implements hook_leaflet_map_prebuild_alter().
 */
function customleafletmap_leaflet_map_prebuild_alter(&$settings) {
  $settings['mapControls']['ControlZoomslider'] = FALSE;
  $settings['mapControls']['ControlViewCenter'] = FALSE;
}

[AlexHoebart-ICPDR]

This new approach looks great, just a few additional comments:

If the JS code would allow this, the API could, additionally to TRUE/FALSE, also set custom options.

For example, in customleafletmap_leaflet_map_prebuild_alter:

$settings['mapControls']['ControlFullscreen'] = array('position' => 'topright');`

Only a tiny change necesary in leaflet.backdrop.js:

        if (controls.ControlFullscreen) {
          settings.fullscreenControl = controls.ControlFullscreen;
        }

For this to work, this code block needs to come after loading the settings object. Same approach could be used to set km/miles and position for ControlScale. I think that would be a nice addition to this approach.

Another thing I noticed, but it's not a real problem: The current ip_geoloc module fails to display a map with this PR as the JS aborts when it does not find this.mapControls. Of course, I would adapt ip_geoloc to set this correctly. But maybe it would be good to set the defaults in the JS as well in any case - not sure, if it is worth the effort.

[indigoxela]

@AlexHoebart-ICPDR many thanks for your feedback, but...

This issue or PR are not supposed to set a position or tweak any options. So don't count on every dream coming true here. :wink: It will be possible to toggle controls on or off - as the issue title says. Requesting more is futile.

Only a tiny change necesary in leaflet.backdrop.js...

My PR works fine for me with the exact code I provided as example in my previous comment. What does ip_geoloc do to break this? (No, I don't see it as my "job" to debug ip_geoloc :wink: ).

Another thing I noticed, but it's not a real problem: The current ip_geoloc module fails to display a map with this PR as the JS aborts when it does not find this.mapControls

We might need some defensive coding in js. But... does ip_geoloc completely bypass leaflet_build_map() (which would be odd), or did you just forget to flush caches after applying the patch?

[indigoxela]

Some updates for my PR, I think it's ready now: #63

A quick search in the ip_geoloc module (via Github UI) - it turned out that your module doesn't seem to implement any leaflet hook at all @AlexHoebart-ICPDR. To be honest, I've no idea, what you're struggling with hook-wise.

Anyway, in file leaflet.api.php I extended the hook documentation for hook_leaflet_map_prebuild_alter() a bit, hope it's helpful. Note: not merged, yet, only in the PR.

If you prefer a diff to apply: https://patch-diff.githubusercontent.com/raw/backdrop-contrib/leaflet/pull/63.diff

[AlexHoebart-ICPDR]

I already mentioned that the PR is great. It works fine.

My additional comments where not based on issues in ip_geoloc module, but based on the ability of the leaflet library itself to not only turn controls on/off but also add some options like "position". It would be a nice-have in general to be able to set such custom options in the hook_leaflet_map_prebuild_alter function. This would be particularly good for the scale control (options "metric" and "imperial") which currently only displays the metric scale.

The ip_geoloc module is not mine, I have also limited understanding of it, just try to make it usable in Backdrop. It uses the leaflet library provided by this module, leaflet.backdrop.js, leaflet_more_maps and leaflet_markercluster and it does it in a "hacky" way as mentioned in the original comment:

    backdrop_add_js(array('leaflet' => array($settings)), $options);

    backdrop_add_library('leaflet', 'leaflet');

    // Little hacky this, but can't see another way to load libraries for
    // Leaflet More Maps, Leaflet MarkerCluster, Leaflet Hash...
    backdrop_alter('leaflet_map_prebuild', $settings);

This just works - and if I ever understand this and find a better way to implement this, I will...

[indigoxela]

The ip_geoloc module is not mine...

Well, now it is, I guess. :stuck_out_tongue_winking_eye:

I don't envy you, it's a complex beast with (obviously) some odd approaches. And still a lot to do. (Leaflet was quite a challenge, too. Loads of commits until I got it working with the most recent Leaflet library.)

This just works...

That's good news, though. Something works.

Does the map rendering of controls change for you with this PR? Without the PR all the additional controls are/were hardcoded in JS, so they always appear. With this PR, though, they no longer get added, if leaflet_build_map didn't run, which seems to be the case in ip_geoloc. But maybe that's what you're after, anyway?