stefanocudini / leaflet-search

Search stuff in a Leaflet map
https://opengeo.tech/maps/leaflet-search/
MIT License
766 stars 332 forks source link

Cannot get search working with geojson layer #22

Closed pwhipp closed 10 years ago

pwhipp commented 10 years ago

The symptom is that there are no records being found. Features may be dynamically added and removed from the map so I've tried re-adding the control when this happens.

This is experimental code rather than actual work towards the final app and everything is running on a micro instance with only a single process non production process thread so apologies for the performance:

http://www.footholdgame.com/experiments/scratch_test/

Zoom in quick to avoid a different bug (boundary condition).

The lmap_add_search_control() adds the search control which is not seeing any of the layer data. I'm new to javascript and was having trouble stepping through your code to find out what is going wrong so I hope you can help.

stefanocudini commented 10 years ago

hi! @pwhipp many of the problems that come about Leaflet.Control.Search reported are caused by bad GeoJSON for the standard format look here: http://geojson.org/geojson-spec.html

I tried to download the json features that are loaded on your map, but I could not, if I can send you the file .json I can better understand the problem

FoxxMD commented 10 years ago

If you open the web console you'll see leaflet-search output Unknown Layer for each of your objects when you attempt to initialize the search function....

By default when adding an object to a geojson layer leaflet assumes the feature should be a simple marker. You are adding what should be L.MultiPolygon vector layer, however you don't transform your feature objects at any point before adding them to geojson.

Since you are using addData but your feature objects have a multipolygon geometry property leaflet may be confused on how to add each layer to the geojson object. Move your code from lmap_territory_data_add into the pointToLayer function in the geojson constructor and make sure you return a new L.MultiPolygon object to geojson. This might fix your problem.

p.s. In your lmap_add_territory_layer() function you are not initializing a new GeoJson object correctly. Change lmap_territory_layer = L.geoJson( to lmap_territory_layer = new L.GeoJSON(..

stefanocudini commented 10 years ago

@FoxxMD thanks for your perfect explanation of the problem. I believe that this is the solution to the problem of him, maybe this can be is help: #19

pwhipp commented 10 years ago

Thanks very much for the input. I will investigate further.

The malformed expression was copied from one of the leaflet examples but I'll fix it anyway. Javascript is not fun coming, as I do, from Python but I'm starting to grok it. The chrome developer tools are a big help.

All of the features are coming in as geojson from a service I wrote which uses the geodjango contrib serialising - the population density based colouring and highlighting/name display on mouseover work fine so it's reasonable to assume that the geojson in the features is all good. I added code to Stefano's js to console log the 'unknown layer' after where my reading of the code lead me to infer that it would cope with arbitrary geojson features as well as simple markers but it was not hitting that code in my local experiments: I'll delve deeper there - with that code being hit now it should be a lot easier to debug. All of my features are multi-polygons so this is probably the root of the problem. I'll post the outcome in this thread for the benefit of anyone else who hits a similar problem.

On 12 October 2013 04:52, stefano cudini notifications@github.com wrote:

@FoxxMD https://github.com/FoxxMD thanks for your perfect explanation of the problem. I believe that this is the solution to the problem of him, maybe this can be is help: #19https://github.com/stefanocudini/leaflet-search/issues/19

— Reply to this email directly or view it on GitHubhttps://github.com/stefanocudini/leaflet-search/issues/22#issuecomment-26162062 .

pwhipp commented 10 years ago

Its hard to see how I'm wrong in my current use of the geoJson leaflet layer, given that I'm following the leaflet examples (http://leafletjs.com/examples/geojson.html and http://leafletjs.com/examples/choropleth.html).

I could transform the features into L.MultiPolygon but that seems counter intuitive. I believe I'm currently adding geojson features to a geojson layer.

The test is working fine in leaflet terms - all of the names are accessible as expected and the highlighting of the mouseover regions is working as per the leaflet examples. I'm only having a problem with the search not working.

I added the "unknown layer" console logging to leaflet-search.js at line 461 to see if that was the problem and it appears that it is. The preceding if clause (layer instanceof L.Path) is interesting because it actually looks fairly generic for features. Changing this clause to ('feature' in layer) solves my problem and gets the search working on my map in so far as it brings up the correct list of names and completions.

I think this should be the general behaviour of leaflet-search.

I still have some tricky behaviour regarding the search panning contesting with the maxbounds settings to resolve but I think leaflet-search is behaving correctly here.

Here is the working _recordsFromLayer function (I left it logging Unknown layers which you might want to remove from production code):

_recordsFromLayer: function() { //return table: key,value from layer
    var retRecords = {},
        propName = this.options.propertyName,
        loc;

    this._layer.eachLayer(function(layer) {

        if(layer instanceof SearchMarker) return;

        if(layer instanceof L.Marker)
        {
            if(layer.options.hasOwnProperty(propName))
            {
                loc = layer.getLatLng();
                loc.layer = layer;
                retRecords[ layer.options[propName] ] = loc;
            }
            else
                console.log("propertyName '"+propName+"' not found in marker", layer);  
        }
        else if ('feature' in layer)//(layer instanceof L.Path)
        {
            if(layer.feature.properties.hasOwnProperty(propName))
            {
                loc = layer.getBounds().getCenter();
                loc.layer = layer;          
                retRecords[ layer.feature.properties[propName] ] = loc;
            }
            else
                console.log("propertyName '"+propName+"' not found in feature", layer);         
        }
            else
                    {
                        console.log("Unknown layer", layer);
                    }

    },this);

    return retRecords;
},
FoxxMD commented 10 years ago

interesting find! @stefanocudini perhaps this could be added? Though it might be better to make the conditional and OR statement else if ('feature' in layer || layer instanceof L.Path)

pwhipp commented 10 years ago

I think using ('feature' in layer) subsumes the L.Path so the OR clause would be redundant and potentially confusing.

On 16 October 2013 06:36, Matt notifications@github.com wrote:

interesting find! @stefanocudini https://github.com/stefanocudiniperhaps this could be added? Though it might be better to make the conditional and OR statement else if ('feature' in layer || layer instanceof L.Path)

— Reply to this email directly or view it on GitHubhttps://github.com/stefanocudini/leaflet-search/issues/22#issuecomment-26369944 .

stefanocudini commented 10 years ago

What do you think of hasOwnProperty instead 'in'? what could be the solution more readable?

pwhipp commented 10 years ago

I think 'in' is correct. If I've specialized a feature object there is no reason it should not work. On 19/10/2013 9:16 AM, "stefano cudini" notifications@github.com wrote:

What do you think of hasOwnProperty instead 'in'?

— Reply to this email directly or view it on GitHubhttps://github.com/stefanocudini/leaflet-search/issues/22#issuecomment-26636793 .