Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate and use SAF for offline map themes #9607

Closed
Lineflyer opened this issue Dec 27, 2020 · 29 comments
Closed

Migrate and use SAF for offline map themes #9607

Lineflyer opened this issue Dec 27, 2020 · 29 comments
Assignees
Labels
Prio - High A significant malfunction of a feature/function. High user impact. Refactoring Issues requires code refactoring

Comments

@Lineflyer
Copy link
Member

This issue is similar to #9606 but for offline maps it might be a bit more complicated as we should not enforce this data to be in the same directory as the external user data (see https://github.com/cgeo/cgeo/wiki/Disk-usage-structure , item <3>) and migration is more relevant (as c:geo kind of needs the offline map data to work as desired by the user).

Starting with target SDK=30 Android enforces to use the StorageAccessFramework (SAF) instead of plain file access permissions.
Therefore we should migrate from the current access mode into using the SAF.

The general approach has been discussed in #8457 already, which I try to summarize below (correct me, if I am wrong) for this item together with my ideas for the offline map case:

  • On fresh installation we might use the SAF location requested in Migrate and use SAF for storing external user data (previous /cgeo/*) #9606 and store maps (downloaded by our own downloader) into ../maps/ below our base directory. This avoids another SAF request to the user for that default case
  • We should however (for existing users and for users preferring to store them a different location than the "normal" external user data) offer the possiibility to set up a dedicated location for those via SAF.
  • This could be implemented by a selection dialog in Settings - Maps, which offers
    • Default = Use the base dir for external user data (corresponds to currently used /cgeo/maps/)
    • User defined = Let the user choose a base directory explicitly for his offline maps

To be agreed/discussed:

  • How to deal with the migration case? Can we analyze the currently set path and if it is not the default we trigger the SAF request and help the user by prompting his current selection and ask him to confirm it via SAF?

  • Do we want to keep another dedicated directory (yet another SAF setup) for map themes or at least combine maps and themes into one base directory?

  • Can we use the opportunity also here to finally get rid of the ugly dir chooser?

@Lineflyer Lineflyer added Refactoring Issues requires code refactoring Prio - High A significant malfunction of a feature/function. High user impact. labels Dec 27, 2020
@Lineflyer Lineflyer added this to To do in Android 10+ storage framework via automation Dec 27, 2020
@eddiemuc
Copy link
Contributor

Largely adressed in branch storage_framework, please use it as a base for contributions.
Aslo please give me (@eddiemuc) a short hint before you do since I currently rebase this branch onto master frequently using forced pushes.

@eddiemuc eddiemuc self-assigned this Dec 27, 2020
@eddiemuc
Copy link
Contributor

Question by @geocachermgo here: #9606 (comment):

Will this affect to read maps from the locus directory?

Yes, as far as I can tell right now you will need to reselect the dir with locus maps in it one more time in maps settings after SAF migration

@geocachermgo
Copy link

geocachermgo commented Dec 27, 2020

I have no problem with that, but was afraid that I have to store 2 copies of the maps

@eddiemuc
Copy link
Contributor

@Lineflyer with regards to the open points in the description:

How to deal with the migration case? Can we analyze the currently set path and if it is not the default we trigger the SAF request and help the user by prompting his current selection and ask him to confirm it via SAF?

As mentioned in #9606, imho we should offer a "complete migration" of the default cgeo folder and no further migration option e.g. for map files. If the user does not use the default map folder under "cgeo/maps", she has to reselect her personal map folder one time using SAF, unfortunately. But she can do that at same place as before in Settings.
Currently I feat we can't effectively "suggest" the former used file path as SAF start Uri because:

  • The Uris used by SAF are not easily "constructable" from existing file paths as far as I can tell. In any case this would be a hackish attempt
  • Preselecting an Uri for a SAF file selection dialog is not supported prior to API26.

Do we want to keep another dedicated directory (yet another SAF setup) for map themes or at least combine maps and themes into one base directory?

Up to now I had no intention to change this. Why do you want to do that?

Can we use the opportunity also here to finally get rid of the ugly dir chooser?

Yes, all the self-made dir choosers and file choosers will be gone once the SAF migration is finished. They will be replaced by the SAF intents. You can already take a glimpse by installing the APK from storage_framework and see it there for the offline map folder selection.

@Lineflyer
Copy link
Member Author

she has to reselect her personal map folder one time using SAF, unfortunately. But she can do that at same place as before in Settings. Currently I feat we can't effectively "suggest" the former used file path as SAF start Uri because:....

Perfectly fine...
But we could before we trigger the SAF show the currently used path to remind the user what he selected before he starts over to select it again via SAF.

Yes, all the self-made dir choosers and file choosers will be gone once the SAF migration is finished. They will be replaced by the SAF intents. You can already take a glimpse by installing the APK from storage_framework and see it there for the offline map folder selection.

Sounds good, I will take a look next days and also provide feedback and/or open new issues if I do see a problem.

eddiemuc added a commit to eddiemuc/cgeo that referenced this issue Jan 3, 2021
eddiemuc added a commit to eddiemuc/cgeo that referenced this issue Jan 4, 2021
eddiemuc added a commit to eddiemuc/cgeo that referenced this issue Jan 5, 2021
eddiemuc added a commit to eddiemuc/cgeo that referenced this issue Jan 6, 2021
eddiemuc added a commit to eddiemuc/cgeo that referenced this issue Jan 6, 2021
eddiemuc added a commit to eddiemuc/cgeo that referenced this issue Jan 7, 2021
@eddiemuc eddiemuc changed the title Migrate and use SAF for offline maps Migrate and use SAF for offline map themes Jan 7, 2021
@eddiemuc
Copy link
Contributor

eddiemuc commented Jan 7, 2021

Map directory has been migrated to SAF by #9606. Left open is the migration of map themes directory.

In order to do that a redesign for themes usage has to be done on master first because it currently is fit for usage of Files only. This should be done in this issue.

See methods "selectMapTheme" in "CGeoMap" and "NewMap". Basically, they must be redesigned to deal with a set of Uris or InputStreams instead of a set of Files

@eddiemuc
Copy link
Contributor

eddiemuc commented Jan 7, 2021

I have discovered a little problem with using SAF-based xml themes with mapsforge.

We use XmlRenderTheme to apply custom themes to our offline maps. There is a version of such a renderer available which accepts a stream (StreamRenderTheme), but even this version wants resources (mainly icons/images) to be provided as Files. I scanned the mapsforge code a bit, and this seems to be burried deep into the code.

Example: here is a part of such an XML renderer file with a resource reference in it (in this case to "ele_res/p_camp_site.svg")

image

I could use a second opinion on how to proceed now. Following options came to my mind:

  • Ask mapsforge to implement a feature request to make resouces supplyable by a custom handler delivering InputStreams for (relative) references. However, my guess would be that this is a fairly large request.
  • Don't use a public folder for themes any more but provide some sort of dialog to import themes (and then store them in an internal File folder)
  • Scan the themes dir on c:geo startup, if there are any new files (check last-modified-date) then copy all non-XML-files (=resource files) to an internal directory where File-access is possible

What do you think? Any alternative ideas? Any preferences?

I would love the first option... Do we have any direct contact to the mapsforge project? How open are they to self-supplied Pull Requests?

For my own reference: key method in mapsforge to change would be org.mapsforge.map.rendertheme.XmlUtils.createInputStream() (has a parameter "relativeFilePath" which at the end comes from XmlRenderTheme and must be exchanged with some sort of callback providing an InputStream for a (relative path) string

@eddiemuc
Copy link
Contributor

eddiemuc commented Jan 7, 2021

Update: I posted a request in mapsforge forum, let's see how they react

@eddiemuc
Copy link
Contributor

eddiemuc commented Jan 8, 2021

Another update: mapsforge responded, next step for me would be to prepare a pull request form them. They may want to go deeper than I thought and provide direct document access support, not just a possibility to "hook in " an InputStream-Provider.

Before I proceed with this: in case this is successful we would of course need to integrate one of the next releases of mapsforge in c:geo to use the changes. Would that be possible?

@eddiemuc
Copy link
Contributor

eddiemuc commented Jan 8, 2021

Link to discussion with mapsforge: https://groups.google.com/g/mapsforge-dev/c/4LcJL93sa14

@moving-bits
Copy link
Member

Thanks for clarifying with the Mapsforge team.

Before I proceed with this: in case this is successful we would of course need to integrate one of the next releases of mapsforge in c:geo to use the changes. Would that be possible?

Yes, no problem. IIRC we are currently using their most up-to-date version.

@eddiemuc
Copy link
Contributor

eddiemuc commented Jan 9, 2021

Update: I wrote a PR for mapsforge which is able to pull xml theme resource files from disk using Android scoped storage as well (they already had something to pull the xml theme isself). While this works, it has shown that performance for this is simply unacceptable: takes about 10-15 seconds to load a theme in SDK30 (emulator). This is mainly because it has to load approx 400 files from disk (all the small icons) and each of these accesses is about 20-30ms long.... Surprisingly time is significantly shorter in SDK29...

Anyway, I had the idea to add support for ZIPPED map theme resources to mapsforge, so mapsforge at least has only to read in ONE file. Let's see how they react.

If you are interested, here is the PR: mapsforge/mapsforge#1186

I am a bit worried we will run also in performance problems in some places in c:geo once SAF is introduced...

@moving-bits
Copy link
Member

Anyway, I had the idea to add support for ZIPPED map theme resources to mapsforge, so mapsforge at least has only to read in ONE file. Let's see how they react.

Good idea, would also make installing a theme for our users easier. (And for us, if someday we want to automate this similar to our map downloader.)

@eddiemuc
Copy link
Contributor

eddiemuc commented Jan 9, 2021

Btw: does anyone know what would be typical sources for mapsforge map themes ? I only know the ones from openandromaps.

@moving-bits
Copy link
Member

IIRC freizeitkarte.de also has Mapsforge-compatible themes, but there may be others.

@Lineflyer
Copy link
Member Author

Usually themes should come with the map (as the theme needs to be aligned to the map content). AFAIK mapsforge only provides one default theme. Some years ago I found nightvision themes and some other variants someone made for these maps (red on black). But I cant remember where.

@Lineflyer
Copy link
Member Author

@moving-bits
Copy link
Member

Usually themes should come with the map (as the theme needs to be aligned to the map content).

Which leads me to a feature wish: Can we somehow build fixed pairs of map and theme?

Currently we have a map directory with a bunch of maps, and a theme directory with a bunch of themes. But those do not necessarily fit to each other, cause map A may work with themes A and B, but map B may require theme C, which will not work with map A, etc.

When we start using zipped theme files, it's just one small file per map (small in comparison to the map file), so it should be ok in terms of space requirements.

Don't know, though, how to make a good UI for that.

@eddiemuc
Copy link
Contributor

eddiemuc commented Jan 9, 2021

When we start using zipped theme files, it's just one small file per map (small in comparison to the map file), so it should be ok in terms of space requirements.

First we need mapsforge support for zipped themes :-) Got no feedback yet for my proposal.

@eddiemuc
Copy link
Contributor

Update: support for zipped themes as well as access to theme ressources via scoped storage is now merged into mapsforge master, but not yet released (last release 0.15.0 does NOT yet contain these changes). We have to wait until next mapsforge release before continuing here I am afraid.

But we can start discussing and deciding how to integrate themes in the future with SAF. The main problem is that integration of themes via scoped storage works but is terribly slow! Reason is that loading all the resource files (icons etc) takes much time. As a example, loading all the resource files of the openandromaps-themes (about 400 icons) takes about 10 seconds in my emulator.

I see following possible scenarios:

  • Integrate as above and accepting these loading times (I just put this here for completeness, I think this is an unacceptable solution)
  • Support (only) zipped themes in the future. This is unfortunately a break with current way and thus a regression
  • Build a c:geo-specific cache solution where resource files are silently copied into private c:geo storage in the background and used from there. This would hopefully restrict the long access times to the first usage of a new downloaded theme but not later. SOme thoughts have to be put into how to correctly detect and handle updates/deletions of themes in public folder though. Solution will become a bit complex.

What do you think?

Interested folks can find details wrt mapsforge changes here:

@moving-bits
Copy link
Member

In what way are theme files distributed? I guess always as a ZIP file.

Then I would vote for supporting only ZIP files for themes, even if that's a breaking change. IMHO users working with map themes are the more advanced users, which will be able to reload their theme file once.

This would save both bad user experience due to very long load times as well as us having to add and maintain a lot of relative complex code for a transient problem.

@Lineflyer
Copy link
Member Author

A few questions:

Integrate as above and accepting these loading times

When do these loading times occur? Only on initially selecting a theme, or each time its loaded / c:geo is started / etc.?

Support (only) zipped themes in the future.

That would be an option for sure although its a breaking change. Technical question: How can this have better performance just due to the fact all the files are zipped?

Build a c:geo-specific cache solution where resource files are silently copied into private c:geo storage in the background and used from there. This would hopefully restrict the long access times to the first usage of a new downloaded theme but not later.

That was also my immediate idea while reading the beginning of your posting. However why would it still have longer loading times later? It would be (after copying) the same solution as today, or not?
But overall there is still a problem here, which makes this solution rather unattractive: What if the user updates the theme files by just overwriting the source (this is what I do once a new version is released). This would mean, we need either to detect changes (and copy again) or make the user aware to reselect it after changing it. Both not user friendly, therefore this solution might not be a favorite.

@fm-sys
Copy link
Member

fm-sys commented Jan 23, 2021

Totally agree with @moving-bits. Keep it simple! Maybe we can detect if the user currently uses map themes and display a OneTimeMessage for it, to explain the change...

@moving-bits
Copy link
Member

If I understood the technical things correctly, the loading time differences are due to different ways of accessing the files:

  • As long as the files are somewhere on a "public" storage, we need to use SAF to access it, which is terribly slow.
  • But as long as such a file is stored in app-private storage, we can access it using the way-faster File based methods.

Alternative a) would mean long loading times on each theme load, so probably multiple times during a c:geo session. (Please correct me, if Mapsforge does some internal caching which survives theme changes.)

Alternative b) would mean only one slow access for reading the ZIP file, whereas everything else is done in-memory. Probably reading the ZIP file happens on each theme reload, same question as above.

Alternative c) would require synching, having slow access at that, but later on continue with the same access speed as today.

@eddiemuc
Copy link
Contributor

@moving-bits got it perfectly right! To answer the remaining questions:

  • Reg solution a): the mapsforge content theme does internal caching, it reads the files just once and holds them in memory. The initial reading however (done when the class is created) takes the mentioned 10 seconds. So how often this happens to the user depends on c:geo: if this is cached across activity openings/closings, thje loading time will only appear once per session.
  • Reg. solution b): all theme sources I know of provide zipped ressources. In theory it is possible that there is an xml-themen WITHOUT any ressources (e.g. without any icons etc). I would assume that those would not be provided as zip. But I don't know any examples
  • Reg. solution c): yes, this would mean a one-time-long-loading-time but fast usage later (also across c:geo sessions). We would however need some sort of "cache sync mechanism" would need to be created, maybe based on timestamp or file-size checks, to make sure we keep the "cached" files in c:geo internal storage up-to-date. This is what would make this solution complicated.

All this performance stuff boils down to the simple fact that basic file operations which are superfast using File objects are very heavy using scoped storage. Things like "listing files in a directory", "opening a file Input Stream" and even "reading a file inout stream" are all operations done in <1ms when using files but take up to 10ms when using scoped storage. PLease compare the following:

  • today's file-based approach simply reads icons from the file system whenever they are needed. Super-fast, no optimization necessary
  • Solution a) reads all icons just ONE TIME and puts them into memory. But it has to scan some directories (10ms each), to open many icon files (400 files x 10ms = 4s alone for this!) and reading them (significantly slower than reading a filestream)
  • Solution b) reduces this time because it has to open only one file and it does not need to scan dirs. Still this approach will be notably slower than todays file-based approach because the input stream reading is slower
  • Solution c) would make the follow-up usages (after cache sync) as fast as today because this will be filebased

To make things even more complicated, the "slowness" of SAF seeems also to be dependent on Android API-level (the higher the slower, at least in the emulators. Longest time I got in Android 11 API30 emulator) and maybe also dependent on device or other factors. I hope we get some real-life processing time once this stuff is in master and the nightlies. But I don't think we are done here.

@eddiemuc
Copy link
Contributor

@moving-bits I remember some efforts were made to include the new mapsforge parts for Offline Theme rendering into c:geo earlier than waiting for next mapsforge release. Indeed I find everything in the mapsforge-libs now.

Question: did you already start to integrate this, and do you want to continue this? Otherwise this would be the next ToDo on my list (probably not before next weekend though).

@moving-bits
Copy link
Member

@eddiemuc
I have implemented the following so far in this regard:

  • Select and download the zipped theme file from openandromaps.org using the map downloader and store it in the map themes folder, but do not unzip it.
  • Update check for theme files downloaded using the map downloader. (Currently elevate.zip only)
  • Integrated persistable folder migration into the installation/migration wizard for both map folder and gpx folder.
  • Addtionally @bekuno has updated gradle config to get the right version of Mapsforge integrated into c:geo to have the ZipRenderTheme available for us.

So everything should be in place for integrating the ZipThemeRenderer and migrating map theme folder to SAF. I haven't started with any of this yet, and would be very happy if you could dive into this, as time allows, as you are much more familiar with this stuff than I'm. I can extend the installation/migration wizard then, as soon as map theme folder is migrated to SAF.

@eddiemuc
Copy link
Contributor

eddiemuc commented Feb 14, 2021

First of allany thanks to you @moving-bits and @bekuno for the great preparation/enhancements! I will happily take over the SAF migration part

Currently I think to do it like this (mixing scenarios 2 and 3 from above):

  • Implement a map theme folder cache (copy) in private app store (=File-based) for faster access. On synchronize it shall detect new/updated/deleted files using filename, modification date and file length
  • Synchronization happens at following places
    • On startup
    • After map theme download
    • Maybe also explicitly triggerable in settings?
  • Sync may take long time (10-15s at least the first time after e.g. copied a resource folder like ele_res to map theme dir), so process will be async with progress bar
  • Available map themes are then displayed by scanning all xml Files and zip files in this cache folder.
  • Currently/last used theme's zip files Zipthemeresourceprovider is caches in memory, but only on first usage.

I will probably create some new class ( MapThemeCache or so) to.encapsulate all this.

I think this handles also the case reasonably well if a user still decides to extract theme zips into theme folder, like he might be used to. And even if zipthemeresourceprovider is reasonably fast to read from saf folder, it is still a lot faster when using a file.

@moving-bits
Copy link
Member

Thanks for pursuing this issue. Concept LGTM.

For copying the zipped theme file after a successful download via the map downloader you can use the method downloader/MapDownloaderElevate:onSuccessfulReceive. (This is also the place to set the received theme as current theme.)

eddiemuc added a commit to eddiemuc/cgeo that referenced this issue Feb 20, 2021
eddiemuc added a commit to eddiemuc/cgeo that referenced this issue Feb 20, 2021
eddiemuc added a commit to eddiemuc/cgeo that referenced this issue Feb 21, 2021
eddiemuc added a commit to eddiemuc/cgeo that referenced this issue Feb 21, 2021
eddiemuc added a commit to eddiemuc/cgeo that referenced this issue Feb 21, 2021
eddiemuc added a commit to eddiemuc/cgeo that referenced this issue Feb 21, 2021
eddiemuc added a commit to eddiemuc/cgeo that referenced this issue Feb 21, 2021
Android 10+ storage framework automation moved this from In progress to Done Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Prio - High A significant malfunction of a feature/function. High user impact. Refactoring Issues requires code refactoring
Projects
No open projects
Development

No branches or pull requests

5 participants