-
Notifications
You must be signed in to change notification settings - Fork 276
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
Refactor account storage and camera upload #444
Refactor account storage and camera upload #444
Conversation
@forouher I have to say it is an elegant way to implement the camera uploads by CameraSyncService&SyncAdapter. The gallery albums feature is awesome, the implementation of subdirectories-of-repositories targets users requirements, the code style is neat and clean, our customized utility methods were accurately used and even avoid several potential bugs, but there are some issues to talk with you.
better to remove that
why not create a separate patch? Please notice that the Account migration patch is at a low priority according to our agenda, anyway I still being excited to welcome your PRs.
Although the UI keeps the same, the transfer list doesn`t show any uploading (or downloading) progress any more. Always show "No upload tasks yet"
That`s the way to make people understand your code even faster, 👍
I will get this done for you after it was merged, just go ahead to other implementations.
Big welcome! 😸 |
@Logan676 thanks for your feedback!
Separating these two patches would require a lot of work. SyncProvider /depends/ on having an Android Authenticator. I could separate them, however the code would probably look quite ugly. In the end it would be a different patch entirely. I agree that the Account Authenticator change is not an important patch, as it neither fixes a bug nor adds a user-visible feature. However from a code perspective, I'd strongly suggest to do this refactoring:
So, unless there is a fundamental problem with Account Authenticator refactoring (and I'm happy to address any other issues that come up), I'd argue for merging these two patches at the same time. If you consider this PR as too big and are worried about potential complications, an alternative would be to merge ec94702 first. And then fdc36f6 at a later date.
Good point, I didn't notice that. Hm... To add entries to the transfer list, I'd have to use the TransferManager. I wanted to avoid that as it would be one more point of failure (creating the service, what to do if it dies,...). But yeah, having entries in the transfer list would be a good thing. I'll look into it.
In what situations would my approach (filter images by "date_added") fail? I like this approach because it is simple (less code) and because it avoids the issue presented in #301.
So, if the user selects "Server S" / "Repo R" / "Folder A".
Correct? No problem, I'll amend a patch. |
Before I started the service by calling
Right, the uploaded photo structure is correct. |
At least it fails to recognize those already uploaded photos. From users I think it |
@forouher Although I have applied gradle build structure to master branch, please just ignore this changes with you pull request because it compares with |
These patches should address the issues discussed above, especially:
Also I've fixed some issues that came up during testing in issue #423 and the one mentioned in issue #451. If there is anything else, please let me know! |
/** | ||
* Type of the account (currently there is only one type) | ||
*/ | ||
public final static String ACCOUNT_TYPE = "com.seafile.seadroid2"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it`s better to append more strings, e.g. "com.seafile.seadroid2.account.type"
@forouher I tested this patch via seacloud.cc, and there are seveal issues listed as below.
But it succeed when I switched to another server, and it works as expected.
|
// there is already a file. move it away. | ||
dataManager.rename(targetRepoId, | ||
Utils.pathJoin(serverPath, dir.name), | ||
dir.name + " (renamed by Seadroid)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to use renamed by Seafile
and move it to strings.xml for the sake of i18n
if you take a new photo by camera, it always fail to upload. |
I'll create an account on seacloud.cc and try to reproduce the issues you observed. Regarding your comment:
That was by design, so users can place photos into subdirectories (as suggested in #311). Most of the code to allow selecting subdirectories was already there, I just fixed it up. In case you don't like that functionality, I could easily disable it again. Regarding your comment:
So, camera upload should disable itself if the user switches to another account via the AccountsActivity? I don't like that. I use 2+ Seafile servers and sometimes switch between them in Seadroid. If camera upload disabled itself every time I switched the account, that would be a real nuisance. |
Please disable that, we don`t plan to support that feature. Since we created "My Photos" under the root of a repo, the #311 will not be fixed. |
Alright, will do. Btw, I could not reproduce the issue you reported regarding seacloud.cc. I tested it yesterday and today. Upload and file viewing work fine. Maybe it was just a network error? |
agree and we will keep that. About the issue
I can upload successfully now.
I will look into it. |
These patches should address all the issues you've noticed up to now. Please be aware that merging commit bc5a2df will remove your existing accounts on your development phone as that patch renames the account type. |
} | ||
public void performFullSync() { | ||
Bundle b = new Bundle(); | ||
b.putBoolean(ContentResolver.SYNC_EXTRAS_INITIALIZE, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If changing the account type will remove existing accounts, that`s not what I meant to. Because users will feel really bad after upgrade to the new version. Another worry is users have to start a refresh upload since we have rewritten the camera upload module and created a completely different remote folder "My Photos", I wonder if they have enough patience and kindness to accept that. To decrease the effects as much as possible, we would better to revert the modifications on account type part and sorry for the comment. |
No, sorry, you missunderstood. Its just YOU (as you've allready using this PR) will loose the accounts. :) Am 26. Dezember 2015 10:34:02 MEZ, schrieb Logan Guo notifications@github.com:
|
Regarding the "My Photos" change, I could add some backwards compability. Currently its Christmas here, I'll write up a patch in a couple of days. Dariush Am 26. Dezember 2015 10:34:02 MEZ, schrieb Logan Guo notifications@github.com:
|
public void onCreate() { | ||
synchronized (sSyncAdapterLock) { | ||
if (sSyncAdapter == null) { | ||
sSyncAdapter = new CameraSyncAdapter(getApplicationContext(), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not passing true
to initialize the sync adapter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. We need to do initialization manually (clearing the photo database). I'll add a comment to explain this.
…pload targets this again simplifies the code a bit
as suggested by Logan676
0844ab7
to
ffb92e2
Compare
ffb92e2
to
6c9f6e9
Compare
…tible Make the structure to be: - Server S / Repo R - "Camera Uploads" - Media from bucket "Camera" (backwards compatibility) - "Album X" - Media from bucket "Album X" - "Album Y" - Media from bucket "Album Y" this way, existing photos uploaded by previous versions of Seadroid won't be uploaded again as they will be placed in the same directory as before.
Okay, I've implemented your suggestions and did the rebase. I've added a patch (a21f844) that changes the directory structure on the server to the following:
You were worried that changing the directory layout on the server might give discomfort to existing users. This directory layout might make the upgrade more transparent for users. The idea is to automatically rename the bucket "Camera" to "Camera Uploads". This should preserve backwards compatibility for existing users of the Camera Upload feature. In case you prefer the previously implemented approach I could easily revert it. |
I tried looking into this PR, but I have trouble building it. It does not contain a build.gradle and the ant build freaks out on my system. It seems that this PR is based on develop instead of master. |
@schwabe Ah yes, the build system in this PR is still based on mvn. Hang on, I'll build and upload an apk for you to test. |
@forouher Thanks! I resigned and installed that file on my phone. My Pictures are getting reuploaded in a different folder (100Android) but that could by design since with the stock seadroid version (with my crude hack) the pictures are not uploaded at all. |
@schwabe Thats good to hear! Ah, that folder name was not intended. Thanks for pointing that out, will amend a fix. |
the previous commit tried to put all camera-related media into the folder "Camera Uploads". However it missed that there are actually three possible bucket names for camera-related media. This patch addresses that.
@schwabe What is the name of the folder on the server? Is it exactly "100Android" ? |
@forouher No it is 100ANDRO to be precise. Same as on the Android device. (/storage/sdcard1/DCIM/100ANDRO) |
@schwabe Ah, good. That bucket name will be covered by the heuristics (Android has some beautiful APIs. Media storage isn't one of them ;) |
We have discussed it, and we prefer the previous one because it is kind of mess to put so many folders under the root directory of the repo, so please revert it. |
Refactor account storage and camera upload
@forouher Is it possible to trigger "instant sync" in order that user can see the uploading progress in Transfer list? Users may not realized that they can go to Settings -> Account -> Seafile -> Sync to trigger instant sync and we want to make that more obvious after the sync service was started. Now it works fine if user browsing under the target repo and dir, folders can be created and photo can be uploaded instantly. But if you go to Transfer list, mostly you need to wait for a while till the progressing list shows up. Same thing happens when taking new photos. Any ideas? To implement the feature as discussed above, can we simply change the flag to be
in |
You want to add a button that allows the user to force an immediate manual camera sync? I've never tried SYNC_EXTRAS_EXPEDITED. From the documentation, yes, it should schedule an immediate sync. But we should not add that flag to all the existing onPerformSync() calls. Android adds a delay precisely to save battery (e.g. it tries to group pending syncs from multiple apps, wait for good 3G signal, whatever). For such a "Sync Now" button, we could add a new method to CameraUploadManager:
That method could be called by the main thread. |
Another important point to be aware is that calling CameraUploadManager.perform*Sync() will currently not overwrite the data plan restrictions. |
[I'm not going to have much time over the next days, so I'm only writing up ideas here.] Also it might be possible to query the timestamp of the latest succesful sync from the SyncAdapter. Displaying that timestamp in Settings (or the Transfer list) might give the user a better understanding of how the camera upload works. |
we will drop that then. |
This PR refactors camera upload and the account storage. To be more precise, it does two things:
The patches have more detailed explanations in their individual commit messages.
They should hopefully address the following issues:
issue #277 (Camera upload function dumps all subdir files into single dir)
issue #301 (Inconsistency in camera upload sync)
issue #311 (second suggestion: target repo sub directories)
issue #336 (file duplication when doing a full resync)
issue #375 (Picture upload not acting as service?)
issue #423 (Pictures never upload) [maybe. not sure what's going on there]
Some further notes:
While this PR adds a feature or two, the main reason for its existence is to bring more stability to camera upload mechanism. So, to avoid replacing 5 old bugs with 10 new ones, this PR will need some thorough testing. Especially on
[EDIT: I realized maybe I should write a couple of lines on the user-visible changes of this PR. Here it goes:]
Accounts
Camera upload
Migration
I'll be happy to address any upcoming bugs, issues and questions.