Feature uploadsubfolders by Spacefish · Pull Request #35 · nextcloud/android

Spacefish · 2016-06-19T15:29:19Z

My InstantUpload folder is pretty crowded with thousands of pictures.. so i programmed a feature, which upload the pictures into subfolders based on year / month.
My folder got too so big, such that opening it takes quite a lot of time..

tobiasKaminsky · 2016-06-19T16:07:39Z

 							android:title="@string/prefs_instant_upload_path_title"
 							android:key="instant_upload_path" />
+		<com.owncloud.android.ui.CheckBoxPreferenceWithLongTitle
+							android:title="@string/prefs_instant_upload_path_use_subfolders_title"


Please only lines with 80 chars, this makes review on github a lot easier.

@tobiasKaminsky 120 is fine with me ;) Since this is still enough for the files changed tab :D So I would vote to go for 120 line length. @przybylski

Longer line would be great I would vote to stick to how android code is written, which is 100 lines.
https://source.android.com/source/code-style.html#limit-line-length

@tobiasKaminsky what do you think? I am with @przybylski on this, so I also vote for 100 💯

100 is also fine for me. I do not know who said 80 back on owncloud, but if 100 is still visible in changes tab on github everything is fine.

I think I introduced 80 chars limit. I don't recall why :D

I think Github would also work with 120, not sure though...

tobiasKaminsky · 2016-06-19T16:16:26Z

Thanks for this feature!
Can you implement this also for video? Maybe it is not your use case, but it is inconsistent to have it only for images. But I think it should be two independent settings.

Spacefish · 2016-06-19T21:56:10Z

implemented your feedback. It now works for videos as well.
I am a littlebit unsure if "use subfolders" is the correct name for the prefence setting.. Maybe we should use "Organize uploads into folders based on date" or something like this?
I am not an native english speaker, so i am unsure if this sounds correct.

AndyScherzinger · 2016-06-19T21:57:38Z

pinging @jancborchardt for input regarding the mechanism and UX. I am unsure if and how this should work. I do like the idea of having more than simply one hard coded folder to where all my photos get uploaded but I am also not sure how to solve this since would use hierarchies as in folders for the year with subfolders in them. Since it is an optional setting I don't care too much about arguing about if and how to do this. Besides that I usually group my photos based on events like a vacation within the corresponding year's folder like "2016/05-Rome" which is not something we can implement since this is something for Google Photos server farm...

AndyScherzinger · 2016-06-19T22:02:07Z

@jancborchardt In general my question here is probably how to handle and deal with additional app settings being introduced. #notAnExpert

Spacefish · 2016-06-20T14:00:55Z

@AndyScherzinger i thought about this too. But opted to hardcode the path. My initial thought was to offer placeholders, so the user is able to use his own subfolder scheme based on different variables. However the current folder selections isn´t done by editing a string, and furthermore it might get to complicated to know all that placeholders / write a correct string.
If someone wants another scheme, we could open a popup with multiple schemes to choose from, once the user clicks on the preference. However as a initial version i opted for one single scheme.

I don´t have a lot of experience in android app development, this is probably the 3rd time i looked into an android app / changed something, so i didn´t want to make my life to complicated ;)

tobiasKaminsky · 2016-06-20T15:19:52Z

@AndyScherzinger this has been discussed on owncloud, ref: owncloud/android#1421

I think it is a good approach. If we see that another mechanism for subfolder is needed we can extend this setting.
@jancborchardt ok for you?

AndyScherzinger · 2016-06-20T15:41:18Z

Perfectly fine with me :)

przybylski · 2016-06-20T16:15:00Z

    <string name="error__upload__local_file_not_copied">%1$s could not be copied to %2$s local folder</string>
    <string name="prefs_instant_upload_path_title">Upload path</string>
+    <string name="prefs_instant_upload_path_use_subfolders_title">Use subfolders</string>
+    <string name="prefs_instant_upload_path_use_subfolders_summary">Store in subfolders based on year and month</string>


year & month: plural or singular ?

both, as year and month refers to a single subfolder, but multiple subfolders are created.

AndyScherzinger · 2016-06-21T19:29:07Z

Hmm, I would rather go for test and merge on master and put that into the Google Play beta and after two weeks release to stable. This is totally aside from the "real" beta you publish on f-droid.

tobiasKaminsky · 2016-06-22T19:28:20Z

@przybylski is the code now fine for you?

przybylski · 2016-06-22T19:39:23Z

