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

Rewrite auto upload #615

Merged
merged 138 commits into from
Mar 3, 2017
Merged

Rewrite auto upload #615

merged 138 commits into from
Mar 3, 2017

Conversation

mario
Copy link
Contributor

@mario mario commented Feb 2, 2017

Due to design flaws of FileObserver on Google side, and lacking implementations on the side of various vendors, the approach Auto Upload was using would never work - at least not in the way we were using it.

Unfortunately, some vendors decided to hide certain inotify events, and you could also never know when the file was finished writing to (CLOSE_WRITE isn't a 100% bulletproof solution).

To remedy the situation, I have rewritten a significant part of Auto Upload, and in the process made it available to Android 5+. In the future, I expect to adapt it to 4+ as well so nobody has to live with pain that is Instant Upload.

As such, this should fix 95% of our troubles with uploads we've had in the past.

I have tested this to the extent possible, and it seems to work quite good.

I appreciate any and all comments, reviews and constructive criticism on both the code and the functionality.

@mario
Copy link
Contributor Author

mario commented Feb 2, 2017

As for Codacy, my intention is to leave those method bodies empty.

return false;
}

return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be replaced with return !pathname.getName().startsWith(".")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This I can do :)

* Magical file alteration listener
*/

public class FileAlterationMagicListener implements FileAlterationListener {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully it is no magic ;-)
As you need a different name as the Implemantion, I suggest something with SyncedFolder or AutoUpload

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where's the fun in renaming it? :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second the request to rermove the "Magic" in the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shrug Will be done in 5.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mario
Copy link
Contributor Author

mario commented Feb 5, 2017

This needs a bit more magic, will be done next week.

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Feb 6, 2017

Just a little Feedback for you @mario : https://help.nextcloud.com/t/nc-beta-not-uploading-photos-videos/5010/15 --> You fixed Xiaomi Mi5 , miui version 8 (android 7.0) 🎉

@mario
Copy link
Contributor Author

mario commented Feb 6, 2017

@AndyScherzinger can you try this PR out for HDR images on Nexus please?

@AndyScherzinger
Copy link
Member

I'll test it after lunch!

@niki77
Copy link

niki77 commented Feb 6, 2017

@mario , on mi Xiaomi mi5 with miui 8 (android 7), HDR photo was automatically uploaded.

@AndyScherzinger
Copy link
Member

@mario tested the latest code base and it now catches both on Nexus5X (running 7.1.2 Beta): "burst mode" and HDR processing, took 7 pictures with/without HDR hitting the shutter button and you caught all of them 🚀
cc: @tobiasKaminsky

@mario
Copy link
Contributor Author

mario commented Feb 6, 2017

@AndyScherzinger thank you. I have isolated a few more bugs and am working on fixing them.

@mario
Copy link
Contributor Author

mario commented Feb 7, 2017

Just for my reference, this PR (so far) fixes/adds:

  • rewrites file detection, and in process fixes uploads (even HDR/Andy-burst)
  • adds support for Auto Upload on Android 5 and 6
  • prevents duplicate entries in synced folders provider
  • prevents hidden DOT files from uploading
  • shows only Uploads for the currently active account in the Uploads view
  • fixes Uploads screen crash
  • cleanly separates and fixes Auto Upload settings when using multiple account (this didn’t work :-/)
  • hopefully fixes auto upload spawning multiple entries of the same file
  • fixes uploads with multiple accounts
  • fixes uploads for non-configured folders

Todo:

  • investigate claims of uploads for non-configured folders
  • cleanup duplicate entries in synced folders provider for people who upgrade
  • fix uploads with multiple accounts
  • test, test, test

@mario
Copy link
Contributor Author

mario commented Feb 7, 2017

@AndyScherzinger if you can test again, I'd appreciate it!

@mario
Copy link
Contributor Author

mario commented Feb 7, 2017

Okay, maybe wait still until beta tomorrow xD

@AndyScherzinger
Copy link
Member

I don't run the beta at the moment, so I would install the branch. Anymore changes to be expected for this branch today?

@mario
Copy link
Contributor Author

mario commented Feb 7, 2017

Yes, there'll be changes to make upload work again xD Thanks for your patience.

@AndyScherzinger
Copy link
Member

build is broken!

@mario
Copy link
Contributor Author

mario commented Feb 7, 2017

@AndyScherzinger everyone, panic! :D

@mario
Copy link
Contributor Author

mario commented Feb 7, 2017

@AndyScherzinger ready for testing!

@AndyScherzinger AndyScherzinger removed the request for review from przybylski March 3, 2017 13:24
@jasonbayton
Copy link
Member

Oh so the checkmark is intentional?

I guess it would be - shows the file is downloaded, which is technically is residing in the app folder as if it had.. good should.

@tobiasKaminsky are you alive? :)

@AndyScherzinger
Copy link
Member

Oh so the checkmark is intentional?

Yes it is :)

@tobiasKaminsky
Copy link
Member

tobiasKaminsky commented Mar 3, 2017

👍
Let's 🎉

Approved with PullApprove

@tobiasKaminsky tobiasKaminsky dismissed their stale review March 3, 2017 15:49

evereything fine now

@mario mario merged commit f0bbb4b into master Mar 3, 2017
@mario mario deleted the rewrite-auto-upload branch March 3, 2017 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants