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

New Approach to Move Cache dir to external SD #4957

Closed
marco-dev opened this issue May 21, 2015 · 76 comments
Closed

New Approach to Move Cache dir to external SD #4957

marco-dev opened this issue May 21, 2015 · 76 comments
Assignees
Labels
Refactoring Issues requires code refactoring

Comments

@marco-dev
Copy link
Contributor

Hi together,

I would like to try a new approach moving cache dir to external sd like Locus does.
Please vote for or against a new approach.

Choices for user:

  • /storage/emulated/0/.cgeo (default)
  • /storage/extSdCard/Android/data/cgeo.geocaching/.cgeo (will be removed on uninstall)

The files should be moved by c:geo with progress dialog.
We already have a function to remove garbage from cache directory.
I suggest to remove garbage before moving files.

Open questions, please give your feedback:

  • Should I do another approach?
  • If the "moving files" dialog is cancel by back button or by sending the app to background, what should be done?
  • Should we remove garbage before moving cache directory?
  • Any other questions?
@marco-dev marco-dev added the Feedback required Issue requires feedback of the author or another user label May 21, 2015
@Cappa-d
Copy link

Cappa-d commented May 21, 2015

As I ran into diskspace issues last night I was close to get rid of c:geo and move to locus last night... so glad this came up again.

I am eager to see a solution to have c:geo on the real extSDCard! too much issues here and I think if someone can identify the difference between emulated and extSDCard we are not talking about a "normal" user who must be protected because otherwise the app gets blamed.

Moving files: would it be possible to trigger the following cascade:

a) 1:1 copy from internal to extSDCard
b) move 100% trigger deletion on emulated (would avoid an issue with "lost" data due to interrupted dialog)
c) deletion 100% triggers final setting changes (path for internal storage)

Garbage removal:

May add this as an internal cleanup service / menu item which could be triggered at any time from the settings menu. Would prefer to separate this from the moving part.

@marco-dev
Copy link
Contributor Author

@Cappa-d
Garbage removal is already a function in settings, besides I did never try it nor know if this removes all or only garbage data. Have to read FAQ again. :-)

@Lineflyer
Copy link
Member

Should I do another approach?

Yes I would highly appreciate that. Thank you very much for coming back to this.

First my thought was, that the topic will get less important as the amount of memory in the device is also increasing with new models. But current user reports show, that also the amount of cache data is rising (more caches, more pictures, etc.) and a lot of users still complain about too less space on their internal disc.

If the "moving files" dialog is cancel by back button or by sending the app to background, what should be done?

Do we need a "Cancel"?

Should we remove garbage before moving cache directory?

Might remove the amount of unneccesary data to be moved, so yes. And we already have such a function which does only need to be called.

Any other questions?

I think those two options
/storage/emulated/0/.cgeo (default)
/storage/extSdCard/Android/data/cgeo.geocaching/.cgeo (will be removed on uninstall)
are the only feasible method to keep the complexity as minimal as possible.
If I look into the other issues we had concerning this topic, I learned that all other scenarios would require extensive checks of writability and highly depend on Android verisons, etc.etc.

One remark:
Due to the fact that /storage/extSdCard/Android/data/cgeo.geocaching/ is not accesible by the user and also removed upon uninstall we should in parallel modify the backup/restore function to target a separate folder (at the moment its targeting the very same folder).

@SammysHP
Copy link
Member

Do we need a "Cancel"?

No, but the process might be interrupted.

Due to the fact that /storage/extSdCard/Android/data/cgeo.geocaching/ is not accesible by the user and also removed upon uninstall we should in parallel modify the backup/restore function to target a separate folder (at the moment its targeting the very same folder).

But we cannot write to other directories. AFAIK there are some ways to bypass this limitation, but I've never looked into detail for this.

Any other questions?

  • Parallel with the option for database on external storage?
  • What if the user removes the SD card and starts c:geo? (And what while it is running?)
  • Will it then ask to fall back to internal memory? What if the DB is stored in /data, so only images etc. are missing?
  • What if the user has the DB stored in /sdcard (internal) and uses the internal memory as well as the real SD card (as a consequence of the previous question)? Then there are two DB files, how to merge them?

@rsudev
Copy link
Contributor

