Rework geocoder

[v1r0x]

fixes #18 #38

This PR includes a new geocoder with suggestions and (hopefully contact addresses #8 ). Except the contact stuff it already works, but I think it needs some polishing @jancborchardt ;).
The old geocoder is still present. Will remove it before merge. But it is already possible to test the new solution.

cc @Henni @DJaeger

[v1r0x]

I updated the routing machine which also fixes #92
Currently I'm trying to combine the routing machine and the geocoder. Thus it doesn't look good, but works ;)

[v1r0x]

Modifying the geocoder control/routing machine is really f*cked up...Because I found no way to modify them to match our needs, I decided to only use the backend functionality and implement the display and marker stuff myself. Thus this needs some polishing @jancborchardt ;)
Currently the input field is displayed above the map div and I couldn't figure out to display it on the map. Same for the suggestion list. So if anyone (@Henni @DJaeger @jancborchardt) has an idea, you are welcome to fix this! As soon as this is fixed, I'll add the routing stuff back.

[v1r0x]

In the current state of this PR the initial two issues (#18 and #38) are fixed and IMO it looks quite good! :)
@jancborchardt Could you please have a look at the design?
@Henni @jancborchardt Could you both please test the current state? If you don't discover any bugs or any other problems, I'll extend the code to fix #92

[Henni]

@v1r0x please remove code comments before merging

[v1r0x]

Thanks for the info! Will do a cleanup before merging.

[Henni]

@v1r0x how do I test the current state?

[v1r0x]

I you checkout this branch you should have a new input field in the top left corner. With that you can search for locations and there should be up to 5 suggestions. Clicking on one of them should center the map to that location.

[Henni]

Works on Firefox
Doesn't work on Chrome:

[v1r0x]

I know why I like firefox more than chrome...
I'll try to fix this later, thanks @Henni !

[v1r0x]

I see nothing in my chromium log. What version do you have and when did this occur?

[Henni]

No idea what I changed but now it works.
Good job 👍

[v1r0x]

It is now possible to use the geocoder and routing machine. The suggestion popups are still a bit ugly and slide down the other input fields. @jancborchardt An idea how to display them above them?
There are still some things do to, but it works ;)

[v1r0x]

Sorry for the spam, but I updated parts of the code.

• It is now possible to search for favorites. Unfortunately this only works if the input value equals the favorite name (case sensitive). I tried to add something like WHERE name LIKE %?% and COLLATE ..., but I got errors in the log file.
• The input field should all work and a route is calculated if at least 2 fields are filled with a geocoded location (works for e.g. start->via1->via3, start->end, via-2->via-4->end, ...)
• map fits to the route and alerts route information (will be displayed in a div in the future)
• the 'via addresses' are limited to 5, because unlimited 'via's could lead to too much input fields which wouldn't be accessible or break the layout. What do you think about this? @jancborchardt @Henni

What doesn't work:

• Search for contacts
• Search for subparts of favorite name
• Update input fields if the user drags a route marker
• Add a via input field if the user clicks on the route and thereby create a via marker
• Remove end/via input fields

I'd love to finish this PR the next days and thus finish the 0.1 milestone.
@jancborchardt Could you please have a look at the style(sheet)?

[v1r0x]

Good news!
Everything should work now. Except the contact search (#8), but I would fix this in another issue and maybe even do this in 0.2. What do you think about that @jancborchardt @Henni ?
Beside that I'd be happy if you could test & review this. Especially @jancborchardt, since this is a minimalistic "design" ;)

Of course I'll clean up the code and squash some of the commits after positive review.

[jancborchardt]

@v1r0x seems you need to rebase? ;)

[v1r0x]

@jancborchardt Should I first rebase or wait for review and rebase it right before merging?

[Henni]

@v1r0x rebasing might introduce mistakes, so it doesn't hurt to keep your branch updated (even during development).
Rebase first so that we can review the final state of the PR.

[v1r0x]

@Henni @jancborchardt rebased and ready for review ;)

[Henni]

Just by clicking around Uncaught TypeError: Cannot read property 'address' of undefined:

Same in firefox:

[Henni]

Strange results:

[Henni]

Another small bug.

Steps to reproduce:

1. Click + next to search input to open another input.
2. Click x next to second input

Now the search input contains the placeholder "End address" instead of "Start address".

[Henni]

@jancborchardt your help is needed! I also don't know how to fix the suggestion list as mentioned in #111 (comment)

