Add DecSync plugin #804

39aldo39 · 2018-11-27T12:31:22Z

This pull request adds a DecSync plugin for synchronization using DecSync. It is based on the local plugin, with some syncing added.

It also adds the possibility for plugins to always use the articleIDs when articles are read, instead of the setFeedRead, setCategoryRead and markAllItemsRead methods. This is needed since DecSync only uses the articleIDs. Moreover, it is not possible to get the IDs from the database, since it can be updated before the plugin method is called. This also may be more correct for other plugins, since there can be new articles between the last sync and the read action, especially when cached actions are used.

Another point as discussed here is that this duplicates some dependencies from the local plugin. Probably the best way is to move them out of the plugin directories.

brendanlong

Looks good for the most part! I think the biggest change I want is to not duplicate all of the libraries. I can make that change if you want, although it may be easier for you to do it (there would be a lot of merge conflicts if I do it).

I'm curious what @jangernert would think of removing the option to set an entire category/feed read at the plugin layer and just always setting articles read individually. I think that's basically what most plugins do anyway, and it might make the logic simpler (fewer plugins will need DB access).

src/Backend/Backend.vala

brendanlong · 2018-11-28T16:50:10Z

plugins/backend/demo/demoInterface.vala

+	public bool alwaysSetReadByID()
+	{
+
+	}


I think I'm convinced that we should just remove setFeedRead/setCategoryRead/markAllItemsRead and make this new behavior the only option. I'm guessing it would be slower in some cases (if the backend API doesn't support setting a list of articles to read at once), but that's something we should solve with async or threaded requests.

brendanlong · 2018-11-28T16:51:33Z

plugins/backend/decsync/rss-glib/rss-version.h

+        (RSS_MAJOR_VERSION == (major) && RSS_MINOR_VERSION == (minor) && \
+         RSS_MICRO_VERSION >= (micro)))
+
+#endif /* __RSS_VERSION_H__ */


Can you move all of these local dependencies up so they're shared between this plugin and the local plugin? The vala dependencies can just go in the top-level src folder and I think the C libraries can go in the top-level libraries directory.

(I'm really worried that if we copy these, they'll get out of sync with each other)

brendanlong · 2018-11-28T16:54:34Z

plugins/backend/decsync/decsyncUtils.vala

