FredJul / Flym

Flym News Reader is a light Android feed reader (RSS/Atom)
Other
953 stars 403 forks source link

Add DecSync synchronization (#443) #696

Open 39aldo39 opened 4 years ago

39aldo39 commented 4 years ago

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:

Please let me know what you think!

FredJul commented 3 years ago

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.

39aldo39 commented 3 years ago

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.

39aldo39 commented 3 years ago

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?

Anym0s commented 3 years ago

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.

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. Moreover, I disagree with DecSync being complicated to set up. IMHO it is the easiest solution to sync feeds without the need for a server. The functionality leaves the choice of how to synchronise the file to the user, and as such the feature is one that can be used in a variety of ways (with syncthing, with nextcloud...).

So thank you so much for your work @39aldo39 and it would be amazing if you could back the implementation @FredJul !