+        }
+        try {
+            SimpleDateFormat formatter = new SimpleDateFormat(
+                    "yyyy" + OCFile.PATH_SEPARATOR + "MM" + OCFile.PATH_SEPARATOR, Locale.ENGLISH);


IMO it should be Locale.getDefault()

jancborchardt · 2016-06-23T11:23:45Z

Just a question regarding the first iteration – is this similar to how Dropbox and Google Drive or other solutions do it, so we don’t do something entirely different which people will not be used to?

AndyScherzinger · 2016-06-23T11:57:13Z

@jancborchardt they don't offer this at all because they don't have to due to a smart clustering. The UI is taking care of displaying clusters of photos as in time, location, people, etc.
So they have a smart, analyzing server side and an (unknown) nice API to access the photo buckets based on these criteria. Nextcloud is a storage solution so we live and breathe folder structures as the only available element to provide structures so it makes sense to offer this but you will probably never see it in the other, mentioned apps.

jancborchardt · 2016-06-23T12:11:38Z

they don't offer this at all because they don't have to due to a smart clustering. The UI is taking care of displaying clusters of photos as in time, location, people, etc.

Right, like iOS does as well in photos. We could do this as well instead of imposing a hard structure via folders, right?

AndyScherzinger · 2016-06-23T12:18:04Z

Theoretically, yes. This would then mean that the client (web, Android, etc.) would need to be implement such a view the tricky part is we have an one-for-all approach, we don't just do media we do any kind of line, so it is hard to decide when to cluster (images) and when not to.
Even then "having a folder per month" might still be legit simply because people might want to have this folder structure, but I can't say because I do grouping topic based most of the time and I create a folder per year and change the instant upload folder, so I also kind of use this feature, but atm by hand.

Spacefish · 2016-06-23T13:59:36Z

A indexing solution on the server with an API would be nice. However this is far off IMHO and not related to the android app currently, server infrastructure has to be in place beforehand.
I would image the server to do automatic tagging of photos, based on GPS location and "time based clusters". Such that photos from one trip, end up in the same cluster.
To be performant, one could use elastic search and such to make this tags / metainformation easily queriable / searchable. Putting the pictures in subfolders would not hurt this concept at all.

jancborchardt · 2016-06-23T14:36:24Z

the tricky part is we have an one-for-all approach, we don't just do media we do any kind of line, so it is hard to decide when to cluster (images) and when not to.

I’d argue that this is what we’re here to solve though. :) cc @LukasReschke @schiessle for the server.

@Spacefish the difficult part about such a feature is that when it’s in, it’s hard to remove. As soon as we have a structure, it will likely need to be in forever. And especially here I’m very hesitant to include it since it adds a lot of complexity and levels for when you just want to have a few photos synced.

jancborchardt · 2016-06-23T14:37:14Z

A giant drawback of this as well is that when you have a vacation which is on the end+beginning of a month or year (such as new year festivities), the pictures will be in completely different folders. That makes no sense at all. ;)

Spacefish · 2016-06-23T15:14:03Z

Yes, there are pro and cons, the solution to put pictures in subfolders based on month, is the same that shotwell choose to order their imported pictures by default.
We can continue bikeshedding ( http://bikeshed.com/ ) or merge the feature. The time required to talk about things like whether we should should pass date/time to a helper, use System.CurrentTime directly, whether a comment is to obvious, or the max length of lines, required was much higher than the actual development time.
I tried to solve a problem i had, 10k pictures in one folder, which broke the WebUI aswell as the App.
Please either merge it or close the PR.

jancborchardt · 2016-06-23T15:22:35Z

We can continue bikeshedding or merge the feature. The time required to talk about things […] was much higher than the actual development time.

Sorry about that, we really value your contribution. But this is not a simple bikeshedding discussion – while it might help your specific case, it’s something which will actually impair the experience for people who just have a few pictures.

You might have also read recently »Rule #1 of open-source: no is temporary, yes is forever.« Meaning that if we »just merge the feature« right now, then it will be incredibly hard to remove it in the future should we reconsider. Discussion time vs development time is never an argument for any sensible software. ;)

With Nextcloud we really want to provide a product which is very easy and fun to use. That often means fussing over details, and discussing stuff. Since this feature wasn’t discussed or specced prior to this implementation, we of course have to weigh it now.

jancborchardt · 2016-06-23T15:24:15Z

(Btw @Spacefish I just saw you are based in Stuttgart – you should meet up with @karlitschek and @schiessle some time as they’re there too! Probably time to get a Stuttgart Nextcloud meetup going. ;)

tobiasKaminsky · 2016-06-23T16:55:49Z

As it is an optional feature I think it is not such a problem.
It as in enhancement and an opportunity for the user and not something they are forced to...

A giant drawback of this as well is that when you have a vacation which is on the end+beginning of a month or year (such as new year festivities), the pictures will be in completely different folders.

This is a very rarely scenario and on the opposite site I could say: what if the folder gets > 10k images and is not managable by the user. This feature is, at least for me, a slightly enhancement and is not that strict as the user still can move the files around, like @AndyScherzinger and me are doing it: regular photos in year/month and special images are moved to a meaningful subfolder.

BTW: As this is the second PR (after #4) that is heavily discussed after developing we should really keep this schema: feature request/idea -> discuss -> implement -> testing -> merging. Of course it can happen that a requirement changes, but the features should be so small that it happens rarely.

So long speech, I would merge this, but by default the behaviour remains the same as now.
And on the long term we can try to enhance this by an intelligent server-side sorting system.
(As said before this is nothing that the app should care about)

AndyScherzinger · 2016-06-25T13:33:44Z

I second @tobiasKaminsky's vote here. The standard way is still the standard behavior and in this case I don't see any problems since this feature isn't a one way street and could be removed whenever we come up and ship a smarter processing and wouldn't run into (technical) problems.

AndyScherzinger · 2016-06-25T16:30:44Z

@jancborchardt we would like to get some final feedback / vote because this is the last open PR for the beta release 🚀

LukasReschke · 2016-06-26T16:15:17Z

Seconding @AndyScherzinger and @tobiasKaminsky here. LGTM.

jancborchardt · 2016-06-27T08:43:36Z

Ok, let’s try this out. Can’t be that bad since it’s optional and apparently lots of people want it. Not going to be against that then ;) 👍 Wording is fine too.

AndyScherzinger · 2016-06-27T08:46:56Z

Thanks for the feedback @jancborchardt

@LukasReschke next up we will prepare the master branch for a Play store release for the beta channel :) @tobiasKaminsky and I will prepare everything for you to simply build and deploy :)

