Merge #840(kitsune): Cleanup again (and a new algorithm)

Quotient/jobs/basejob.h

@@ -277,9 +277,9 @@ public Q_SLOTS:
     void statusChanged(Quotient::BaseJob::Status newStatus);
 
     //! \brief A retry of the network request is scheduled after the previous request failed
-    //! \param nextAttempt the 1-based number of attempt (will always be more than 1)
+    //! \param nextRetryNumber the number of the next retry, starting from 1
     //! \param inMilliseconds the interval after which the next attempt will be taken
-    void retryScheduled(int nextAttempt, Quotient::BaseJob::duration_ms_t inMilliseconds);
+    void retryScheduled(int nextRetryNumber, Quotient::BaseJob::duration_ms_t inMilliseconds);
 
     //! \brief The job has been rate-limited
     //!

Quotient/jobs/downloadfilejob.cpp

@@ -23,15 +23,16 @@ class Q_DECL_HIDDEN DownloadFileJob::Private {
     explicit Private(QString serverName, QString mediaId, const QString& localFilename)
         : serverName(std::move(serverName))
         , mediaId(std::move(mediaId))
-        , targetFile(!localFilename.isEmpty() ? new QFile(localFilename) : nullptr)
-        , tempFile(!localFilename.isEmpty() ? new QFile(targetFile->fileName() + ".qtntdownload"_L1)
-                                            : new QTemporaryFile())
+        , targetFile(!localFilename.isEmpty() ? std::make_unique<QFile>(localFilename) : nullptr)
+        , tempFile(!localFilename.isEmpty()
+                       ? std::make_unique<QFile>(targetFile->fileName() + ".qtntdownload"_L1)
+                       : std::make_unique<QTemporaryFile>())
     {}
 
     QString serverName;
     QString mediaId;
-    QScopedPointer<QFile> targetFile;
-    QScopedPointer<QFile> tempFile;
+    std::unique_ptr<QFile> targetFile;
+    std::unique_ptr<QFile> tempFile;
 
     std::optional<EncryptedFileMetadata> encryptedFileMetadata;
 };
