Minimize window = images gone

[sooofunky]

Hello,

I am using the paintEvent method to show images on a QFrame. When starting the program, the images are shown correctly most of the times (sometimes they do not appear or in the wrong places). When I minimize the window or it gets overlapped by another, all images disappear. Any ideas what the problem might be?

Thanks!

[wysota]

The problem might be with your code.

[sooofunky]

Thanks for replying. In WeatherFrame.cpp I am doing the drawing - not sure what is wrong.





#include "WeatherFrame.h"

WeatherFrame::WeatherFrame(QWidget *parent)
: QFrame(parent)
{
mEngine = new GoogleEngine();
connect(mEngine, SIGNAL(weatherReady()), this, SLOT(handleData()));
connect(mEngine, SIGNAL(weatherError()), this, SLOT(handleError()));

mEngine->location().city() = "Salzburg";
mEngine->update("Salzburg");
}

WeatherFrame::~WeatherFrame()
{
delete mEngine;
}

void WeatherFrame::handleData()
{
mCurrentLocation = mEngine->location();
mCurrentWeather = mEngine->weather();
mListForecast = mEngine->forecast();
mReady = true;
update();
}

void WeatherFrame::handleError()
{
qDebug() << "handleerror";
}

void WeatherFrame::paintEvent(QPaintEvent* event)
{
if (mReady) {
Q_UNUSED(event);
QPainter painter(this);
painter.setRenderHints(QPainter::HighQualityAntial iasing | QPainter::SmoothPixmapTransform);

// draw current temperature
painter.setPen(Qt::black);
painter.setFont(QFont("Arial", 16));
painter.drawText(QRect(5,2,100,30), Qt::AlignLeft, "Wetter -");

// draw location's city name
painter.setFont(QFont("Arial", 16));
painter.drawText(QRect(100,2,100,30), Qt::AlignLeft, mCurrentLocation.city());

// draw condition pixmap
painter.drawPixmap(QRect(10,55,50,50),mCurrentWeat her.pixmap());

painter.setFont(QFont("Arial", 12));
painter.drawText(QRect(20,25,60,40), Qt::AlignCenter, "HEUTE");

painter.setFont(QFont("Arial", 14));
painter.drawText(QRect(20,105,40,40), Qt::AlignCenter,
QString::number(FahrenheitToGrad(mCurrentWeather.t emperature())));

for (int i = 0; i < mListForecast.count(); i++) {
Weather weather = mListForecast.at(i);

// draw forecast day
painter.setFont(QFont("Arial", 12));
painter.drawText(QRect((i+1)*100+20,25,40,40), Qt::AlignCenter, weather.day().toUpper());

// draw condition pixmap
painter.drawPixmap(QRect((i+1)*100+20,55,50,50), weather.pixmap());

// draw forecast low and high temperature
painter.setFont(QFont("Arial", 10));
painter.drawText(QRect((i+1)*100+20, 95,40,40), Qt::AlignCenter, QString::number(weather.lowTemp()));
painter.drawText(QRect((i+1)*100+20, 110,40,40), Qt::AlignCenter, QString::number(weather.highTemp()));

}

mReady = false;
} else {
QWidget::paintEvent(event);
}
}

int WeatherFrame::FahrenheitToGrad(int f)
{
int c = (f-32) / 1.8;
return c;
}

[pan]

Put a break point in your paint event. Check the value of mReady. Or put in some logging.

You will notice how often it repaints and how often your mReady is false. This should help you see why it is not repainted after you have minimized.

[sooofunky]

ouch, somehow I thought it must be a much more severe problem... Thanks!

[Rachol]

It is quite obvious to me what is happenig. You are drawin the widget only once when the data is available and then you draw nothing. However you must draw something when window is being restored.

Much better solution would be do have some QPixmap m_weatherPixmap declared as your private member variable. Then also declare something like updatePixmap, where you draw your weather info into a pixmap;

