From ff884302277a3554ec73b40d594c35df3e0fc0b6 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Tue, 27 Feb 2024 19:19:45 +0400 Subject: [PATCH 1/4] Documented the problem with startDownload() --- include/downloader.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/include/downloader.h b/include/downloader.h index f385823c9..897d3eb92 100644 --- a/include/downloader.h +++ b/include/downloader.h @@ -177,7 +177,16 @@ class Downloader /** * Start a new download. * - * This method is thread safe and return a pointer to a newly created `Download`. + * This method is thread safe and returns a pointer to a newly created + * `Download` or an existing one with a matching URI. In the latter case + * the options parameter is ignored, which can lead to surprising results. + * For example, if the old and new download requests (sharing the same URI) + * have different values for the download directory or output file name + * options, after the download is reported to be complete the downloaded file + * will be present only at the location specified for the first request. + * Also, due to the above peculiarity there is no straightforward way to + * repeat a completed or cancelled download whose files have been deleted. + * * User should call `update` on the returned `Download` to have an accurate status. * * @param uri: The uri of the thing to download. From 4ab62150464a5adb9432e444228016e1b8f320af Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Tue, 27 Feb 2024 19:09:27 +0400 Subject: [PATCH 2/4] Downloader::Options typedef --- include/downloader.h | 9 ++++++--- src/downloader.cpp | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/include/downloader.h b/include/downloader.h index 897d3eb92..93a57603b 100644 --- a/include/downloader.h +++ b/include/downloader.h @@ -168,7 +168,10 @@ class Download { */ class Downloader { - public: + public: // types + typedef std::vector> Options; + + public: // functions Downloader(); virtual ~Downloader(); @@ -193,7 +196,7 @@ class Downloader * @param options: A series of pair to pass to aria. * @return: The newly created Download. */ - std::shared_ptr startDownload(const std::string& uri, const std::vector>& options = {}); + std::shared_ptr startDownload(const std::string& uri, const Options& options = {}); /** * Get a download corrsponding to a download id (did) @@ -215,7 +218,7 @@ class Downloader */ std::vector getDownloadIds() const; - private: + private: // data mutable std::mutex m_lock; std::map> m_knownDownloads; std::shared_ptr mp_aria; diff --git a/src/downloader.cpp b/src/downloader.cpp index d874ad899..d5a8adf7e 100644 --- a/src/downloader.cpp +++ b/src/downloader.cpp @@ -166,7 +166,7 @@ std::vector Downloader::getDownloadIds() const { return ret; } -std::shared_ptr Downloader::startDownload(const std::string& uri, const std::vector>& options) +std::shared_ptr Downloader::startDownload(const std::string& uri, const Options& options) { std::unique_lock lock(m_lock); for (auto& p: m_knownDownloads) { From 9fe81e9bce76786130a9e6bae470f7488aac1fd1 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Tue, 27 Feb 2024 19:13:08 +0400 Subject: [PATCH 3/4] Introduced downloadCanBeReused() helper The dubious logic of when an existing download can be reused by Downloader::startDownload() is preserved. --- src/downloader.cpp | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/downloader.cpp b/src/downloader.cpp index d5a8adf7e..44de4d40d 100644 --- a/src/downloader.cpp +++ b/src/downloader.cpp @@ -166,13 +166,27 @@ std::vector Downloader::getDownloadIds() const { return ret; } +namespace +{ + +bool downloadCanBeReused(const Download& d, + const std::string& uri, + const Downloader::Options& /*options*/) +{ + const auto& uris = d.getUris(); + const bool sameURI = std::find(uris.begin(), uris.end(), uri) != uris.end(); + + return sameURI; +} + +} // unnamed namespace + std::shared_ptr Downloader::startDownload(const std::string& uri, const Options& options) { std::unique_lock lock(m_lock); for (auto& p: m_knownDownloads) { auto& d = p.second; - auto& uris = d->getUris(); - if (std::find(uris.begin(), uris.end(), uri) != uris.end()) + if ( downloadCanBeReused(*d, uri, options) ) return d; } std::vector uris = {uri}; From 3733e506c11f65a3079a530449ff6ebbcdee0184 Mon Sep 17 00:00:00 2001 From: Veloman Yunkan Date: Tue, 27 Feb 2024 19:51:12 +0400 Subject: [PATCH 4/4] More reasonable criteria for reusing a download --- include/downloader.h | 2 -- src/downloader.cpp | 21 ++++++++++++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/include/downloader.h b/include/downloader.h index 93a57603b..4cc7b7730 100644 --- a/include/downloader.h +++ b/include/downloader.h @@ -187,8 +187,6 @@ class Downloader * have different values for the download directory or output file name * options, after the download is reported to be complete the downloaded file * will be present only at the location specified for the first request. - * Also, due to the above peculiarity there is no straightforward way to - * repeat a completed or cancelled download whose files have been deleted. * * User should call `update` on the returned `Download` to have an accurate status. * diff --git a/src/downloader.cpp b/src/downloader.cpp index 44de4d40d..25dfc1e95 100644 --- a/src/downloader.cpp +++ b/src/downloader.cpp @@ -18,6 +18,7 @@ */ #include "downloader.h" +#include "tools.h" #include "tools/pathTools.h" #include "tools/stringTools.h" @@ -176,7 +177,25 @@ bool downloadCanBeReused(const Download& d, const auto& uris = d.getUris(); const bool sameURI = std::find(uris.begin(), uris.end(), uri) != uris.end(); - return sameURI; + if ( !sameURI ) + return false; + + switch ( d.getStatus() ) { + case Download::K_ERROR: + case Download::K_UNKNOWN: + case Download::K_REMOVED: + return false; + + case Download::K_ACTIVE: + case Download::K_WAITING: + case Download::K_PAUSED: + return true; // XXX: what if options are different? + + case Download::K_COMPLETE: + return fileExists(d.getPath()); // XXX: what if options are different? + } + + return false; } } // unnamed namespace