@@ -156,8 +157,8 @@ BaseJob::Status DownloadFileJob::prepareResult()
         }
     } else {
         if (d->encryptedFileMetadata.has_value()) {
-            QScopedPointer<QFile> tempTempFile(new QTemporaryFile);
-            if (!tempTempFile->open(QFile::ReadWrite)) {
+            std::unique_ptr<QFile> tempTempFile = std::make_unique<QTemporaryFile>();
+            if (!tempTempFile->isWritable()) {
                 qCWarning(JOBS) << "Failed to open temporary file for decryption"
                                 << tempTempFile->errorString();
                 return { FileError, "Couldn't open temporary file for decryption"_L1 };

Quotient/ranges_extras.h

@@ -6,8 +6,13 @@ namespace Quotient {
 
 //! Same as std::projected but Proj is checked against the reference under the iterator
 template <std::indirectly_readable IterT,
-          std::indirectly_regular_unary_invocable<std::iter_reference_t<IterT>> Proj>
-using IndirectlyProjected = std::projected<std::iter_reference_t<IterT>, Proj>;
+          std::indirectly_regular_unary_invocable<std::iter_reference_t<IterT>> ProjT>
+using IndirectlyProjected = std::projected<std::iter_reference_t<IterT>, ProjT>;
+
+//! Same as std::indirectly_comparable but uses IndirectlyProjected<> instead of std::projected<>
+template <typename IterT, typename ValT, typename ProjT, typename CompT = std::ranges::equal_to>
+concept IndirectlyProjectedComparable =
+    std::indirect_binary_predicate<CompT, IndirectlyProjected<IterT, ProjT>, const ValT*>;
 
 //! \brief Find a value in a container of (smart) pointers
 //!
@@ -17,26 +22,33 @@ using IndirectlyProjected = std::projected<std::iter_reference_t<IterT>, Proj>;
 //! searching for events that match a specific simple criterion; e.g., to find an event with a given
 //! id in a container you can now write `findIndirect(events, eventId, &RoomEvent::id);` instead
 //! of having to supply your own lambda to dereference the timeline item and check the event id.
-template <std::input_iterator IterT, typename ValT, typename Proj = std::identity>
-    requires std::indirect_binary_predicate<std::ranges::equal_to,
-                                            IndirectlyProjected<IterT, Proj>, const ValT*>
-// Most of constraints here (including IndirectlyProjected) are based on the definition of
-// std::ranges::find and things around it
-inline constexpr auto findIndirect(IterT from, IterT to, const ValT& value, Proj proj = {})
+template <std::input_iterator IterT, typename ValT, typename ProjT = std::identity>
+    requires IndirectlyProjectedComparable<IterT, ValT, ProjT>
+inline constexpr auto findIndirect(IterT from, IterT to, const ValT& value, ProjT proj = {})
 {
     return std::ranges::find(from, to, value, [p = std::move(proj)](auto& itemPtr) {
         return std::invoke(p, *itemPtr);
     });
 }
 
 //! The overload of findIndirect for ranges
-template <typename RangeT, typename ValT, typename Proj = std::identity>
-    requires std::indirect_binary_predicate<
-        std::ranges::equal_to, IndirectlyProjected<std::ranges::iterator_t<RangeT>, Proj>,
-        const ValT*>
-inline constexpr auto findIndirect(RangeT&& range, const ValT& value, Proj proj = {})
+template <typename RangeT, typename ValT, typename ProjT = std::identity>
+    requires IndirectlyProjectedComparable<std::ranges::iterator_t<RangeT>, ValT, ProjT>
+inline constexpr auto findIndirect(RangeT&& range, const ValT& value, ProjT proj = {})
 {
     return findIndirect(std::ranges::begin(range), std::ranges::end(range), value, std::move(proj));
 }
 
+//! \brief An indexOf() alternative for any range
+//!
+//! Unlike QList::indexOf(), returns `range.size()` if \p value is not found
+template <typename RangeT, typename ValT, typename ProjT = std::identity>
+    requires std::indirectly_comparable<std::ranges::iterator_t<RangeT>, const ValT*,
+                                        std::ranges::equal_to, ProjT>
+inline auto findIndex(const RangeT& range, const ValT& value, ProjT proj = {})
+{
+    using namespace std::ranges;
+    return distance(begin(range), find(range, value, std::move(proj)));
+}
+
 }

Quotient/util.h

@@ -8,10 +8,11 @@
 
 #include <QtCore/QDebug>
 #include <QtCore/QElapsedTimer>
+#include <QtCore/QFuture>
 #include <QtCore/QHashFunctions>
 #include <QtCore/QLatin1String>
+#include <QtCore/QScopedPointer>
 #include <QtCore/QUrl>
-#include <QtCore/QFuture>
 
 #include <memory>
 #include <optional>
@@ -230,6 +231,15 @@ inline std::pair<InputIt, ForwardIt> findFirstOf(InputIt first, InputIt last,
     return { last, sLast };
 }
 
+//! \brief Common custom deleter for std::unique_ptr and QScopedPointer
+//!
+//! Since Qt 6, this is merely an alias for QScopedPointerDeleteLater (which is suitable
+//! for std::unique_ptr too).
+using DeleteLater = QScopedPointerDeleteLater;
+
+template <std::derived_from<QObject> T>
+using QObjectHolder = std::unique_ptr<T, DeleteLater>;
+
 //! \brief An owning implementation pointer
 //!
 //! This is basically std::unique_ptr<> to hold your pimpl's but without having

autotests/testolmaccount.cpp

@@ -214,7 +214,7 @@ void TestOlmAccount::uploadOneTimeKeys()
                                           QCOMPARE(oneTimeKeyCounts.value(Curve25519Key), 5);
                                       },
                                       [] { QFAIL("upload failed"); });
-    QVERIFY(waitForFuture(uploadFuture));
+    QVERIFY(waitForJob(uploadFuture));
 }
 
 void TestOlmAccount::uploadSignedOneTimeKeys()
@@ -233,7 +233,7 @@ void TestOlmAccount::uploadSignedOneTimeKeys()
                     QCOMPARE(oneTimeKeyCounts.value(SignedCurve25519Key), nKeys);
                 },
                 [] { QFAIL("upload failed"); });
-    QVERIFY(waitForFuture(uploadFuture));
+    QVERIFY(waitForJob(uploadFuture));
 }
 
 void TestOlmAccount::uploadKeys()
@@ -249,7 +249,7 @@ void TestOlmAccount::uploadKeys()
                                QCOMPARE(oneTimeKeyCounts.value(SignedCurve25519Key), 1);
                            },
                            [] { QFAIL("upload failed"); });
-    QVERIFY(waitForFuture(request));
+    QVERIFY(waitForJob(request));
 }
 
 void TestOlmAccount::queryTest()
@@ -265,7 +265,7 @@ void TestOlmAccount::queryTest()
             .then([](const QHash<QString, int>& aliceOneTimeKeyCounts) {
                 QCOMPARE(aliceOneTimeKeyCounts.value(SignedCurve25519Key), 1);
             });