In handleData you should clear the pixmap: m_weatherPixmap = QPixmap();

in paintEvent:


if(m_weatherPixmap.isEmpty())
{
if(mReady)
{
updatePixmap(m_weatherPixmap);
}
}

QPainter p(this);
p->drawPixmap(0,0,m_weatherPixmap);


Hope this helps.

[wysota]

Changing any variable value in paintEvent is a sure misdesign.

[Rachol]

Not really, I have never heard of anyone saying that before, in many situations that is the only way you can do something, especially when performance comes into the game.

[wysota]

Example please.

[Rachol]

Example:

You have a QWidget that draws itself based on some multiple properties. This code will be run on a very slow machine. Drawing the widget from the beginning every time is slower than using stored pixmap. Now, if you don't want this pixmap to be redrawn every time you change a property, what do you do?

[nish]

Example:

You have a QWidget that draws itself based on some multiple properties. This code will be run on a very slow machine. Drawing the widget from the beginning every time is slower than using stored pixmap. Now, if you don't want this pixmap to be redrawn every time you change a property, what do you do?

Sorry to pop in, Your sentence is little bit confusing to me. Can you give some sample code and we will see if we can improve upon it?

[Rachol]

I don't see anything confusing in there, it is quite simple. Where is the confusing part? And why should I get a code for imaginary case? I just try to prove that saying that "changing a variable in paintEvent is a misdesign" is totally wrong.

[wysota]

You have a QWidget that draws itself based on some multiple properties. This code will be run on a very slow machine. Drawing the widget from the beginning every time is slower than using stored pixmap. Now, if you don't want this pixmap to be redrawn every time you change a property, what do you do?

Then you render the widget to the pixmap before the paint event is invoked and then only render the pixmap in the event. But I'm not sure why you wouldn't want it the widget to be redrawn when its contents change...

[Rachol]

Because sometimes you can change couple properties and you don't want the pixmap to be rendered twice, or more times before the paintEvent comes.

[wysota]

Why would the pixmap be rendered twice? You can delay your pixmap generating code the same way as paint event is delayed. I agree you can generate the pixmap in the paint event but there are alternatives that have their advantages. Like doing the rendering in an external thread.

[Rachol]

Thanks for agreeing that indeed changing variables in the paintEvent ain't misdesign. Delaying... can't be as optimized as drawing in paintEvent, cause paintEvent is called only and only when necessary. You could of course do it so it is done just before paintEvent is called, but then it is quite much the same thing, but more complicated and unnecessary work is done. Then rendering in a separated thread could be done, but then again it depends on the case. I used to work with a embedded device that couldn't handle well multithreading applications..., so I agree that there are many options it just to choose the option that fits best your case.

[wysota]

Thanks for agreeing that indeed changing variables in the paintEvent ain't misdesign.
I said nothing like that. I said you could do it in paintEvent but I didn't say it's a good design. Unless of course you like having frozen GUI. Paint event should be executed as fast as possible and all unnecessary code should be stripped out of the event handler.


Delaying... can't be as optimized as drawing in paintEvent, cause paintEvent is called only and only when necessary.
I don't see your point. The amount of work to be done is the same -- you need to render to a pixmap and then render the pixmap to the widget. It's the decision when to do it that makes a difference. There is nothing "optimal" in drawing to the pixmap in paint event. If you change a property that affects the looks of the widget then eventually you'll want to display those changes so every update of the pixmap involves triggering a repaint. And every repaint doesn't have to check and update the pixmap because if there was something to update, pixmap updating code would have triggered a repaint.


You could of course do it so it is done just before paintEvent is called, but then it is quite much the same thing
No, it's not the same thing. Especially if your point is that you're working with a slow machine. In that case you want to save every cycle you can.


