Refactor download receiver to background service

[fm-sys]

Fixes #10256

Yippee - it works ;-)

[bekuno]

retest this please

[bekuno]

retest this please

[moving-bits]

@fm-sys
Oops, somehow I must have missed this PR, sorry. I like the idea of putting receiving a downloaded file into the background for less disturbing the user! Thanks for your work on it.

@moving-bits The onFollowup method heavily uses an Activity parameter currently, which won't be possible when working with background services for sure.

If I'm not mistaken I'm using the onFollowup currently only for theme downloads (Freizeitkarte / OAM). This could be easily avoided if we move checking for theme existence and asking the user (if necessary) from onFollowUp to the beginning of the download, so that from that on everything can be done in the background then

Future more, I do not really get why OpenAndroMaps/Freizeitkarte should require a special theme.

It may work with the standard theme, but works best with the themes specifically created for it, so we should mention it and give the user the choice. (Besides that, auto-downloaded of accompanying theme was specifically requested for.)

In any case, no Activity should ever pop up automatically out of the background.

Totally agree. Just had no better way at hand back when implementing the downloader.

How do we want to proceed?

Using a background service seems promising, please continue... ;-)

Or are you even interested in collaborate on this issue?

Sure, if I can be of any help with this, I'm happy to participate / support.

[moving-bits]

I have to add that file overwrite check (name conflicts etc.) are handled as well on receiving a download (although not in the onFollowup method) - we would need a solution for this as well.

A few observations from a quick test:

• If I start a second download (small) while another (big) one is running, this confuses the downloader. In my case, downloading OAM California (600 MB) was ongoing when I started OAM Hawaii (40 MB). Hawaii then was downloaded successfully, while California file got truncated, without any info.

E/cgeo: [main] Problem opening map file 'content://com.android.externalstorage.documents/tree/primary%3Acgeo%2Fmaps/document/primary%3Acgeo%2Fmaps%2FCarolina_Georgia_Florida%20(OAM).map'
    org.mapsforge.map.reader.header.MapFileException: invalid file size: 928406635
        at org.mapsforge.map.reader.MapFile.<init>(MapFile.java:293)

• After that, I started yet another download (OAM Guam in this case, about 3 MB), which made c:geo crash on Log.e (I had still activated debugging mode)

E/AndroidRuntime: FATAL EXCEPTION: Thread-374
    Process: cgeo.geocaching, PID: 15008
    java.lang.RuntimeException: Aborting on Log.e()
        at cgeo.geocaching.utils.Log.e(Log.java:309)
        at cgeo.geocaching.downloader.ReceiveMapFileService.copyInternal(ReceiveMapFileService.java:239)
        at cgeo.geocaching.downloader.ReceiveMapFileService.handleMapFile(ReceiveMapFileService.java:184)
        at cgeo.geocaching.downloader.ReceiveMapFileService.lambda$onStartCommand$0(ReceiveMapFileService.java:105)
        at cgeo.geocaching.downloader.ReceiveMapFileService.lambda$onStartCommand$0$ReceiveMapFileService(Unknown Source:0)
        at cgeo.geocaching.downloader.-$$Lambda$ReceiveMapFileService$U-Qeq2FXdDXo6MUcMyfwZX0IZmA.run(Unknown Source:4)
        at java.lang.Thread.run(Thread.java:919)

• After disabling debug mode, I started yet another small download (OAM Samoa, 4 MB). It ended with log error IOException on receiving map file: Stream closed (which is probably the one fixed with avoid IOException on successful OAM downloads (fix #10319) #10336), but neither Guam (shown in the map file list) nor the Samoa (not being shown in map file list) work. Both files are present as map files on storage, though without companion file.
• Anyway, a short notification for successful or unsuccessful download should be shown (application toast?) to let the user know whether the download has worked and the downloaded file can be used now.

[fm-sys]

Thanks for testing it :)

but neither Guam (shown in the map file list) nor the Samoa (not being shown in map file list) work. Both files are present as map files on storage, though without companion file.

Probably, because the map file list is not recreated after an successful download yet. Does it work for you after restarting c:geo?

Anyway, a short notification for successful or unsuccessful download should be shown (application toast?)

Agreed, although a notification should be used for it. Toasts can be quite confusing if they show up while another app is currently opened. IIRC I have already implemented such a notification. But I can use a higher priority for it be more prominent.

---

As already pointed out, this is just a quick and thirty draft. I will look into the exceptions and hope, to get simultaneous downloads working soon...

[fm-sys]

Note to myself:
"A service can't be started twice, it will remain running if you try to start it again.

However, everytime you start the service, the onStartCommand() method is called."

So we might need to use a different method than onStartCommand and need to think of some kind of cached todo-query...

http://developer.android.com/guide/components/services.html#StartingAService

[moving-bits]

@fm-sys
I've merged the two changes to the downloader (auto-overwrite existing files, auto-download required themes), so follow-up action is no longer required for (or even supported by) the downloader. So "Feuer frei" for the background service ;-) (as time permits)

[fm-sys]

@moving-bits thank you! Unfortunately I'm quite busy for the next days (need to write a scientific paper for school). Anyway, I think it shouldn't take too long. Background service might be ready within the next 2 weeks... :)