-    QVERIFY(waitForFuture(aliceUploadKeysRequest));
+    QVERIFY(waitForJob(aliceUploadKeysRequest));
 
     auto bobOlm = bob->olmAccount();
     bobOlm->generateOneTimeKeys(1);
@@ -274,7 +274,7 @@ void TestOlmAccount::queryTest()
             .then([](const QHash<QString, int>& bobOneTimeKeyCounts) {
                 QCOMPARE(bobOneTimeKeyCounts.value(SignedCurve25519Key), 1);
             });
-    QVERIFY(waitForFuture(bobUploadKeysRequest));
+    QVERIFY(waitForJob(bobUploadKeysRequest));
 
     // Each user is requests each other's keys.
     const QHash<QString, QStringList> deviceKeysForBob{ { bob->userId(), {} } };
@@ -292,7 +292,7 @@ void TestOlmAccount::queryTest()
                 QCOMPARE(aliceDevKeys.keys, bobOlm->deviceKeys().keys);
                 QCOMPARE(aliceDevKeys.signatures, bobOlm->deviceKeys().signatures);
             });
-    QVERIFY(waitForFuture(queryBobKeysResult));
+    QVERIFY(waitForJob(queryBobKeysResult));
 
     const QHash<QString, QStringList> deviceKeysForAlice{ { alice->userId(), {} } };
     const auto queryAliceKeysResult =
@@ -309,7 +309,7 @@ void TestOlmAccount::queryTest()
                 QCOMPARE(devKeys.keys, aliceOlm->deviceKeys().keys);
                 QCOMPARE(devKeys.signatures, aliceOlm->deviceKeys().signatures);
             });
-    QVERIFY(waitForFuture(queryAliceKeysResult));
+    QVERIFY(waitForJob(queryAliceKeysResult));
 }
 
 void TestOlmAccount::claimKeys()
@@ -324,7 +324,7 @@ void TestOlmAccount::claimKeys()
                        .then([bob](const QHash<QString, int>& oneTimeKeyCounts) {
                            QCOMPARE(oneTimeKeyCounts.value(SignedCurve25519Key), 1);
                        });
-    QVERIFY(waitForFuture(request));
+    QVERIFY(waitForJob(request));
 
     // Alice retrieves bob's keys & claims one signed one-time key.
     const QHash<QString, QStringList> deviceKeysToQuery{ { bob->userId(), {} } };
@@ -336,7 +336,7 @@ void TestOlmAccount::claimKeys()
             QVERIFY(verifyIdentitySignature(bobDevices.value(bob->deviceId()), bob->deviceId(),
                                             bob->userId()));
         });
-    QVERIFY(waitForFuture(queryKeysJob));
+    QVERIFY(waitForJob(queryKeysJob));
 
     // Retrieve the identity key for the current device to check after claiming
     // const auto& bobEd25519 =
@@ -363,7 +363,7 @@ void TestOlmAccount::claimKeys()
                     }
                 },
                 [] { QFAIL("Claim job failed"); });
-    QVERIFY(waitForFuture(claimKeysJob));
+    QVERIFY(waitForJob(claimKeysJob));
 }
 
 void TestOlmAccount::claimMultipleKeys()
@@ -380,7 +380,7 @@ void TestOlmAccount::claimMultipleKeys()
             .then([](const QHash<QString, int>& oneTimeKeyCounts) {
                 QCOMPARE(oneTimeKeyCounts.value(SignedCurve25519Key), 10);
             });
-    QVERIFY(waitForFuture(aliceUploadKeyRequest));
+    QVERIFY(waitForJob(aliceUploadKeyRequest));
 
     auto olm1 = alice1->olmAccount();
     olm1->generateOneTimeKeys(10);
@@ -389,7 +389,7 @@ void TestOlmAccount::claimMultipleKeys()
             .then([](const QHash<QString, int>& oneTimeKeyCounts) {
                 QCOMPARE(oneTimeKeyCounts.value(SignedCurve25519Key), 10);
             });
-    QVERIFY(waitForFuture(alice1UploadKeyRequest));
+    QVERIFY(waitForJob(alice1UploadKeyRequest));
 
     auto olm2 = alice2->olmAccount();
     olm2->generateOneTimeKeys(10);
@@ -398,7 +398,7 @@ void TestOlmAccount::claimMultipleKeys()
             .then([](const QHash<QString, int>& oneTimeKeyCounts) {
                 QCOMPARE(oneTimeKeyCounts.value(SignedCurve25519Key), 10);
             });