Then rendering in a separated thread could be done, but then again it depends on the case. I used to work with a embedded device that couldn't handle well multithreading applications...
That's not an argument. If you have a device that's not fit to be used with some solution then you don't use that solution. Extreme cases sometimes require extreme measures. However it doesn't make those measures good solutions. And it doesn't make a good design bad just because there exists a device that involves some special treatment.

I've seen many people do such crazy flag resetting in paint events. It's caused by a false assumption that the painting code will be called only in conditions they specify which is not the case because the code can be called an arbitrary number of times per second. Updating pixmaps in paint events is also such a false assumption. If you start updating pixmaps at this pace in every paint event (assuming some changes indeed trigger those repaints), you'll be creating pixmaps your user will never even have a chance to see simply because the eye is not able to spot differences at such pace. It's enough to update the pixmap one-two times a second (depending on the nature of the data you are displaying) even if you happen to miss 1000 repaints that will render a stale pixmap. Eventually you'll render the right pixmap and it doesn't matter if it happens at repaint 1000 or 1001. What matters is that you'll render the pixmap once which, as you said yourself, is an expensive operation. You don't have such abilities if you update the thread in paint event --- you'll have to do it in every paint event.

[Rachol]

Unless of course you like having frozen GUI

Most ridiculous thing I have read in this thread. So you mean doing heavy operation anywhere else in one threaded app will not freeze the GUI?


If you change a property that affects the looks of the widget then eventually you'll want to display those changes so every update of the pixmap involves triggering a repaint. And every repaint doesn't have to check and update the pixmap because if there was something to update, pixmap updating code would have triggered a repaint.

Again wrong. So in case where 10 of properties change, you will update the pixmap 10 times? Yeah... that sounds like a solution...


That's not an argument. If you have a device that's not fit to be used with some solution then you don't use that solution.

Isn't that exactly the same thing what I have written?


You don't have such abilities if you update the thread in paint event --- you'll have to do it in every paint event.

Do you actually follow what I am writing? Please take a look at the code I have posted already in this thread. Short version:


if(m_weatherPixmap.isEmpty())
{
updatePixmap(m_weatherPixmap);
}

I do only an update when it is needed. I am updating something in every paintEvent?

Final word:

I see this conversion becoming unpleasant and I start seeing no point in going with it any forward. We can point out our imaginary situations and keep fighting over stuff, when in most cases we are both right. One thing for me is clear in here with all the respect, you are wrong and you can't say it is a misdesign to set a member variable in paintEvent. If it was so, then I bet that paintEvent would be declared as const method and it isn't. sizeHint() for example is a const method.

And now grande finale:

Take a look at http://doc.qt.nokia.com/latest/painting-svgviewer-svgview-cpp.html and its implementation of paintEvent.

[wysota]

Most ridiculous thing I have read in this thread. So you mean doing heavy operation anywhere else in one threaded app will not freeze the GUI?
If it is not trying to redraw at this point then no.


Again wrong. So in case where 10 of properties change, you will update the pixmap 10 times? Yeah... that sounds like a solution...
No, I will update it at most once. Which part of the sentence I have written suggests that each property change will cause a separate update of the pixmap?


Isn't that exactly the same thing what I have written?
Please don't cut out my thoughts in half. A special case doesn't change a bad solution into a good one (we usually call them "workarounds" then).


Do you actually follow what I am writing? Please take a look at the code I have posted already in this thread. Short version:


if(m_weatherPixmap.isEmpty())
{
updatePixmap(m_weatherPixmap);
}

I do only an update when it is needed. I am updating something in every paintEvent?
Consider the following logic. Suppose you have a property "prop" that influences the way a widget looks.

void Cls::setProp(... val) {
if(m_prop == val) return;
m_prop = val;
update();
}
// ...
QTimer *t = new QTimer;
connect(t, SIGNAL(timeout()), SLOT(doSomething()));
t->start(10); // 100Hz
// ...
void Cls::doSomething() {
clsObj->setProp(...);
}
Your pixmap will be updated 100 times a second, the sequence of events will be timeout, paint, timeout, paint, timeout, paint. I take it that you are a super human and will notice all those changes. Most people won't. A much better implementation will schedule a pixmap rebuild instead of calling update() in setProp. And the routine will make sure the pixmap is recreated at most X times a second where X is a sufficiently low value. I'm sure your slow device will benefit more from such solution than updating the pixmap at 100Hz.


