Replace download progress callback with a signal

adazem009 · adazem009 · commit d3e1e251d25f · 2024-03-22T15:56:56.000+01:00
diff --git a/include/scratchcpp/project.h b/include/scratchcpp/project.h
@@ -7,6 +7,7 @@
 #include "spimpl.h"
 
 #include "global.h"
+#include "signal.h"
 
 namespace libscratchcpp
 {
@@ -42,7 +43,7 @@ class LIBSCRATCHCPP_EXPORT Project
 
         std::shared_ptr<IEngine> engine() const;
 
-        void setDownloadProgressCallback(const std::function<void(unsigned int, unsigned int)> &&f);
+        sigslot::signal<unsigned int, unsigned int> &downloadProgressChanged();
 
     private:
         spimpl::unique_impl_ptr<ProjectPrivate> impl;
diff --git a/src/internal/iprojectdownloader.h b/src/internal/iprojectdownloader.h
@@ -5,6 +5,7 @@
 #include <string>
 #include <vector>
 #include <functional>
+#include <scratchcpp/signal.h>
 
 namespace libscratchcpp
 {
@@ -18,7 +19,7 @@ class IProjectDownloader
         virtual bool downloadAssets(const std::vector<std::string> &assetIds) = 0;
         virtual void cancel() = 0;
 
-        virtual void setDownloadProgressCallback(const std::function<void(unsigned int, unsigned int)> &f) = 0;
+        virtual sigslot::signal<unsigned int, unsigned int> &downloadProgressChanged() = 0;
 
         virtual const std::string &json() const = 0;
         virtual const std::vector<std::string> &assets() const = 0;
diff --git a/src/internal/projectdownloader.cpp b/src/internal/projectdownloader.cpp
@@ -149,13 +149,7 @@ bool ProjectDownloader::downloadAssets(const std::vector<std::string> &assetIds)
                 m_assets[index] = downloader->text();
                 m_downloadedAssetCount++;
                 std::cout << "Downloaded assets: " << m_downloadedAssetCount << " of " << count << std::endl;
-
-                m_downloadProgressCallbackMutex.lock();
-
-                if (m_downloadProgressCallback)
-                    m_downloadProgressCallback(m_downloadedAssetCount, count);
-
-                m_downloadProgressCallbackMutex.unlock();
+                m_downloadProgressChanged(m_downloadedAssetCount, count);
                 m_assetsMutex.unlock();
             }
         }
@@ -182,11 +176,9 @@ void ProjectDownloader::cancel()
     m_downloadedAssetCount = 0;
 }
 
-void ProjectDownloader::setDownloadProgressCallback(const std::function<void(unsigned int, unsigned int)> &f)
+sigslot::signal<unsigned int, unsigned int> &ProjectDownloader::downloadProgressChanged()
 {
-    m_downloadProgressCallbackMutex.lock();
-    m_downloadProgressCallback = f;
-    m_downloadProgressCallbackMutex.unlock();
+    return m_downloadProgressChanged;
 }
 
 const std::string &ProjectDownloader::json() const
diff --git a/src/internal/projectdownloader.h b/src/internal/projectdownloader.h
@@ -23,7 +23,7 @@ class ProjectDownloader : public IProjectDownloader
         bool downloadAssets(const std::vector<std::string> &assetIds) override;
         void cancel() override;
 
-        void setDownloadProgressCallback(const std::function<void(unsigned int, unsigned int)> &f) override;
+        sigslot::signal<unsigned int, unsigned int> &downloadProgressChanged() override;
 
         const std::string &json() const override;
         const std::vector<std::string> &assets() const override;
@@ -38,8 +38,7 @@ class ProjectDownloader : public IProjectDownloader
         std::atomic<unsigned int> m_downloadedAssetCount = 0;
         bool m_cancel = false;
         std::mutex m_cancelMutex;
-        std::function<void(unsigned int, unsigned int)> m_downloadProgressCallback = nullptr;
-        std::mutex m_downloadProgressCallbackMutex;
+        sigslot::signal<unsigned int, unsigned int> m_downloadProgressChanged;
 };
 
 } // namespace libscratchcpp
diff --git a/src/internal/projectdownloaderstub.cpp b/src/internal/projectdownloaderstub.cpp
@@ -22,8 +22,9 @@ void ProjectDownloaderStub::cancel()
 {
 }
 
-void ProjectDownloaderStub::setDownloadProgressCallback(const std::function<void(unsigned int, unsigned int)> &)
+sigslot::signal<unsigned int, unsigned int> &ProjectDownloaderStub::downloadProgressChanged()
 {
+    return m_downloadProgressChanged;
 }
 
 const std::string &ProjectDownloaderStub::json() const