+
+	public Feed? downloadFeed(Soup.Session session, string xmlURL, string feedID, Gee.List<string> catIDs, out string errmsg)
+	{
+		errmsg = "";


In general, you should prefer to throw exceptions instead of setting error parameters. This makes the code simpler and also makes it harder to misuse the API.

I think the use of dirty practices like these throughout the codebase is still a relic of the time when we had 2 processes talking via dbus =/

brendanlong · 2018-11-28T16:55:17Z

plugins/backend/decsync/decsyncUtils.vala

+		if (msg == null)
+		{
+			errmsg = @"Couldn't parse feed URL: $xmlURL";
+			Logger.warning(errmsg);


Generally if we're returning or throwing an error message, the caller should print it (to avoid duplicate logging).

brendanlong · 2018-11-28T17:03:47Z

plugins/backend/decsync/decsyncListeners.vala

+			return;
+		}
+		var cat = new Category(catID, catID, 0, 99, CategoryID.MASTER.to_string(), 1);
+		var list = new Gee.LinkedList<Category>();


You can use ListUtils.single(cat) to generate a list with one element as efficiently as possible.

(Also, LinkedLists are almost always worse than ArrayLists)

brendanlong · 2018-11-28T17:09:14Z

plugins/backend/decsync/decsyncInterface.vala

+
+	public string uncategorizedID()
+	{
+		return "0";


Should this return CategoryID.MASTER.to_string()?

brendanlong · 2018-11-28T17:11:53Z

plugins/backend/decsync/decsyncInterface.vala

+			tagID = (int.parse(m_db.getMaxID("tags", "tagID")) + 1).to_string();
+
+		Logger.info("createTag: ID = " + tagID);
+		return tagID;


I'm confused about this function since supportTags() is false. This function should probably either return an empty string or throw an exception.

(At some point I want to redo how plugins work so these functions don't even exist unless the plugin advertises supporting them, but for now just not having any logic in them is good enough)

brendanlong · 2018-11-28T17:13:17Z

plugins/backend/decsync/decsyncInterface.vala

+		{
+			if(!m_db.feed_exists(Feed.getURL())) {
+				var list = new Gee.ArrayList<Feed>();
+				list.add(Feed);


Consider using ListUtils.single(feed)

brendanlong · 2018-11-28T17:14:13Z

plugins/backend/decsync/decsyncInterface.vala

+		feedID = feedURL;
+
+		Logger.info(@"addFeed: ID = $feedID");
+		Feed? Feed = m_utils.downloadFeed(m_session, feedURL, feedID, catIDs, out errmsg);


Variables should be lowercase:

Feed? feed = m_utils.downloadFeed(m_session, feedURL, feedID, catIDs, out errmsg);

brendanlong · 2018-11-28T17:22:17Z

This seems to be causing a build failure in CircleCI due to a missing meson.build file:

https://circleci.com/gh/jangernert/FeedReader/632

Also, is any of this easily testable? I'm trying to get more pieces of FeedReader unit-tested, although I admit I don't know how to do async tests yet. If you think any of it can be tested, you can see an example in the Feedbin plugin (TestFeedbin.vala and the last section of the feedbin plugin's meson.build).

39aldo39 · 2018-11-29T15:50:27Z

I have implemented most of requested changes. Only the function from localUtils.downloadFeed still uses error messages. Some changes applied to the local backend as well. It is a bit ugly that some code is almost the same as the local backend. But I don't think it is easy to deduplicate it, since there are still some small changes.

The CircleCI build fails since libdecsync is a submodule, which probably isn't cloned.

I do think this plugin is testable, as long writing/reading files is possible. We can then test whether the changes are correctly written/executed.

brendanlong · 2018-11-29T17:29:37Z

It is a bit ugly that some code is almost the same as the local backend. But I don't think it is easy to deduplicate it, since there are still some small changes.

Yeah that's fine. The plugins probably shouldn't depend on each other, I just didn't want to libraries they use to be duplicated too.

The CircleCI build fails since libdecsync is a submodule, which probably isn't cloned.

I just created #809 to fix this. If you rebase once that lands on master it should fix it.

I'll review the updated code after work today.

brendanlong · 2018-11-29T17:30:34Z

plugins/backend/local/localInterface.vala

-			var list = new Gee.ArrayList<Category>();
-			list.add(cat);
-			m_db_write.write_categories(list);
+			m_db_write.write_categories(ListUtils.single(cat));


Thanks for fixing these too! I didn't realize this was existing code you had copied.

brendanlong · 2018-12-03T23:44:44Z

I just got a chance to test this and it seems to work. Thanks for creating this!

brendanlong · 2018-12-03T23:49:30Z

I just triggered a v2.6.0 release so this should show up in Flathub soon.

brendanlong suggested changes Nov 28, 2018

View reviewed changes

brendanlong reviewed Nov 29, 2018

View reviewed changes

39aldo39 added 6 commits November 29, 2018 18:33

Add alwaysSetReadByID option to plugins

4eb2972

Add DecSync plugin

420fdcd

Use db.read_category instead of removed db.category_exists

d60546d

Small improvements

8102acd

Deduplicate shared libraries between local and decsync backends

35e61e4

Small improvements in local/decsync backend

fd8c55e

39aldo39 force-pushed the decsync branch from ea3c305 to fd8c55e Compare November 29, 2018 17:34

brendanlong approved these changes Dec 3, 2018

View reviewed changes

brendanlong merged commit fd8c55e into jangernert:master Dec 3, 2018

39aldo39 deleted the decsync branch December 26, 2018 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DecSync plugin #804

Add DecSync plugin #804

39aldo39 commented Nov 27, 2018

brendanlong left a comment

brendanlong Nov 28, 2018

brendanlong Nov 28, 2018

brendanlong Nov 28, 2018

jangernert Nov 28, 2018

brendanlong Nov 28, 2018

brendanlong Nov 28, 2018

brendanlong Nov 28, 2018

brendanlong Nov 28, 2018

brendanlong Nov 28, 2018

brendanlong Nov 28, 2018

brendanlong commented Nov 28, 2018

39aldo39 commented Nov 29, 2018

brendanlong commented Nov 29, 2018

brendanlong Nov 29, 2018

brendanlong commented Dec 3, 2018

brendanlong commented Dec 3, 2018

Add DecSync plugin #804

Add DecSync plugin #804

Conversation

39aldo39 commented Nov 27, 2018

brendanlong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendanlong commented Nov 28, 2018

39aldo39 commented Nov 29, 2018

brendanlong commented Nov 29, 2018

Choose a reason for hiding this comment

brendanlong commented Dec 3, 2018

brendanlong commented Dec 3, 2018