Qt Timers: disallow setting negative intervals

As disccussed in the code review. Replace negative values with 1ms (not 0ms as that would spin up the event-loop too many times). Move QTimer::start(std::chrono::milliseconds msec) API docs next to the method definition so as to not forget changing it when the implementation changes. Drive-by, qWarning() and co. are marked cold so there is no need to use Q_UNLIKELY in an if-condition leading to calling these methods. [ChangeLog][QtCore][Important Behvaior Change] Timers with negative intervals aren't allowed anymore, that is, if you try to start a timer (or set the interval) with a negative value, that interval will be set to 1ms. Previously Qt timers would let you set the interval to a negative value, but behave in surprising ways (for example stop the timer if it was running or not start it at all). This change affects QTimer, QChronoTimer, QBasicTimer and QObject::startTimer(). Fixes: QTBUG-132359 Change-Id: I09309063665db051337f91160971993d9ce7911e Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
author: Ahmad Samir <a.samirh78@gmail.com> 2024-12-19 14:10:31 +0200
committer: Ahmad Samir <a.samirh78@gmail.com> 2025-02-10 01:13:29 +0200
commit: f1f610bc67bfd5c2ef31270a6945e7bae93b5e4a (patch)
tree: 95df313ad5a5a6bd79501073a6f7311a257d1a1b /src
parent: d4c312f68aee526848b7b2f0e4b77948e8b55e8f (diff)
5 files changed, 84 insertions, 26 deletions
diff --git a/src/corelib/kernel/qbasictimer.cpp b/src/corelib/kernel/qbasictimer.cpp
index b7e2f114519..60387381da6 100644
--- a/src/corelib/kernel/qbasictimer.cpp
+++ b/src/corelib/kernel/qbasictimer.cpp
@@ -145,6 +145,8 @@ QT_BEGIN_NAMESPACE
 
     The given \a object will receive timer events.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
 //! [start-nanoseconds-note]
     \note Starting from Qt 6.9 this method takes std::chrono::nanoseconds,
           before that it took std::chrono::milliseconds. This change is
@@ -170,6 +172,8 @@ QT_BEGIN_NAMESPACE
     given \a timerType. See Qt::TimerType for information on the different
     timer types.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \a obj will receive timer events.
 
     \include qbasictimer.cpp start-nanoseconds-note
