Merge pull request #2178 from juntuu/reload-fixes

Reload fixes and improvements

Fixes #2065
Fixes #2067

10 files changed, 97 insertions, 161 deletions

diff --git a/doc/internal/hackers-guide.asciidoc b/doc/internal/hackers-guide.asciidoc
index 1d93d37f..535c938e 100644
--- a/doc/internal/hackers-guide.asciidoc
+++ b/doc/internal/hackers-guide.asciidoc
@@ -48,9 +48,6 @@ is the connector between view and cache. It parses the command line option,
 controls the configuration parser and hands over data to the view. It also
 contains code to control the reloading of feeds.
 
-*class downloadthread*: a thread that does nothing but start a reload operation
-for all feeds. Derived from thread.
-
 *class htmlrenderer*: takes a string and renders the HTML it contains into a
 textual representation. It also extracts URLs from a and img tag for later use.
 
@@ -63,8 +60,8 @@ Logging can be enabled by developers (see below).
 
 *class mutex*: a C++ wrapper around the pthread mutex.
 
-*class reloadthread*: similar to downloadthread, but starts a reload every n
-minutes (configurable).
+*class reloadthread*: a thread that does nothing but starts a reload every n
+minutes (configurable) for all feeds.
 
 *class rss_feed*: represents an RSS feed, including RSS url, page link, title,
 author, description and RSS items (articles). Uses the cache to persist itself.
diff --git a/include/downloadthread.h b/include/downloadthread.h
deleted file mode 100644
index 07b52215..00000000
--- a/include/downloadthread.h
+++ /dev/null
@@ -1,23 +0,0 @@
-#ifndef NEWSBOAT_DOWNLOADTHREAD_H_
-#define NEWSBOAT_DOWNLOADTHREAD_H_
-
-#include <thread>
-
-#include "reloader.h"
-
-namespace newsboat {
-
-class DownloadThread {
-public:
-	DownloadThread(Reloader& r, const std::vector<int>& idxs = {});
-	virtual ~DownloadThread();
-	void operator()();
-
-private:
-	Reloader& reloader;
-	std::vector<int> indexes;
-};
-
-} // namespace newsboat
-
-#endif /* NEWSBOAT_DOWNLOADTHREAD_H_ */
diff --git a/include/reloader.h b/include/reloader.h
index 6b11366d..40e2894e 100644
--- a/include/reloader.h
+++ b/include/reloader.h
@@ -1,6 +1,8 @@
 #ifndef NEWSBOAT_RELOADER_H_
 #define NEWSBOAT_RELOADER_H_
 
+#include <atomic>
+#include <functional>
 #include <mutex>
 #include <vector>
 
@@ -26,29 +28,14 @@ public:
 	/// If \a indexes is empty, all feeds will be reloaded.
 	void start_reload_all_thread(const std::vector<int>& indexes = {});
 
-	void unlock_reload_mutex()
-	{
-		reload_mutex.unlock();
-	}
-	bool trylock_reload_mutex();
-
 	/// \brief Reloads given feed.
 	///
 	/// Reloads the feed at position \a pos in the feeds list (as kept by
-	/// feedscontainer). \a show_progress specifies if a progress indicator
-	/// (`[<pos>/<total_feeds>]`) should be included when updating the status
-	/// message (at the bottom of the screen). Status messages are only shown
-	/// if \a unattended is false. All network requests are made through
-	/// \a easyhandle, unless it is a nullptr, in which case this method creates
-	/// a temporary handle which is destroyed before returning from it.
-	void reload(unsigned int pos,
-		bool show_progress = false,
-		bool unattended = false);
-
-	void reload(unsigned int pos,
-		CurlHandle& easyhandle,
-		bool show_progress = false,
-		bool unattended = false);
+	/// feedscontainer). Only updates status bar if \a unattended is false.
+	void reload(unsigned int pos, bool unattended = false)
+	{
+		reload(pos, false, unattended);
+	}
 
 	/// \brief Reloads all feeds, spawning threads as necessary.
 	///
