Message ID | 20190930215906.1058301-3-niklas.soderlund@ragnatech.se |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Niklas, Thank you for the patch. On Mon, Sep 30, 2019 at 11:59:05PM +0200, Niklas Söderlund wrote: > Stopping the timer will reset the Timer::deadline_ field to 0 fixing > potential bugs and call QtEventDispatcher::unregisterTimer() which will > take care of the cleanup. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/qcam/qt_event_dispatcher.cpp | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/qcam/qt_event_dispatcher.cpp b/src/qcam/qt_event_dispatcher.cpp > index 98d2472c37856642..43588ef081a3f633 100644 > --- a/src/qcam/qt_event_dispatcher.cpp > +++ b/src/qcam/qt_event_dispatcher.cpp > @@ -131,9 +131,9 @@ void QtEventDispatcher::unregisterTimer(Timer *timer) > void QtEventDispatcher::timerEvent(QTimerEvent *event) > { > auto it = timers_.find(event->timerId()); > - timerIds_.erase(it->second); > - killTimer(it->first); > - timers_.erase(it); > + Timer *timer = it->second; Could this be simplified to Timer *timer = timers_[event->timerId()]; timer->stop(); ? If you think there's a risk that timerEvent would be called with a non-registered timer (bug in Qt or in qcam ?), then you can keep using find(), but you should then check for it == timers_.end(). I'll let you check if this would be overkill or useful. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + timer->stop(); > } > > void QtEventDispatcher::processEvents()
Hi Laurent, Thanks for your feedback. On 2019-10-01 04:06:45 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Mon, Sep 30, 2019 at 11:59:05PM +0200, Niklas Söderlund wrote: > > Stopping the timer will reset the Timer::deadline_ field to 0 fixing > > potential bugs and call QtEventDispatcher::unregisterTimer() which will > > take care of the cleanup. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/qcam/qt_event_dispatcher.cpp | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/src/qcam/qt_event_dispatcher.cpp b/src/qcam/qt_event_dispatcher.cpp > > index 98d2472c37856642..43588ef081a3f633 100644 > > --- a/src/qcam/qt_event_dispatcher.cpp > > +++ b/src/qcam/qt_event_dispatcher.cpp > > @@ -131,9 +131,9 @@ void QtEventDispatcher::unregisterTimer(Timer *timer) > > void QtEventDispatcher::timerEvent(QTimerEvent *event) > > { > > auto it = timers_.find(event->timerId()); > > - timerIds_.erase(it->second); > > - killTimer(it->first); > > - timers_.erase(it); > > + Timer *timer = it->second; > > Could this be simplified to > > Timer *timer = timers_[event->timerId()]; > timer->stop(); > > ? If you think there's a risk that timerEvent would be called with a > non-registered timer (bug in Qt or in qcam ?), then you can keep using > find(), but you should then check for it == timers_.end(). I'll let you > check if this would be overkill or useful. I like this change and I have taken it in together with your tag, thanks! > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + > > + timer->stop(); > > } > > > > void QtEventDispatcher::processEvents() > > -- > Regards, > > Laurent Pinchart
diff --git a/src/qcam/qt_event_dispatcher.cpp b/src/qcam/qt_event_dispatcher.cpp index 98d2472c37856642..43588ef081a3f633 100644 --- a/src/qcam/qt_event_dispatcher.cpp +++ b/src/qcam/qt_event_dispatcher.cpp @@ -131,9 +131,9 @@ void QtEventDispatcher::unregisterTimer(Timer *timer) void QtEventDispatcher::timerEvent(QTimerEvent *event) { auto it = timers_.find(event->timerId()); - timerIds_.erase(it->second); - killTimer(it->first); - timers_.erase(it); + Timer *timer = it->second; + + timer->stop(); } void QtEventDispatcher::processEvents()
Stopping the timer will reset the Timer::deadline_ field to 0 fixing potential bugs and call QtEventDispatcher::unregisterTimer() which will take care of the cleanup. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/qcam/qt_event_dispatcher.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)