I see this conversion becoming unpleasant and I start seeing no point in going with it any forward. We can point out our imaginary situations and keep fighting over stuff, when in most cases we are both right. One thing for me is clear in here with all the respect, you are wrong and you can't say it is a misdesign to set a member variable in paintEvent. If it was so, then I bet that paintEvent would be declared as const method and it isn't. sizeHint() for example is a const method.
It can't be const because you are passing a pointer to "this" to QPainter constructor which would break constness. In other words it wouldn't be possible to paint on a widget from within a const method without breaking constness.


And now grande finale:

Take a look at http://doc.qt.nokia.com/latest/painting-svgviewer-svgview-cpp.html and its implementation of paintEvent.

So your argument is that all code in Qt is perfectly designed? It's a pitty it is not so :) Just take a look at the "threaded fortune cookie" example and ask Bradley Hughes what he thinks of it :) Or look at one of the last Qt blog entries introducing ideas for Qt5 to see how much of Qt4 is considered "flawed design".

My opinion is that you should act as if paintEvent() was const and so far I haven't heard anything that would convince me otherwise. Your pixmap updating idea is of course fine and it's often used (I often use it myself) but it is not necessary to violate "constness" of paintEvent to use such an approach. Not every simple code is good code. Simple solutions are often the best but not in every case. Please don't take it personally, I find this discussion very interesting and useful. It's one of those times when you have to really stretch your skills instead of just pointing people to Qt docs as it is the case with 90% of issues people come here with.

[Rachol]

I like your solution, but it is with some disadvantages:

it just makes me do extra stuff ( handle updates every X ms ->extra timer, connection, method).
If during the update time, m_prop changes twice, coming back to previous value, I miss that... Updating in paintEvent would do the update as soon as possible after update() is called. In such situation it is about changing the update value to something really small, which brings as back to the same thing, updating pixmap rapidly and then we can still miss the change. On the other hand a hidden widget wouldn't have to update itself, but in your solution it still has to do some extra checks and repainting. Going further it forces you not to do unnecessary updates when widget hidden and also show the correct content of the widget when it becomes visible, most probably without a delay. All this introduces a huge amount of things you have to take care of, that you wouldn't even think of if the logic was in paintEvent.

[wysota]

it just makes me do extra stuff ( handle updates every X ms ->extra timer, connection, method).
Yes, you need to write some more code. But that's usually the case with "smart" solutions.


If during the update time, m_prop changes twice, coming back to previous value, I miss that... Updating in paintEvent would do the update as soon as possible after update() is called.
I'm afraid I don't understand. Oh... you meant the 100Hz thing. Yes, you can miss that a value was different for a short period. But then if you are interested in such cases you would rather draw the value as a plot and the spike would be visible there. There can always be a situation when data updates more often than it is redrawn and you'll always miss such changes. That's the discrete nature of computers for you. As I always say, you can't beat the Nyquist-Shannon theorem.


On the other hand a hidden widget wouldn't have to update itself, but in your solution it still has to do some extra checks and repainting.
If the widget is hidden, you can disable updating mechanism and enable it back in showEvent(). There is still a case when someone calls QWidget::render() on you while the widget is hidden but that will end badly for regular hidden widgets as well.


All this introduces a huge amount of things you have to take care of, that you wouldn't even think of if the logic was in paintEvent.
That's true. I already admitted that a solution with paintEvent() is much simpler. But it doesn't change my opinion. Implicit sharing is far from being simple but it can save quite a lot of cpu cycles. And putting our feet back on the ground again -- the approach OP has taken is a bad design without any discussion. Maybe calling any assignments in paint event a misdesign was a bit too much to say (although that is my opinion, I just might have kept it to myself, maybe I should have said "poor design") but deciding about painting based on value of a flag you are manipulating in the paint event is definitely an error.