-    QVERIFY(waitForFuture(alice2UploadKeyRequest));
+    QVERIFY(waitForJob(alice2UploadKeyRequest));
 
     // Bob will claim keys from all Alice's devices
     CREATE_CONNECTION(bob, "bob3"_L1, "secret"_L1, "BobPhone"_L1)
@@ -412,22 +412,24 @@ void TestOlmAccount::claimMultipleKeys()
         bob->callApi<ClaimKeysJob>(oneTimeKeys).then([alice](const ClaimKeysJob::Response& r) {
             QCOMPARE(r.oneTimeKeys.value(alice->userId()).size(), 3);
         });
-    QVERIFY(waitForFuture(claimResult));
+    QVERIFY(waitForJob(claimResult));
 }
 
 void TestOlmAccount::enableEncryption()
 {
     CREATE_CONNECTION(alice, "alice9"_L1, "secret"_L1, "AlicePhone"_L1)
 
-    const auto futureRoom = alice->createRoom(Connection::PublishRoom, {}, {}, {}, {})
-                                .then([alice](const QString& roomId) {
-                                    auto room = alice->room(roomId);
-                                    room->activateEncryption();
-                                    return room;
-                                });
-    QVERIFY(waitForFuture(futureRoom));
+    auto createRoomJob = alice->createRoom(Connection::PublishRoom, {}, {}, {}, {});
+    auto futureEncryptedRoom = createRoomJob.then([alice](const QString& roomId) {
+        auto room = alice->room(roomId);
+        room->activateEncryption();
+        return room;
+    });
+    QVERIFY(waitForJob(createRoomJob));
     alice->syncLoop();
-    QVERIFY(QTest::qWaitFor([room = futureRoom.result()] { return room->usesEncryption(); }, 40000));
+    // Give extra 5 seconds for the network roundtrip
+    QVERIFY(QTest::qWaitFor(std::bind_front(&Room::usesEncryption, futureEncryptedRoom.result()),
+                            SyncJob::defaultTimeoutMillis * 2 + 5000));
 }
 
 QTEST_GUILESS_MAIN(TestOlmAccount)

autotests/testutils.h

@@ -11,6 +11,8 @@
 namespace Quotient {
 
 class Connection;
+template <class JobT>
+class JobHandle;
 
 std::shared_ptr<Connection> createTestConnection(QLatin1StringView localUserName,
                                                  QLatin1StringView secret,
@@ -22,8 +24,13 @@ std::shared_ptr<Connection> createTestConnection(QLatin1StringView localUserName
     if (!VAR)                                                             \
         QFAIL("Could not set up test connection");
 
-inline bool waitForFuture(const auto& ft)
-    requires requires { ft.isFinished(); }
+template <typename JobT>
+inline bool waitForJob(const Quotient::JobHandle<JobT>& job)
 {
-    return QTest::qWaitFor([ft] { return ft.isFinished(); }, 40000);
+    const auto& [timeouts, retryIntervals, _] = job->currentBackoffStrategy();
+    return QTest::qWaitFor([job] { return job.isFinished(); },
+                           std::chrono::milliseconds(
+                               std::reduce(timeouts.cbegin(), timeouts.cend())
+                               + std::reduce(retryIntervals.cbegin(), retryIntervals.cend()))
+                               .count());
 }

quotest/quotest.cpp

@@ -470,12 +470,13 @@ TEST_IMPL(sendFile)
     return false;
 }
 
-void getResource(const QUrl& url, QScopedPointer<QNetworkReply, QScopedPointerDeleteLater>& r,
-                 QEventLoop& el)
+using NetworkReplyPtr = QObjectHolder<QNetworkReply>;
+
+void getResource(const QUrl& url, NetworkReplyPtr& r, QEventLoop& el)
 {
     r.reset(NetworkAccessManager::instance()->get(QNetworkRequest(url)));
     QObject::connect(
-        r.data(), &QNetworkReply::finished, &el,
+        r.get(), &QNetworkReply::finished, &el,
         [url, &r, &el] {
             if (r->error() != QNetworkReply::NoError)
                 getResource(url, r, el);
@@ -490,7 +491,7 @@ bool testDownload(const QUrl& url)
     // The actual test is separate from the download invocation to help debugging
     const auto results = QtConcurrent::blockingMapped(QVector<int>{ 1, 2, 3 }, [url](int) {
         thread_local QEventLoop el;
-        thread_local QScopedPointer<QNetworkReply, QScopedPointerDeleteLater> reply{};
+        thread_local NetworkReplyPtr reply{};
         getResource(url, reply, el);
         el.exec();
         return reply->error();