[WIP] Rework again

[julien-nc]

Hey! Maps app is alive, aliiiive.

Discussion started in #32 and #39 (there's a task list in this one).

This branch is based on rework. It's far from a first release but it's functional. Device management is coming soon from eneiluj-devices branch.

PhpUnit tests are working but are almost empty. CI script has been fixed. Makefile has been fixed...

[jancborchardt]

Great great stuff @eneiluj! :) Here’s a first design review, and I’m focusing on the basic maps stuff first so we get that right:

• The maximum zoom-out level is too much, as you can see further than the edges of the world ;)
  
  This is roughly what would be maximally needed, to simply fit the viewport:
  
• The map ends in the pacific and it’s not possible to go from e.g. New Zealand to Venezuela. It should be possible to go around, so it’s not eurocentric:
  
• Zoom and geolocation buttons could be adjusted to Nextcloud button style (class="button"). Also they could move to the bottom right so to not take too much focus away from search:
  
• Both layer switchers at the same time are not needed, only the one which is not used at the moment. Also they are both duplicated in the "More layers" box, which is not necessary:
  
• The lat/long info on the bottom left is not needed:
  
• The time bar on the bottom is only needed when a layer with time info is actually displayed. Otherwise it can be hidden. Also it should use the var(--color-primary) as color if possible:
  
• The size ruler is too present. It could be on the bottom right after the OSM credits – check Google Maps to compare:
  
  
• The search box could use an inline icon-search instead of the big button:
  
• Also, routing should just add a second field instead of using a separate box on the top right – compare with Google Maps for example:
  
  They also use the search box contents as destination by default. (osm.org uses it as the starting point, which makes less sense):
  
  
• The right click action menu is pretty core to the app but is not so discoverable. The issue here is that there need to be clickable elements on the map to interact with. Is that possible? (And it doesn’t seem to use the popover menu standard as there’s very little whitespace?)
  
• Then lastly the layers I didn’t look into so much yet. Mostly I was confused about when a layer is actually displayed. The eye indicator is not clear and relies on color change for indication which is not sufficient. I’d recommend to show the entries slightly greyed / with reduced transparency by default, and then every activated layer could show with its entry at full opacity and with the primary color bar on the left. Then the eye indicator is not needed and we save space for the names. (Also, I don’t know what the yellow image icon means.):
  

What do you think @eneiluj? :) As said I first focused on the core map basics. All layer fanciness is of course awesome and integrates with the rest of Nextcloud, but we need to get the core stuff right first. :)

Also what do you think @nextcloud/maps @v1r0x @nextcloud/designers?

[julien-nc]

Thanks for the feedback @jancborchardt ! I completely agree base map aspects should be fixed before talking about bigger features.

• map view does not have limits for movement and display anymore
• minimum zoom is now 2
• zoom control is now in the bottom right corner
• osm and aerial are not listed in layer selector anymore
• aerial layer button is visible when any other layer is selected
• osm button is visible when aerial layer is selected
• mouse position control (coordinates in bottom left corner) has been removed (even if I loved it 😉)
• scale control has been moved to bottom left. it's quite complex to integrate it with the provider credits... maybe later
• search submit button is nicer

Now about the rest:

• zoom and geolocation buttons: i gave up after 5 minutes (booo) trying to make them look more nextcloudish. May the PR gods be with us.
• time filter slider is actually already supposed to appear in var(--color-primary) but i guess you didn't enable any information layer so it stayed in its initial state where there sometimes is a color bug. this bug is now fixed
• hiding time filter slider: i put this in my todo list, i still need to harmonize filtering between all js controllers...
• search: i didn't touch anything, it's still in rework state. there's a lot to do there. for the moment it only searches for addresses. it does not interact with information layers. todo again.
• routing: i just use a leaflet plugin with very small modifications. with a bit of effort it can elegantly be combined with the search form.....todo 😁 let's discuss that in a specific issue
• context menu: i'm not sure i get what you wrote. do you mean it's not very useful unless there are elements to "rightclick" on? for the moment, it only triggers actions related to where the right-click was made. i'll check if the leaflet plugin allows to make a difference between rightclick on the map and on a marker/line.
• toggling layers: i made it like that to postpone the reflection. i agree we could get rid of the eye. problem is: some menu entries are foldable. what should happen when we fold one? should it disable the layer? if so, what if the user wants to fold it but keep the layer visible?
• non localized photos: this is to be changed. the warning color means the coordinates are not trustable yet because they were guessed. do you think this should become a top-level menu entry?

and this is just the visible part of the mapsberg. a lot of work in perspective! 🎉 💻

[e-alfred]