[Rachol]

So what other events deserve to be maintained as they were const?

[wysota]

I don't think there are any or at least none that I use myself. paintEvent() is special because we want to maximize its speed as it influences the user experience the most.

[Rachol]

I still can't quite get why a paintEvent should be treated differently... Could you give me an example?

I know you have already pointed out that Qt code is not ideal, but then think about QLabel. QLabel changes its member variables( QImage and QPixmap instances). How would you otherwise implement image in QLabel, so it is working as fast as possible?

[wysota]

I don't understand what you mean. paintEvent is executed much more often than setPixmap() or setText() in QLabel so you want to make the event method as efficient as possible. That's also why you should pay attention which part of a widget needs redrawing.

[Rachol]

QLabel changes its member variables in paintEvent, as many other classes as well.

So, you try to say that again Qt implementation is wrong and QLabel should be implemented differently?

[wysota]

Yes, I would say so. But then it is only my opinion so you don't have to agree. With the current implementation QLabel will be slow if you enable pixmap scaling and start smoothly resizing the widget. Of course it doesn't come from modifying variables in paintEvent but from the whole approach in generating the pixmap during paint event. If possible, all such actions should be delegated to another thread. It could be controlled by some environment variable or compilation switch.

[Rachol]

whole approach in generating the pixmap during paint event

Please propose some more rapid solution( one threaded ). You can't make it any way better than it is right now. The only thing you can do is to decrease the performance or the user experience. The code that is executed in the paintEvent() must be eventually executed anywhere else, so there is no gain in removing staff from paintEvent. You can get your widget flicker, On top of that you will get a bunch of code that is quite much unnecessary complicated. Then overriding virtual methods makes your widget slower as well.

Now I have been trying to show examples where changing member variables in paintEvent is a recommended/preffered/better way of doing it. However I failed to make you see it. Now please give me an example where changing member variable in paintEvent is a bad thing.

[wysota]

Please propose some more rapid solution( one threaded ). You can't make it any way better than it is right now.
To be honest I'd get rid of "scaledContents" property of QLabel at all :) But seriously, I don't have answers to all the questions and I won't pull a rabbit out of my hat here.


The code that is executed in the paintEvent() must be eventually executed anywhere else
That's true. But it doesn't have to be executed here.


Now I have been trying to show examples where changing member variables in paintEvent is a recommended/preffered/better way of doing it.
Recommended by whom? By Trolls? Is overriding access scopes by static casting to an artificial subclass declaring some other class or method a friend a recommended approach as well?

class X {
private:
Member x;
};

class XHack : public X {
public:
friend class SomeClass;
};
// ...
X *a = ...
XHack *aHack = (XHack*)a;
aHack->x;

It's hacky, I admit. I use it myself, I admit. But is it recommended or good? Certainly not. It's a workaround and a bad one, considering C++...


Now please give me an example where changing member variable in paintEvent is a bad thing.
Have a look at the beginning of this thread, you have an example there. And some people even do all their calculations in paintEvent(), changing members or not. My point is not about changing variables but about doing stuff that should be done elsewhere. Setting variables is only a result of such bad decisions. And since we perceive the world by results and not reasons, settings variables in paintEvent is a symptom of a possible bad design. I hope I made myself clear now. I don't say what happens in QLabel is bad design per se the same way I don't say what "threaded fortune server" example does is wrong. But it leads to abuses that would have not been possible if the design was different. People tend to copy things without thinking whether their situation is similar to the one in the example. In my opinion it should have been done differently and I stand by what I said. For me paintEvent() should be made const if that was possible without breaking too many rules. We can continue this discussion forever but this simply won't change my opinion and I understand it won't change yours as well, I respect that.