From a1ec77c9a6d4df0372eefc2695d4fd6b487e3a65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Kunz?= Date: Thu, 17 Oct 2024 12:35:54 +0200 Subject: [PATCH 1/4] KDESKTOP-1305 - Normalise path in SyncFileItem map --- src/libsyncengine/progress/progressinfo.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/libsyncengine/progress/progressinfo.cpp b/src/libsyncengine/progress/progressinfo.cpp index 7fdd08ede..d734898a9 100644 --- a/src/libsyncengine/progress/progressinfo.cpp +++ b/src/libsyncengine/progress/progressinfo.cpp @@ -77,35 +77,34 @@ void ProgressInfo::updateEstimates() { } void ProgressInfo::initProgress(const SyncFileItem &item) { - SyncPath path = item.newPath().has_value() ? item.newPath().value() : item.path(); + const SyncPath path = item.newPath().has_value() ? item.newPath().value() : item.path(); ProgressItem progressItem; progressItem.setItem(item); progressItem.progress().setTotal(item.size()); progressItem.progress().setCompleted(0); - _currentItems[path].push(progressItem); + _currentItems[Utility::normalizedSyncName(path)].push(progressItem); _fileProgress.setTotal(_fileProgress.total() + 1); _sizeProgress.setTotal(_sizeProgress.total() + item.size()); } bool ProgressInfo::getSyncFileItem(const SyncPath &path, SyncFileItem &item) { - auto it = _currentItems.find(path); - if (_currentItems.find(path) == _currentItems.end() || it->second.empty()) { + const auto it = _currentItems.find(Utility::normalizedSyncName(path)); + if (it == _currentItems.end() || it->second.empty()) { return false; } item = it->second.front().item(); return true; } -void ProgressInfo::setProgress(const SyncPath &path, int64_t completed) { - auto it = _currentItems.find(path); +void ProgressInfo::setProgress(const SyncPath &path, const int64_t completed) { + const auto it = _currentItems.find(Utility::normalizedSyncName(path)); if (it == _currentItems.end() || it->second.empty()) { return; } - SyncFileItem &item = it->second.front().item(); - if (!shouldCountProgress(item)) { + if (const SyncFileItem &item = it->second.front().item(); !shouldCountProgress(item)) { return; } From 449ecbe64188a8254660a6ec1ffd03ad0a83d896 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Kunz?= Date: Thu, 17 Oct 2024 15:52:52 +0200 Subject: [PATCH 2/4] KDESKTOP-1305 - Another missing normalization --- src/libsyncengine/syncpal/syncpal.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsyncengine/syncpal/syncpal.cpp b/src/libsyncengine/syncpal/syncpal.cpp index 775c0d866..4f476ca7d 100644 --- a/src/libsyncengine/syncpal/syncpal.cpp +++ b/src/libsyncengine/syncpal/syncpal.cpp @@ -1032,7 +1032,7 @@ bool SyncPal::existOnServer(const SyncPath &path) const { bool SyncPal::canShareItem(const SyncPath &path) const { // Path is normalized on server side const SyncPath normalizedPath = Utility::normalizedSyncPath(path); - const NodeId nodeId = _remoteSnapshot->itemId(path); + const NodeId nodeId = _remoteSnapshot->itemId(normalizedPath); if (!nodeId.empty()) { return _remoteSnapshot->canShare(nodeId); } From 0a2ed754ddef9c77656f2a7ee9ea2659b6ebc727 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Kunz?= Date: Thu, 17 Oct 2024 16:57:02 +0200 Subject: [PATCH 3/4] KDESKTOP-1305 - Add unit tests --- src/libsyncengine/progress/progressinfo.cpp | 16 ++----- src/libsyncengine/progress/progressinfo.h | 33 +++++++-------- src/libsyncengine/progress/syncfileitem.h | 2 + src/libsyncengine/syncpal/syncpal.cpp | 3 +- test/libsyncengine/syncpal/testsyncpal.cpp | 47 +++++++++++++++++++++ test/libsyncengine/syncpal/testsyncpal.h | 3 ++ 6 files changed, 72 insertions(+), 32 deletions(-) diff --git a/src/libsyncengine/progress/progressinfo.cpp b/src/libsyncengine/progress/progressinfo.cpp index d734898a9..6241a644f 100644 --- a/src/libsyncengine/progress/progressinfo.cpp +++ b/src/libsyncengine/progress/progressinfo.cpp @@ -21,8 +21,7 @@ namespace KDC { -ProgressInfo::ProgressInfo(std::shared_ptr syncPal) : - _syncPal(syncPal), _totalSizeOfCompletedJobs(0), _maxFilesPerSecond(0), _maxBytesPerSecond(0), _update(false) { +ProgressInfo::ProgressInfo(std::shared_ptr syncPal) : _syncPal(syncPal) { reset(); } @@ -112,8 +111,8 @@ void ProgressInfo::setProgress(const SyncPath &path, const int64_t completed) { recomputeCompletedSize(); } -void ProgressInfo::setProgressComplete(const SyncPath &path, SyncFileStatus status) { - auto it = _currentItems.find(path); +void ProgressInfo::setProgressComplete(const SyncPath &path, const SyncFileStatus status) { + const auto it = _currentItems.find(Utility::normalizedSyncName(path)); if (it == _currentItems.end() || it->second.empty()) { return; } @@ -182,15 +181,6 @@ bool ProgressInfo::trustEta() const { return totalProgress().estimatedEta() < 100 * optimisticEta(); } -Estimates ProgressInfo::fileProgress(const SyncFileItem &item) { - auto it = _currentItems.find(item.path()); - if (it == _currentItems.end() || it->second.empty()) { - return Estimates(); - } - - return it->second.front().progress().estimates(); -} - void ProgressInfo::recomputeCompletedSize() { int64_t r = _totalSizeOfCompletedJobs; for (auto &itemElt: _currentItems) { diff --git a/src/libsyncengine/progress/progressinfo.h b/src/libsyncengine/progress/progressinfo.h index dd488c316..0ee93cdf1 100644 --- a/src/libsyncengine/progress/progressinfo.h +++ b/src/libsyncengine/progress/progressinfo.h @@ -34,24 +34,24 @@ class SyncPal; class ProgressInfo { public: - ProgressInfo(std::shared_ptr syncPal); + explicit ProgressInfo(std::shared_ptr syncPal); ~ProgressInfo(); void reset(); - inline void setUpdate(bool value) { _update = value; } - inline bool update() const { return _update; } + void setUpdate(bool value) { _update = value; } + [[nodiscard]] bool update() const { return _update; } void updateEstimates(); void initProgress(const SyncFileItem &item); void setProgress(const SyncPath &path, int64_t completed); void setProgressComplete(const SyncPath &path, SyncFileStatus status); bool getSyncFileItem(const SyncPath &path, SyncFileItem &item); - inline int64_t totalFiles() const { return _fileProgress.total(); } - inline int64_t completedFiles() const { return _fileProgress.completed(); } - inline int64_t totalSize() const { return _sizeProgress.total(); } - inline int64_t completedSize() const { return _sizeProgress.completed(); } - inline int64_t currentFile() const { return completedFiles(); } - Estimates totalProgress() const; + [[nodiscard]] int64_t totalFiles() const { return _fileProgress.total(); } + [[nodiscard]] int64_t completedFiles() const { return _fileProgress.completed(); } + [[nodiscard]] int64_t totalSize() const { return _sizeProgress.total(); } + [[nodiscard]] int64_t completedSize() const { return _sizeProgress.completed(); } + [[nodiscard]] int64_t currentFile() const { return completedFiles(); } + [[nodiscard]] Estimates totalProgress() const; private: std::shared_ptr _syncPal; @@ -60,16 +60,15 @@ class ProgressInfo { // DELETE a file and CREATE a directory with exact same name) Progress _sizeProgress; Progress _fileProgress; - int64_t _totalSizeOfCompletedJobs; - double _maxFilesPerSecond; - double _maxBytesPerSecond; - bool _update; + int64_t _totalSizeOfCompletedJobs{0}; + double _maxFilesPerSecond{0.0}; + double _maxBytesPerSecond{0.0}; + bool _update{false}; - int64_t optimisticEta() const; - bool trustEta() const; - Estimates fileProgress(const SyncFileItem &item); + [[nodiscard]] int64_t optimisticEta() const; + [[nodiscard]] bool trustEta() const; void recomputeCompletedSize(); - bool isSizeDependent(const SyncFileItem &item) const; + [[nodiscard]] bool isSizeDependent(const SyncFileItem &item) const; }; } // namespace KDC diff --git a/src/libsyncengine/progress/syncfileitem.h b/src/libsyncengine/progress/syncfileitem.h index c00372c67..a55e18430 100644 --- a/src/libsyncengine/progress/syncfileitem.h +++ b/src/libsyncengine/progress/syncfileitem.h @@ -74,6 +74,8 @@ class SyncFileItem { inline bool isDirectory() const { return _type == NodeType::Directory; } + bool operator==(const SyncFileItem &) const = default; + private: NodeType _type{NodeType::Unknown}; SyncPath _path; // Sync folder relative filesystem path diff --git a/src/libsyncengine/syncpal/syncpal.cpp b/src/libsyncengine/syncpal/syncpal.cpp index 4f476ca7d..d70854e7f 100644 --- a/src/libsyncengine/syncpal/syncpal.cpp +++ b/src/libsyncengine/syncpal/syncpal.cpp @@ -1032,8 +1032,7 @@ bool SyncPal::existOnServer(const SyncPath &path) const { bool SyncPal::canShareItem(const SyncPath &path) const { // Path is normalized on server side const SyncPath normalizedPath = Utility::normalizedSyncPath(path); - const NodeId nodeId = _remoteSnapshot->itemId(normalizedPath); - if (!nodeId.empty()) { + if (const NodeId nodeId = _remoteSnapshot->itemId(normalizedPath); !nodeId.empty()) { return _remoteSnapshot->canShare(nodeId); } return false; diff --git a/test/libsyncengine/syncpal/testsyncpal.cpp b/test/libsyncengine/syncpal/testsyncpal.cpp index a7244b13b..ccce47399 100644 --- a/test/libsyncengine/syncpal/testsyncpal.cpp +++ b/test/libsyncengine/syncpal/testsyncpal.cpp @@ -152,6 +152,53 @@ void TestSyncPal::testCopySnapshots() { CPPUNIT_ASSERT(_syncPal->snapshot(ReplicaSide::Local, true)->isValid() != _syncPal->snapshot(ReplicaSide::Local, false)->isValid()); } +void TestSyncPal::testSyncFileItem() { + _syncPal->_progressInfo = std::make_shared(_syncPal); + + CPPUNIT_ASSERT_EQUAL(static_cast(0), _syncPal->_progressInfo->completedSize()); + CPPUNIT_ASSERT_EQUAL(static_cast(0), _syncPal->_progressInfo->completedFiles()); + CPPUNIT_ASSERT_EQUAL(static_cast(0), _syncPal->_progressInfo->totalSize()); + CPPUNIT_ASSERT_EQUAL(static_cast(0), _syncPal->_progressInfo->totalFiles()); + + const SyncPath nfcPath = "/ééé/test.txt"; + const SyncPath nfdPath = "/ééé/test.txt"; + SyncFileItem initItem; + initItem.setType(NodeType::File); + initItem.setPath(nfcPath); + initItem.setLocalNodeId("l123"); + initItem.setDirection(SyncDirection::Up); + initItem.setInstruction(SyncFileInstruction::Put); + initItem.setSize(testhelpers::defaultFileSize); + initItem.setModTime(testhelpers::defaultTime); + _syncPal->initProgress(initItem); + + SyncFileItem testItem; + CPPUNIT_ASSERT(_syncPal->getSyncFileItem(nfcPath, testItem)); + CPPUNIT_ASSERT(testItem == initItem); + CPPUNIT_ASSERT(_syncPal->getSyncFileItem(nfdPath, testItem)); + CPPUNIT_ASSERT(testItem == initItem); + + CPPUNIT_ASSERT_EQUAL(static_cast(0), _syncPal->_progressInfo->completedSize()); + CPPUNIT_ASSERT_EQUAL(static_cast(0), _syncPal->_progressInfo->completedFiles()); + CPPUNIT_ASSERT_EQUAL(testhelpers::defaultFileSize, _syncPal->_progressInfo->totalSize()); + CPPUNIT_ASSERT_EQUAL(static_cast(1), _syncPal->_progressInfo->totalFiles()); + + constexpr int64_t progress = 15; + _syncPal->setProgress(nfdPath, progress); + CPPUNIT_ASSERT(_syncPal->getSyncFileItem(nfdPath, testItem)); + + CPPUNIT_ASSERT_EQUAL(progress, _syncPal->_progressInfo->completedSize()); + CPPUNIT_ASSERT_EQUAL(static_cast(0), _syncPal->_progressInfo->completedFiles()); + CPPUNIT_ASSERT_EQUAL(testhelpers::defaultFileSize, _syncPal->_progressInfo->totalSize()); + CPPUNIT_ASSERT_EQUAL(static_cast(1), _syncPal->_progressInfo->totalFiles()); + + _syncPal->setProgressComplete(nfdPath, SyncFileStatus::Success); + + CPPUNIT_ASSERT_EQUAL(testhelpers::defaultFileSize, _syncPal->_progressInfo->completedSize()); + CPPUNIT_ASSERT_EQUAL(static_cast(1), _syncPal->_progressInfo->completedFiles()); + CPPUNIT_ASSERT_EQUAL(testhelpers::defaultFileSize, _syncPal->_progressInfo->totalSize()); + CPPUNIT_ASSERT_EQUAL(static_cast(1), _syncPal->_progressInfo->totalFiles()); +} void TestSyncPal::testAll() { // Start sync diff --git a/test/libsyncengine/syncpal/testsyncpal.h b/test/libsyncengine/syncpal/testsyncpal.h index 33a6cb05a..3cf550aa2 100644 --- a/test/libsyncengine/syncpal/testsyncpal.h +++ b/test/libsyncengine/syncpal/testsyncpal.h @@ -30,6 +30,7 @@ class TestSyncPal : public CppUnit::TestFixture { CPPUNIT_TEST(testSnapshot); CPPUNIT_TEST(testCopySnapshots); CPPUNIT_TEST(testOperationSet); + CPPUNIT_TEST(testSyncFileItem); CPPUNIT_TEST_SUITE_END(); public: @@ -50,6 +51,8 @@ class TestSyncPal : public CppUnit::TestFixture { void testOperationSet(); void testCopySnapshots(); + void testSyncFileItem(); + void testAll(); void testConflictQueue(); bool exec_case_6_4(); From 89c34c3dab1409e46f98059f7bcc97c66e720bd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Kunz?= Date: Fri, 18 Oct 2024 08:43:57 +0200 Subject: [PATCH 4/4] KDESKTOP-1305 - Fix missing normalization while removing item --- src/libsyncengine/progress/progressinfo.cpp | 10 +++++----- test/libsyncengine/syncpal/testsyncpal.cpp | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libsyncengine/progress/progressinfo.cpp b/src/libsyncengine/progress/progressinfo.cpp index 6241a644f..4359637f7 100644 --- a/src/libsyncengine/progress/progressinfo.cpp +++ b/src/libsyncengine/progress/progressinfo.cpp @@ -82,14 +82,14 @@ void ProgressInfo::initProgress(const SyncFileItem &item) { progressItem.progress().setTotal(item.size()); progressItem.progress().setCompleted(0); - _currentItems[Utility::normalizedSyncName(path)].push(progressItem); + _currentItems[Utility::normalizedSyncPath(path)].push(progressItem); _fileProgress.setTotal(_fileProgress.total() + 1); _sizeProgress.setTotal(_sizeProgress.total() + item.size()); } bool ProgressInfo::getSyncFileItem(const SyncPath &path, SyncFileItem &item) { - const auto it = _currentItems.find(Utility::normalizedSyncName(path)); + const auto it = _currentItems.find(Utility::normalizedSyncPath(path)); if (it == _currentItems.end() || it->second.empty()) { return false; } @@ -98,7 +98,7 @@ bool ProgressInfo::getSyncFileItem(const SyncPath &path, SyncFileItem &item) { } void ProgressInfo::setProgress(const SyncPath &path, const int64_t completed) { - const auto it = _currentItems.find(Utility::normalizedSyncName(path)); + const auto it = _currentItems.find(Utility::normalizedSyncPath(path)); if (it == _currentItems.end() || it->second.empty()) { return; } @@ -112,7 +112,7 @@ void ProgressInfo::setProgress(const SyncPath &path, const int64_t completed) { } void ProgressInfo::setProgressComplete(const SyncPath &path, const SyncFileStatus status) { - const auto it = _currentItems.find(Utility::normalizedSyncName(path)); + const auto it = _currentItems.find(Utility::normalizedSyncPath(path)); if (it == _currentItems.end() || it->second.empty()) { return; } @@ -133,7 +133,7 @@ void ProgressInfo::setProgressComplete(const SyncPath &path, const SyncFileStatu it->second.pop(); if (it->second.empty()) { - _currentItems.erase(path); + _currentItems.erase(Utility::normalizedSyncPath(path)); } recomputeCompletedSize(); } diff --git a/test/libsyncengine/syncpal/testsyncpal.cpp b/test/libsyncengine/syncpal/testsyncpal.cpp index ccce47399..cb68164f2 100644 --- a/test/libsyncengine/syncpal/testsyncpal.cpp +++ b/test/libsyncengine/syncpal/testsyncpal.cpp @@ -160,8 +160,8 @@ void TestSyncPal::testSyncFileItem() { CPPUNIT_ASSERT_EQUAL(static_cast(0), _syncPal->_progressInfo->totalSize()); CPPUNIT_ASSERT_EQUAL(static_cast(0), _syncPal->_progressInfo->totalFiles()); - const SyncPath nfcPath = "/ééé/test.txt"; - const SyncPath nfdPath = "/ééé/test.txt"; + const SyncPath nfcPath = SyncPath(Str("/") + testhelpers::makeNfcSyncName() + Str("/test.txt")).native(); + const SyncPath nfdPath = SyncPath(Str("/") + testhelpers::makeNfdSyncName() + Str("/test.txt")).native(); SyncFileItem initItem; initItem.setType(NodeType::File); initItem.setPath(nfcPath);