-
Notifications
You must be signed in to change notification settings - Fork 405
Add DecSync synchronization (#443) #696
base: master
Are you sure you want to change the base?
Conversation
Hi, First of all sorry for the late answer. It is not a simple PR to review and I had difficulties to find times for it, I however greatly thank you for this proposal. But I currently have two main issues with that PR: 1/ I cannot accept the targetApi reduced to 29. I understand why you did that, however one day Google will force to use 30 or more (they are currently doing it for 29) 2/ The DecSync code is deeply integrated to existing classes (like the DAOs and Service). The added complexity is quite important (like the new insert/update methods of the DAOs) and I actually still didn't understand all of it. I'm not sure it is easily doable but I would have preferred the DecSync code to be almost totally separated from original files. Moreover, to be perfectly honest, I even wonder if the added complexity worth it: I didn't know DecSync before and while I understand its goal I don't know if it's really popular or not. There is clearly a demand for feed synchronisation, but DecSync is not super simple to setup I don't know if a lot of people will actually use it. Hope you understand, and maybe you can reassure me. |
Thank you for your reply! I understand that it can take some time to review it. 1/ Indeed, the targetApi cannot be reduced indefinitely. It can be reset to 30 whenever it is necessary from Google or for some other reason. However, it would be preferable to have it on 29 for some time, so I have some time to migrate the format. But if you don't want that, staying on 30 is also acceptable. 2/ This PR is indeed a bit complex, and after I made the PR I gave it some thought and wasn't satisfied with the complexity. I looked a bit on how I could simplify the implementation, and I think it can be done by using Observers on the data, like the UI is already doing. I will update the PR with this solution when ready. It should make the DecSync code mostly separated from the original. Furthermore, I don't think DecSync is that hard to setup. I think most people already synchronize files between their devices, which means that they only have to create a folder inside it. And if you already use it for something else, you don't even have to do that. But I do see that it doesn't use a standard account where you log in. |
Use the DecsyncObserver introduced in libdecsync v1.8.0
I have updated the PR with the simplifications I mentioned in the previous comment. I also reset the targetApi back to 30, as all operations are executed in the background. The initial sync can still be slow for a large collection on SAF, but it is only on Android 11, and the issue will be mitigated with a new DecSync version. Is the PR with these changes understandable and acceptable? |
I cannot speak for all Flym users, but I personally would absolutely love to see DecSync implemented! This feature is the only one that keeps me on spaRSS-DecSync, and sadly spaRSS is not actively maintained anymore. So thank you so much for your work @39aldo39 and it would be amazing if you could back the implementation @FredJul ! |
This pull requests adds DecSync synchronization. I actually didn't plan to have it ready in one pull request, but it wasn't a lot of work after a first draft. After all, I already implemented it in spaRSS-DecSync, based on spaRSS, which is based on Flym v1.
Some notes:
requestLegacyExternalStorage
and reduced the targetApi to 29. This allows normal file access for more Android versions. DecSync works fine for both normal files and SAF, but SAF is significantly slower. I do have a draft of DecSync v2 which improves the situation, but using level 29 gives more time to finalize and distribute it. Moreover, upgrading should work without user interaction when ready.Please let me know what you think!