Geofeatures Leaflet implementation #1129

lmgonzales · 2018-03-02T06:37:57Z

Provides the additional option to use Leaflet with the Geographic Features functionality for both the MapSelection and MapTab features.

@demiankatz , there were a couple of files where I had some questions for you:

MapSelection.php
-- I re-wrote much of this code, but wasn't sure how to handle the setConfig method, so I just set all the options in this method. It seems like this could be simpler, but I'm not sure how.
theme.config.php
-- I included geofeatures.css in the css file list in this file, but am not 100% sure that's the best place to put it.

Thanks!

demiankatz · 2018-03-02T13:05:17Z

@lmgonzales, I'll start by answering your questions above.

1.) Regarding setConfig, that code is necessary to implement the conditional .ini loading supported by the MapSelection module, as documented in searches.ini:

; MapSelection:[ini section]:[ini name]
;       Enable geographic searching capability via OpenLayers3 API by activating
;       this module. Records must be indexed using the geographic search and display
;       fields. See the marc_local.properties file for more information on indexing.
;       Default settings and more comments may be found in the  [MapSelection]
;       section in this file. The section name and ini file name loaded by the
;       module may be overridden through the [ini section]/[ini name] parameters.

The question is, do we care about that functionality? The main reason to allow ini section/ini name to be overridden is so that you can load MapSelection with different sets of configurations in different contexts. If that would never happen, these settings are pointless. So, I think there are two options:

a.) If the configuration is pointless, drop it from the documentation and note in the changelog that it is gone.

b.) If the configuration is useful, make the configuration loading class more flexible, and do the loading inside setConfig based on the incoming settings.

2.) Regarding geofeatures.css, you should only add that to theme.config.php if it is important to load it on every single page of the site. If it is only needed in cases where a map is present, it is better to conditionally load it in the relevant template with $this->headLink()->appendStylesheet('geofeatures.css');. My guess is that you would only need to put this in two templates (the MapSelection recommendation module and the Map tab) to get things working... but perhaps I am mistaken.

I hope that is helpful; let me know if you still have questions.

If it's smooth sailing from here, please inform me when you're ready for me to actually test this -- I figure I should wait until you've made any adjustments based on these answers before I take a closer look.

lmgonzales · 2018-03-02T16:36:59Z

@demiankatz , thanks for the clarification on those points.

for 1) One thing we talked about at the summit last year was possibly making it possible to configure where the map selection feature is loaded. So for example, we could have it load in its default location, or perhaps load it as an add-on at the bottom of the advanced search page (like is done here: https://www.finna.fi/Search/Advanced). With that in mind, I have no problem leaving the code as it is to allow for future flexibility.

for 2) I've removed the changes to the theme.config.php file and inserted the css call in the MapSelection.phtml file. We don't need this css for the Map Tab since it only applies to the clusters.

So at this point, I think it's all ready for you to test. Thanks!

demiankatz · 2018-03-02T17:22:11Z

Sounds good -- I'll try to take a look on Monday!

demiankatz

@lmgonzales, see below for a few comments based on another quick read-through. Also, two other things:

1.) I was unable to get the code to run on my test server -- Solr is returning a "400 bad request" when I try to do a geographic search. I'm guessing there may be a bug in the configuration loading somewhere. Let me know if you are unable to reproduce the problem and I can do more digging on this end. All I did was check out your branch, start up a test instance with phing startup, uncomment the default_top_recommend[]=MapSelection line in searches.ini, and try a geo search.

2.) For some reason, Codacy was complaining about some of the indentation in your JS files... but after I merged master and made my ZF ServiceManager fix, it ran again and stopped complaining. Weird.

demiankatz · 2018-03-05T16:23:36Z

config/vufind/config.ini

-; (see import/marc_local.properties for more information).
+; These configuration settings have been superseded by the geofeatures.ini file.
+; See the [MapTab] section of the geofeatures.ini file for more information.
+;


If geofeatures.ini is the master configuration now, I'd suggest deleting all extra configuration comments that duplicate information found in that file. It makes the documentation easier to maintain if we only have any given piece of it in one place at a time... The "see for more information" note is helpful, but the rest can probably be deleted.

Makes perfect sense to me. I'll delete all the unnecessary configuration comments and leave the "see for more information..."note in place.

Should we also remove all of the legacy configuration settings too and only read from the geofeatures.ini file (rather than from config.ini and searches.ini and then geofeatures.ini)? That could streamline the code a lot if we did this.

As long as geofeatures.ini offers the same flexibility as the old code did (and I'm pretty sure it does), I don't mind removing the legacy locations... but if we do, we'll need to be sure to document it, and also add support to VuFind\Config\Upgrader to detect the old settings and move them to the new locations. So ultimately it's a question of whether it's more work to write the upgrade code or retain the legacy compatibility.

demiankatz · 2018-03-05T16:24:41Z

config/vufind/geofeatures.ini

+[GeoPlatform]
+; Configures the mapping platform that should be used.
+; Replaces config.ini recordMap setting.
+; Options:  leaflet (default), openlayers (legacy)


How long do you plan on maintaining openlayers as a legacy platform? Is it worth doing?

I'd prefer not to maintain it if possible, and honestly, I'm don't think it's really worth it given the improvements with the leaflet code. I'm happy to make this a clean transition and remove all the OpenLayers code with this PR if you think it's advisable.

If that's the end goal, I think it's probably simpler to just go ahead and do it now.

Ok. Will do.

demiankatz · 2018-03-05T16:25:28Z

config/vufind/searches.ini

@@ -628,6 +628,9 @@ view=full
 ; in the default recommendations section. To set the configuration settings 
 ; for this feature, adjust the parameters below.
 [MapSelection]
+; These configuration settings have been superseded by the geofeatures.ini file.
+; See the [MapSelection] section of the geofeatures.ini file for more information.
+;


As noted above, we should delete redundant documentation here. Also note that the [MapSelection] section is referenced a couple other places earlier in this file. We should update those references too if necessary.

Will do. Thanks for the note.

demiankatz · 2018-03-05T16:27:59Z

module/VuFind/src/VuFind/Recommend/Factory.php

        $backend = $sm->get('VuFind\Search\BackendManager');
        $solr = $backend->get('Solr');

        // add basemap options
        $basemapConfig = $sm->get('VuFind\GeoFeatures\BasemapConfig');
        $basemapOptions = $basemapConfig->getBasemap('MapSelection');

-        return new MapSelection($config, $solr, $basemapOptions);
+        // get MapSelection options
+        $mapSelectionConfig = $sm->get('VuFind\GeoFeatures\MapSelectionConfig');


When I first tested this branch, I got a fatal error because there was a $sm->getServiceLocator()->get() call here, which no longer works following the upgrade to ServiceManager v3. I've gone ahead and fixed it, but just wanted to let you know. In the old SMv2, factories were passed a plugin-manager, and you had to use getServiceLocator to move up to the parent top-level service manager. In SMv3, the top-level service manager is always the one passed to factories, which makes things a lot more consistent and readable.

Ok. Good to know and thanks for the catch and explanation. Did this resolve the 400 bad request error?

Fixing this didn't help with the bad request -- it resolved a 500 error that I was getting before I ran into the 400 error. I haven't tested the code since your latest revisions, though; I'm in meetings all day today, and tomorrow we're expecting a blizzard, but I'll try to give it another shot later in the week!

Ok. Let me see if pulling out all the OpenLayers code and refactoring things fixes this. Let me see if I can reproduce it.

I've mentioned that this problem was fixed elsewhere, but I'm also commenting on it here so we can see that this conversation is resolved when reviewing the PR in future. This is all better now! :-)

demiankatz · 2018-03-05T16:31:09Z

themes/bootstrap3/templates/RecordTab/map.phtml

