fixes #4957 new approach for local storage #6396

pstorch · 2017-03-11T20:59:27Z

Maybe it's time to get some first feedback.

It's implementing the https://github.com/cgeo/cgeo/wiki/Target-disk-usage-structure

❗ If you want to test it, be careful: it moves your files around. ❗

mention-bot · 2017-03-11T20:59:30Z

@pstorch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @samueltardieu, @Bananeweizen and @marco-dev to be potential reviewers.

pstorch · 2017-03-12T21:27:52Z

Looks like codacy has some problems with checking this PR. It doesn't show up here and on codacy.com it checks and checks and checks ...

pstorch · 2017-03-14T21:13:49Z

Codacy is fixed.

pstorch · 2017-03-16T21:25:29Z

Fixed open Issues from #4957 (comment).

Lineflyer · 2017-03-17T08:09:55Z

Will test it again this evening or tomorrow

Lineflyer · 2017-03-17T08:51:20Z

That is strange:
I went the same way I used for the first test:
Installed an older debug.apk retrieved from CI, then wanted to update to this debug.apk. But installation fails, as the signature is different. Do we use different signatures on CI?

kumy · 2017-03-17T11:26:04Z

If I m not mistaken, release/nigthly/legacy are build on master node using "main" keys. PR are built on sam1/2, keys _may_ differ here.

…

On March 17, 2017 9:51:20 AM GMT+01:00, Lars ***@***.***> wrote: That is strange: I went the same way I used for the first test: Installed an older debug.apk retrieved from CI, then wanted to update to [this](http://ci.cgeo.org/job/cgeo%20pull%20request/801/) debug.apk. But installation fails, as the signature is different. Do we use different signatures on CI? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #6396 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

Lineflyer · 2017-03-19T19:45:22Z

retest this please

pstorch · 2017-03-20T22:07:43Z

retest this please

Bananeweizen · 2017-03-21T17:58:34Z

main/src/cgeo/geocaching/settings/Settings.java

+                            (HtmlImage.SHARED.equals(name) || LocalStorage.GEOCACHE_FILE_PATTERN.matcher(name).find());
+                }
+            };
+            final File[] list = LocalStorage.getLegacyExternalCgeoDirectory().listFiles(geocacheDirectories);


listFiles can return null. That should not happen, but me may want to be safe.

On the other hand, if getLegacyExternalCgeoDirectory() is supposed to be a directory, then we might want to crash if it isn't :)

As described below, I think I should catch null here to be more fault tolerant.

Bananeweizen · 2017-03-21T18:02:54Z

main/src/cgeo/geocaching/utils/FileUtils.java

