Merge pull request #1066 from kiwix/smarter_startDownload

include/downloader.h

@@ -168,7 +168,10 @@ class Download {
  */
 class Downloader
 {
- public:
+ public: // types
+  typedef std::vector<std::pair<std::string, std::string>> Options;
+
+ public: // functions
   Downloader();
   virtual ~Downloader();
 
@@ -177,14 +180,21 @@ 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.
+   *
    * User should call `update` on the returned `Download` to have an accurate status.
    *
    * @param uri: The uri of the thing to download.
    * @param options: A series of pair <option_name, option_value> to pass to aria.
    * @return: The newly created Download.
    */
-  std::shared_ptr<Download> startDownload(const std::string& uri, const std::vector<std::pair<std::string, std::string>>& options = {});
+  std::shared_ptr<Download> startDownload(const std::string& uri, const Options& options = {});
 
   /**
    * Get a download corrsponding to a download id (did)
@@ -206,7 +216,7 @@ class Downloader
    */
   std::vector<std::string> getDownloadIds() const;
 
- private:
+ private: // data
   mutable std::mutex m_lock;
   std::map<std::string, std::shared_ptr<Download>> m_knownDownloads;
   std::shared_ptr<Aria2> mp_aria;

src/downloader.cpp

@@ -18,6 +18,7 @@
  */
 
 #include "downloader.h"
+#include "tools.h"
 #include "tools/pathTools.h"
 #include "tools/stringTools.h"
 
@@ -166,13 +167,45 @@ std::vector<std::string> Downloader::getDownloadIds() const {
   return ret;
 }
 
-std::shared_ptr<Download> Downloader::startDownload(const std::string& uri, const std::vector<std::pair<std::string, std::string>>& options)
+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();
+
+  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
+
+std::shared_ptr<Download> Downloader::startDownload(const std::string& uri, const Options& options)
 {
   std::unique_lock<std::mutex> lock(m_lock);
   for (auto& p: m_knownDownloads) {
     auto& d = p.second;
-    auto& uris = d->getUris();
+    if ( downloadCanBeReused(*d, uri, options) )
-    if (std::find(uris.begin(), uris.end(), uri) != uris.end())
       return d;
   }
   std::vector<std::string> uris = {uri};