@@ -72,6 +59,24 @@ public:
 		bool unattended = false);
 
 private:
+	/// \brief Reloads given feed.
+	///
+	/// Reloads the feed at position \a pos in the feeds list (as kept by
+	/// feedscontainer). \a show_progress specifies if a progress indicator
+	/// (`[<progress>/<total_feeds>]`) should be included when updating the status
+	/// message (at the bottom of the screen). Status messages are only shown
+	/// if \a unattended is false. All network requests are made through
+	/// \a easyhandle. If the handle is not provided, this method creates
+	/// a temporary handle which is destroyed before returning from it.
+	void reload(unsigned int pos,
+		bool show_progress,
+		bool unattended);
+
+	void reload(unsigned int pos,
+		CurlHandle& easyhandle,
+		bool show_progress,
+		bool unattended);
+
 	/// \brief Notify in various ways that there are new unread feeds or
 	/// articles.
 	///
@@ -86,10 +91,22 @@ private:
 	void notify_reload_finished(unsigned int unread_feeds_before,
 		unsigned int unread_articles_before);
 
+	void unlock_reload_mutex()
+	{
+		reload_mutex.unlock();
+	}
+	bool trylock_reload_mutex();
+
+	void partition_reload_to_threads(
+		std::function<void(unsigned int start, unsigned int end)> handle_range,
+		unsigned int num_feeds);
+
 	Controller* ctrl;
 	Cache* rsscache;
 	ConfigContainer* cfg;
 	std::mutex reload_mutex;
+	std::atomic<unsigned int> reload_progress;
+	unsigned int reload_progress_max;
 };
 
 } // namespace newsboat