+        final String urlExt;
+        if (url.startsWith("data:")) {
+            // "data:image/png;base64,i53…" -> ".png"
+            urlExt = StringUtils.substringAfter(StringUtils.substringBefore(url, ";"), "/");


StringUtils.substringBetween(...)?

You have to handle null if you use substringBetween (the current construct returns an empty string) if it doesn't match. I'm not sure the subsequent call to defaultString will get better performances, but it may indeed may more readable as

StringUtils.defaultString(StringUtils.substringBetween("/", ";"));

I haven't thought about this line, because I just moved it here from somewhere else ;)

Yeah, I recognized this code, but I couldn't tell why I chose after+before instead of between+default.

Bananeweizen · 2017-03-21T18:07:08Z

I've just been wondering about the fault tolerance of that migration. E.g. if one of the operation fails, should we still continue with some of the other? That is, should we try-catch several blocks in the migration code? And how much time will this take, if a user has 5000 caches on disk? I believe we don't show any UI about this, so he might decide to stop the application for any reason. Could that lead to issues?

pstorch · 2017-03-21T19:57:03Z

Regarding fault tolerance: at the moment I'm more in favor of being tolerant and don't stop. The migration and the moving of the Geocache data directory can go wrong easily. The source directory might be missing, some target dirs might already be there. What about no disk space is left? Currently I try to do what is possible and skip over errors, so the user doesn't get stuck. Some cached data might not be copied, but it remains on the other disk and is recoverable. We have to deal with partly migrations anyway, because a filesystem doesn't work with transactions.
This is just my view on this, let's discuss.

The copy of the Geocache data has a UI progress dialog. I don't know how long copying of 5000 caches would take. Unfortunately I don't have a device with a real extSdCard. Maybe I should still order one for testing. Or @Lineflyer might be able to test such a volume.

samueltardieu · 2017-03-21T20:20:11Z

main/src/cgeo/geocaching/models/Geocache.java

-     * Add spoilers stored locally in <tt>/sdcard/GeocachePhotos</tt>. If a cache is named GC123ABC, the
-     * directory will be <tt>/sdcard/GeocachePhotos/C/B/GC123ABC/</tt>.
+     * Add spoilers stored locally in <tt>/sdcard/cgeo/GeocachePhotos</tt>. If a cache is named GC123ABC, the
+     * directory will be <tt>/sdcard/cgeo/GeocachePhotos/C/B/GC123ABC/</tt>.


I think the choice of not using cgeo in the path name was because additional data imported from geocaching software could be used by several applications, and did not belong to cgeo.

I tried to reread all threads on the subject, and it looks like this has no importance after all, so feel free to rename.

As far as I could find it, it's from a GSAK macro. I also found the corresponding issues and commits here: #4957 (comment)

When I look into the GSAK macro it is actually Garmin\GeocachePhotos. So the user of this macro need to copy the subfolder anyhow. I don't know if this directory might be used by some other Apps in parallel. Should we keep it one level up?

Probably not. As you say, it has to be moved through a file manager or similar anyway, so your proposed change is fine.

Lineflyer

Good to go from testing/Ui perspective

pstorch added the Do not merge / WIP Not for merging and/or work still in progress label Mar 11, 2017

pstorch force-pushed the 4957_new_local_storage branch from f36b6a0 to 05643b3 Compare March 12, 2017 19:40

pstorch force-pushed the 4957_new_local_storage branch from 05643b3 to 68a5045 Compare March 14, 2017 20:50

pstorch force-pushed the 4957_new_local_storage branch from 68a5045 to 63ce7c7 Compare March 16, 2017 21:10

Lineflyer self-assigned this Mar 17, 2017

Lineflyer added the Field test Issue to be (re)tested in the field label Mar 17, 2017

pstorch force-pushed the 4957_new_local_storage branch 2 times, most recently from 887bfa2 to c2ec89a Compare March 20, 2017 21:42

samueltardieu mentioned this pull request Mar 21, 2017

Remove redundant copy operation, use IOUtils instead #6421

Closed

Lineflyer self-requested a review March 21, 2017 16:13

Bananeweizen reviewed Mar 21, 2017

View reviewed changes

samueltardieu reviewed Mar 21, 2017

View reviewed changes

fixes cgeo#4957 new approach for local storage

cecbe0e

pstorch force-pushed the 4957_new_local_storage branch from c2ec89a to cecbe0e Compare March 21, 2017 20:56

pstorch removed the Do not merge / WIP Not for merging and/or work still in progress label Mar 21, 2017

samueltardieu merged commit 0a0b6a8 into cgeo:master Mar 21, 2017

Lineflyer removed their assignment Mar 21, 2017

Lineflyer removed the Field test Issue to be (re)tested in the field label Mar 21, 2017

Lineflyer reviewed Mar 21, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #4957 new approach for local storage #6396

fixes #4957 new approach for local storage #6396

pstorch commented Mar 11, 2017

mention-bot commented Mar 11, 2017

pstorch commented Mar 12, 2017

pstorch commented Mar 14, 2017

pstorch commented Mar 16, 2017

Lineflyer commented Mar 17, 2017

Lineflyer commented Mar 17, 2017

kumy commented Mar 17, 2017 via email

Lineflyer commented Mar 19, 2017

pstorch commented Mar 20, 2017

Bananeweizen Mar 21, 2017

samueltardieu Mar 21, 2017

pstorch Mar 21, 2017

Bananeweizen Mar 21, 2017

samueltardieu Mar 21, 2017

pstorch Mar 21, 2017

samueltardieu Mar 21, 2017

Bananeweizen commented Mar 21, 2017

pstorch commented Mar 21, 2017

samueltardieu Mar 21, 2017

samueltardieu Mar 21, 2017

pstorch Mar 21, 2017

samueltardieu Mar 21, 2017

Lineflyer left a comment

fixes #4957 new approach for local storage #6396

fixes #4957 new approach for local storage #6396

Conversation

pstorch commented Mar 11, 2017

mention-bot commented Mar 11, 2017

pstorch commented Mar 12, 2017

pstorch commented Mar 14, 2017

pstorch commented Mar 16, 2017

Lineflyer commented Mar 17, 2017

Lineflyer commented Mar 17, 2017

kumy commented Mar 17, 2017 via email

Lineflyer commented Mar 19, 2017

pstorch commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bananeweizen commented Mar 21, 2017

pstorch commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lineflyer left a comment

Choose a reason for hiding this comment