Bug 66492

Have same problem in fedora core 1.

anybody can still reproduce this?

*** Bug 112872 has been marked as a duplicate of this bug. ***

*** Bug 119797 has been marked as a duplicate of this bug. ***

is this still reproducible with kde4?

Waiting for anybody to confirm the bug still exists.

*** Bug 167456 has been marked as a duplicate of this bug. ***

*** Bug 163536 has been marked as a duplicate of this bug. ***

lubos, i think we have enough confirmations now. :}

I'll just note that I never have and still do not see this in a KDE3 session. But bug #163536 is extremely reproducible (much more than 1/50) in KDE4.

I just wanted to open a report for this issue.
During the Beta and RC-phase of KDE 4.1 I had the described issue a few times.
Since an RC+-build of SuSE I have this problem all of the time.
I now use KDE 4.1.0 and the problem is still there always.
Using NVidia GeForce 7600 GT with NVidia 173.14.09 drivers

I can confirm for KDE 4.1.0 (Qt 4.4.1), NVidia GeForce 8600 GT with 177.13 beta proprietary drivers.

The problem still exists in KDE 4.1.2. This happens about 80% of the time. So it is VERY reproducable.

(In reply to comment #13)
> The problem still exists in KDE 4.1.2. This happens about 80% of the time. So
> it is VERY reproducable.

I'm also using nVidia. Driver version 169.12. FeForce 7100 GS

I also have this issue running on Nvidia and Dual Screen set up.

Compiled from source on 2008/10/01.

main screen: the screen saver uses 1/4 of the screen (top left hand)
Secondary screen: All blank.

I have exactly the same problem. It wasnt on ubuntu 8.04, but on upgrade to ubuntu 8.10  w/ kde 4 it happens most of the time 9/10.

I'm willing to give any information.

I'm seeing this problem with 4.1.85 (unofficial packages from kde42.debian.net at version 4:4.1.85+svn899741-0r1). It's reproducible about 50% of the time for certain GL screensavers, e.g. Euphoria, Flux, Gravity, KRotation, Particle Fountain, Solar Winds. None of the screensaver packaged in xscreensaver-gl have this issue.

Like some reporters, I'm using a nVidia GPU (GeForce 8600M GS with driver 177.82). Is this reproducible on Intel or ATI GPUs?

Further, I haven't see this happening prior (and including in) 4.1.x. I'm not sure whether the upgrade to 4.2 beta or something else caused it to appear.

I'm having the same problem as in Comment #17, except I'd say it's more like 66% of the time.

To reproduce, set screensaver as "Solar Winds (GL)" and click test a few times.
It results in the screensaver starting in the top-left corner with the rest of the screen white.

This happens in KDE 4.1.96 (KDE 4.2 RC1) with the latest beta NVIDIA drivers, version 180.22. My graphics card is GeForce Go7300.

Does not seem to be nvidia related. I get this problem with my Intel GMA X3000, too. It looks like the screensaver is run in demo mode instead of root mode.

*** Bug 165552 has been marked as a duplicate of this bug. ***

Ok if Bug 165552 is a duplicate, so i confirm that this bug is still not resolved even with kde-4.2.1 and also it happens to me with qt-4.5.0.

I also confirmed this bug with KDE 4.2.1 and QT 4.5.0, but I noticed something very interesting: this just happen (in my case) with OpenGL screensavers that comes with KDE. I mean, when I use an OpenGL screensaver of xscreensaver, like GLMatrix, I wasn't able to reproduce this bug. I've turned on and off GLMatrix more than 20 times and it always works correctly, with and without the option to show Plasma Widgets with screensaver.

My system:
- Gentoo Linux
- nVidia Drivers: 180.37
- QT: 4.5.0
- KDE: 4.2.1

* KScreenSaver was compiled with xscreensaver support (xscreensaver USE flag in Gentoo)

Created attachment 32504 [details]
Fix

Ok so I finally got fed up with this bug this week, and decided to not rest until it's solved :)

My analysis of the problem is: most screensavers just subclass KScreenSaver, and do their stuff on paintEvent(). Opengl screensavers on the other hand, need to work with a QGlWidget, instead of QWidget which is what KScreenSaver is, so they create a QGlWidget, and then use XEmbed to place that widget inside a KScreenSaver. The problem is that by doing this using X11 calls, the QGlWidget isn't alerted of changes on his parent in this case (because it has no parent, unlike normal widgets).

So my solution is for KScreenSaver to keep a pointer to the widget when embed() gets called, and to propagate resizeEvents to this widget when it receives them.

The attached patch implements this.

This patch fixes all the opengl screensavers I tested: kflux, kfountain, ksolarwinds, kwave, keuphoria and kgravity. To test it just start any screensaver from the shell (for example kflux.kss) and then resize the window: you'll notice that without the patch the "opengl" view is never resized.

I haven't commited this yet, but if I can get some confirmations that it's all ok (including trying with --setup and inside systemsettings), I'll commit this to trunk and to branch. Thanks in advance :)

I just tested this and resizing works fine now. But the main problem remains: sometimes the OpenGL screensavers take up only about a quarter of the screen in the upper left. Sorry.

Created attachment 32507 [details]
Fix, try 2

Thanks a lot for testing. Yeah you were right, after hammering on it for a while eventually I get the bug again, so it seems that patch reduced the frequency it happened, but didn't fully remove the bug.

Here's a new patch with a fix to set the gl widget size before XEmbed'ing it: I think this was what was missing to get it fully working.

If you could please re-test, thanks :)

