mobile: enable the map widget #1156

neolit123 · 2018-03-08T20:52:30Z

Describe the pull request:

Pull request long description:

this PR enables the map widget on mobile. it is placed in a Kirigami Page and it's possible to
navigate to the page using these methods:

open left drawer -> Map
open dive details -> Map it
open GPS fix -> Show on map

the map is the same as on the desktop version with a few limitations, like:

not being able to click on cluster of dives and select all of them in the dive list.
the mobile version does not allow multi-selection, thus only a clicked marker is selected.
since selecting multiple dives in the dive list is not possible on mobile, the map always zooms to
a single dive, rather than zooming on a rectangle of dives.
the map widget context menu action to "select visible dive locations" does not work (no multi-selection) and always the first dive location is selected.
when selecting dive markers on the map the dive list moves the selection without animation, as otherwise this can take minutes for a huge dive list. there seems to be no way to control the speed of this animation.
editing from the map by dragging markers is not supported as we don't have the UX for that. editing by entering coordinates in the dive details works.

Changes made:

added small ifdef wrapped modifications in mapwidgethelper to comply with the limits of the mobile version.
use a Breeze icon for the map in the drawer on mobile.
add a Kirigami Page for the map.
expose signals and functions to make it possible to select dives in the dive list from the map and open a map location from the dive details or from a gps fix.
update CMake and QRC
make the view stack behave slightly differently when MapPage is active

Related issues:

Closes #1037

Additional information:

untested on an actual mobile device by me for the time being.
i'm not sure if the QtLocation, QtPositioning modules need extra deployment steps.
not sure how the packaging of the google maps plugin would work? ideas?

Release note:

- Mobile: enable the built-in map widget

Documentation change:

Requires work flow updates in the documentation to include the map widget navigation - e.g.
screenshots.

Mentions:

@dirkhh @janmulder @atdotde @glance- @willemferguson

Use one of the breeze icons for the map in the drawer on the mobile version. Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

The map widget on the mobile version requires that a dive object from a model has a dive_site uuid. Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

Add the following includes in the header - <QVariant> and <QGeoCoordinate> otherwise the mobile build fails. It is unclear how the desktop build does not complain about this. Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

centerOnDiveSiteUUID() is a helper to center the map on a dive_site UUID instead of a dive_site pointer. Make it call centerOnDiveSite(). Make both this function and reloadMapLocations() Q_INVOKABLE as these are going to be called from QML. Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

neolit123 · 2018-03-08T21:27:47Z

APK:
https://transfer.sh/GghZ9/subsurface-mobile-build-arm-debug.apk

the build works but running the APK results in "cannot find a plugin named: googlemaps".
which means that the APK does not include the plugin. help with that would be appreciated.

edit: looks like packaging/android/build.sh isn't building the plugin.

janmulder · 2018-03-09T10:43:48Z

Tested on device, after hacking through the build problem on my local desktop. Some review comment:

Trivial: add title: qsTr("Map") to the new map page. This populates the breadcrumb navigation in the application header.
See mp4 (after decompress)
flashing.mp4.gz
When navigating to a site far away, the map screen does flash. Not sure what is the origin of this issue, but it does not look very nice.
See mp4 (after decompress)
list-navigate.mp4.gz
Something else I do not like. When selecting a different dive on the map, and back to the original one, the dive list is navigated to the first occurrence of the last selected dive. For example, in the movie , I navigate from dive nr 2879 to 2109 just by clicking on the flags. I'm not sure what is right in this case (design-wize). As there is 1 dive selected when clicking on a flag, it is always needed to choose one of the dives that has that flag location tied to it. And basically, any choice is "wrong" here.
with respect to building. In my hack, I reverted the commit 726ad51. Those hard coded paths to Qt 5.9.3 are just not the right thing to do. Things should respect the "rules" stated in the top of the build script, such as ANDROID_NDK_ROOT/SDK_ROOT. I currently build against (locally) Qt 5.10.1