@@ -179,9 +183,10 @@ QT_BEGIN_NAMESPACE
 void QBasicTimer::start(Duration duration, Qt::TimerType timerType, QObject *obj)
 {
     QAbstractEventDispatcher *eventDispatcher = QAbstractEventDispatcher::instance();
-    if (Q_UNLIKELY(duration < 0ns)) {
-        qWarning("QBasicTimer::start: Timers cannot have negative timeouts");
-        return;
+    if (duration < 0ns) {
+        qWarning("QBasicTimer::start: negative intervals aren't allowed; the "
+                 "interval will be set to 1ms.");
+        duration = 1ms;
     }
     if (Q_UNLIKELY(!eventDispatcher)) {
         qWarning("QBasicTimer::start: QBasicTimer can only be used with threads started with QThread");
diff --git a/src/corelib/kernel/qchronotimer.cpp b/src/corelib/kernel/qchronotimer.cpp
index a0f89f3a208..0d36d22bcb7 100644
--- a/src/corelib/kernel/qchronotimer.cpp
+++ b/src/corelib/kernel/qchronotimer.cpp
@@ -279,10 +279,18 @@ QBindable<bool> QChronoTimer::bindableSingleShot()
     stop() and then start() the timer, and acquire a new id().
     If the timer is not running, only the interval is changed.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \sa singleShot
 */
 void QChronoTimer::setInterval(std::chrono::nanoseconds nsec)
 {
+    if (nsec < 0ns) {
+        qWarning("QChronoTimer::setInterval: negative intervals aren't allowed; the "
+                 "interval will be set to 1ms.");
+        nsec = 1ms;
+    }
+
     auto *d = d_func();
     d->intervalDuration.removeBindingUnlessInWrapper();
     const bool intervalChanged = nsec != d->intervalDuration.valueBypassingBindings();
diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index d3566b1081e..707c12ae0fd 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -1821,6 +1821,8 @@ void QObjectPrivate::setThreadData_helper(QThreadData *currentData, QThreadData
     startTimer(std::chrono::milliseconds{interval}, timerType);
     \endcode
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \sa timerEvent(), killTimer(), QChronoTimer, QBasicTimer
 */
 
@@ -1843,6 +1845,8 @@ int QObject::startTimer(int interval, Qt::TimerType timerType)
     then the timer event occurs once every time there are no more window
     system events to process.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     The virtual timerEvent() function is called with the QTimerEvent
     event parameter class when a timer event occurs. Reimplement this
     function to get timer events.
@@ -1887,9 +1891,10 @@ int QObject::startTimer(std::chrono::nanoseconds interval, Qt::TimerType timerTy
 
     using namespace std::chrono_literals;
 
-    if (Q_UNLIKELY(interval < 0ns)) {
-        qWarning("QObject::startTimer: Timers cannot have negative intervals");
-        return 0;
+    if (interval < 0ns) {
+        qWarning("QObject::startTimer: negative intervals aren't allowed; the "
+                 "interval will be set to 1ms.");
+        interval = 1ms;
     }
 
     auto thisThreadData = d->threadData.loadRelaxed();
diff --git a/src/corelib/kernel/qtimer.cpp b/src/corelib/kernel/qtimer.cpp
index e3b4b0ec36d..796a82afe0b 100644
--- a/src/corelib/kernel/qtimer.cpp
+++ b/src/corelib/kernel/qtimer.cpp
@@ -233,6 +233,8 @@ void QTimer::start()
         timer.start();
     \endcode
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \note   Keeping the event loop busy with a zero-timer is bound to
             cause trouble and highly erratic behavior of the UI.
 */
@@ -241,9 +243,40 @@ void QTimer::start(int msec)
     start(msec * 1ms);
 }
 
+static std::chrono::milliseconds
+checkInterval(const char *caller, std::chrono::milliseconds interval)
+{
+    if (interval < 0ms) {
+        qWarning("%s: negative intervals aren't allowed; the interval will be set to 1ms.", caller);
+        interval = 1ms;
+    }
+    return interval;
+}
+
+/*!
+    \since 5.8
+    \overload
+
+    Starts or restarts the timer with a timeout of duration \a interval milliseconds.
+
+    \include qtimer.cpp stop-restart-timer
+
+    \include qtimer.cpp singleshot-activation
+    This is equivalent to:
+
+    \code
+        timer.setInterval(interval);
+        timer.start();
+    \endcode
+
+    \include timers-common.qdocinc negative-intervals-not-allowed
+*/
 void QTimer::start(std::chrono::milliseconds interval)
 {
     Q_D(QTimer);
+
+    interval = checkInterval("QTimer::start", interval);
+
     // This could be narrowing as the interval is stored in an `int` QProperty,
     // and the type can't be changed in Qt6.
     const int msec = interval.count();
@@ -370,6 +403,8 @@ void QTimer::singleShotImpl(std::chrono::nanoseconds ns, Qt::TimerType timerType
     The \a receiver is the receiving object and the \a member is the
     slot. The time interval is \a msec milliseconds.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \sa start()
 */
 
@@ -388,6 +423,8 @@ void QTimer::singleShotImpl(std::chrono::nanoseconds ns, Qt::TimerType timerType
     time interval is \a msec milliseconds. The \a timerType affects the
     accuracy of the timer.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \sa start()
 */
 
@@ -395,8 +432,9 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
                         const QObject *receiver, const char *member)
 {
     if (ns < 0ns) {
-        qWarning("QTimer::singleShot: Timers cannot have negative timeouts");
-        return;
+        qWarning("QTimer::singleShot: negative intervals aren't allowed; the "
+                 "interval will be set to 1ms.");
+        ns = 1ms;
     }
     if (receiver && member) {
         if (ns == 0ns) {
@@ -440,6 +478,8 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
     The \a interval parameter can be an \c int (interpreted as a millisecond
     count) or a \c std::chrono type that implicitly converts to nanoseconds.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \note In Qt versions prior to 6.8, the chrono overloads took chrono::milliseconds,
     not chrono::nanoseconds. The compiler will automatically convert for you,
     but the conversion may overflow for extremely large milliseconds counts.
@@ -462,6 +502,8 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
     The \a receiver is the receiving object and the \a member is the slot. The
     time interval is given in the duration object \a nsec.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
 //! [qtimer-ns-overflow]
     \note In Qt versions prior to 6.8, this function took chrono::milliseconds,
     not chrono::nanoseconds. The compiler will automatically convert for you,
@@ -487,6 +529,9 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
     time interval is given in the duration object \a nsec. The \a timerType affects the
     accuracy of the timer.
 
+
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \include qtimer.cpp qtimer-ns-overflow
 
     \sa start()
@@ -527,24 +572,6 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
 */
 
 /*!
-    \fn void QTimer::start(std::chrono::milliseconds msec)
-    \since 5.8
-    \overload
-
-    Starts or restarts the timer with a timeout of duration \a msec milliseconds.
-
-    \include qtimer.cpp stop-restart-timer
-
-    \include qtimer.cpp singleshot-activation
-    This is equivalent to:
-
-    \code
-        timer.setInterval(msec);
-        timer.start();
-    \endcode
-*/
-
-/*!
     \fn std::chrono::milliseconds QTimer::intervalAsDuration() const
     \since 5.8
 
@@ -604,6 +631,8 @@ QBindable<bool> QTimer::bindableSingleShot()
     stop() and then start() the timer, and acquire a new id().
     If the timer is not running, only the interval is changed.
 
+    \include timers-common.qdocinc negative-intervals-not-allowed
+
     \sa singleShot
 */
 void QTimer::setInterval(int msec)
@@ -614,6 +643,9 @@ void QTimer::setInterval(int msec)
 void QTimer::setInterval(std::chrono::milliseconds interval)
 {
     Q_D(QTimer);
+
+    interval = checkInterval("QTimer::setInterval", interval);
+
     // This could be narrowing as the interval is stored in an `int` QProperty,
     // and the type can't be changed in Qt6.
     const int msec = interval.count();
diff --git a/src/corelib/kernel/timers-common.qdocinc b/src/corelib/kernel/timers-common.qdocinc
index 2c87d6cf577..e801df3b2a3 100644
--- a/src/corelib/kernel/timers-common.qdocinc
+++ b/src/corelib/kernel/timers-common.qdocinc
@@ -22,3 +22,11 @@
     A disadvantage of using timerEvent() is that some high-level features,
     such as single-shot timers and signals, aren't supported.
 //! [q-chrono-timer-alternatives]
+
+// \1 is the method argument's name e.g. `\a interval`
+//! [negative-intervals-not-allowed]
+    Starting from Qt 6.10, setting a negative interval will result in a run-time warning
+    and the value being reset to 1ms. Before Qt 6.10 a Qt Timer would let you set a negative
+    interval but behave in surprising ways (for example stop the timer if it was running or
+    not start it at all).
+//! [negative-intervals-not-allowed]
author	Ahmad Samir <a.samirh78@gmail.com>	2024-12-19 14:10:31 +0200
committer	Ahmad Samir <a.samirh78@gmail.com>	2025-02-10 01:13:29 +0200
commit	f1f610bc67bfd5c2ef31270a6945e7bae93b5e4a (patch)
tree	95df313ad5a5a6bd79501073a6f7311a257d1a1b /src
parent	d4c312f68aee526848b7b2f0e4b77948e8b55e8f (diff)