diff --git a/include/reloadrangethread.h b/include/reloadrangethread.h
deleted file mode 100644
index ff313c6f..00000000
--- a/include/reloadrangethread.h
+++ /dev/null
@@ -1,27 +0,0 @@
-#ifndef NEWSBOAT_RELOADRANGETHREAD_H_
-#define NEWSBOAT_RELOADRANGETHREAD_H_
-
-#include <thread>
-
-#include "reloader.h"
-
-namespace newsboat {
-
-class ReloadRangeThread {
-public:
-	ReloadRangeThread(Reloader& r,
-		unsigned int start,
-		unsigned int end,
-		bool unattended);
-	void operator()();
-
-private:
-	Reloader& reloader;
-	unsigned int start;
-	unsigned int end;
-	bool unattended;
-};
-
-} // namespace newsboat
-
-#endif /* NEWSBOAT_RELOADRANGETHREAD_H_ */
diff --git a/mk/newsboat.deps b/mk/newsboat.deps
index 4b0f4f95..b02673ab 100644
--- a/mk/newsboat.deps
+++ b/mk/newsboat.deps
@@ -1 +1 @@
-newsboat.cpp src/cache.cpp  src/htmlrenderer.cpp src/urlreader.cpp src/view.cpp src/controller.cpp src/reloadthread.cpp src/tagsouppullparser.cpp src/downloadthread.cpp src/rssignores.cpp src/rssparser.cpp src/formaction.cpp src/listformaction.cpp src/feedlistformaction.cpp src/itemlistformaction.cpp src/itemviewformaction.cpp src/helpformaction.cpp src/dirbrowserformaction.cpp src/filebrowserformaction.cpp src/urlviewformaction.cpp src/selectformaction.cpp src/history.cpp src/filtercontainer.cpp src/listformatter.cpp src/regexmanager.cpp src/dialogsformaction.cpp src/ttrssapi.cpp src/ttrssurlreader.cpp src/newsblurapi.cpp src/newsblururlreader.cpp src/oldreaderurlreader.cpp src/oldreaderapi.cpp src/feedcontainer.cpp src/feedhqapi.cpp src/feedhqurlreader.cpp src/freshrssapi.cpp src/freshrssurlreader.cpp src/textformatter.cpp src/ocnewsapi.cpp src/ocnewsurlreader.cpp src/remoteapi.cpp src/inoreaderapi.cpp src/inoreaderurlreader.cpp src/cliargsparser.cpp src/configpaths.cpp src/reloader.cpp src/reloadrangethread.cpp src/opml.cpp src/fileurlreader.cpp src/opmlurlreader.cpp src/itemrenderer.cpp src/queuemanager.cpp src/rssitem.cpp src/rssfeed.cpp src/listwidget.cpp src/textviewwidget.cpp src/regexowner.cpp src/configactionhandler.cpp src/minifluxapi.cpp src/minifluxurlreader.cpp src/emptyformaction.cpp src/statusline.cpp src/file_system.cpp src/searchresultslistformaction.cpp src/itemutils.cpp
+newsboat.cpp src/cache.cpp  src/htmlrenderer.cpp src/urlreader.cpp src/view.cpp src/controller.cpp src/reloadthread.cpp src/tagsouppullparser.cpp src/rssignores.cpp src/rssparser.cpp src/formaction.cpp src/listformaction.cpp src/feedlistformaction.cpp src/itemlistformaction.cpp src/itemviewformaction.cpp src/helpformaction.cpp src/dirbrowserformaction.cpp src/filebrowserformaction.cpp src/urlviewformaction.cpp src/selectformaction.cpp src/history.cpp src/filtercontainer.cpp src/listformatter.cpp src/regexmanager.cpp src/dialogsformaction.cpp src/ttrssapi.cpp src/ttrssurlreader.cpp src/newsblurapi.cpp src/newsblururlreader.cpp src/oldreaderurlreader.cpp src/oldreaderapi.cpp src/feedcontainer.cpp src/feedhqapi.cpp src/feedhqurlreader.cpp src/freshrssapi.cpp src/freshrssurlreader.cpp src/textformatter.cpp src/ocnewsapi.cpp src/ocnewsurlreader.cpp src/remoteapi.cpp src/inoreaderapi.cpp src/inoreaderurlreader.cpp src/cliargsparser.cpp src/configpaths.cpp src/reloader.cpp src/opml.cpp src/fileurlreader.cpp src/opmlurlreader.cpp src/itemrenderer.cpp src/queuemanager.cpp src/rssitem.cpp src/rssfeed.cpp src/listwidget.cpp src/textviewwidget.cpp src/regexowner.cpp src/configactionhandler.cpp src/minifluxapi.cpp src/minifluxurlreader.cpp src/emptyformaction.cpp src/statusline.cpp src/file_system.cpp src/searchresultslistformaction.cpp src/itemutils.cpp
diff --git a/src/controller.cpp b/src/controller.cpp
index ad48e194..f957cc3f 100644
--- a/src/controller.cpp
+++ b/src/controller.cpp
@@ -31,7 +31,6 @@
 #include "configexception.h"
 #include "configpaths.h"
 #include "dbexception.h"
-#include "downloadthread.h"
 #include "exception.h"
 #include "feedhqapi.h"
 #include "feedhqurlreader.h"