@v1r0x BTW limiting the search fields to 5 is ok. Maybe we should rethink this in the future, but let's leave it like this for now.

[v1r0x]

Just by clicking around Uncaught TypeError: Cannot read property 'address' of undefined:

This is old code, I think checking if result.originalObject is null should work.

Strange results:

I think this is a race condition of the jquery stuff. I thought using setTimeout() and clearTimeout() would be enough. Any idea how to kill the last call of a method?

Now the search input contains the placeholder "End address" instead of "Start address".

Totally forgot to fix this^^'

@jancborchardt your help is needed! I also don't know how to fix the suggestion list as mentioned in #111 (comment)

What exactly do you mean? The positioning and floating of the list should work as planned.

@v1r0x BTW limiting the search fields to 5 is ok. Maybe we should rethink this in the future, but let's leave it like this for now.

Ok. Thanks for your comments!

[Henni]

@jancborchardt your help is needed! I also don't know how to fix the suggestion list as mentioned in #111 (comment)
What exactly do you mean? The positioning and floating of the list should work as planned.

This is what I mean:

There is a scrollbar for the whole content, if the suggestions are too long.

[v1r0x]

This is a bug and there should be not more than 5 results. The problem is the same as with your "strange results". If you type too fast, it calls the function several times...async...

[Henni]

@v1r0x you probably want to use a debounce function (https://davidwalsh.name/javascript-debounce-function)

[v1r0x]

@Henni Thanks! I'll test it later

[v1r0x]

Seems to work! :)
Could you please test if this is fixed? @Henni @jancborchardt

[matiasdelellis]

Hi,
I'm testing the branch a few days ago.. But never achieved showing the detected locations.. Using firefox 43.0.3 on Fedora, and Debian 8.1.

I write and correctly detects the direction, but every time I click shows the next error in the console, and it does nothing

Error: Couldn't autodetect L.Icon.Default.imagePath, set it manually.

p.s: You are using Nominatim????? According to their policy you should not make more that a query per second..

http://wiki.openstreetmap.org/wiki/Nominatim_usage_policy

Thanks you!!.. 😁

[Henni]

@v1r0x you didn't use the debounce function correctly. The search still fired multiple times.
The debounce function returns a "debounced" function. So you should use it like this:

input.addEventListener('keyup', debounce(function() {
        geocodeSearch.performGeocode(input)
}, 250));

And maybe you should replace keyup with input as it might prevent issues with copy-paste or pressing and holding a key (didn't test this yet).

@matiasdelellis thanks for your comment. Did you try the latest state of the branch? Maybe it is already fixed.
Regarding your comment on the policy:
I believe we do use the API mentioned at http://wiki.openstreetmap.org/wiki/Nominatim_usage_policy
@v1r0x is this correct? If so we should add a throttle function, add an appropriate attribution and respect all the other requirements including:

Unacceptable Use
The following uses are strictly forbidden and will get you banned:
Auto-complete search This is not yet supported by Nominatim and you must not implement such a service on the client side using the API.
[...]

Could you please take a look @v1r0x and verify if we are allowed to use this API.

[Henni]

Error in browser console:

• I can't reproduce Cannot read property 'lan' of null but it happened while quickly clicking around on the map.
• Cannot read property 'lan' of null happens by clicking on "navigate here"

@v1r0x It would be great if you rebase once more and also squash (at least some of) the commits.

[v1r0x]

I can't reproduce Cannot read property 'lan' of null but it happened while quickly clicking around on the map.

Should be fixed.

Cannot read property 'lan' of null happens by clicking on "navigate here"

Should be fixed as well.

@v1r0x It would be great if you rebase once more and also squash (at least some of) the commits.

squashed the commits down to 7.

@Henni Could you test once more?

cc @jancborchardt

[Henni]

While moving the mouse in and out of a details popup:

[v1r0x]

Hopefully this is now fixed as well!
@Henni thanks for testing. But could you please re-test again? ;)

[Henni]

looks good to me
Let's get this merged (after squashing the "fix commits" and resolving the conflicts by rebasing)

[v1r0x]

Ok, ready to merge?

[v1r0x]

Will remove some old code and squash again...done!

[Henni]

One small thing to nag about: do you want to squash the "small fixes and updates" commit to keep the commit history clean? Or rename it to something like "cleanup geocoder rework"?

Otherwise 👍

[Henni]

Awesome!