diff --git a/src/internal/projectdownloaderstub.h b/src/internal/projectdownloaderstub.h
@@ -16,11 +16,14 @@ class ProjectDownloaderStub : public IProjectDownloader
         bool downloadAssets(const std::vector<std::string> &) override;
         void cancel() override;
 
-        virtual void setDownloadProgressCallback(const std::function<void(unsigned int, unsigned int)> &) override;
+        sigslot::signal<unsigned int, unsigned int> &downloadProgressChanged() override;
 
         const std::string &json() const override;
         const std::vector<std::string> &assets() const override;
         unsigned int downloadedAssetCount() const override;
+
+    private:
+        sigslot::signal<unsigned int, unsigned int> m_downloadProgressChanged;
 };
 
 } // namespace libscratchcpp
diff --git a/src/project.cpp b/src/project.cpp
@@ -100,10 +100,10 @@ std::shared_ptr<IEngine> Project::engine() const
 }
 
 /*!
- * Sets the function which will be called when the asset download progress changes.
+ * Emits when the asset download progress changes.
  * \note The first parameter is the number of downloaded assets and the latter is the number of all assets to download.
  */
-void Project::setDownloadProgressCallback(const std::function<void(unsigned int, unsigned int)> &&f)
+sigslot::signal<unsigned int, unsigned int> &Project::downloadProgressChanged()
 {
-    impl->setDownloadProgressCallback(f);
+    return impl->downloadProgressChanged();
 }
diff --git a/src/project_p.cpp b/src/project_p.cpp
@@ -183,7 +183,7 @@ void ProjectPrivate::setScratchVersion(ScratchVersion version)
         std::cerr << "Unsupported Scratch version: " << static_cast<int>(version) << std::endl;
 }
 
-void ProjectPrivate::setDownloadProgressCallback(const std::function<void(unsigned int, unsigned int)> &f)
+sigslot::signal<unsigned int, unsigned int> &ProjectPrivate::downloadProgressChanged()
 {
-    downloader->setDownloadProgressCallback(f);
+    return downloader->downloadProgressChanged();
 }
diff --git a/src/project_p.h b/src/project_p.h
@@ -28,7 +28,7 @@ struct ProjectPrivate
         void detectScratchVersion();
         void setScratchVersion(ScratchVersion version);
 
-        void setDownloadProgressCallback(const std::function<void(unsigned int, unsigned int)> &f);
+        sigslot::signal<unsigned int, unsigned int> &downloadProgressChanged();
 
         ScratchVersion scratchVersion = ScratchVersion::Invalid;
         std::string fileName;
diff --git a/test/mocks/projectdownloadermock.h b/test/mocks/projectdownloadermock.h
@@ -12,7 +12,7 @@ class ProjectDownloaderMock : public IProjectDownloader
         MOCK_METHOD(bool, downloadAssets, (const std::vector<std::string> &), (override));
         MOCK_METHOD(void, cancel, (), (override));
 
-        MOCK_METHOD(void, setDownloadProgressCallback, (const std::function<void(unsigned int, unsigned int)> &), (override));
+        MOCK_METHOD((sigslot::signal<unsigned int, unsigned int> &), downloadProgressChanged, (), (override));
 
         MOCK_METHOD(const std::string &, json, (), (const, override));
         MOCK_METHOD(const std::vector<std::string> &, assets, (), (const, override));
diff --git a/test/project/project_test.cpp b/test/project/project_test.cpp
@@ -9,6 +9,7 @@
 using namespace libscratchcpp;
 
 using ::testing::Return;
+using ::testing::ReturnRef;
 
 class ProjectTest : public testing::Test
 {
@@ -158,7 +159,7 @@ TEST_F(ProjectTest, ScratchVersion)
     ASSERT_EQ(p.scratchVersion(), ScratchVersion::Scratch3);
 }
 
-TEST(LoadProjectTest, DownloadProgressCallback)
+TEST(LoadProjectTest, DownloadProgressChanged)
 {
     ProjectDownloaderFactoryMock factory;
     auto downloader = std::make_shared<ProjectDownloaderMock>();
@@ -168,8 +169,7 @@ TEST(LoadProjectTest, DownloadProgressCallback)
     ProjectPrivate p;
     ProjectPrivate::downloaderFactory = nullptr;
 
-    auto lambda = [](unsigned int, unsigned int) {};
-    // TODO: Check the function parameter, if possible (std::function doesn't have operator== for this)
-    EXPECT_CALL(*downloader, setDownloadProgressCallback);
-    p.setDownloadProgressCallback(lambda);
+    sigslot::signal<unsigned int, unsigned int> signal;
+    EXPECT_CALL(*downloader, downloadProgressChanged).WillOnce(ReturnRef(signal));
+    ASSERT_EQ(&p.downloadProgressChanged(), &signal);
 }