This PR works really good and is already pretty awesome, but I found a few problems with the app:

• It seems to load content from http servers which leads to mixed content warnings/errors if the server is run with https (probably almost all Nextcloud servers out there hopefully):

Loading mixed (insecure) display content “http://a.tile.stamen.com/watercolor/11/2345/4526.jpg” on a secure page

• The contacts view works, but if I click in the contact it opens the contacts app with a GUID which cannot be found:

https://myserver/apps/contacts/All%20contacts/fe55d384-b989-42a1-b5e9-7dd802fd939b%40myserver~contacts

• The track parser has some problems with a few *.gpx files created by GPSLogger and the spinner loads forever. In the browser console I see these messages:

XML Parsing Error: not well-formed
Location: 
Line Number 1, Column 171:

XML Parsing Error: unclosed token
Location: https://mylink.com/apps/maps/#
Line Number 2176, Column 1: mylink.com:2176:1
Error: Invalid XML: <?xml version="1.0" encoding="UTF-8" ?><gpx version="1.0" creator="GPSLogger 101 - http://gpslogger.mendhak.com/" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://www.topografix.com/GPX/1/0" xsi:schemaLocation="http://www.topografix.com/GPX/1/0 http://www.topografix.com/GPX/1/0/gpx.xsd"><time>2019-04-17T09:42:58.593Z</time><trk><trkseg><trkpt lat="23.3500434" lon="43.1943276"><ele>612.7000122070312</ele><time>2019-04-17T09:42:58.593Z</time><src>network</src></trkpt>
<trkpt lat="23.3500127" lon="43.1943589"><ele>612.7000122070312</ele><time>2019-04-17T09:43:03.552Z</time><src>network</src></trkpt>
<trkpt lat="23.35000504" lon="43.19433953"><ele>621.0</ele><time>2019-04-17T09:43:06.774Z</time><course>309.2</course><speed>1.48</speed><src>gps</src></trkpt>
<trkpt lat="23.3499923" lon="43.19433325"><ele>626.0</ele><time>2019-04-17T09:43:07.774Z</time><course>280.2</course><speed>0.85</speed><geoidheight>45.0</geoidheight><src>gps</src><sat>14</sat><hd… core.js:2:1821

• If I try to enable the pictures view I get an error message "Failed to load photos" and "Failed to load non-geolocalized photos" respectively. If I execute php occ maps:scan-photos in verbose mode no output is shown whatsoever. There is an error 500 showing up in the browser console if I try to enable the pictures view:

core.js?v=b87aa008-1:4 GET https://myserver/apps/maps/photos 500 (Internal Server Error)
send	@	core.js?v=b87aa008-1:4
ajax	@	core.js?v=b87aa008-1:4
callForImages	@	photosController.js?v=b87aa008-1:259
showLayer	@	photosController.js?v=b87aa008-1:64
toggleLayer	@	photosController.js?v=b87aa008-1:83
(anonymous)	@	script.js?v=b87aa008-1:139
j	@	core.js?v=b87aa008-1:2
fireWith	@	core.js?v=b87aa008-1:2
x	@	core.js?v=b87aa008-1:4
(anonymous)	@	core.js?v=b87aa008-1:4
load (async)		
send	@	core.js?v=b87aa008-1:4
ajax	@	core.js?v=b87aa008-1:4
restoreOptions	@	script.js?v=b87aa008-1:124
(anonymous)	@	script.js?v=b87aa008-1:16
j	@	core.js?v=b87aa008-1:2
fireWith	@	core.js?v=b87aa008-1:2
ready	@	core.js?v=b87aa008-1:2
I	@	core.js?v=b87aa008-1:2

In my server log I found this error if I activate the photos view:

{"reqId":"XLrihkjfQKzYSLMAaRnTgQAAAAo","level":3,"time":"2019-04-20T11:12:40+02:00","remoteAddr":"1.1.1.1","user":"user","app":"index","method":"GET","url":"\/nc\/apps\/maps\/photos\/nonlocalized","message":{"Exception":"Doctrine\\DBAL\\Exception\\InvalidFieldNameException","Message":"An exception occurred while executing 'SELECT * FROM `oc_maps_photos` where `user_id` = ? and (`lat` is null or `long` is  null)' with params [\"user\"]:\n\nSQLSTATE[42S22]: Column not found: 1054 Unknown column 'long' in 'where clause'","Code":0,"Trace":[{"file":"\/var\/www\/nc\/3rdparty\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/DBALException.php","line":128,"function":"convertException","class":"Doctrine\\DBAL\\Driver\\AbstractMySQLDriver","type":"->","args":["An exception occurred while executing 'SELECT * FROM `oc_maps_photos` where `user_id` = ? and (`lat` is null or `long` is  null)' with params [\"user\"]:\n\nSQLSTATE[42S22]: Column not found: 1054 Unknown column 'long' in 'where clause'",{"errorInfo":["42S22",1054,"Unknown column 'long' in 'where clause'"],"__class__":"Doctrine\\DBAL\\Driver\\PDOException"}]},{"file":"\/var\/www\/nc\/3rdparty\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/Statement.php","line":177,"function":"driverExceptionDuringQuery","class":"Doctrine\\DBAL\\DBALException","type":"::","args":[{"__class__":"Doctrine\\DBAL\\Driver\\PDOMySql\\Driver"},{"errorInfo":["42S22",1054,"Unknown column 'long' in 'where clause'"],"__class__":"Doctrine\\DBAL\\Driver\\PDOException"},"SELECT * FROM `oc_maps_photos` where `user_id` = ? and (`lat` is null or `long` is  null)",{"1":"user"}]},{"file":"\/var\/www\/nc\/lib\/public\/AppFramework\/Db\/Mapper.php","line":256,"function":"execute","class":"Doctrine\\DBAL\\Statement","type":"->","args":[]},{"file":"\/var\/www\/nc\/lib\/public\/AppFramework\/Db\/Mapper.php","line":344,"function":"execute","class":"OCP\\AppFramework\\Db\\Mapper","type":"->","args":["SELECT * FROM `*PREFIX*maps_photos` where `user_id` = ? and (`lat` is null or `long` is  null)",["user"],null,null]},{"file":"\/var\/www\/nc\/apps\/maps\/lib\/DB\/GeophotoMapper.php","line":49,"function":"findEntities","class":"OCP\\AppFramework\\Db\\Mapper","type":"->","args":["SELECT * FROM `*PREFIX*maps_photos` where `user_id` = ? and (`lat` is null or `long` is  null)",["user"],null,null]},{"file":"\/var\/www\/nc\/apps\/maps\/lib\/Service\/GeophotoService.php","line":95,"function":"findAllNonLocalized","class":"OCA\\Maps\\DB\\GeophotoMapper","type":"->","args":["user"]},{"file":"\/var\/www\/nc\/apps\/maps\/lib\/Controller\/PhotosController.php","line":52,"function":"getNonLocalizedFromDB","class":"OCA\\Maps\\Service\\GeophotoService","type":"->","args":["user"]},{"file":"\/var\/www\/nc\/lib\/private\/AppFramework\/Http\/Dispatcher.php","line":166,"function":"getNonLocalizedPhotosFromDb","class":"OCA\\Maps\\Controller\\PhotosController","type":"->","args":[]},{"file":"\/var\/www\/nc\/lib\/private\/AppFramework\/Http\/Dispatcher.php","line":99,"function":"executeController","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->","args":[{"__class__":"OCA\\Maps\\Controller\\PhotosController"},"getNonLocalizedPhotosFromDb"]},{"file":"\/var\/www\/nc\/lib\/private\/AppFramework\/App.php","line":118,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->","args":[{"__class__":"OCA\\Maps\\Controller\\PhotosController"},"getNonLocalizedPhotosFromDb"]},{"file":"\/var\/www\/nc\/lib\/private\/AppFramework\/Routing\/RouteActionHandler.php","line":47,"function":"main","class":"OC\\AppFramework\\App","type":"::","args":["OCA\\Maps\\Controller\\PhotosController","getNonLocalizedPhotosFromDb",{"__class__":"OC\\AppFramework\\DependencyInjection\\DIContainer"},{"_route":"maps.photos.getNonLocalizedPhotosFromDb"}]},{"function":"__invoke","class":"OC\\AppFramework\\Routing\\RouteActionHandler","type":"->","args":[{"_route":"maps.photos.getNonLocalizedPhotosFromDb"}]},{"file":"\/var\/www\/nc\/lib\/private\/Route\/Router.php","line":297,"function":"call_user_func","args":[{"__class__":"OC\\AppFramework\\Routing\\RouteActionHandler"},{"_route":"maps.photos.getNonLocalizedPhotosFromDb"}]},{"file":"\/var\/www\/nc\/lib\/base.php","line":987,"function":"match","class":"OC\\Route\\Router","type":"->","args":["\/apps\/maps\/photos\/nonlocalized"]},{"file":"\/var\/www\/nc\/index.php","line":42,"function":"handleRequest","class":"OC","type":"::","args":[]}],"File":"\/var\/www\/nc\/3rdparty\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/Driver\/AbstractMySQLDriver.php","Line":71,"Previous":{"Exception":"Doctrine\\DBAL\\Driver\\PDOException","Message":"SQLSTATE[42S22]: Column not found: 1054 Unknown column 'long' in 'where clause'","Code":"42S22","Trace":[{"file":"\/var\/www\/nc\/3rdparty\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/Statement.php","line":168,"function":"execute","class":"Doctrine\\DBAL\\Driver\\PDOStatement","type":"->","args":[null]},{"file":"\/var\/www\/nc\/lib\/public\/AppFramework\/Db\/Mapper.php","line":256,"function":"execute","class":"Doctrine\\DBAL\\Statement","type":"->","args":[]},{"file":"\/var\/www\/nc\/lib\/public\/AppFramework\/Db\/Mapper.php","line":344,"function":"execute","class":"OCP\\AppFramework\\Db\\Mapper","type":"->","args":["SELECT * FROM `*PREFIX*maps_photos` where `user_id` = ? and (`lat` is null or `long` is  null)",["user"],null,null]},{"file":"\/var\/www\/nc\/apps\/maps\/lib\/DB\/GeophotoMapper.php","line":49,"function":"findEntities","class":"OCP\\AppFramework\\Db\\Mapper","type":"->","args":["SELECT * FROM `*PREFIX*maps_photos` where `user_id` = ? and (`lat` is null or `long` is  null)",["user"],null,null]},{"file":"\/var\/www\/nc\/apps\/maps\/lib\/Service\/GeophotoService.php","line":95,"function":"findAllNonLocalized","class":"OCA\\Maps\\DB\\GeophotoMapper","type":"->","args":["user"]},{"file":"\/var\/www\/nc\/apps\/maps\/lib\/Controller\/PhotosController.php","line":52,"function":"getNonLocalizedFromDB","class":"OCA\\Maps\\Service\\GeophotoService","type":"->","args":["user"]},{"file":"\/var\/www\/nc\/lib\/private\/AppFramework\/Http\/Dispatcher.php","line":166,"function":"getNonLocalizedPhotosFromDb","class":"OCA\\Maps\\Controller\\PhotosController","type":"->","args":[]},{"file":"\/var\/www\/nc\/lib\/private\/AppFramework\/Http\/Dispatcher.php","line":99,"function":"executeController","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->","args":[{"__class__":"OCA\\Maps\\Controller\\PhotosController"},"getNonLocalizedPhotosFromDb"]},{"file":"\/var\/www\/nc\/lib\/private\/AppFramework\/App.php","line":118,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->","args":[{"__class__":"OCA\\Maps\\Controller\\PhotosController"},"getNonLocalizedPhotosFromDb"]},{"file":"\/var\/www\/nc\/lib\/private\/AppFramework\/Routing\/RouteActionHandler.php","line":47,"function":"main","class":"OC\\AppFramework\\App","type":"::","args":["OCA\\Maps\\Controller\\PhotosController","getNonLocalizedPhotosFromDb",{"__class__":"OC\\AppFramework\\DependencyInjection\\DIContainer"},{"_route":"maps.photos.getNonLocalizedPhotosFromDb"}]},{"function":"__invoke","class":"OC\\AppFramework\\Routing\\RouteActionHandler","type":"->","args":[{"_route":"maps.photos.getNonLocalizedPhotosFromDb"}]},{"file":"\/var\/www\/nc\/lib\/private\/Route\/Router.php","line":297,"function":"call_user_func","args":[{"__class__":"OC\\AppFramework\\Routing\\RouteActionHandler"},{"_route":"maps.photos.getNonLocalizedPhotosFromDb"}]},{"file":"\/var\/www\/nc\/lib\/base.php","line":987,"function":"match","class":"OC\\Route\\Router","type":"->","args":["\/apps\/maps\/photos\/nonlocalized"]},{"file":"\/var\/www\/nc\/index.php","line":42,"function":"handleRequest","class":"OC","type":"::","args":[]}],"File":"\/var\/www\/nc\/3rdparty\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/Driver\/PDOStatement.php","Line":107,"Previous":{"Exception":"PDOException","Message":"SQLSTATE[42S22]: Column not found: 1054 Unknown column 'long' in 'where clause'","Code":"42S22","Trace":[{"file":"\/var\/www\/nc\/3rdparty\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/Driver\/PDOStatement.php","line":105,"function":"execute","class":"PDOStatement","type":"->","args":[null]},{"file":"\/var\/www\/nc\/3rdparty\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/Statement.php","line":168,"function":"execute","class":"Doctrine\\DBAL\\Driver\\PDOStatement","type":"->","args":[null]},{"file":"\/var\/www\/nc\/lib\/public\/AppFramework\/Db\/Mapper.php","line":256,"function":"execute","class":"Doctrine\\DBAL\\Statement","type":"->","args":[]},{"file":"\/var\/www\/nc\/lib\/public\/AppFramework\/Db\/Mapper.php","line":344,"function":"execute","class":"OCP\\AppFramework\\Db\\Mapper","type":"->","args":["SELECT * FROM `*PREFIX*maps_photos` where `user_id` = ? and (`lat` is null or `long` is  null)",["user"],null,null]},{"file":"\/var\/www\/nc\/apps\/maps\/lib\/DB\/GeophotoMapper.php","line":49,"function":"findEntities","class":"OCP\\AppFramework\\Db\\Mapper","type":"->","args":["SELECT * FROM `*PREFIX*maps_photos` where `user_id` = ? and (`lat` is null or `long` is  null)",["user"],null,null]},{"file":"\/var\/www\/nc\/apps\/maps\/lib\/Service\/GeophotoService.php","line":95,"function":"findAllNonLocalized","class":"OCA\\Maps\\DB\\GeophotoMapper","type":"->","args":["user"]},{"file":"\/var\/www\/nc\/apps\/maps\/lib\/Controller\/PhotosController.php","line":52,"function":"getNonLocalizedFromDB","class":"OCA\\Maps\\Service\\GeophotoService","type":"->","args":["user"]},{"file":"\/var\/www\/nc\/lib\/private\/AppFramework\/Http\/Dispatcher.php","line":166,"function":"getNonLocalizedPhotosFromDb","class":"OCA\\Maps\\Controller\\PhotosController","type":"->","args":[]},{"file":"\/var\/www\/nc\/lib\/private\/AppFramework\/Http\/Dispatcher.php","line":99,"function":"executeController","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->","args":[{"__class__":"OCA\\Maps\\Controller\\PhotosController"},"getNonLocalizedPhotosFromDb"]},{"file":"\/var\/www\/nc\/lib\/private\/AppFramework\/App.php","line":118,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->","args":[{"__class__":"OCA\\Maps\\Controller\\PhotosController"},"getNonLocalizedPhotosFromDb"]},{"file":"\/var\/www\/nc\/lib\/private\/AppFramework\/Routing\/RouteActionHandler.php","line":47,"function":"main","class":"OC\\AppFramework\\App","type":"::","args":["OCA\\Maps\\Controller\\PhotosController","getNonLocalizedPhotosFromDb",{"__class__":"OC\\AppFramework\\DependencyInjection\\DIContainer"},{"_route":"maps.photos.getNonLocalizedPhotosFromDb"}]},{"function":"__invoke","class":"OC\\AppFramework\\Routing\\RouteActionHandler","type":"->","args":[{"_route":"maps.photos.getNonLocalizedPhotosFromDb"}]},{"file":"\/var\/www\/nc\/lib\/private\/Route\/Router.php","line":297,"function":"call_user_func","args":[{"__class__":"OC\\AppFramework\\Routing\\RouteActionHandler"},{"_route":"maps.photos.getNonLocalizedPhotosFromDb"}]},{"file":"\/var\/www\/nc\/lib\/base.php","line":987,"function":"match","class":"OC\\Route\\Router","type":"->","args":["\/apps\/maps\/photos\/nonlocalized"]},{"file":"\/var\/www\/nc\/index.php","line":42,"function":"handleRequest","class":"OC","type":"::","args":[]}],"File":"\/var\/www\/nc\/3rdparty\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/Driver\/PDOStatement.php","Line":105}},"CustomMessage":"--"},"userAgent":"Mozilla\/5.0 (X11; Linux x86_64; rv:66.0) Gecko\/20100101 Firefox\/66.0","version":"15.0.7.0"}

It seems like the table oc_maps_photos has a "lng" column but the app looks for "long" instead:

[mysql> describe oc_maps_photos;
+------------+-------------+------+-----+---------+----------------+
| Field      | Type        | Null | Key | Default | Extra          |
+------------+-------------+------+-----+---------+----------------+
| id         | bigint(20)  | NO   | PRI | NULL    | auto_increment |
| user_id    | varchar(64) | NO   |     | NULL    |                |
| file_id    | bigint(20)  | NO   |     | NULL    |                |
| lat        | double      | YES  |     | NULL    |                |
| lng        | double      | YES  |     | NULL    |                |
| date_taken | bigint(20)  | YES  | MUL | NULL    |                |
+------------+-------------+------+-----+---------+----------------+
6 rows in set (0.00 sec)
]

[jancborchardt]

Thanks for the feedback @jancborchardt ! I completely agree base map aspects should be fixed before talking about bigger features.

• map view does not have limits for movement and display anymore
• minimum zoom is now 2
• zoom control is now in the bottom right corner
• osm and aerial are not listed in layer selector anymore
• aerial layer button is visible when any other layer is selected
• osm button is visible when aerial layer is selected
• mouse position control (coordinates in bottom left corner) has been removed (even if I loved it wink)
• scale control has been moved to bottom left. it's quite complex to integrate it with the provider credits... maybe later
• search submit button is nicer

Veeeery nice! :)

Now about the rest:

• zoom and geolocation buttons: i gave up after 5 minutes (booo) trying to make them look more nextcloudish. May the PR gods be with us.

Right, I will try to look into this at some point. It’s not that big of a deal for now. :)

• time filter slider is actually already supposed to appear in var(--color-primary) but i guess you didn't enable any information layer so it stayed in its initial state where there sometimes is a color bug. this bug is now fixed

Yep, seems to be fixed now. Although it seems to be a lightened color-primary? And when I pull the slider on either side almost to the end, the slider color goes orange again.

• hiding time filter slider: i put this in my todo list, i still need to harmonize filtering between all js controllers...

👍

• search: i didn't touch anything, it's still in rework state. there's a lot to do there. for the moment it only searches for addresses. it does not interact with information layers. todo again.
• routing: i just use a leaflet plugin with very small modifications. with a bit of effort it can elegantly be combined with the search form.....todo grin let's discuss that in a specific issue

Yes, makes sense, it’s a bigger thing. :)

• context menu: i'm not sure i get what you wrote. do you mean it's not very useful unless there are elements to "rightclick" on? for the moment, it only triggers actions related to where the right-click was made. i'll check if the leaflet plugin allows to make a difference between rightclick on the map and on a marker/line.

I mean that the menu is very useful, but that people don’t necessarily know how to get to it as right-click is not such a common thing on Maps. I for example never ever used right click on Google Maps until just yesterday to test if they have it. And all the functionality in there is also available on simple clicking of the map.

• toggling layers: i made it like that to postpone the reflection. i agree we could get rid of the eye. problem is: some menu entries are foldable. what should happen when we fold one? should it disable the layer? if so, what if the user wants to fold it but keep the layer visible?

Folding/unfolding is only toggled on clicking the triangle on the left, not the whole entry. So clicking the entry should toggle it (make it opacity: 1; and add the .active bar on the left). And folding is separate from being active.

• non localized photos: this is to be changed. the warning color means the coordinates are not trustable yet because they were guessed. do you think this should become a top-level menu entry?

I think it should either just be part of the normal photos layer, or not shown at all. It’s fine if coordinates are guessed I think, and just show them as if normal. If something is wrong they can be edited.

and this is just the visible part of the mapsberg. a lot of work in perspective! 🎉 💻

🎉 !

---

And here come some additional things. :)

• @v1r0x wanted to port the Maps app to Vue.js at some point. Maybe before we build more and more stuff in this is a good point to do it? Or what do you think @nextcloud/javascript?
• It’s still possible to scroll out of the map on the top and bottom. This is not necessary, and the grey part outside of the world doesn’t ever need to be visible. It can just end there
  
• The geolocation button could move to the bottom right too as the other controls now are. :)
  