rsudev commented May 21, 2015

My impression from the previous discussions was, that we agreed to decouple database storage and backup from geocache-related file storage.
This would prevent a number of possible issues, and the storage benefit of moving the cache database to the external sd card is relatively low IMO.
So my suggestion is, to leave the handling for the database as it is and allow moving the file storage to this defined location.
Of course it can be a challenge, to find a wording which a user without technical background can understand and also is true for the variety of platforms we support (2.3 -> 5.1).
It might be a good idea to take this opportunity to also move the default folder from .cgeo to cgeo/cachedata and perhaps have also cgeo/database and cgeo/backup, but should probably rather be a separate item.
So database could be stored in:

  • app storage (default)
  • sdcard/internal

and cache files in:

  • sdcard/internal (default)
  • external

and backup will always go to:

  • sdcard/internal

Given this, in my opinion it is not necessary to put too much emphasis on the transactional nature of the move process. I would say, change the setting, then start moving (this could even be optional). If it is interrupted, then you would have to re-download a couple of images, not a big deal IMO.
When setting back, the move back can be started, and if we do not forcefully clean the target, you can re-fuse your stored files, if so wished. Additionally clean out options for both areas can be offered, as long it is ensured, that the (possibly external) database and the backup is not affected.

@Lineflyer
Copy link
Member

@SammysHP
Regarding backup/restore I was not suggesting to write it to external SD but similar to the existing approach to store it always on the internal SD maybe just in /
Mainly just make it independent from the general storage location

DB on external should no longer be a separate but combined with the general setting. Of course all possible migration cases have to be handled. This would make the last two questions obsolete.

For removing SD while data is stored there we already had a good approach in another thread AFAIR.
I think this was detecting it and asking to replace or startover with a new DB on internal. This should also cover the case of the SD being formatted, etc.

@Lineflyer
Copy link
Member

@rsudev
While those ideas are good, I would prefer to keep the first step as simple as possible. Just handle DB storage, backup files and cached data.

Once this is working we can work on the details of all the other folders/files we are generating.

@mucek4
Copy link
Member

mucek4 commented May 21, 2015

Could we made backup to online serves? Gmail, dropbox, seafile, ...

@SammysHP
Copy link
Member

@rsudev
From what I've heard one problem is that the database file grows too much, so it must be possible to move it the the real SD card.

@mucek4
That's a different issue.

@Lineflyer
Copy link
Member

@SammysHP
Thats why I suggested to just combine that settings of DB and cache folder. Users need more space, they probably dont care which data is behind it

@rsudev
Copy link
Contributor

rsudev commented May 21, 2015

If we make backup target an independent location (like /sdcard/cgeo/backup) always, I am fine with whatever approach, even though I do not buy the database size argument.

@rsudev
Copy link
Contributor

rsudev commented May 21, 2015

I have to modify my last statement slightly: I would like to retain the possibility to keep the database in appstorage, regardless of any setting for the cache directory.

@Lineflyer
Copy link
Member

Let me try to summarize before this thread will get too long:

  • We seem to agree that another approach for implementation is appreciated
  • The two choices for the cache folder are OK as mentioned:
    /storage/emulated/0/.cgeo (default as it is now)
    /storage/extSdCard/Android/data/cgeo.geocaching/.cgeo (will be removed on uninstall)
  • The folder for backup/restore needs to be a separate (new) one to avoid the backup being stored in a non user accesible folder.
    This could even be implemented in an early step (right now) to have clear conditions for a future version implementing the external storage.
  • We should not put too much effort in moving. Just try it if the user whiches, otherwise just switch to the new location and maybe delete the old folder.
  • Regarding the DB storage there have been different opinions, but rethinking different scenarios (e.g. as mentioned by @SammysHP) it would be best to remove that "DB on SD" option completely (at least in the first step) and always store it at the internal system dir.
    This has the big advantage, that even if the SD is removed or the content deleted, the user will have all his lists but only loose some images, etc. The DB needs to be safe as it is mandatory to run c:geo.
    So we need to implement a one time fallback by moving the DB in case it is on "external" when starting c:geo after the upgrade to the version.
    This could even be implemented in an early step (right now) to have clear conditions for a future version implementing the external storage.
  • On startup of c:geo and if external storage is active, it should be checked whether it is available. If not, the user should be made aware of this and get the choice to close c:geo (and we keep the link to the external) or run anyway (and we fall back to the internal folder).