neolit123 · 2018-03-09T11:31:08Z

Tested on device, after hacking through the build problem on my local desktop. Some review comment:

good thing it doesn't crash at least.

Trivial: add title: qsTr("Map") to the new map page. This populates the breadcrumb navigation in the application header.

easy to do.

See mp4 (after decompress)
flashing.mp4.gz
When navigating to a site far away, the map screen does flash. Not sure what is the origin of this issue, but it does not look very nice.

the only fix i see is to make the zoom animation very slow. tiles are loaded slower than the map requesting them. then again how can you predict how fast is one's internet, as that is the cause for the flashing? i see similar behavior with google maps, except it's not as visually striking.

See mp4 (after decompress)
list-navigate.mp4.gz
Something else I do not like. When selecting a different dive on the map, and back to the original one, the dive list is navigated to the first occurrence of the last selected dive. For example, in the movie , I navigate from dive nr 2879 to 2109 just by clicking on the flags. I'm not sure what is right in this case (design-wize). As there is 1 dive selected when clicking on a flag, it is always needed to choose one of the dives that has that flag location tied to it. And basically, any choice is "wrong" here.

i don't see any good solutions without enabling multi selection and multiediting.
selecting the first dive seems right to me for the time being.

with respect to building. In my hack, I reverted the commit 726ad51. Those hard coded paths to Qt 5.9.3 are just not the right thing to do. Things should respect the "rules" stated in the top of the build script, such as ANDROID_NDK_ROOT/SDK_ROOT. I currently build against (locally) Qt 5.10.1

my 5.9.3 was a temporary solution to try to get the CI to build the google maps plugin last night.
qmake was looking for a x86 compiler while the NDK installs a x86_64
just push your fix on top of this branch and i will remove my temporary one android/build.sh: build the googlemaps plugin.

janmulder · 2018-03-09T12:16:31Z

the only fix i see is to make the zoom animation very slow. tiles are loaded slower than the map requesting them. then again how can you predict how fast is one's internet, as that is the cause for the flashing? i see similar behavior with google maps, except it's not as visually striking.

I'm not sure it is limited by network loading speed. I looked at the video in slow speed, and see that the tile data (the scale) is the same before and after the first full screen white flash. In addition, a fast zoom out on the google maps client on my device looks very different

i don't see any good solutions without enabling multi selection and multiediting.
selecting the first dive seems right to me for the time being.

As the dive list is ordered in a descending way, and users might tend to be more interested in recent dives compared to dives long ago, it might be better to select the last dive instead of the first. My oldest dives in Subsurface are 10 years old, and I currently have exact 1000 dives in Subsurface, so a random click on a flag brings me back multiple years in the list (most of the time). This does not feel right.

my 5.9.3 was a temporary solution to try to get the CI to build the google maps plugin last night.
qmake was looking for a x86 compiler while the NDK installs a x86_64
just push your fix on top of this branch and i will remove my temporary one android/build.sh: build the googlemaps plugin

I currently have no fix, I just hacked around on the command line. Maybe tomorrow morning some time to make a commit for it.

neolit123 · 2018-03-09T12:32:53Z

I'm not sure it is limited by network loading speed. I looked at the video in slow speed, and see that the tile data (the scale) is the same before and after the first full screen white flash. In addition, a fast zoom out on the google maps client on my device looks very different

so once the tiles have loaded for a certain area the flashing can be seen again over the same area afterwards? i have to say that i don't have a good fix for that either way.

As the dive list is ordered in a descending way, and users might tend to be more interested in recent dives compared to dives long ago, it might be better to select the last dive instead of the first. My oldest dives in Subsurface are 10 years old, and I currently have exact 1000 dives in Subsurface, so a random click on a flag brings me back multiple years in the list (most of the time). This does not feel right.

understood, this makes sense and i would try to get the latest single dive instead.

I currently have no fix, I just hacked around on the command line. Maybe tomorrow morning some time to make a commit for it.