• Clicking the geolocation button directly jumps to your location and a very close zoom level (scale shows 10m). Ideally there should be some sort of quick animation and then not zoom in so far, more like the scale showing 100m.
• The satellite view has no labels or anything at all. It’s kind of confusing as you have no sense of space with that. Is it possible to half-overlay the street map, or show important structural stuff like cities and roads so you have some reference?
  
• Searching for a place moves the map only so it’s barely visible in the corner. It should center it in the viewport instead:
  
• On smaller screens the map credits can get multi-line, on mobile actually 4–5 rows. Can we force it to be a single row, ellipsized, max 90% width (so it doesn’t overlap the size ruler), and it could show the full credits on hover and/or click with a popover?
  
• In the detail popover of a favorite there’s too many controls: The fields are not clear as they only have icons and no text. I’d say only "Name" is needed for now for simplicy (also with placeholder "Name" instead of "no name". And then the "Cancel" button duplicates the x in the top right. Also, move is not necessary as you could simply drag the icon on the map directly. And the remaining "Save" and "Delete" buttons: Save should be on the right, and have .icon-checkmark instead of a floppy disk. :)
  
• Bonus: in the additional layers I would love to have an inverted map like that since it’s not so centric on the northern hemisphere ;)
  
• Also – a bigger topic, but – our map is of course very skewed due to its projection, showing central Europe and northern US bigger than they actually are related to places close to the equator like African countries. Google Maps solves this by at some point switching to a globe view, which I don’t know if we can do? :D But just for reference also the true size of Africa
  
  

