Bug 1289

Don't try to apply it yet. The patch breaks compilation.

Created attachment 939 [details]
fixes the compilation without xcomposite

Comment on attachment 939 [details]
fixes the compilation without xcomposite

Now it should work fine.

Would you combine these patches into one?

How does this patching affect usability? For example, in kcontrol -> Desktop -> Window Behavior -> Translucency, does the top level check box (Enable the Trinity window composition manager) remain disabled//ghosted? Does a kdialog appear when a user tries to enable these features and compositing has not been built into the system? I think we need an affirmative indication for users so they understand or know why they can't enable compositing. Possibly a stderr message in the xsession log and a kdialog box. :)

If I remember correctly, compositing can be disabled in an xorg.conf but there is no feedback or indication to the user in kcontrol. This patch would have a similar effect.

Created attachment 941 [details]
combined patch

(In reply to comment #4)
> Would you combine these patches into one?
fixed
> How does this patching affect usability? For example, in kcontrol -> Desktop ->
> Window Behavior -> Translucency, does the top level check box (Enable the
> Trinity window composition manager) remain disabled//ghosted? Does a kdialog
> appear when a user tries to enable these features and compositing has not been
> built into the system? I think we need an affirmative indication for users so
> they understand or know why they can't enable compositing. Possibly a stderr
> message in the xsession log and a kdialog box. :)
>
> If I remember correctly, compositing can be disabled in an xorg.conf but there
> is no feedback or indication to the user in kcontrol. This patch would have a
> similar effect.

sorry can't test now, for some reason my virtualbox refuses to go into hardware-accelerated mode, so compositing doesn't work either.

Okay. I don't trust virtual machines for hardware testing anyway. :)

I'll test the patch here next time I run a full build set.

I'm updating the summary to "Build issue:" because that is what we have configured in our search links. :)

I tested the patch in both Slackware 13.1 32-bit and Slackware 14.0 64-bit. No build issues.

We need a way to test the actual results. Bug report 1297 addresses that challenge. I'm adding 1297 to the "Depends on" list and adding this bug report to the "Blocks" list in bug report 1297.

There is ABI and API breakage in this patch; for example, KApplication::KARGBApplication( bool allowStyles ) was completely removed.

Why was this done?  At the very least a stub method could be utilized when xcomposite is disabled to avoid ABI compability problems.

Created attachment 1314 [details]
03-tdelibs-make-xrender-required.patch

Fixed new-API issues.

Created attachment 1315 [details]
tdelibs-make-xcomposite-mandatory.patch

As far as this bug not supposed to resolved before release, please make xcomposite mandatory. The reasons are the same as for xrender in Bug 1554.

Created attachment 1316 [details]
02-tdelibs-make-xcomposite-optional.patch

Fixed new-API issues. The right one patch.

OK I understand the CMAKE-related part of your second patch, but not the rest in "tdecore/tdeapplication.cpp"


189 defined(COMPOSITE)
=> Should'nt it be "defined(HAVE_XCOMPOSITE)" ?


1986 bool TDEApplication::detectCompositionManagerAvailable(bool force_available, bool available) {
=> What's the purpose of adding "bool available" here ??? It will cause a conflict with a same-name function, line 1819 !


2028, 2032
=> Is the modification related to the current patch ? It's not obvious to me.


2040
=> Why removing the code ??

(In reply to comment #14)
> OK I understand the CMAKE-related part of your second patch, but not the rest
> in "tdecore/tdeapplication.cpp"
> 
> 
> 189 defined(COMPOSITE)
> => Should'nt it be "defined(HAVE_XCOMPOSITE)" ?
It's for code consistent. COMPOSITE is defined on the top of the file and it's used all over it later. I found it a little confusing too, but it's matter of taste and I'm still a bit shy to make big and style-related changes...

> 
> 1986 bool TDEApplication::detectCompositionManagerAvailable(bool
> force_available, bool available) {
> => What's the purpose of adding "bool available" here ??? It will cause a
> conflict with a same-name function, line 1819 !

No, those functions are under different parts of ifdef/elseif directive, so no conflicts. Moreover, their prototype must be the same.
 
> 2028, 2032
> => Is the modification related to the current patch ? It's not obvious to me.
The same, those functions should have the same prototypes as their xcomposite versions above. They seems to be broken from the beginning.
 
> 2040
> => Why removing the code ??
The XComposite version was removed on commit b56a10ae. The non-xcomposite is leaved behind.

OK, I did not understood that there was a big #ifdef #else... and that functions were written twice in the file.
Of course they must have the same prototype then.

Your patch looks correct then.

Attachment 1316 [details] pushed to GIT in hash 6e81fa9c.