(In reply to comment #25)

the good news: the problem seems to be gone

the bad news: WIth intensive testing I get kernel panics (Linux)

the even worse news: I have not figured out, yet, if it is related to the fix.

> Here's a new patch with a fix to set the gl widget size before XEmbed'ing it: I
> think this was what was missing to get it fully working.

I don't want to be mean, but... is this really a fix? Shouldn't the inital size be the right one without setting it explicitely?

Nice to see it seems to be fixed for you :)

As for the kernel panics, I tested it pretty hard before posting this second patch, and no problems at all here (pc on since I woke up this morning). I'm running on an nvidia 8600GT with the 180.29 drivers.

Finally,
> I don't want to be mean, but... is this really a fix? Shouldn't the inital size
> be the right one without setting it explicitely?

The problem is that the real screensaver widget is KScreenSaver. The XEmbed is there to suck another top-level window (the one with the opengl animation) inside the screensaver one. I did a small test application and found that if you don't give a size to a top-level QGlWidget, the default will be 640x480 so I'm guessing part of the bug was a race: maybe the default size was set before the embed, maybe after, and that was causing the "works sometimes, other times it doesn't".

In both my small test app and kscreensaver, setting the right size (e.g. telling the gl widget that its size should be that of the real screensaver widget) before XEmbed'ding did the trick.

The crash is indeed related to the fix. The crash is even worse and I can reproduce it. But I agree, that this is probably driver and maybe hardware related (but I am not sure, I am using an Intel G965 with xorg-video-intel-2.5.1).

steps to reproduce (DANGER!):
1. open Konsole
2. run keuphoria.kss
3. close it
4. run keuphoria.kss --root
5. resize that window (this is where the patch comes in!)
6. close the window
7. keuphoria.kss will not stop before killing the process with SIGTERM (ctrl+c)
8. press ctrl+c
9. open systemsettings
10. choose Desktop, then Screen Saver
11. choose Euphoria and click Test

This results in a hard lock. Caps Lock and Scroll Lock are blinking.

(After turning the computer off by pressing the power button for five seconds, and turning it on again, X does not start on first try, because the graphics chip is still in an invalid state.)

Anyway, this probably does not have anything to do with this bug. However, I am not sure, yet, where to open a new bug. :)

I can reproduce that crash here -- although I get some X Errors on the shell when I close the screensaver window, but those were already there before the patch.

Let's hope someone with an Intel driver can test this. Unfortunately, I only have access to ati and nvidia hardware.

Damn, I meant to say "I can't", not I can. It works fine here.

the patch is, ermm, "suboptimal" (it breaks binary compatibility and disregards the coding style), but the analysis of the problem is very helpful. :)

martin: nvidia is very happy about every linux driver bug report they get (of course they are, right? there can never be enough). so make them happy. :D

(In reply to comment #31)
> the patch is, ermm, "suboptimal" (it breaks binary compatibility and disregards
> the coding style), but the analysis of the problem is very helpful. :)

I could catch the resize inside the event() call, instead of inside resizeEvent(). As for avoiding the slot for saving the QWidget the only way I can think of is to create a wrapper class that is a QObject, and setting the KScreenSaver as parent, and when we need to access it (not very often), ask for the children of KScreenSaver and search for that object.

As for the coding style, I tried to copy the original. Could you point out where I got it wrong? The only thing I can think of is adding a space after each '(' and before each ')' .

> martin: nvidia is very happy about every linux driver bug report they get (of
> course they are, right? there can never be enough). so make them happy. :D

Martin seems to be having problems with intel, not nvidia hardware.

Created attachment 32515 [details]
Fix, try 3

So, third version of the patch. This one should preserve binary compatibility: I catch the resize event inside event() and use the d-pointer to keep the extra member I needed (before I was confused because I thought other subclasses were also using the d-pointer but I understand now).