LukasReschke · 2016-06-27T08:50:08Z

@LukasReschke next up we will prepare the master branch for a Play store release for the beta channel :) @tobiasKaminsky and I will prepare everything for you to simply build and deploy :)

🚀

jancborchardt · 2016-06-27T10:01:54Z

@Spacefish @AndyScherzinger @tobiasKaminsky sorry about the longer discussion btw – while the basic points regarding design of course stand, I must have missed that this is an optional subsetting. :)

AndyScherzinger · 2016-06-27T12:16:51Z

No worries, thanks for the feedback. All new features are now on master and we are preparing the beta release :)

Spacefish added 2 commits June 19, 2016 16:43

introduce upload subfolders configuration variable

f68b53b

make InstantUploader honor the SubFolder setting

bd8ab10

tobiasKaminsky reviewed Jun 19, 2016
View reviewed changes

Spacefish added 2 commits June 19, 2016 23:25

use long for timestamp and don´t fail if timestamp is empty

89ae5bf

implement the same for videos and do some code cleanup

3c388ad

Spacefish added enhancement 3. to review labels Jun 19, 2016

przybylski reviewed Jun 20, 2016
View reviewed changes

Spacefish added 4 commits June 20, 2016 23:05

tidy up code based on feedback

e231caa

introduce upload subfolders configuration variable

23e1071

make InstantUploader honor the SubFolder setting

7f02bb0

use long for timestamp and don´t fail if timestamp is empty

f85cf5d

tobiasKaminsky added this to the Nextcloud App 1.1.0 milestone Jun 21, 2016

przybylski reviewed Jun 22, 2016
View reviewed changes

AndyScherzinger merged commit c875bba into master Jun 27, 2016

AndyScherzinger deleted the feature_uploadsubfolders branch June 27, 2016 08:45

AndyScherzinger added 4. to release and removed 3. to review labels Jun 27, 2016

AndyScherzinger removed the 4. to release label Aug 11, 2017

Villard mentioned this pull request Mar 17, 2018

Android Client problem with external storage #2366

Closed

Uh oh!

Uh oh!

Conversation

Spacefish commented Jun 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasKaminsky commented Jun 19, 2016

Uh oh!

Spacefish commented Jun 19, 2016

Uh oh!

AndyScherzinger commented Jun 19, 2016

Uh oh!

AndyScherzinger commented Jun 19, 2016

Uh oh!

Spacefish commented Jun 20, 2016

Uh oh!

tobiasKaminsky commented Jun 20, 2016

Uh oh!

AndyScherzinger commented Jun 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyScherzinger commented Jun 21, 2016

Uh oh!

tobiasKaminsky commented Jun 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jancborchardt commented Jun 23, 2016

Uh oh!

AndyScherzinger commented Jun 23, 2016

Uh oh!

jancborchardt commented Jun 23, 2016

Uh oh!

AndyScherzinger commented Jun 23, 2016

Uh oh!

Spacefish commented Jun 23, 2016

Uh oh!

jancborchardt commented Jun 23, 2016

Uh oh!

jancborchardt commented Jun 23, 2016

Uh oh!

Spacefish commented Jun 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jancborchardt commented Jun 23, 2016

Uh oh!

jancborchardt commented Jun 23, 2016

Uh oh!

tobiasKaminsky commented Jun 23, 2016

Uh oh!

AndyScherzinger commented Jun 25, 2016

Uh oh!

AndyScherzinger commented Jun 25, 2016

Uh oh!

LukasReschke commented Jun 26, 2016

Uh oh!

jancborchardt commented Jun 27, 2016

Uh oh!

AndyScherzinger commented Jun 27, 2016

Uh oh!

LukasReschke commented Jun 27, 2016

Uh oh!

jancborchardt commented Jun 27, 2016

Uh oh!

Spacefish commented Jun 23, 2016 •

edited

Loading