[jancborchardt]

Oh and btw, on a build there are these untracked files:

composer.lock
package-lock.json
vendor/

They should either be committed or in .gitignore, not sure which is best practice. :)

[e-alfred]

I found another problem regarding the non-localized images showing up in the log causing an error 500:

{"reqId":"XLr12W03pdK5Bjmdksl6kwAAABg","level":3,"time":"2019-04-20T12:35:06+02:00","remoteAddr":"1.1.1.1","user":"user","app":"PHP","method":"GET","url":"\/nc\/apps\/maps\/photos\/nonlocalized","message":"Trying to get property 'trk' of non-object at \/var\/www\/nc\/apps\/maps\/lib\/Service\/GeophotoService.php#179","userAgent":"Mozilla\/5.0 (X11; Linux x86_64; rv:66.0) Gecko\/20100101 Firefox\/66.0","version":"15.0.7.0"}
{"reqId":"XLr12W03pdK5Bjmdksl6kwAAABg","level":3,"time":"2019-04-20T12:35:06+02:00","remoteAddr":"1.1.1.1","user":"user","app":"PHP","method":"GET","url":"\/nc\/apps\/maps\/photos\/nonlocalized","message":"Invalid argument supplied for foreach() at \/var\/www\/nc\/apps\/maps\/lib\/Service\/GeophotoService.php#179","userAgent":"Mozilla\/5.0 (X11; Linux x86_64; rv:66.0) Gecko\/20100101 Firefox\/66.0","version":"15.0.7.0"}
{"reqId":"XLr12W03pdK5Bjmdksl6kwAAABg","level":3,"time":"2019-04-20T12:35:05+02:00","remoteAddr":"1.1.1.1","user":"user","app":"PHP","method":"GET","url":"\/nc\/apps\/maps\/photos\/nonlocalized","message":"simplexml_load_string(): <trkpt lat="22.32452 at \/var\/www\/nc\/apps\/maps\/lib\/Service\/GeophotoService.php#178","userAgent":"Mozilla\/5.0 (X11; Linux x86_64; rv:66.0) Gecko\/20100101 Firefox\/66.0","version":"15.0.7.0"}

[julien-nc]

@e-alfred and @jancborchardt : wow thanks for the precise and fast feedback!

@e-alfred:

• watercolor tile server address is now in https
• contacts: All%20contacts must be translated to correctly get to the contact view... Is there another way (locale independent) to get a link to a contact?
• could you provide one or two problematic gpx files?
• photo query issue is now fixed. my fault, i didn't fully review the code from @tacruc.

@jancborchardt:

• about the slider:
  • Color was primary-light. It's now primary
  • The warning color means the slider range is going to be dynamically changed when the user drops the slider outside its current max range. It's doing the same when the interval becomes too small.
  • The slider behaviour is a big topic 😉. I don't know how to improve it but it needs improvement.
• context menu:
  • I forgot to tell you about the style: I didn't make any modification to leaflet-contextmenu plugin. CSS can easily be overridden but html structure is another story
  • As there is no action that is only available in the context menu for the moment, i'm ok if the user does not "find" the context menu... If at some point we add interesting features in this context menu, we'll have to make it obvious it exists...
• folding/toggling: Ok fine by me. It's been done and pushed. A small addition: When a top level menu entry disabled and folded, clicking on it enables it AND unfold it. What do you think about that?
• non-localized photos: This is also a big topic we can discuss in a specific issue. I agree it needs efforts 😄
• untracked files: they shouldn't be tracked because they are generated/modified for local needs. I've updated .gitignore file

I'll have a look to the rest of your comments later 😉. Thanks again. We're in a nice dynamic! ❤️ 🤓

[julien-nc]

@jancborchardt Thanks for your enthusiasm! And also for your nice suggestion storm! 🌀 🐧

Yep it can be merged into master. Apparently I'm not allowed to do it. Would you prefer authorizing me or doing the merge yourself?

I added a first generated translation template in case transifex needs it.
Merge conflicts are just about l10n files and the README.

How would you like to work after the merge? Who would be the integrator? Should rework-again branch be deleted?

[tacruc]

👥 Contacts

* Actually haven’t gotten that to run yet. Am not getting any errors or feedback. What am I doing wrong? :D Does it load really long, or do _all_ contacts need a correct address?

👥 Contacts

Contacts part is very basic for the moment. You expected too much from it wink. It displays contacts which already have a "geo" information (https://www.evenx.com/vcard-3-0-format-specification). The map context menu allows you to place a contact (edit its vcard geo field). Then it's displayed.

Using the address is not that simple. It need one or two brainstorm sessions smile. When should the coordinates be deduced from the address?

Client side, each time the contacts layer is loaded? => No because it's heavy and repetitive
Server side? => Mmmmmaybe but when? If it's done for all contacts of all users, it's quite heavy.

To be continued

Lets move the contacts discussion into #34.

[jancborchardt]

Yep it can be merged into master. Apparently I'm not allowed to do it. Would you prefer authorizing me or doing the merge yourself?
Merge conflicts are just about l10n files and the README.

I have to check how to do it myself, because of the conflicts. I would just force push it to master – or how t odo it? You should be able to do it too as I just removed master branch protection. Let me know if it doesn’t work and if you are ok with that, then we can do that.

I added a first generated translation template in case transifex needs it.

👍

How would you like to work after the merge? Who would be the integrator? Should rework-again branch be deleted?

Yes, so how we usually work in Nextcloud apps is that master is generally always working, and we use small topic branches for individual fixes or features. All the rework-stuff can then be closed. I would also activate branch protection for the master branch so that 2 reviews are necessary for any pull request to be merged.

It seems the Maps team for now is you @eneiluj, @tacruc and me? So I would update that at @nextcloud/maps and make you both coordinators for the team too.

Does that all work? :)

[julien-nc]

Ok let's do that! I'll force-push rework-again into master in 15 minutes (for obvious administrative and legal reasons 😉 or if someone prefers to properly merge with master).

Fine by me for keeping master functional and playing in this team with those rules.

[julien-nc]

@jancborchardt master is still protected so I can't force push into it. I don't have access to project settings. Could you temporarily remove protection on master?

[jancborchardt]

@eneiluj hmm, thought it was already removed, unset all the settings. Now removed it all again, could you try? :)