+      $popupTitle = $this->transEsc('map_results_label');
+      $params = [json_encode($mapTabData), json_encode($popupTitle), json_encode($mapGraticule), json_encode($basemap)];
+    } elseif ($mapType == 'leaflet') {
+      $mapTabData = $this->tab->getLeafletMapTabData();


If you're always setting a $mapTabData variable in the template, I wonder if it would be cleaner to make $this->tab->getMapTabData smarter internally, so it just returns the right kind of data, rather than having to create a separate public getLeafletMapTabData method. That way, if in future you remove the OpenLayers implementation, there's not a Leaflet-specific method name floating around as a potential source of confusion.

(This is a very minor point, by the way -- just something that struck me as a potential code clarification).

Good point. And it would give us the flexibility to change mapping platforms in the future if something better than leaflet comes along. I'll update the Map.php file accordingly.

…ufind into geofeatures-leaflet

demiankatz · 2018-03-06T14:10:37Z

@lmgonzales, a couple of new notes:

1.) I had a few minutes before my meeting, so I managed to repeat my tests... I'm still getting the 400 error. The issue seems to be that the geographic search link is pointing to this url: /Search/Results?filter[]=long_lat:Intersects(ENVELOPE()) -- note the empty ENVELOPE. I assume that means that some configuration is not loading correctly. As noted before, all I did was uncomment the appropriate top recommend setting in searches.ini to turn on geographic search, and it looks like there is a default box set in geofeatures.ini.... so I don't think the configs look wrong by default, but something seems to be preventing them from working.

2.) The intermittent Codacy errors about Javascript indentation are back -- you can see them if you click through the "Details" link on the Codacy review line in the "Some checks were not successful" box. Probably worth cleaning that up if you can.

Let me know if you need more help from my end! Thanks again!

lmgonzales · 2018-03-07T01:51:10Z

Thanks for the feedback @demiankatz . I'll work on streamlining the code to remove all the OpenLayers code and this should hopefully fix those issues. I'll also go back and fix the spacing and other things codacy is complaining about with the js files. Thanks again!

demiankatz · 2018-03-21T13:53:42Z

@lmgonzales, I've merged the latest master into this branch to save you time on conflict resolution; hopefully I haven't broken anything in the process! Anyway, I imagine you've been busy with other things; no rush to get back to this, especially since I'll be away at a conference most of next week!

…ufind into geofeatures-leaflet Conflicts: module/VuFind/src/VuFind/RecordTab/Map.php themes/bootstrap3/templates/RecordTab/map.phtml

lmgonzales · 2018-03-25T07:18:19Z

@demiankatz, I've removed all the openlayers code, and found and fixed the issue that was causing the 400 issue. Please let me know if you find any other issues when you get a chance to test the code. Thanks!

demiankatz · 2018-04-09T17:36:12Z

module/VuFind/src/VuFind/GeoFeatures/BasemapConfig.php

@@ -90,76 +71,61 @@ public function __construct(\VuFind\Config\PluginManager $configLoader)
     */
    public function getBasemap($origin)
    {
+        $validFields = ['basemap_url', 'basemap_attribution'];


If $validFields is always going to be the same thing for all getOptions calls made by this class, perhaps it would be better to set it as a property... then you could use $this->validFields in all of the getOptions calls, and avoid the need to pass the value down to getMapSelectionBasemap and getMapTabBasemap. Might improve readability a little bit and also potentially increase extensibility (though I don't think it's incredibly likely that anyone would care about overriding it).

demiankatz · 2018-04-09T18:14:06Z

themes/root/templates/HelpTranslations/en/geosearch.phtml

-        pointing at the cluster. Click on the cluster, and a pop-up window will appear showing a list of titles for the
-        records at that location. Each title is hyperlinked to the associated record.
-        Click on the title to view the full record.</p>
+        double-click (zoom in) or shift + double-click (zoom out). You can also click on a cluster to zoom to 


@xmorave2, the geographic interface is changing slightly due to the switch to Leaflet instead of OpenLayers. Would you have a moment to adjust the Czech translation of this file to match?

demiankatz · 2018-04-09T18:23:18Z

@lmgonzales, as you've probably already noticed, I just left a handful of comments and made a few minor edits here and there. When testing the code, I continued to see the missing point problem, but I at least discovered a relevant clue -- it looks like there may be a /vufind path hard-coded somewhere. I'm getting 404 errors for paths like this:

/vufind/themes/bootstrap3/css/vendor/leaflet/images/rectangle-icon-2x-red.png

However, on my test instance, the correct path should be:

/vufind_test/themes/bootstrap3/css/vendor/leaflet/images/rectangle-icon-2x-red.png

Any idea where that problem might be coming from? I'm happy to stick around after tomorrow's dev call for a screen share if time permits and you're interested.

lmgonzales · 2018-04-10T15:56:49Z

@demiankatz, Thanks for the code improvement suggestions. I've updated the js files so the relative links are gone, and implemented the getDefaultOptions for all of the Config files in the GeoFeatures directory. Please let me know if you see any other improvements/edits/fixes to make. Thanks!

… settings).