I hope I did not forget an important thing.

@mucek4
Copy link
Member

mucek4 commented May 22, 2015

Only think is. On my HTC the internal space is /storage/emulated/0/ and SD card is on /storage/ext_sd/

@Lineflyer
Copy link
Member

Only think is. On my HTC the internal space is /storage/emulated/0/ and SD card is on /storage/ext_sd/

Should be no problem at all:
Paths for both options are delivered by the android system as fas as I understood that.

@SammysHP
Copy link
Member

Paths for both options are delivered by the android system as fas as I understood that.

Nope.

it would be best to remove that "DB on SD" option completely

I've seen DB files with more than 200 MB and users complaining about not enough free storage space.

@rsudev
Copy link
Contributor

rsudev commented May 22, 2015

Making backup address a new and different directory on sdcard/internal as a preparation step sounds like a very good idea. Issue created: #4963

@marco-dev
Copy link
Contributor Author

We should not mix up database file with cache directory. This should go into another issue.

So one step going before this can be implemented is to separate cache directory path from db file path.

@Lineflyer
Copy link
Member

So now we started two new issue #4963 and #4964 to do necessary preparation.
I would suggest to start these activities right after next feature release (respectively after copying master to release) which is planned for hopefully next week (refer to #4953).

@marco-dev
Copy link
Contributor Author

Yes. Just as a feedback: I already prepared the current master on my local repository and downloaded all the new APIs. :-)

@marco-dev
Copy link
Contributor Author

Should we migrate the cache dir to visible "cgeo" subdir (without leading dot)?
Do we have problems to make it visible because the images will be found by gallery app?

@SammysHP
Copy link
Member

Simply put a .nomedia file in that directory, this excludes all media from the indexer.

@marco-dev
Copy link
Contributor Author

Because no one answered my question here:

Should we migrate the cache / backup / db dir to visible "cgeo" subdir (without leading dot)?

@Lineflyer
Copy link
Member

Without dot... as our target design might be, that those files stored there should be accessible by the user without a problem lateron.

@Lineflyer
Copy link
Member

I added a section about GPX import/export to the Wiki "Target structure" page.
Hope this is correct and the migration proposal will work.

@Lineflyer
Copy link
Member

@marco-dev
Maybe we should really "meet" on IRC with some people who want to help defining the whole thing.
Any timing proposal?

@SammysHP
Copy link
Member

Another idea: WebRTC

@marco-dev
Copy link
Contributor Author

At work 7:20

pstorch added a commit to pstorch/cgeo that referenced this issue Mar 11, 2017
pstorch added a commit to pstorch/cgeo that referenced this issue Mar 11, 2017
pstorch added a commit to pstorch/cgeo that referenced this issue Mar 12, 2017
pstorch added a commit to pstorch/cgeo that referenced this issue Mar 14, 2017
@Lineflyer
Copy link
Member

Lineflyer commented Mar 15, 2017