[julien-nc]

@jancborchardt It worked. Thanks. You can put the protection back.

[jancborchardt]

Gooood goood stuff! :) Put the branch protection back with 2 reviews required, we can also do 1 if 2 is too much.

I’m a bit busy with other stuff today and tomorrow probably too, so feel free to check more of the notes above – some day this week I will copy the remaining stuff over to issues, maybe first to ones grouped on topic (Routing, Photo display, Favorites etc) to keep it simple and have a unified vision. Does that work for you?

[julien-nc]

Ow yeah!

I think one review is enough. If I create a PR, maybe only one of us will be available to review. Can someone approve his/her own PR? (it would be silly but i wonder...)

I'll slow down a bit on Maps. I left a bunch of stuff behind 😁.

[jancborchardt]

@eneiluj cool stuff! Edited it to 1 required review. And no, people can’t approve their own PR, as that would indeed be silly. :D

Yeah, it’s good to relax a bit. :) Then when coming back to it, one sees more issues and where stuff could be simplified.

Btw @eneiluj do you already want to request a certificate for the app store (with ID "maps")? We should probably still do some polishing before publishing – and maybe we can also do it to coincide with a Nextcloud major release, but just to have that already done. :)

[julien-nc]

@jancborchardt Yes I'll make a certificate request.