demiankatz · 2018-04-10T19:45:00Z

@lmgonzales, thanks again for the latest round of work. This is looking great! Just one small action item and one note:

1.) The text "This is the center point for the highlighted rectangle" is currently hard-coded in map_selection_leaflet.js; we need to run this through the translator. As noted in earlier discussion, you'll probably want to add this string to en.ini, and then use the jsTranslation() view helper to register the translation in the templates that are including map_selection_leaflet.js. Once the string is registered through the template, you can use VuFind.translate() in your Javascript code.

2.) I changed the way that getDefaultOptions() is used in all of your config loading classes. Instead of returning defaults only when all configuration files are empty, I'm instead always returning $options + $this->getDefaultOptions() -- this means that any settings that are omitted from the .ini file will be automatically populated with default values. I was encountering some weird errors when I filled in one .ini setting but omitted a different one. Merging in the defaults solved the issues. I think this is a further sign that it was the right choice to go this route!

You might want to look over my last few changes and make sure I haven't broken anything. At this stage, I think the finishing touches on translation are the only thing holding us up from merging.

lmgonzales · 2018-04-10T21:21:29Z

@demiankatz, Thanks for the suggestion and catch on the hard coded string. I've updated the en.ini file and the template and js file so that the "rectangle_center_message" can be translated accordingly. I took a look through your changes, and they all look good to me. Thanks again for your help with the code development.

demiankatz · 2018-04-16T18:24:43Z

Thanks again, @lmgonzales! I have just merged this after making a few more small changes.

demiankatz · 2018-04-16T18:28:11Z

@lmgonzales, if you think I should add anything more to the changelog, please let me know. I put a couple of notes in the 5.0 section.

lmgonzales · 2018-04-16T18:29:10Z

