Add basemap specification

[lmgonzales]

Add the ability for the user to specify the geographic base map for the geographic search display and the geographic display (i.e. Map Tab display). Users have the option of 5 open source base map sources to use. The default is osm-intl which provides international language support. Users can add their own basemap by adding it to the geo_basemaps.txt lookup file in the config/vufind/ directory.

[demiankatz]

Thanks for sharing this, Leila. See below for a few thoughts for discussion.

[demiankatz]

Is there a compelling reason to use a tab-delimited text file here? It's not the most readable of format options. Might it make sense to use an .ini, JSON or YAML format instead for greater consistency with other VuFind configuration files? If using an .ini, the section name could be the alias, and it could contain url and attribution options. A similar structure could be built with JSON or YAML if more flexibility was needed... but for this case, .ini may actually be perfectly adequate. (Using .ini would also add the benefit of being able to take advantage of VuFind's built in .ini inheritance functionality through Parent_Config, etc.).

[lmgonzales]

That's a great idea Demian. I like the idea of creating a geofeatures.ini file. What do you think of putting all of the geographic search and display configuration options into one geofeatures.ini file? That way we could pull the configuration settings out of config.ini and searches.ini and it goes into one file. Probably would be easier for long-term maintenance of the code. What do you think?

[demiankatz]

There are a couple of things to think about here:

1.) Right now, some of the geographic features can be configured in two different places. This is either a good thing because it adds flexibility, or it's a bad thing because it requires you to configure everything twice. My gut feeling is that most of the time, users would want these settings to be uniform, and so creating a single .ini file would be an improvement... but we might want to allow the ability to configure somewhere which .ini file is read, defaulting to geofeatures.ini, so that it's still possible to create context-sensitive configurations if necessary.

2.) One disadvantage to the .ini format is that it doesn't allow a lot of nesting. You basically have sections and settings, and in a pinch, you can treat some settings as arrays, for a maximum depth of three levels. My original idea for .ini is that you would have the section name be the name of the basemap, and the settings inside specify the parameters for that map. However, having a design with arbitrary section names doesn't play well with including other types of settings, because you run into a risk of collisions (e.g. if you have a [general] section and then somebody tries to name a basemap 'general'). You could probably work around this by having a [basemaps] section, and using arbitrary key names as arrays, like:

[Basemaps]
basemap1[url] = http://foo
basemap1[license] = foo

basemap2[url] = http://bar
basemap2[license] = bar

...but that's a little less elegant.

Not to say that I have an opinion one way or the other about how to proceed -- but I think those are the important issues to think about when making the decision.

I'd also suggest that no matter what strategy you select, let's not move anything as part of this pull request. We'll just put the basemaps in the place where we ultimately want them to end up, and then we can take care of moving existing settings in a separate PR. That will help us focus on finishing this part before we tackle some new challenges.

[lmgonzales]

Sounds good. I would still opt for a geofeatures.ini file that we eventually migrate all the geo-related configuration settings into (see geofeatures.ini.txt for a general idea).

One thing we could do is create a geofeatures.ini file like attached, and then check the MapSelection (for map search) and MapTab (for map display) sections for the basemap_url and basemap_attribution settings first. If those settings aren't there, then we can fall back to the Basemaps section, and if that fails, we default to the osm-intl basemap. (and for backwards compatibility we can check config.ini and searches.ini before we check geofeatures.ini for the non-basemap related settings).

But for now, I agree - we can just put the basemap settings in geofeatures.ini and look to do a separate PR where we move the rest of the configuration settings over to that ini file.

[demiankatz]

Sounds good. Another thing we can consider is adding some logic to VuFind\Config\Upgrade so that the upgrade script is aware of the moved settings and can either relocate them or at very least warn the user about deprecations so that we can eventually eliminate some of the more complicated extra checking.

[demiankatz]

If we change the other comment, we should remember to make this one match.

[lmgonzales]

Will do.

[demiankatz]

