QTimer does not stop

herrgross

Hi everyone,
maybe somebody can help me understand, why the QTimer does not stop in this piece of code, where I use a Button for start and stop. What's wrong?

void Wanderung::on_wanderButton_clicked()
{
static bool state = true;
QTimer *timer = new QTimer(this);
connect(timer, &QTimer::timeout, this, &Wanderung::wandern);
if (state)
{
state = !state;
timer->start(100);
}
else
{
state = !state;
timer->stop();
}
}

JonB

@herrgross
Because each time you call it you create a new QTimer instance! :) The second time it is called the first instance will not be stopped, and will never be stopped, so it will keep going....

J.Hilk

@herrgross you're creating multiple QTimers, on on each button click -> stop call is on the new timer, and the old one simply exists in the ether with no pointer pointing to it

herrgross

@J-Hilk @JonB
ok, right, got it... now I have to find the place where to create the timer...
thanks

JonB

@herrgross
The QTimer *timer needs to be a member variable of your class, so that it persists outside of whatever method it is created in. Usually in constructor, but you can also create it on demand in on_wanderButton_clicked() so long as you check whether the pointer already points to a created instance. In C++ you can also just create a QTimer timer (not pointer) in your class and use that, no creating/destroying needed.

herrgross

@JonB
wow!!! it works! thanxalot!!

Pl45m4

@herrgross

You state logic also seems not correct.
I can see no way how state is ever false. You set a new variable state to true when the button is clicked, after that it can never be false in the same slot. So even if you fixed the timer issue, this logic might not work as expected.

JonB

@Pl45m4
It's static, and he toggles its value in both/all paths each time button is clicked.

@herrgross
static is bad actually nowadays. Change that (state) also to a member variable, or retrieve state some other way (e.g. timer.isActive() should suffice for this code to toggle its started/stopped).

Pl45m4

@JonB said in QTimer does not stop:

@Pl45m4
It's static, and he toggles it's value in both/all paths each time button is clicked.

Ouh, missed the static keyword.

@herrgross Declaring a variable static in a slot every time the slot is called , is also not very good.

Edit: As @JonB said above, if you make it a member like your timer, you don't even need it to be static.

herrgross

@Pl45m4 @JonB
yes, if (!timer.isActive())
works as well and is shorter and no need to add in constructor....
thankyou

aha_1980

@Pl45m4 said in QTimer does not stop:

@herrgross Declaring a variable static in a slot every time the slot is called , is also not very good.

Sorry, I have to disagree. A static variable is just a global variable with limited scope (in this case: function scope) and initialization at the first time.

There is no performance penalty, and of course you can use static variable in a slot.

Regards

SimonSchroeder

@aha_1980 said in QTimer does not stop:

There is no performance penalty, and of course you can use static variable in a slot.

You are correct about that. However, just because you can use it, it doesn't mean you should. And in this specific case you should not. In case there are more instances of Wanderung in the future, the behavior becomes very confusing! And even if there won't be more instances of Wanderung this will start a really bad habit which is hard/annoying to debug & fix in other places. Most of the times in member functions/slots you will actually need a member variable and not a static variable (and a distant second is class variables before static variables inside member functions; static variables make a lot more sense in free-standing functions).

aha_1980

@SimonSchroeder You are absolutely right about that, but see that I wrote: is just a global variable, so everything about global variables apply.

I also fully agree that here a static variable is unneeded, my point was to justify "Declaring a variable static in a slot every time the slot is called , is also not very good", because that is not correct.

Regards

Pl45m4

@aha_1980 said in QTimer does not stop:

I also fully agree that here a static variable is unneeded, my point was to justify "Declaring a variable static in a slot every time the slot is called , is also not very good", because that is not correct.

Haha yes, but my statement was that it's not good. As we can see from @herrgross reply, it worked.
But this doesn't mean, that it's good style or a good practise to do so all the time. I wouldnt expect a static variable in a slot, especially when there are at least two better ways to achieve the same :)
(using the QTimer API, or adding some non-static member to check if timer needs to be stopped or started)