Original file line number	Diff line number	Diff line change
`@@ -149,13 +149,7 @@ bool ProjectDownloader::downloadAssets(const std::vector<std::string> &assetIds)`
`149`	`149`	`m_assets[index] = downloader->text();`
`150`	`150`	`m_downloadedAssetCount++;`
`151`	`151`	`std::cout << "Downloaded assets: " << m_downloadedAssetCount << " of " << count << std::endl;`
`152`		`-`
`153`		`- m_downloadProgressCallbackMutex.lock();`
`154`		`-`
`155`		`- if (m_downloadProgressCallback)`
`156`		`- m_downloadProgressCallback(m_downloadedAssetCount, count);`
`157`		`-`
`158`		`- m_downloadProgressCallbackMutex.unlock();`
	`152`	`+ m_downloadProgressChanged(m_downloadedAssetCount, count);`
`159`	`153`	`m_assetsMutex.unlock();`
`160`	`154`	`}`
`161`	`155`	`}`
`@@ -182,11 +176,9 @@ void ProjectDownloader::cancel()`
`182`	`176`	`m_downloadedAssetCount = 0;`
`183`	`177`	`}`
`184`	`178`
`185`		`-void ProjectDownloader::setDownloadProgressCallback(const std::function<void(unsigned int, unsigned int)> &f)`
	`179`	`+sigslot::signal<unsigned int, unsigned int> &ProjectDownloader::downloadProgressChanged()`
`186`	`180`	`{`
`187`		`- m_downloadProgressCallbackMutex.lock();`
`188`		`- m_downloadProgressCallback = f;`
`189`		`- m_downloadProgressCallbackMutex.unlock();`
	`181`	`+ return m_downloadProgressChanged;`
`190`	`182`	`}`
`191`	`183`
`192`	`184`	`const std::string &ProjectDownloader::json() const`
Original file line number	Diff line number	Diff line change
`@@ -22,8 +22,9 @@ void ProjectDownloaderStub::cancel()`
`22`	`22`	`{`
`23`	`23`	`}`
`24`	`24`
`25`		`-void ProjectDownloaderStub::setDownloadProgressCallback(const std::function<void(unsigned int, unsigned int)> &)`
	`25`	`+sigslot::signal<unsigned int, unsigned int> &ProjectDownloaderStub::downloadProgressChanged()`
`26`	`26`	`{`
	`27`	`+ return m_downloadProgressChanged;`
`27`	`28`	`}`
`28`	`29`
`29`	`30`	`const std::string &ProjectDownloaderStub::json() const`
Original file line number	Diff line number	Diff line change
`@@ -100,10 +100,10 @@ std::shared_ptr<IEngine> Project::engine() const`
`100`	`100`	`}`
`101`	`101`
`102`	`102`	`/*!`
`103`		`- * Sets the function which will be called when the asset download progress changes.`
	`103`	`+ * Emits when the asset download progress changes.`
`104`	`104`	`* \note The first parameter is the number of downloaded assets and the latter is the number of all assets to download.`
`105`	`105`	`*/`
`106`		`-void Project::setDownloadProgressCallback(const std::function<void(unsigned int, unsigned int)> &&f)`
	`106`	`+sigslot::signal<unsigned int, unsigned int> &Project::downloadProgressChanged()`
`107`	`107`	`{`
`108`		`- impl->setDownloadProgressCallback(f);`
	`108`	`+ return impl->downloadProgressChanged();`
`109`	`109`	`}`
Original file line number	Diff line number	Diff line change
`@@ -183,7 +183,7 @@ void ProjectPrivate::setScratchVersion(ScratchVersion version)`
`183`	`183`	`std::cerr << "Unsupported Scratch version: " << static_cast<int>(version) << std::endl;`
`184`	`184`	`}`
`185`	`185`
`186`		`-void ProjectPrivate::setDownloadProgressCallback(const std::function<void(unsigned int, unsigned int)> &f)`
	`186`	`+sigslot::signal<unsigned int, unsigned int> &ProjectPrivate::downloadProgressChanged()`
`187`	`187`	`{`
`188`		`- downloader->setDownloadProgressCallback(f);`
	`188`	`+ return downloader->downloadProgressChanged();`
`189`	`189`	`}`