I took the time and installed the corresponding the debug build based on this PR from CI.
Before doing that I installed a debug version based on an older stable PR (http://ci.cgeo.org/job/cgeo%20pull%20request/786/) to be able to do the migration step.

Before installing the debug version based on this PR I did the following:

  • Enter login data
  • Changed setting to store log pictures and static maps offline
  • Store 35 caches distributed over two lists
  • Export a GPX file
  • Export two field notes
  • Take a log picture photo with c:geo
  • Create a database backup

Afterwards I created a backup copy of all the data in the file system I produced while doing this.
Then I installed the new debug version and started it once waiting for login to be finished.

Now I analyzed the new status of the app and files as follows:

  • Lists are still complete, so database move was successful. I do now see the DB stored in /storage/emulated/0/Android/data/cgeo.geocaching/files/databases/ while it was in non accesible folder before
  • Additional data for caches has correctly been moved from /.cgeo/* to /storage/emulated/0/Android/data/cgeo.geocaching/files/GeoacheData and access to it is working (tested by opening static maps and log picture tab for a saved cache)
  • The backup previously created and saved into /.cgeo/cgeo.sqlite has NOT been migrated. The new version tells me that no backup is available and the folder /storage/emulated/0/cgeo/backup does not even exist. The existing /.cgeo/cgeo.sqlite is no longer there. Backup is lost.
  • The exported GPX files have been migrated from the default dir /storage/emulated/0/gpx/ to storage/emulated/0/cgeo/gpx
  • The exported field notes have been migrated from the default dir /storage/emulated/0/field-notes/ to storage/emulated/0/cgeo/field-notes
  • The existing log picture in /storage/emulated/0/Pictures/cgeo has not been migrated and is still stored at its original location.

Conclusion:

Migration step Result Comment
Database PASSED Small remark: The dir could be named database instead of databases
Additional cache data PASSED
Backup FAILED Backup is lost (Edit Lineflyer: It is actaully not lost but moved to the wrong place. See posts below)
Exported GPX PASSED
Exported Field Notes PASSED
Log Pictures Taken INCONCLUSIVE Was this planned to be migrated?

Device information:
screenshot_2017-03-15-21-38-55

@Lineflyer
Copy link
Member

Lineflyer commented Mar 15, 2017

Additional remarks:

  • Should we remove our old directories (like storage/emulated/0/.cgeo, storage/emulated/0/gpx, etc. as well during migration? Right now they are emptied but kept.
  • A new folder on the real external SD /Android/data/cgeo.geocaching/files seems to be created but its empty. Empty is OK, but is it correct, that it has been created already?

@Lineflyer
Copy link
Member

Lineflyer commented Mar 15, 2017

In a next step I tried to move the GeocacheData from internal to external using the new menu option.
This have been the options shown:
screenshot_2017-03-15-21-43-15

After selecting the second option c:geo crashed.
Here is the stack trace:

21:43:34.186 Error cgeo 11198  [RxCachedThreadScheduler-7] Couldn't rename /storage/emulated/0/Android/data/cgeo.geocaching/files/GeocacheData to /storage/extSdCard/Android/data/cgeo.geocaching/files/GeocacheData
21:43:34.295 Warning cgeo 11198  [main] UncaughtException
21:43:34.295 Warning cgeo 11198  io.reactivex.exceptions.OnErrorNotImplementedException: Aborting on Log.e()
21:43:34.295 Warning cgeo 11198  	at io.reactivex.internal.functions.Functions$14.accept(Functions.java:229)
21:43:34.295 Warning cgeo 11198  	at io.reactivex.internal.functions.Functions$14.accept(Functions.java:226)
21:43:34.295 Warning cgeo 11198  	at io.reactivex.internal.observers.LambdaObserver.onError(LambdaObserver.java:72)
21:43:34.295 Warning cgeo 11198  	at io.reactivex.internal.operators.observable.ObservableTakeWhile$TakeWhileObserver.onError(ObservableTakeWhile.java:100)
21:43:34.295 Warning cgeo 11198  	at io.reactivex.internal.operators.observable.ObservableObserveOn$ObserveOnObserver.checkTerminated(ObservableObserveOn.java:276)
21:43:34.295 Warning cgeo 11198  	at io.reactivex.internal.operators.observable.ObservableObserveOn$ObserveOnObserver.drainNormal(ObservableObserveOn.java:172)
21:43:34.295 Warning cgeo 11198  	at io.reactivex.internal.operators.observable.ObservableObserveOn$ObserveOnObserver.run(ObservableObserveOn.java:252)
21:43:34.295 Warning cgeo 11198  	at io.reactivex.android.schedulers.HandlerScheduler$ScheduledRunnable.run(HandlerScheduler.java:109)
21:43:34.295 Warning cgeo 11198  	at android.os.Handler.handleCallback(Handler.java:739)
21:43:34.295 Warning cgeo 11198  	at android.os.Handler.dispatchMessage(Handler.java:95)
21:43:34.295 Warning cgeo 11198  	at android.os.Looper.loop(Looper.java:145)
21:43:34.295 Warning cgeo 11198  	at android.app.ActivityThread.main(ActivityThread.java:5951)
21:43:34.295 Warning cgeo 11198  	at java.lang.reflect.Method.invoke(Native Method)
21:43:34.295 Warning cgeo 11198  	at java.lang.reflect.Method.invoke(Method.java:372)
21:43:34.295 Warning cgeo 11198  	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1400)
21:43:34.295 Warning cgeo 11198  	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1195)
21:43:34.295 Warning cgeo 11198  Caused by: java.lang.RuntimeException: Aborting on Log.e()
21:43:34.295 Warning cgeo 11198  	at cgeo.geocaching.utils.Log.e(Log.java:98)
21:43:34.295 Warning cgeo 11198  	at cgeo.geocaching.utils.FileUtils.rename(FileUtils.java:113)
21:43:34.295 Warning cgeo 11198  	at cgeo.geocaching.utils.FileUtils.renameIfNotExists(FileUtils.java:120)
21:43:34.295 Warning cgeo 11198  	at cgeo.geocaching.utils.FileUtils.moveTo(FileUtils.java:125)
21:43:34.295 Warning cgeo 11198  	at cgeo.geocaching.storage.LocalStorage$2.call(LocalStorage.java:305)
21:43:34.295 Warning cgeo 11198  	at cgeo.geocaching.storage.LocalStorage$2.call(LocalStorage.java:301)
21:43:34.295 Warning cgeo 11198  	at io.reactivex.internal.operators.observable.ObservableDefer.subscribeActual(ObservableDefer.java:32)
21:43:34.295 Warning cgeo 11198  	at io.reactivex.Observable.subscribe(Observable.java:10700)
21:43:34.295 Warning cgeo 11198  	at io.reactivex.internal.operators.observable.ObservableSubscribeOn$1.run(ObservableSubscribeOn.java:39)
21:43:34.295 Warning cgeo 11198  	at io.reactivex.Scheduler$1.run(Scheduler.java:138)
21:43:34.295 Warning cgeo 11198  	at io.reactivex.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:59)
21:43:34.295 Warning cgeo 11198  	at io.reactivex.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:51)
21:43:34.295 Warning cgeo 11198  	at java.util.concurrent.FutureTask.run(FutureTask.java:237)
21:43:34.295 Warning cgeo 11198  	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:152)
21:43:34.295 Warning cgeo 11198  	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:265)
21:43:34.295 Warning cgeo 11198  	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
21:43:34.295 Warning cgeo 11198  	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
21:43:34.295 Warning cgeo 11198  	at java.lang.Thread.run(Thread.java:818)

After this error the second option is now selected but the data is still stored in the first listed location.
Consequences not tested yet.

@Lineflyer
Copy link
Member

[21:53] I can confirm it is still working after the failed switch:
[21:53] If I now open a cache and swipe to the picture tab it redownloads the pictures online (as they are no longer found) and correctly stores them on external SD. Thats good!

@Lineflyer
Copy link
Member

[22:04] Regarding the database: i think you mixed up something here. I try to explain:
[22:04] Regarding DB: the switch moves between /data/data/cgeo.geocaching/databases and /Android/data/cgeo.geocaching/files/databases
[22:04] data/data ?
[22:05] yes, that is the internal storage, where you don't have access without root
[22:05] Oh something is mixed up here I guess. Let me try to explain
[22:05] the default directory where the app can store its data
[22:05] ok
[22:07] I found the database now after enabling "DatabaseonSD". it seems to be on the real external SD in storage/extSDcard/Android/data/cgeo.geocaching/files/databases and there are two files named data and data-journal (which I never seen before). I assume thats the real active DB??
[22:07] Might it be that the sqlite file in /storage/emulated/0/Android/data/cgeo.geocaching/databases/ is in reality themissing backup?
[22:08] In that case database handling is perfectly fine just backup has the problem.
[22:09] active db is called "data" and backup is "cgeo.sqlite". The data.journal is some extra from sqlite.
[22:09] I'm afk for a moment
[22:09] so.... /storage/emulated/0/Android/data/cgeo.geoaching/files/databases/cgeo.sqlite is the backup!
[22:10] It should not be there as this folder is removed on uninstall. It should be in /storage/emulated/0/cgeo/backup/
[22:10] Rest seems fine then if you confirm so far.

@pstorch
Copy link
Contributor

pstorch commented Mar 15, 2017

Summary of open issues:

  • move of GeocacheData between emulated and real extSdCard not working
  • move of sqlite backup file in wrong directory, should go to public external storage: /storage/emulated/0/cgeo/backup/
  • after migration empty legacy directories should be deleted
  • GeocacheData Settings should be moved below the DBonSD Setting (Edit Lineflyer: and get an explanatory text (like the DBonSD item))

@Lineflyer Lineflyer removed the Feedback required Issue requires feedback of the author or another user label Mar 16, 2017
pstorch added a commit to pstorch/cgeo that referenced this issue Mar 16, 2017
@Lineflyer
Copy link
Member

I tested the updated PR once again with following result:

Migration step Result Comment
Database PASSED
Additional cache data PASSED
Backup PASSED
Exported GPX PASSED
Exported Field Notes PASSED

For moving the GeocacheDatafrom internal to external SD it is however still not working. This time is does not crash anymore but the existing files are not moved.
Once after having selected the external storage, it stores new GeocacheData without a problem there.

Here is the log:

21:45:56.623 Warning cgeo 17991  [RxCachedThreadScheduler-6] Couldn't rename /storage/emulated/0/Android/data/cgeo.geocaching/files/GeocacheData/shared to /storage/extSdCard/Android/data/cgeo.geocaching/files/GeocacheData/shared
21:45:56.623 Warning cgeo 17991  [RxCachedThreadScheduler-6] Couldn't rename /storage/emulated/0/Android/data/cgeo.geocaching/files/GeocacheData/GC352Y3 to /storage/extSdCard/Android/data/cgeo.geocaching/files/GeocacheData/GC352Y3
21:45:56.623 Warning cgeo 17991  [RxCachedThreadScheduler-6] Couldn't rename /storage/emulated/0/Android/data/cgeo.geocaching/files/GeocacheData/GC3EQ4W to /storage/extSdCard/Android/data/cgeo.geocaching/files/GeocacheData/GC3EQ4W
21:45:56.623 Warning cgeo 17991  [RxCachedThreadScheduler-6] Couldn't rename /storage/emulated/0/Android/data/cgeo.geocaching/files/GeocacheData/GC533KT to /storage/extSdCard/Android/data/cgeo.geocaching/files/GeocacheData/GC533KT
21:45:56.623 Warning cgeo 17991  [RxCachedThreadScheduler-6] Couldn't rename /storage/emulated/0/Android/data/cgeo.geocaching/files/GeocacheData/GC40D80 to /storage/extSdCard/Android/data/cgeo.geocaching/files/GeocacheData/GC40D80
21:45:56.623 Warning cgeo 17991  [RxCachedThreadScheduler-6] Couldn't rename /storage/emulated/0/Android/data/cgeo.geocaching/files/GeocacheData/GC3MCKK to /storage/extSdCard/Android/data/cgeo.geocaching/files/GeocacheData/GC3MCKK
21:45:56.623 Warning cgeo 17991  [RxCachedThreadScheduler-6] Couldn't rename /storage/emulated/0/Android/data/cgeo.geocaching/files/GeocacheData/GC6TPD4 to /storage/extSdCard/Android/data/cgeo.geocaching/files/GeocacheData/GC6TPD4
21:45:56.623 Warning cgeo 17991  [RxCachedThreadScheduler-6] Couldn't rename /storage/emulated/0/Android/data/cgeo.geocaching/files/GeocacheData/OC1120A to /storage/extSdCard/Android/data/cgeo.geocaching/files/GeocacheData/OC1120A
21:45:56.623 Warning cgeo 17991  [RxCachedThreadScheduler-6] Couldn't rename /storage/emulated/0/Android/data/cgeo.geocaching/files/GeocacheData/GC150HM to /storage/extSdCard/Android/data/cgeo.geocaching/files/GeocacheData/GC150HM
21:45:56.623 Warning cgeo 17991  [RxCachedThreadScheduler-6] Couldn't rename /storage/emulated/0/Android/data/cgeo.geocaching/files/GeocacheData/GC5HQ4J to /storage/extSdCard/Android/data/cgeo.geocaching/files/GeocacheData/GC5HQ4J

@Lineflyer
Copy link
Member

Lineflyer commented Mar 19, 2017

Not to forget:
Besides the move of GeocacheData the implementation looks good to me.
IMHO it could be merged after the move problem has been solved. Other minor problems can be corrected after merge.

@Lineflyer
Copy link
Member

@pstorch:
After reading https://github.com/cgeo/cgeo/wiki/Target-disk-usage-structure#database once again.
My testing results looks slightly different:

  • DB was stored internally with old version (DBonSD not active)
  • Also after upgrading to the new model I could not see it in /storage/emulated/0/Android/data/cgeo.geocaching/files/ which is OK. Nothing seems to have changed.
  • After selecting to move it to SD I could see it in /externalSD/Android/data/cgeo.geocaching/files/databases

For me this is all OK but seems different to the Wiki description.

@pstorch
Copy link
Contributor

pstorch commented Mar 20, 2017

Ok, that means that a File rename does not work across mount points. I have to change it to a real copy to the target and delete of the old.
Regarding the DB: if DBonSD is NOT active then the DB should be on the internal storage referenced by <1>. You don't have access to this directory on normal circumstances. You need to have a root Explorer or via adb USB debugging.

@pstorch
Copy link
Contributor

pstorch commented Mar 20, 2017

If you want to check the DB from internal storage via adb: https://gist.github.com/15b5d237a65e833a4c40856e2ab7dc38

@Lineflyer
Copy link
Member

Lineflyer commented Mar 20, 2017

Regarding the DB: if DBonSD is NOT active then the DB should be on the internal storage referenced by <1>. You don't have access to this directory on normal circumstances. You need to have a root Explorer or via adb USB debugging.

But in the case of DBonSD is active, the Wiki should either read <5>/databases/ instead <2>/databases (thats how you implemented it) or the DB should be on /storage/emulated/0/Android/data/cgeo.geocaching/files/databases/ instead.
We should think about what is best suitable and free of risk:
Actually if DBonSD is active in current model, the DB is in /storage/emulated/0/.cgeo (which is the simulated SD). So IMHO the less risky method should be to migrate it to /storage/emulated/0/Android/data/cgeo.geocaching/files/databases/ in this case as it is on the same mount (no problem with disk space or move procedure).

@Lineflyer
Copy link
Member

Ok, that means that a File rename does not work across mount points. I have to change it to a real copy to the target and delete of the old.

See here:
https://developer.android.com/reference/java/io/File.html#renameTo(java.io.File)

@pstorch
Copy link
Contributor

pstorch commented Mar 20, 2017

Ah, yes. Thanks for looking it up. It's like I suspected. Will change it accordingly.

pstorch added a commit to pstorch/cgeo that referenced this issue Mar 20, 2017
pstorch added a commit to pstorch/cgeo that referenced this issue Mar 20, 2017
@pstorch
Copy link
Contributor

pstorch commented Mar 20, 2017

I've now implemented a fallback to copy+delete the geocache data directories if the rename doesn't work. So we have the fast move, if it is on the same mount point and the longer copy+delete else.
The DBonSD is now kept on the first external dir, which is usually the emulated one (as described in the wiki page).

@Lineflyer
Copy link
Member

Thanks @pstorch
Will test it using the debug build tonight or tomorrow. Expect feedback accordingly :)

@pstorch
Copy link
Contributor

pstorch commented Mar 21, 2017

@Lineflyer fingers crossed ;)

@Lineflyer
Copy link
Member

Lineflyer commented Mar 21, 2017

I tested the latest status of the PR as of today with following result:

Migration step Result Comment
Database PASSED
Additional cache data PASSED
Backup PASSED
Exported GPX PASSED
Exported Field Notes PASSED

Also moving the GeocacheData from internal to external SD works now without a problem.

Moving the DB from internal to extenal also works as described in Wiki.

So from testing / usage perspective its good to merge to master to allow more intense testing (and some more test users). Can't judge the code review though.

As just discussed on IRC it might be helpful to implement some Log.i and Log.w during the migration process for more easy debugging later on.

Remaining tests like SDfull, SD removed, Huge amount of caches will be tested in a next step.
For some minor issues I found, I will open dedicated issues once it is merged.

Great work @pstorch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring Issues requires code refactoring
Projects
None yet
Development

No branches or pull requests

8 participants