Avoid dangling QTimer object

hbatalha

I am using QTimer to get a task update, when the task is finished I call the stop function and get a QSstemTrayIcon notification.

Here is the code I had:

// this function can be called more than once
// there is some condition that must be met before checking the task completion
void MainWindow::foo()
{
...

    if(checkTaskCompletion == true)
    {
         // timer is declared in the mainwindow class
        timer = new QTimer(this);

        connect(timer, &QTimer::timeout, this, &MainWindow::taskUpdate);
        
        timer->start(2'000);
    }
}

void MainWindow::taskUpdate()
{
    bool taskCompleted = false;
    // some code here that checks the task completion

    if(taskCompleted)
    {
        timer->stop();

       // I know that the MainWindow deletes it as it is its parent but I always avoid doing so when the object can be created dynamically multiple times
        timer->deleteLater();
        timer = nullptr;

        //notification code here
    }
}

I feared that if the foo() is called twice and the condition to check task completion is met and the timer is stiil running that another QTimer will be created and the previous timer object(the one that was still running) will be lost as timer now points to another location and is never stopped.

It is very unlikely that the scenario above described occurs but I don't want to risk it.

The main reason I am concerned about this is the in one time the timer didn't stop and and the taskUpdate() kept getting called and the notification kept being shown in an infinite loop.
I tried reproducing that behavior in every way I could with no luck. So changed the taskUpdate function to the following:

void MainWindow::taskUpdate()
{
    QTimer * timer1 = qobject_cast<QTimer*>(sender());

    bool taskCompleted = false;
    // some code here that checks the task completion

    if(taskCompleted)
    {
        timer1->stop();
        timer1->deleteLater();
        timer1 = nullptr;

        //notification code here
    }
}

The code works as expected but I am not sure it solves the problem. I fear that some creepy bug just might be lurking around.

jsulm

@hbatalha said in Avoid dangling QTimer object:

connect(timer, &QTimer::timeout, this, &MainWindow::taskUpdate);

You can connect a lambda to the slot and pass the timer as parameter to lambda and then to the actual slot:

connect(timer, &QTimer::timeout, [this, timer]() { MainWindow::taskUpdate(timer)});

SGaist

Hi,

Why do you need to delete your timer ?
From the looks of it, handling it with start and stop seems like it would be enough.

hbatalha

@SGaist said in Avoid dangling QTimer object:

From the looks of it, handling it with start and stop seems like it would be enough.

Because I know that the MainWindow deletes it as it is its parent but I always avoid doing so when the object can be created dynamically multiple times and that what happens in my app, the timer can be created with new dynamically as long as the app stays open, thus , even if it is small, it will be consuming memory.

hbatalha

@jsulm said in Avoid dangling QTimer object:

connect(timer, &QTimer::timeout, this, timer { MainWindow::taskUpdate(timer)});

Nice suggestion, I had thought about lambda but it was in a different way.
Thank you, I will probably change my code to this.

Christian Ehrlicher

@hbatalha said in Avoid dangling QTimer object:

the timer can be created with new dynamically as long as the app stays open, thus , even if it is small, it will be consuming memory.

Do you really think the downsides (make sure to delete it every time you don't need it, continuous memory (de)allocation) is it worth for ~20bytes? A member which is allocated one time would be much easier here.

hbatalha

Sorry for the late reply!

@Christian-Ehrlicher said in Avoid dangling QTimer object:

Do you really think the downsides (make sure to delete it every time you don't need it, continuous memory (de)allocation) is it worth for ~20bytes?

Actually that never crossed my mind, thanks. Are there any best practices guide for memory management in Qt?

A member which is allocated one time would be much easier here.

Does the same apply for Dialogs? Just an example:

void MainWindow::onPushButtonClicked()
{
    SomeDialog* diag = new SomeDialog(this);

    diag->setWindowFlags(Qt::WindowStaysOnTopHint);
    diag->setWindowFlags(Qt::Dialog);

    diag->exec();

    m_name = add->lineEdit->text();
    m_password = lineEdit2->text();
}

Should I just do it like this:

 SomeDialog*diag.;
...

 diag.exec();

Since I don't need SomeDialog outside that block of code.

Edit: I researched a little and found that since .exec() is blocking the object will not run out of scope so I should put them on stack.

JoeCFD

void MainWindow::foo()
{
...

if(checkTaskCompletion == true)
{
     // timer is declared in the mainwindow class
    if ( nullptr ==  timer ) {
       timer = new QTimer(this);
      connect(timer, &QTimer::timeout, this, &MainWindow::taskUpdate);
    }
   
   if ( !timer->isActive() ) { 
    timer->start(2'000);
  }
}

}

you can call foo n times.

JonB

@JoeCFD , @ anyone
Why not have just one QTimer timer instance as a class member? No newing, no deleteing.

See also https://forum.qt.io/topic/124404/class-pointer-and-non-pointer-member-function-question !

JoeCFD

@JonB I assume s/he has only one. It may be a good practice to write code like that. No more worries about crash or multiple calls to foo.

JonB

@JoeCFD
Yes, and? Sorry I don't understand what point you are making?

JoeCFD

Both QTimer timer and QTimer * timer{} are ok. The only difference is QTimer header file is included or not in the class header file.
This is not an issue. If C++ is used, new and delete is not a problem. You do it all day long.