ok, if you think you wont have the time for this just send me your hack now so that i can have a look.
trying to get the CI VM happy without remote access is difficult, to be subtle about this.

janmulder · 2018-03-09T13:17:56Z

This was what I did from the command line to get the plugin compiled and installed. Notice that this installs to my local Qt tree, not to the Subsurface install-root.

 cd googlemaps/
 mkdir build-arm
 cd build-arm/
 /opt/Qt/5.10.1/android_armv7/bin/qmake -query
 /opt/Qt/5.10.1/android_armv7/bin/qmake 
 echo "QMAKE_CXX = ../../ndk-arm/bin/arm-linux-androideabi-g++" >> ../googlemaps.pro
 export ANDROID_NDK_ROOT=/home/janmulder/android/android-ndk/android-ndk-r14b
 /opt/Qt/5.10.1/android_armv7/bin/qmake ../googlemaps.pro
 make
 make install (failed, no write permission in target)
 sudo /opt/Qt/5.10.1/android_armv7/bin/qmake -install qinstall -exe libqtgeoservices_googlemaps.so /opt/Qt/5.10.1/android_armv7/plugins/geoser
vices/libqtgeoservices_googlemaps.so
 ./subsurface/packaging/android/build.sh (with commit 726ad51d477 reverted)

output of qmake -query

QT_SYSROOT:
QT_INSTALL_PREFIX:/opt/Qt/5.10.1/android_armv7
QT_INSTALL_ARCHDATA:/opt/Qt/5.10.1/android_armv7
QT_INSTALL_DATA:/opt/Qt/5.10.1/android_armv7
QT_INSTALL_DOCS:/opt/Qt/Docs/Qt-5.10.1
QT_INSTALL_HEADERS:/opt/Qt/5.10.1/android_armv7/include
QT_INSTALL_LIBS:/opt/Qt/5.10.1/android_armv7/lib
QT_INSTALL_LIBEXECS:/opt/Qt/5.10.1/android_armv7/libexec
QT_INSTALL_BINS:/opt/Qt/5.10.1/android_armv7/bin
QT_INSTALL_TESTS:/opt/Qt/5.10.1/android_armv7/tests
QT_INSTALL_PLUGINS:/opt/Qt/5.10.1/android_armv7/plugins
QT_INSTALL_IMPORTS:/opt/Qt/5.10.1/android_armv7/imports
QT_INSTALL_QML:/opt/Qt/5.10.1/android_armv7/qml
QT_INSTALL_TRANSLATIONS:/opt/Qt/5.10.1/android_armv7/translations
QT_INSTALL_CONFIGURATION:/opt/Qt/5.10.1/android_armv7
QT_INSTALL_EXAMPLES:/opt/Qt/Examples/Qt-5.10.1
QT_INSTALL_DEMOS:/opt/Qt/Examples/Qt-5.10.1
QT_HOST_PREFIX:/opt/Qt/5.10.1/android_armv7
QT_HOST_DATA:/opt/Qt/5.10.1/android_armv7
QT_HOST_BINS:/opt/Qt/5.10.1/android_armv7/bin
QT_HOST_LIBS:/opt/Qt/5.10.1/android_armv7/lib
QMAKE_SPEC:linux-g++
QMAKE_XSPEC:android-g++
QMAKE_VERSION:3.1
QT_VERSION:5.10.1

dirkhh · 2018-03-09T15:34:42Z

build system integration for both Android and iOS always is a major pain with new dependencies. That's fundamentally broken on both platforms. I continue to be completely under water with work and real life - and will be at least through mid next week.
It's nice if we get to "working apk built on Travis" - that might get me close on Android. iOS is then a completely different problem because it is qmake based and decides in a different way which QML modules to bundle.
I have twice started to try to add iOS to the Travis builds, that's still high on my list (and then still begs the question how this can easily be tested for "oh, actually works").

All this aside, a few comments on the discussion here:

please don't go down the path of multi-selection on mobile
instead, let's remove things that assume multi-selection (i.e., make them conditional for the desktop)
the "which dive to pick" logic when clicking on a flag... would picking the "chronologically closest" to the currently selected dive be an improvement? My thought is "I go to this site every year... if I'm on a dive from this year and click on a different flag, likely I also want a dive from this year, not the first time I ever dove there". But maybe I'm missing other scenarios where this wouldn't be the right answer?

neolit123 · 2018-03-09T16:15:15Z

the "which dive to pick" logic when clicking on a flag... would picking the "chronologically closest" to the currently selected dive be an improvement? My thought is "I go to this site every year... if I'm on a dive from this year and click on a different flag, likely I also want a dive from this year, not the first time I ever dove there". But maybe I'm missing other scenarios where this wouldn't be the right answer?

it's hard for me to tell which is right as a non-user of the mobile app, so maybe we need feedback from more users here.

what Jan proposed seems like a good and easy to implement solution - always pick the latest dive chronologically.
your solution seems good too, but i wonder if it is going to confuse the users without knowing the logic behind it.

neolit123 · 2018-03-09T16:16:57Z

It's nice if we get to "working apk built on Travis"

i will try to get the APK building via Travis this weekend.

The mobile version is limited as it does not support dive list selection of multiple dives and editing multiple dives. Also the dive list on mobile does not follow the same indexing as the desktop version dive list. Use the SUBSURFACE_MOBILE macro and for: - centerOnDiveSite() either deselect map markers or center on a single one (never on a rectangle like the desktop version) - selectedLocationChanged() and selectVisibleLocations() return a list of single dive ID Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

Add the setCurrentDiveListIndex() wrapper for: diveListView.currentIndex = idx wich also makes it possible to disable the scroll animation when selecting dive list indexes which are too far apart. Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

The signal to emit is selectedDivesChanged() and it accepts a list if diveIds. The 'nSelectedDives' counter is redundant. Also expose the 'map' and 'mapHelper' objects as aliases. Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

The Page object has the following functionality: - reloadMap(): reload all map markers. - centerOnDiveSiteUUID(): center the map on a dive site uuid. - centerOnLocation(): the map on a latitude, longitude in decimal. - Select a dive list entry based on a marker selected on the map via diveList.setCurrentDiveListIndex() Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

1) QML files 2) Map widget specific icons 3) The Breeze map-globe.svg icon Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

showMap() uses a location to open Google Maps in a browser. Make showMap() a generic function to push the mapPage on the view stack. Update the calls to this function from child widgets and pages. Also either call mapPage.centerOnLocation() or mapPage.centerOnDiveSiteUUID() depending if the caller wants the map to center on a dive site or on map coordinates. Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

This action uses the Breeze icon "map-globe.svg" and calls showMap(). Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

When the slot pageStack.onCurrentItemChanged() is reached make sure that the stackView becomes non-interactive. This prevents swiping left on the map to "go back". Also, always reload the map markers when the map becomes visible. This is not optimal and instead something in the lines of: DiveList.model.onChanged() is a much better solution. Ideally the map reload should happen on dive removal, dive addition, dive edits and sync from cloud. Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

neolit123 · 2018-03-10T19:16:50Z

update:

added title for the map page
we only support single dive selection in the dive list on mobile. select latest chronological dive in the dive list when clicking a map marker. still a subject to discussion, see Dirk's comment above. maybe we can merge this as is and modify later once we have a clear idea?
build the android version via CI.
latest APK (armv7-a):
(not sure how long this link will last)
https://transfer.sh/j0Cpw/subsurface-mobile-build-arm-debug.apk
log:
https://api.travis-ci.org/v3/job/351760999/log.txt
tested it on my phone and it works.

janmulder · 2018-03-10T19:56:31Z

Tested my own compile on device. All working. Also did a quick read over the code changes and all looks good to me. So, approved and for me ready to merge.