diff --git a/src/downloadthread.cpp b/src/downloadthread.cpp
deleted file mode 100644
index c187d6a6..00000000
--- a/src/downloadthread.cpp
+++ /dev/null
@@ -1,32 +0,0 @@
-#include "downloadthread.h"
-
-#include "logger.h"
-
-namespace newsboat {
-
-DownloadThread::DownloadThread(Reloader& r, const std::vector<int>& idxs)
-	: reloader(r), indexes(idxs) {}
-
-DownloadThread::~DownloadThread() {}
-
-void DownloadThread::operator()()
-{
-	/*
-	 * the DownloadThread class drives the reload-all process.
-	 * A DownloadThread is spawned whenever "reload all" is invoked, and
-	 * whenever an auto-reload comes up.
-	 */
-	LOG(Level::DEBUG,
-		"DownloadThread::run: inside DownloadThread, reloading all "
-		"feeds...");
-	if (reloader.trylock_reload_mutex()) {
-		if (indexes.size() == 0) {
-			reloader.reload_all();
-		} else {
-			reloader.reload_indexes(indexes);
-		}
-		reloader.unlock_reload_mutex();
-	}
-}
-
-} // namespace newsboat
diff --git a/src/feedlistformaction.cpp b/src/feedlistformaction.cpp
index 626fb4e3..de5e6765 100644
--- a/src/feedlistformaction.cpp
+++ b/src/feedlistformaction.cpp
@@ -287,6 +287,11 @@ REDO:
 					"reload-only-visible-feeds");
 			std::vector<int> idxs;
 			if (reload_only_visible_feeds) {
+				if (visible_feeds.empty()) {
+					// Do not reload *all* feeds, when
+					// there is no visible feeds at all.
+					break;
+				}
 				for (const auto& feed : visible_feeds) {
 					idxs.push_back(feed.second);
 				}
diff --git a/src/reloader.cpp b/src/reloader.cpp
index f657fbc1..71b8c0ac 100644
--- a/src/reloader.cpp
+++ b/src/reloader.cpp
@@ -9,10 +9,8 @@
 #include "controller.h"
 #include "curlhandle.h"
 #include "dbexception.h"
-#include "downloadthread.h"
 #include "fmtstrformatter.h"
 #include "matcherexception.h"
-#include "reloadrangethread.h"
 #include "reloadthread.h"
 #include "rss/exception.h"
 #include "rssfeed.h"
@@ -39,7 +37,19 @@ void Reloader::spawn_reloadthread()
 void Reloader::start_reload_all_thread(const std::vector<int>& indexes)
 {
 	LOG(Level::INFO, "starting reload all thread");
-	std::thread t(DownloadThread(*this, indexes));
+	std::thread t([=]() {
+		LOG(Level::DEBUG,
+			"Reloader::start_reload_all_thread: inside thread, reloading all "
+			"feeds...");
+		if (trylock_reload_mutex()) {
+			if (indexes.empty()) {
+				reload_all();
+			} else {
+				reload_indexes(indexes);
+			}
+			unlock_reload_mutex();
+		}
+	});
 	t.detach();
 }
 
@@ -79,9 +89,8 @@ void Reloader::reload(unsigned int pos,
 		std::string errmsg;
 		std::shared_ptr<AutoDiscardMessage> message_lifetime;
 		if (!unattended) {
-			const auto max = ctrl->get_feedcontainer()->feeds_size();
 			const std::string progress = show_progress ?
-				strprintf::fmt("(%u/%u) ", pos + 1, max) :
+				strprintf::fmt("(%u/%u) ", ++reload_progress, reload_progress_max) :
 				"";
 			message_lifetime = ctrl->get_view()->get_statusline().show_message_until_finished(
 					strprintf::fmt(_("%sLoading %s..."),
@@ -139,50 +148,60 @@ void Reloader::reload(unsigned int pos,
 	}
 }
 
-void Reloader::reload_all(bool unattended)
+void Reloader::partition_reload_to_threads(
+	std::function<void(unsigned int start, unsigned int end)> handle_range,
+	unsigned int num_feeds)
 {
-	ScopeMeasure sm("Reloader::reload_all");
-
-	const auto unread_feeds =
-		ctrl->get_feedcontainer()->unread_feed_count();
-	const auto unread_articles =
-		ctrl->get_feedcontainer()->unread_item_count();
 	int num_threads = cfg->get_configvalue_as_int("reload-threads");
-
-	ctrl->get_feedcontainer()->reset_feeds_status();
-	const auto num_feeds = ctrl->get_feedcontainer()->feeds_size();
-
 	// TODO: change to std::clamp in C++17
 	const int min_threads = 1;
 	const int max_threads = num_feeds;
 	num_threads = std::max(min_threads, std::min(num_threads, max_threads));
 
-	LOG(Level::DEBUG, "Reloader::reload_all: starting with reload all...");
+	LOG(Level::DEBUG, "Reloader::partition_reload_to_threads: starting with reload...");
+	reload_progress = 0;
+	reload_progress_max = num_feeds;
 	if (num_threads == 1) {
-		reload_range(0, num_feeds - 1, unattended);
+		handle_range(0, num_feeds - 1);
 	} else {
 		std::vector<std::pair<unsigned int, unsigned int>> partitions =
 				utils::partition_indexes(0, num_feeds - 1, num_threads);
 		std::vector<std::thread> threads;
 		LOG(Level::DEBUG,
-			"Reloader::reload_all: starting reload threads...");
+			"Reloader::partition_reload_to_threads: starting reload threads...");
 		for (int i = 0; i < num_threads - 1; i++) {
-			threads.push_back(std::thread(ReloadRangeThread(*this,
-						partitions[i].first,
-						partitions[i].second,
-						unattended)));
+			auto range = partitions[i];
+			threads.emplace_back([=]() {
+				handle_range(range.first, range.second);
+			});
 		}
 		LOG(Level::DEBUG,
-			"Reloader::reload_all: starting my own reload...");
-		reload_range(partitions[num_threads - 1].first,
-			partitions[num_threads - 1].second,
-			unattended);
+			"Reloader::partition_reload_to_threads: starting my own reload...");
+		handle_range(partitions[num_threads - 1].first,
+			partitions[num_threads - 1].second);
 		LOG(Level::DEBUG,
-			"Reloader::reload_all: joining other threads...");
+			"Reloader::partition_reload_to_threads: joining other threads...");
 		for (size_t i = 0; i < threads.size(); i++) {
 			threads[i].join();
 		}
 	}
+}
+
+void Reloader::reload_all(bool unattended)
+{
+	ScopeMeasure sm("Reloader::reload_all");
+
+	const auto unread_feeds =
+		ctrl->get_feedcontainer()->unread_feed_count();
+	const auto unread_articles =
+		ctrl->get_feedcontainer()->unread_item_count();
+
+	ctrl->get_feedcontainer()->reset_feeds_status();
+	const auto num_feeds = ctrl->get_feedcontainer()->feeds_size();
+
+	partition_reload_to_threads([=](unsigned int start, unsigned int end) {
+		reload_range(start, end, unattended);
+	}, num_feeds);
 
 	// refresh query feeds (update and sort)
 	LOG(Level::DEBUG, "Reloader::reload_all: refresh query feeds");
@@ -212,9 +231,11 @@ void Reloader::reload_indexes(const std::vector<int>& indexes, bool unattended)
 	const auto unread_articles =
 		ctrl->get_feedcontainer()->unread_item_count();
 
-	for (const auto& idx : indexes) {
-		reload(idx, true, unattended);
-	}
+	partition_reload_to_threads([&](unsigned int start, unsigned int end) {
+		for (auto i = start; i <= end; ++i) {
+			reload(indexes[i], true, unattended);
+		}
+	}, indexes.size());
 
 	notify_reload_finished(unread_feeds, unread_articles);
 }
diff --git a/src/reloadrangethread.cpp b/src/reloadrangethread.cpp
deleted file mode 100644
index de9e6449..00000000
--- a/src/reloadrangethread.cpp
+++ /dev/null
@@ -1,21 +0,0 @@
-#include "reloadrangethread.h"
-
-namespace newsboat {
-
-ReloadRangeThread::ReloadRangeThread(Reloader& r,
-	unsigned int s,
-	unsigned int e,
-	bool u)
-	: reloader(r)
-	, start(s)
-	, end(e)
-	, unattended(u)
-{
-}
-
-void ReloadRangeThread::operator()()
-{
-	reloader.reload_range(start, end, unattended);
-}
-
-} // namespace newsboat