If this code is copied and pasted in two different places, it might make sense to add something like a VuFind\Map\BasemapTrait to avoid redundancy (maybe there's a better namespace than "Map" we could use for this -- just brainstorming here). The two classes could then use the trait, and the only difference between them would be how they configure themselves to override the default $this->basemap.

[lmgonzales]

I'm wondering if just reading the configuration options from a .ini file would solve this issue. That way no lookup function (like getBasemap) is needed, since the settings will just be passed in from the geofeatures.ini file and if those settings are blank, we use the default settings.

[demiankatz]

True -- we can see how the code settles out, but it's definitely possible that moving the configuration will simplify this to the point where it's trivial and the need for a trait simply goes away.

[lmgonzales]

@demiankatz, I've gone ahead and rewrote the code around a .ini implementation to allow for future extensibility. This gets rid of the lookup table and instead puts the basemap configurations into a geofeatures.ini file. The code now checks to see if the basemap configurations are set in either config.ini (for MapTab display), or searches.ini (for Map Selection display) first, and then checks the geofeatures.ini file, and then defaults to the osm-intl setting if all else fails. Please let me know if there are additional places where this implementation can be improved. Thanks!

[demiankatz]

Found time to add a few more comments on this. Shaping up nicely as well... but I want to be sure we're on the same page about the end goal, and had some suggestions for streamlining a little more.

[demiankatz]

The default coordinates have changed here. Is this intentional? If so, we can probably merge the reformatting and adjustment of this .ini file independently of the other unrelated changes....

[lmgonzales]

I will change that back to what it was. Thanks for catching that. Those are the default coordinates I was using on my development server.

[demiankatz]

Thanks!

[demiankatz]

Are you viewing this class as only handling basemap configurations, or would this eventually be expanded to address other geo configurations? I don't object to keeping this small and creating other classes for other things -- but if you're planning to add basemap-unrelated features in future, we might want to change the name.

[lmgonzales]

I'd prefer to keep this as a small class that only handles the basemap configuration settings. If we need other classes for other configuration settings, I figure we can add them in as needed.

[demiankatz]

Great, I agree that that's probably the best approach... and we have a whole GeoFeatures namespace now where we can put additional things. Again, just double-checking.

[demiankatz]

Rather than using the service manager as a parameter to these functions, I'd instead recommend injecting the VuFind\Config service as a dependency through the constructor (and using a factory to build the class). It's generally a good idea to keep the service manager as far away from your classes as possible, since that makes the code easier to maintain -- the service manager can potentially offer access to an infinite variety of unrelated things. For the purposes of understanding relationships between classes, it's better to pass in more specific things so we can figure out where the dependencies are.

[lmgonzales]

Good point. I hadn't considered that. Thanks for the feedback. I'll re-write this to use a factory to build the class then.

[demiankatz]

I see that you've added the factory, but it does not appear that the matching changes to this class were committed. I don't see a constructor to accept the config manager or the removal of $sm parameters to all the methods.

[demiankatz]

This might be a little more concise as:

return [
    $this->basemapOptions['basemap_url'],
    $this->basemapOptions['basemap_attribution']
];

[lmgonzales]

Thanks. I'll make the adjustment.

[demiankatz]

Thanks! I think there's one other place with identical code that can also be adjusted.

[demiankatz]

It seems that the configuration has evolved a little differently than I had expected -- which is not necessarily a bad thing. But just to clarify what I had expected...

I thought we'd have something like this:

[Basemaps]
basemap_url[osm-intl] = https://maps.wikimedia.org/osm-intl/{z}/{x}/{y}.png
basemap_attribution[osm-intl] = "Wikimedia | © OpenStreetMap"

basemap_url[stamen-toner] = http://tile.stamen.com/toner/{z}/{x}/{y}.png
basemap_attribution[stamen-toner] = "Map tiles by Stamen Design, under CC BY 3.0. Data by OpenStreetMap, under ODbL."

...etc.

And then I assumed that some other setting could be set to osm-intl or stamen-toner, which would control which array elements from the [Basemaps] section would be loaded.

That's useful if you want to offer something like user-selectable basemaps, since it lets you define multiple settings at once. But if there's only going to be one setting at a time, then having a list of options and making the user simply uncomment one set is certainly more direct and simpler.

Does that make any sense?

[lmgonzales]

I did think about that, but thought the simpler approach would be easier for the user to specify a different basemap url and attribution by simply adding those values into the basemap_url = and basemap_attribution = settings. I know it's not the original direction, but I thought this would be simpler and more straightforward while allowing for a greater amount of flexibility for the long term. I saw this more as a system configuration setting rather than as a user-selectable option (like a map layer) on the map interface. Do you see any downsides to this approach?

[demiankatz]

The only downside I see is the one I mentioned: that you can't define multiple basemaps for selection by the user using this approach... but if you never see a need to do that, it's not a problem... and honestly, even if that need arises, we might be able to add support in a backward-compatible way by doing an is_array check on the setting... so this is probably fine as it is. I just wanted to double-check!

[lmgonzales]

@demiankatz ,
Thanks for the code improvement suggestions. Creating the BasemapConfigFactory.php to build the BasemapConfig class was much easier than I originally thought it would be. I've updated the code with those changes and also changed the default_coordinates setting back to the original string.

[demiankatz]

I've updated this for compatibility with the Zend ServiceManager 3 changes recently merged to master. However, it appears that at least some of your changes are not quite completed, and I also noticed a couple of property names with underscores that we should probably rename for style reasons (camelCase is preferred for properties). See below for more detailed comments. Please let me know if you need any help getting this up to date. I'll test (and hopefully merge) once I hear back from you.

[demiankatz]

For style consistency, I'd suggest changing this to basemapUrl (I realize we do have some underscores in property names elsewhere, but it's generally frowned upon).