And with the current behavior (chronological last) of selecting a flag on the map, we even have (primitive) search capability for the last dive on that location. I like that as well.

dirkhh · 2018-03-11T18:40:08Z

Reading through the code it looks all good. This also needs to be added for iOS - but that shouldn't stop us from adding it to master and releasing a new Android beta in the meantime.

neolit123 added 4 commits March 8, 2018 21:16

scrips/mobilecomponents.sh: fetch icon for the map

c804f65

Use one of the breeze icons for the map in the drawer on the mobile version. Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

DiveObjectHelper: add dive_site_uuid()

7903afb

The map widget on the mobile version requires that a dive object from a model has a dive_site uuid. Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

qmapwidgethelper: move includes to the header

8d0c641

Add the following includes in the header - <QVariant> and <QGeoCoordinate> otherwise the mobile build fails. It is unclear how the desktop build does not complain about this. Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

neolit123 added enhancements-we-intend-to-add ui-mobile widget-map needs-user-feedback needs-developer-feedback needs-documentation-change googlemaps labels Mar 8, 2018

neolit123 force-pushed the map-mobile branch 8 times, most recently from 4729802 to 726ad51 Compare March 9, 2018 02:10

neolit123 added 2 commits March 10, 2018 16:50

neolit123 added 10 commits March 10, 2018 16:50

mobile-resources.qrc: add the map related resources for mobile

3c909a8

1) QML files 2) Map widget specific icons 3) The Breeze map-globe.svg icon Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

main.qml: create an instance of MapPage

73f773f

Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

main.qml: add a drawer action for the map

8c00615

This action uses the Breeze icon "map-globe.svg" and calls showMap(). Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

subsurface-mobile-helper: register map related types

7c4ed42

Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

cmake: update CMake to build the map components on mobile

6b13bb7

Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

CHANGELOG.md: add note about the map widget changes

1ad0a48

Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

neolit123 force-pushed the map-mobile branch 4 times, most recently from 6750dea to 4578b49 Compare March 10, 2018 18:05

android/build.sh: build the googlemaps plugin

b28f684

Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>

neolit123 force-pushed the map-mobile branch from 4578b49 to b28f684 Compare March 10, 2018 18:18

dirkhh merged commit cdba355 into subsurface:master Mar 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mobile: enable the map widget #1156

mobile: enable the map widget #1156

neolit123 commented Mar 8, 2018 •

edited

neolit123 commented Mar 8, 2018 •

edited

janmulder commented Mar 9, 2018

neolit123 commented Mar 9, 2018

janmulder commented Mar 9, 2018

neolit123 commented Mar 9, 2018 •

edited

janmulder commented Mar 9, 2018

dirkhh commented Mar 9, 2018

neolit123 commented Mar 9, 2018

neolit123 commented Mar 9, 2018

neolit123 commented Mar 10, 2018 •

edited

janmulder commented Mar 10, 2018

dirkhh commented Mar 11, 2018

mobile: enable the map widget #1156

mobile: enable the map widget #1156

Conversation

neolit123 commented Mar 8, 2018 • edited

Describe the pull request:

Pull request long description:

Changes made:

Related issues:

Additional information:

Release note:

Documentation change:

Mentions:

neolit123 commented Mar 8, 2018 • edited

janmulder commented Mar 9, 2018

neolit123 commented Mar 9, 2018

janmulder commented Mar 9, 2018

neolit123 commented Mar 9, 2018 • edited

janmulder commented Mar 9, 2018

dirkhh commented Mar 9, 2018

neolit123 commented Mar 9, 2018

neolit123 commented Mar 9, 2018

neolit123 commented Mar 10, 2018 • edited

janmulder commented Mar 10, 2018

dirkhh commented Mar 11, 2018

neolit123 commented Mar 8, 2018 •

edited

neolit123 commented Mar 8, 2018 •

edited

neolit123 commented Mar 9, 2018 •

edited

neolit123 commented Mar 10, 2018 •

edited