Thanks Demian. Yes, updating the changelog and the Geographic Features pages on the wiki are on my to do list. Kind regards, Leila *From:* Demian Katz [mailto:notifications@github.com] *Sent:* Monday, April 16, 2018 11:28 AM *To:* vufind-org/vufind *Cc:* Leila Gonzales; Mention *Subject:* Re: [vufind-org/vufind] Geofeatures Leaflet implementation (#1129) @lmgonzales <https://github.com/lmgonzales>, if you think I should add anything more to the changelog <https://vufind.org/wiki/changelog>, please let me know. I put a couple of notes in the 5.0 section. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1129 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMc0UWEcYn00d71Wk8Vl_eRj8prM8oY5ks5tpOK_gaJpZM4SZdwg> .

lmgonzales added 2 commits March 2, 2018 06:20

geofeatures leaflet implementation

a42a256

fix trailing commas

f35804c

fix spaces, relocate css call

38108fb

demiankatz added 2 commits March 5, 2018 11:15

Merge branch 'master' into geofeatures-leaflet

4cbec89

Update to ServiceManager 3 style.

f71ca8d

demiankatz requested changes Mar 5, 2018

View reviewed changes

lmgonzales and others added 5 commits March 6, 2018 02:19

update config files, getMapTabData method

f985bbf

Merge branch 'geofeatures-leaflet' of https://github.com/lmgonzales/v…

b4fd6ea

…ufind into geofeatures-leaflet

update config files

efd3dbf

Merge branch 'master' into geofeatures-leaflet

b2af765

php-cs-fixer

d8d7ef8

Merge branch 'master' into geofeatures-leaflet

8f5e2c5

lmgonzales added 7 commits March 25, 2018 05:42

Removed OpenLayers code. Fixed GeoSearch call

8c2be59

Merge branch 'geofeatures-leaflet' of https://github.com/lmgonzales/v…

be22eb6

…ufind into geofeatures-leaflet Conflicts: module/VuFind/src/VuFind/RecordTab/Map.php themes/bootstrap3/templates/RecordTab/map.phtml

fixed spacing in js files

f946295

resolve diffs

f7eb068

fix diff in map.phtml

9866946

fix php tags

3205588

fix php tags

7ffb819

lmgonzales and others added 4 commits March 25, 2018 07:19

fix php tags

3dfdf90

Merge branch 'master' into geofeatures-leaflet

bcd4fac

Add missing space.

88927f7

Remove unused code.

4836411

demiankatz reviewed Apr 9, 2018

View reviewed changes

demiankatz added 5 commits April 9, 2018 13:48

Remove unused parameter.

def33ce

Cleanup/simplification.

63f30b1

Merge branch 'master' into geofeatures-leaflet

d80c1f7

Style tweaks

f7160bd

Typo fix.

378f18d

demiankatz reviewed Apr 9, 2018

View reviewed changes

lmgonzales added 4 commits April 10, 2018 04:30

remove hardcoded icon paths

3ad1a03

fix path to icons

c7b3b74

remove debug code

8116778

refactor with getDefaultOptions

586be5f

demiankatz added 6 commits April 10, 2018 15:07

Merge branch 'master' into geofeatures-leaflet

07b1d98

Comment out settings by default (to avoid trumping legacy non-default…

07316af

… settings).

Remove unused parameter.

71195d0

Code simplification.

8859d1b

More code simplification.

ea34169

Fill in missing default configs.

0ee6b7f

demiankatz and others added 2 commits April 10, 2018 15:55

Merge branch 'master' into geofeatures-leaflet

4ef1530

update to translate center point message

8ea1d31

lmgonzales and others added 3 commits April 10, 2018 21:25

clean up spacing

1c92b8a

Code simplification/cleanup.

c9bb73a

Eliminate non-ASCII characters in .ini

418217d

demiankatz merged commit c4a584a into vufind-org:master Apr 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Geofeatures Leaflet implementation #1129

Geofeatures Leaflet implementation #1129

lmgonzales commented Mar 2, 2018

demiankatz commented Mar 2, 2018

lmgonzales commented Mar 2, 2018

demiankatz commented Mar 2, 2018

demiankatz left a comment

demiankatz Mar 5, 2018

lmgonzales Mar 5, 2018

lmgonzales Mar 5, 2018

demiankatz Mar 6, 2018

demiankatz Mar 5, 2018

lmgonzales Mar 5, 2018

demiankatz Mar 6, 2018

lmgonzales Mar 7, 2018

demiankatz Mar 5, 2018

lmgonzales Mar 5, 2018

demiankatz Mar 5, 2018

lmgonzales Mar 5, 2018 •

edited

demiankatz Mar 6, 2018

lmgonzales Mar 7, 2018

demiankatz Apr 9, 2018

demiankatz Mar 5, 2018

lmgonzales Mar 5, 2018 •

edited

demiankatz commented Mar 6, 2018

lmgonzales commented Mar 7, 2018

demiankatz commented Mar 21, 2018

lmgonzales commented Mar 25, 2018

demiankatz Apr 9, 2018

demiankatz Apr 9, 2018

demiankatz commented Apr 9, 2018 •

edited

lmgonzales commented Apr 10, 2018

demiankatz commented Apr 10, 2018

lmgonzales commented Apr 10, 2018

demiankatz commented Apr 16, 2018

demiankatz commented Apr 16, 2018

lmgonzales commented Apr 16, 2018 via email

Geofeatures Leaflet implementation #1129

Geofeatures Leaflet implementation #1129

Conversation

lmgonzales commented Mar 2, 2018

demiankatz commented Mar 2, 2018

lmgonzales commented Mar 2, 2018

demiankatz commented Mar 2, 2018

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmgonzales Mar 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmgonzales Mar 5, 2018 • edited

Choose a reason for hiding this comment

demiankatz commented Mar 6, 2018

lmgonzales commented Mar 7, 2018

demiankatz commented Mar 21, 2018

lmgonzales commented Mar 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demiankatz commented Apr 9, 2018 • edited

lmgonzales commented Apr 10, 2018

demiankatz commented Apr 10, 2018

lmgonzales commented Apr 10, 2018

demiankatz commented Apr 16, 2018

demiankatz commented Apr 16, 2018

lmgonzales commented Apr 16, 2018 via email

lmgonzales Mar 5, 2018 •

edited

lmgonzales Mar 5, 2018 •

edited

demiankatz commented Apr 9, 2018 •

edited