[fm-sys]

After some more research, i noticed that there's something called IntentService. It already has a query pre-implemented, so multiple running downloads will be scheduled automatically by this class which avoids much ugly dealing with threads and makes things way easier.

(this class might be interesting for background cache downloads as well ;-) )

[fm-sys]

Oh, dear. It wasn't easy to rebase this PR to our current master 😅

Current State:

• Still WIP
• Simultaneous downloads are working
• No major known bugs
• Notification handling needs to be optimized. e.g. if a second file get's downloaded it will dismiss the final status notification of the first download
• Still some cleanup to do

@moving-bits would you mind another test in the emulator?

[moving-bits]

@moving-bits would you mind another test in the emulator?

Sure, will try to test it during the next days. Only I'm a bit on a tight time budget currently, so it may take a few days.

[fm-sys]

PR is ready to merge from my side.

Some short notes:

• Everything should work even if c:geo will be closed or screen gets turned off
• It takes some time (up to 30 sec.) until the service gets called (android notification appears). Don't know why this is the case, but this shouldn't be a problem anyway.
• The downloaded map file gets listed in the maps activity only if you leave and reopen the map activity. This could be counted as a bug. If we want to fix it, we should IMHO create a follow up issue. Don't want to play around in the live map activities with this PR.
• The download result will be shown as well as a android notification.
• I used the opportunity to create some generic cgeo notification classes, and refactored existing notification usagesnto use this "framework". Furthermore, I abstracted the usage of ForegroundIntentServices, so that it could easily be reused for a cache download service as well (Download caches as non blocking service #330)

---

BTW, I will not let codacy dictate what to call my classes - it's the same semantic as for "Dialogs.java", "Intents.java", ... ;-)

[moving-bits]

I've given it a short test-drive, even with five downloads in parallel, which the new background service handled pretty well - a big thanks!

Two minor things:

• It would be nice to see the name of the file being imported. (Ok, downloading five maps in parallel may not be the regular use case, but downloading a map in parallel with its theme will be.)
• Auto-activation of the downloaded map and theme would be nice. Not sure it it's possible by a background service, though.

[fm-sys]

I've given it a short test-drive, even with five downloads in parallel, which the new background service handled pretty well - a big thanks!

Nice to hear! Big thanks for cross-testing as well :)

Auto-activation of the downloaded map and theme would be nice. Not sure it it's possible by a background service, though.

IIRC it gets activated if you reopen the maps activity. But for sure, these two things could be optimized.

I would like to get it into master first, and then we can work on these minor things and hopefully will implement some more background services in the future ;-)

[moving-bits]

One more question, just discovered: How will it react to downloading routing tile data? (Or further download types, not yet implemented.)

From looking in the code I can see the continueWithForegroundService being called for type MAP in HandleLocalFilesActivity only.

Tested a bit more, somewhere deep in an area without downloaded routing tile info. Tile data got downloaded, and routing does work, so everything seems to be fine.

[moving-bits]

Auto-activation of the downloaded map and theme would be nice. Not sure it it's possible by a background service, though.

IIRC it gets activated if you reopen the maps activity. But for sure, these two things could be optimized.

Haven't checked for map (as the last finished download was the map already being active), but a theme does not get activated automatically after successful import.

I would like to get it into master first, and then we can work on these minor things

understandable, and ok for me

and hopefully will implement some more background services in the future ;-)

Looking forward to it ;-)

[fm-sys]

One more question, just discovered: How will it react to downloading routing tile data? (Or further download types, not yet implemented.)

Have not checked this explicitly, but actually I have just copied everything from the original receiveDownloadActivity (nearly) without deleting/changing any code parts. So if the service is working in total, nothing should have got broken...

[moving-bits]

One more question, just discovered: How will it react to downloading routing tile data? (Or further download types, not yet implemented.)

Have not checked this explicitly, but actually I have just copied everything from the original receiveDownloadActivity (nearly) without deleting/changing any code parts. So if the service is working in total, nothing should have got broken...

My bad, haven't remembered my own code... I got distracted by the explicit call from within the MAP label - but that is not for the download receiver, but for opening local files from a file manager. The call for downloaded files is handled within the download receiver (and does work indeed without further modification).