I'll be happy to publish the first release when we feel it's time, for a major NC release or not.

[jancborchardt]

@eneiluj what we could do is publish it in conjunction with when the GNOME Maps favorites sync project is done and working. Just an idea – but when great work has been done, it also needs to be promoted with the appropriate greatness. ;)

[julien-nc]

@jancborchardt Let's hope a good favorite client is ready soon(ish). I would ❤️ looooove to be able to sync favorites with OsmAnd. Being able to interact with such great projects as Gnome Maps and OsmAnd would be more than awesome! 😺 Make some noise! 😉

[e-alfred]

This app is already so awesome and has more features than the old Maps app so it should definitely be released to the App Store as soon as possible. Integration with other apps would be a huge bonus, maybe one day an Android companion app could be done as well?

[v1r0x]

It seems the Maps team for now is you @eneiluj, @tacruc and me? So I would update that at @nextcloud/maps and make you both coordinators for the team too.

Hey, I'm still here! 😝
I'm pretty busy, so can't help much on code side right now. But can help testing and commenting until I have more time for coding again :)

[jancborchardt]

@e-alfred it’s better to wait a bit and polish it more, especially since there are so many features and things to check still. :) When people try a thing for the first time and are confused, they are not going to try it again so quickly.

Hey, I'm still here! 😝
I'm pretty busy, so can't help much on code side right now. But can help testing and commenting until I have more time for coding again :)

Yaaaaay! :) Check out current master! All your work and @eneiluj’s on top is in there. :)