fixing review comments

KDAB · Oct 23, 2023 · 00ddc48 · 00ddc48
1 parent 7fcbec2
commit 00ddc48
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 27 deletions.
diff --git a/src/kdbindings/signal.h b/src/kdbindings/signal.h
@@ -305,40 +305,37 @@ class Signal
         // Connect a callback function with Throttling
         Private::GenerationalIndex connectWithThrottling(std::function<void(Args...)> const &slot, int interval)
         {
-            bool shouldWait = false;
             std::chrono::milliseconds throttleDelay(interval);
+            auto lastCallTime = std::chrono::high_resolution_clock::now() - throttleDelay; // Initialize so it can be triggered immediately the first time.
 
-            auto throttleCallBack = [slot = std::move(slot), &shouldWait, throttleDelay](Args... args) mutable {
-                if (shouldWait)
-                    return;
-
-                slot(args...);
-
-                shouldWait = true;
-                std::thread([&shouldWait, throttleDelay]() {
-                    std::this_thread::sleep_for(throttleDelay);
+            auto throttleCallBack = [slot = std::move(slot), throttleDelay, lastCallTime](Args... args) mutable {
+                auto now = std::chrono::high_resolution_clock::now();
+                auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(now - lastCallTime);
 
-                    shouldWait = false;
-                }).join();
+                if (elapsed.count() >= throttleDelay.count()) {
+                    slot(args...);
+                    lastCallTime = now;
+                }
             };
+
             return m_connections.insert({ throttleCallBack });
         }
 
         // Connect a callback function with Debouncing
         Private::GenerationalIndex connectWithDebouncing(std::function<void(Args...)> const &slot, int interval)
         {
-            bool debouncing = false;
-
             std::chrono::milliseconds debounceDelay(interval);
-            auto timer = std::chrono::high_resolution_clock::now();
+            auto lastEventTime = std::chrono::high_resolution_clock::now();
+            auto lastCallTime = lastEventTime - debounceDelay; // Initialize so it can be triggered immediately the first time.
 
-            auto debounceCallBack = [slot = std::move(slot), debounceDelay, timer](Args... args) mutable {
+            auto debounceCallBack = [slot = std::move(slot), debounceDelay, lastEventTime, lastCallTime](Args... args) mutable {
                 auto now = std::chrono::high_resolution_clock::now();
-                auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(now - timer);
+                lastEventTime = now;
 
-                if (elapsed.count() >= debounceDelay.count()) {
+                auto timeSinceLastCall = std::chrono::duration_cast<std::chrono::milliseconds>(now - lastCallTime);
+                if (timeSinceLastCall.count() >= debounceDelay.count()) {
                     slot(args...);
-                    timer = now;
+                    lastCallTime = now;
                 }
             };
 

diff --git a/tests/signal/tst_signal.cpp b/tests/signal/tst_signal.cpp
@@ -100,27 +100,39 @@ TEST_CASE("Signal connections")
         int count = 0;
         auto handle = signal.connectWithThrottling([&count](int value) {
             count++;
-        }, 100);
+        },
+                                                   100);
 
         signal.emit(2);
+        REQUIRE(count == 1); // First emission should trigger the slot immediately
+
+        std::this_thread::sleep_for(std::chrono::milliseconds(50)); // Within the throttling interval
         signal.emit(2);
-        REQUIRE(count == 2);
-    }
+        REQUIRE(count == 1); // Second emission shouldn't trigger the slot due to throttling
 
+        std::this_thread::sleep_for(std::chrono::milliseconds(100)); // After the throttling interval
+        signal.emit(2);
+        REQUIRE(count == 2); // Third emission should trigger the slot
+    }
     SUBCASE("Test connectWithDebouncing")
     {
         Signal<int> signal;
         int count = 0;
         auto handle = signal.connectWithDebouncing([&count](int value) {
             count++;
-        }, 100);
+        },
+                                                   100);
 
-         signal.emit(1);
-         REQUIRE(count == 0);  // Debouncing interval hasn't passed
+        signal.emit(1);
+        REQUIRE(count == 1); // Debouncing interval hasn't passed
 
-        std::this_thread::sleep_for(std::chrono::milliseconds(100)); // Wait for debouncing interval
+        std::this_thread::sleep_for(std::chrono::milliseconds(50)); // Still within the debouncing interval
         signal.emit(2);
-        REQUIRE(count == 1); // The function should be called after the pause
+        REQUIRE(count == 1); // Debouncing interval still hasn't passed
+
+        std::this_thread::sleep_for(std::chrono::milliseconds(50)); // After the debouncing interval
+        signal.emit(2);
+        REQUIRE(count == 2);
     }
 
     SUBCASE("A signal with arguments can be connected to a lambda and invoked with const l-value args")