I also tried to add the extra spaces to preserve the original style.

Is this version of the patch acceptable? Thanks for the feedback so far.

That fix also works fine.
Concerning binary compatibility: also seems to be good. Works for me without relinking.

that one is technically correct. there is still room for improvement, though: :)

i don't think you need to include QResizeEvent.
there is a resize() overload which takes a QSize.
trivial constructors of private classes are typically inlined.
the creation of the private object is typically put into the c'tor's variable initializer list (i.e., before the opening brace).
you are leaking the private object.
given that the private object contains exactly one pointer, it might be preferable to replace the unused d pointer with that one, adding a big fat note that if more members are added, they need to be moved to a private object.
make a line break after the if's condition.

Created attachment 32516 [details]
Fix, try 4

(In reply to comment #35)

Fixed most of the suggestions, thanks (the patch seems to be getting smaller, a few more iterations and don't I anything will be left).

> given that the private object contains exactly one pointer, it might be
> preferable to replace the unused d pointer with that one, adding a big fat note
> that if more members are added, they need to be moved to a private object.

As for this one, I think that would just complicate things, for a savings of a couple of extra bytes, so I think this way is cleaner. (But if you think that it really should be done that way, I could do that too).

Thanks

(In reply to comment #36)
> (the patch seems to be getting smaller, a
> few more iterations and don't I anything will be left).
> 
hehe

> > [...] it might be
> > preferable to replace the unused d pointer with that one, [...]
> 
> As for this one, I think that would just complicate things, for a savings of a
> couple of extra bytes, so I think this way is cleaner.
>
it wouldn't complicate anything, as it would be just the straight-forward, non-d-pointer-ized variant. in fact, it would work towards the goal of making the patch disappear. :D
it is true that it would complicate the next addition, as that one would have to do the d pointer transformation in the first place, but given the frequency at which new members are added, this seems fairly acceptable to me. :)

Created attachment 32517 [details]
Fix, try 6

This new version uses the d-pointer to store the widget.

If it's ok with you I'll commit it (always wanted do kill a many-year-old-bug) :)

I'd prefer if we didn't ask for ubergeek ugly hacks for reasons that absolutely don't matter in practice. Please use the version with the proper d pointer.

huh, i didn't ask you to do typecasts - that's just plain ugly. replace the declaration of d accordingly and leave a note that it needs to be transformed into a d pointer once more members are needed. this "trick" is used fairly often in qt and kde code.

of course, the unnecessary use of a private object doesn't matter at all in this case, but having the minimal implementation just makes the code nicer to read. and, after all, ivo will now have tried about every conceivable approach to the problem - for good practice. try 4 is good enough if the practice is not called for. :D

Created attachment 32523 [details]
Fix, try 7 (hey, whatever happened to 5!?)

I misunderstood what Oswald was saying. I think I got it, so here's a version that "replaces" the dpointer with the pointer to the widget.

So, try 4 or try 7, take your pick :)

haha, ok, one more try: use the member name from try 1, and move the comment above the commented out d pointer, altering it to something like "for binary compatibility reasons, this class must have exactly one data member which is a pointer. if more members are added, uncomment the private object and move the members into it".

yes, i was also wondering about #5. i assume it was eaten by today's date.

Created attachment 32527 [details]
Fix, try 8

Ok, so I've gone back to embeddedWidget, and added the note to the header file.

u can't trick me - this is try 7! and that's a fairly good number for a final version. :-D

i assume do do the backport as well?

Ok so I'll commit try 8 (or is it 7!?) when I get home, and I'll also backport it (don't know if there's going to be a 4.2.3 though, but I hope so, so I can get this fix via distro packages).

Attachment #32527 [details] also works fine. Resizing works, full screen is used and it
is binary compatible.

(What I did not test was the Kernel panic, which is very likely not gone, but I
do not want to let my system crash again :))

SVN commit 948339 by ianjo:

Pass along resize events to the embedded widget, and also set the correct
size for the widget before using XReparentWindow on it.

Thanks for all the feedback and testing of the patch!

BUG: 66492



 M  +5 -1      kscreensaver.cpp  
 M  +5 -1      kscreensaver.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=948339

SVN commit 948349 by ianjo:

Backport r948339.
Pass along resize events to the embedded widget, and also set the
correct size for the widget before using XReparentWindow on it.

Thanks for all the feedback and testing of the patch!

CCBUG: 66492



 M  +5 -1      kscreensaver.cpp  
 M  +5 -1      kscreensaver.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=948349

Hey, thanks for fixing this!! Now I can use all the beautiful screen savers again! :D

*** Bug 165413 has been marked as a duplicate of this bug. ***

*** Bug 183819 has been marked as a duplicate of this bug. ***