[demiankatz]

Style: I'd suggest basemapAttribution here.

[demiankatz]

I see that you've added the factory, but it does not appear that the matching changes to this class were committed. I don't see a constructor to accept the config manager or the removal of $sm parameters to all the methods.

[demiankatz]

Style: I'd suggest basemapUrl.

[demiankatz]

Style: I'd suggest basemapAttribution.

[demiankatz]

@lmgonzales, I finally had a chance to thoroughly test and review this. It's looking great. Just a few comments and questions before I merge it:

1.) I've significantly rewritten BaseMapConfig.php -- I noticed some repeating logic with a bug in it (there was a check for a missing option INSIDE a loop, but it should have been after the loop), and so I refactored it to a support method to fix the bug. After that was done, one thing led to another, and the whole thing kind of took on a different shape. I'm pretty sure I've preserved the logic, and I think it's a bit easier to follow now -- but I'd be interested in your opinion and double-check. Also note that I made a couple of previously public methods protected since nothing was calling them externally; always best to keep the public interface as small as possible to simplify future refactoring.

2.) I've noticed that some of the attribution strings are so long that they cannot fully display in the available screen real estate. Perhaps this is no longer a problem in Leaflet, in which case I won't worry about it for now.

3.) Do you think we need to worry about internationalization of attribution messages? If so, that adds an extra layer of complexity to everything. I'm not sure if it's worth the effort at this stage -- but it's the only thing I can think of that might still be missing here.

Anyway, thanks again for all the work on this; I think we're very nearly ready to move forward. If you don't think any of the above issues need further action, just let me know and I'll merge as-is.

[lmgonzales]

Thanks for the thorough review @demiankatz. I really appreciate your feedback and help in getting this in shape for merging.

1. Thank you for the re-write. I like what you did here and I've improved and refactored the rest of the GeoFeatures code I wrote for the leaflet implementation that I'll be submitting in an upcoming PR. Thanks finding and fixing that bug and for the explanation on the public/private methods too. Very helpful!

2. Ah, the attribution strings ... thanks for that catch. I updated the basemap attributions so that the information was preserved in the title of the a href tags. This shortens the displayed text considerably. Leaflet has no built in attribution button yet like OpenLayers does, so this will help.

3. Internationalization of attribution messages - We are carrying the attribution as is requested by the basemap sources, and they do not provide internationalization support on their attribution text, so I think we're fine leaving this is for now.

[demiankatz]

Thanks, @lmgonzales, looks great. I've merged it!