owncloud-archive / maps

:globe_with_meridians: Maps app for ownCloud
GNU Affero General Public License v3.0
42 stars 20 forks source link

Finish maki icons #127

Open v1r0x opened 8 years ago

v1r0x commented 8 years ago

This PR fixes #120

v1r0x commented 8 years ago

cc @Henni @jancborchardt

Henni commented 8 years ago

At first glance it looks like you just delay all requests. But I haven't tested it in detail yet.

Maybe take a look at underscore.js' implementation (I believe you are allowed to copy that code as it's MIT licensed. Maybe just add a comment to reference underscore.js)

v1r0x commented 8 years ago

Maybe take a look at underscore.js' implementation (I believe you are allowed to copy that code as it's MIT licensed. Maybe just add a comment to reference underscore.js)

I'm not sure if it is possible to use this, since the function has different parameters on every call. If you have an idea or want to test it, just do it! ;) JS' async stuff really bugs me, so I'd be very happy if you'd have a solution for the API calls and the second TODO in the OP.

v1r0x commented 8 years ago

I modified the code. Worked for me in some first tests on my test machine. So it could work on all machines ;) Would be great if you could test it, after your exams. @Henni

Henni commented 8 years ago

Still throws: image

v1r0x commented 8 years ago

Bummer...maybe we have to increase the timeout. Could you test int with 200 or 250ms in this line?

Henni commented 8 years ago

A lot of these errors: image

But no 429s anymore.

Moving around freezes the app (sometimes even the browser). I'm not sure if this PR is responsible for this performance issue.

v1r0x commented 8 years ago

I think this is related to the PR/branch. On lower zoom levels (~ <10) there are a lot of icons and layers which have to be rendered. It also loads a lot of icons in the background (which should be async). Don't know what's the best way to fix this.

v1r0x commented 8 years ago

Maybe we could use something like that? But still we have to fix the async load performance.

I'll add some TODOs to the OP.

Henni commented 8 years ago

MarkerClusters would fix the visual glitchiness, but the load performance still remains as you said.

@v1r0x Take a look at Web Workers. If it is possible to move the API calls to a worker this might solve the performance issue.

v1r0x commented 8 years ago

I added marker clustering and adjusted the zoom levels (from 9-16 to 12-16) and the performance is better. Not good, but better. Remaining problem is the slow API for poi polling. The default (http://overpass-api.de/api/) is extremely slow, but the alternative (http://overpass.osm.rambler.ru/) is blocked by firefox.

v1r0x commented 8 years ago

Maybe we could cache the POIs in the database and check on startup (or somethink like once a week) for updates. Thus the load performance would be a lot better.

v1r0x commented 8 years ago

What do you think? @jancborchardt @Henni

jancborchardt commented 8 years ago

@v1r0x maybe let’s merge this and fix the other stuff going forward?

v1r0x commented 8 years ago

If you are happy with the current state ;)

jancborchardt commented 8 years ago

I’m not sure what these numbers are which suddenly appeared on the map: capture du 2016-04-06 16-07-06 Almost seems like placeholders for icons to be loaded? There should just be nothing until loaded I’d say.

v1r0x commented 8 years ago

If there are several icons in the same area the get clustered so the app doesn't display thousands of icons and the UI starts to lag. If you zoom in they get separated (and clustered if you zoom out). Added this to fix #129. I thought I added the POI circle background, but seems I forgot that, maybe this way it doesn't look like a placeholder and can be associated with POIs?

jancborchardt commented 8 years ago

Yup, the circle should be around it.

Also, do all POIs get clustered? I would say it only makes sense to group POIs which have the same type (like supermarket).

v1r0x commented 8 years ago

Yes, all together. Don't know if it is better if we only cluster POIs of the same type. Thus we would have a couple of cluster markers in one area and the user don't know which cluster is which POI type. Maybe I we'd add unique colors for every type and set this color for the specific cluster as well?

jancborchardt commented 8 years ago

Maybe just use the normal type icon with an overlaid number on top? Or in a circle in a corner.

v1r0x commented 8 years ago

I hope I have some time this week to fix this. Would be good if we could finish the 0.1 release...finally ;)