Merge lp:~vanvugt/compiz-core/fix-930412 into lp:compiz-core

[Daniel van Vugt]

Fixed: Core keybindings like Alt+F4 not being registered due to bad construct/init ordering (LP: #930412)

Fixed: Core keybindings like Alt+F4 not being registered due to bad construct/init ordering (LP: #930412)

Moved CoreOptions initialization to ensure that it only happens after the critical parts of PrivateScreen::init are done. This ensures ::screen and PrivateScreen::dpy are both initialized before CoreOptions, so that CompAction::KeyBinding::fromString no longer fails due to screen being NULL.

[Sam Spilsbury (smspillaz)]

177 - <xsl:text>Options ();
178 + <xsl:text>Options (bool init = true);
179 virtual ~</xsl:text>
180 <xsl:value-of select="$Plugin"/>
181 <xsl:text>Options ();
182
183 + void initOptions ();
184 +
185 virtual CompOption::Vector &amp; getOptions ();
186 virtual bool setOption (const CompString &amp;name, CompOption::Value &amp;value);
187
188 @@ -1090,7 +1092,7 @@
189 <xsl:value-of select="$Plugin"/>
190 <xsl:text>Options::</xsl:text>
191 <xsl:value-of select="$Plugin"/>
192 - <xsl:text>Options () :
193 + <xsl:text>Options (bool init /* = true */) :

I'll take this now because it does actually fix the problem, but there are three things I don't like in this approach, and after 0.9.7.0 we should seek to change

1. We are changing bcop for the sake of core's initialization order and adding a default parameter for the sake of working around it.
2. This generally goes against the RAII principles of bcop
3. We'll need to trigger a rebuild of all the plugins.

The (more invasive) way I think this should be done would be to have a separate class Keybindings who PrivateScreen inherits from first before CoreOptions. This way we can DI a Display * into Keybindings and ensure that the relevant data is initialized before CoreOption's constructor is called.

Like I say, its invasive and I'd rather see the problem fixed before release.

Please merge.

review: Approve

[Daniel van Vugt (vanvugt)]

I don't think "We'll need to trigger a rebuild of all the plugins". Existing plugins using the old generated code will still work without a rebuild if necessary.

Also regarding generated *Options, it is bad design to have so much activity in the constructor of any class (which happens to be the cause of this bug). I think separating initialization from construction was always going to be cleaner. I don't think any of this qualifies as a hack. In fact, this proposal removes hacks such as PrivateScreen::addScreenActions ()

[Sam Spilsbury (smspillaz)]

On Mon, 13 Feb 2012, Daniel van Vugt wrote:

> Also regarding generated *Options, it is bad design to have so much activity in the constructor of any class (which happens to be the cause of this bug). I think separating initialization from construction was always going to be cleaner. I don't think any of this qualifies as a hack. In fact, this proposal removes hacks such as PrivateScreen::addScreenActions ()
>

I generally prefer to keep as much init code in constructors as possible
as a matter of principle. I don't like the idea of having ::init* on
objects because it means that there is coupling between the ::init* and
other functions.

Of course, the only place I'm willing to throw aside that rule is where
you need to make virtual function call in a constructor, but I generally
try to rework my design if I have to do that.

> --
> https://code.launchpad.net/~vanvugt/compiz-core/fix-930412/+merge/92720
> You are reviewing the proposed merge of lp:~vanvugt/compiz-core/fix-930412 into lp:compiz-core.
>

[Tim Penhey (thumper)]

Well, the point of a constructor is to make an object in a well
defined, consistent state.

There is nothing wrong with an extra init if it is needed.

[Daniel van Vugt (vanvugt)]

It's a principle that sounded messy to me when I was first taught it. However in this bug you will see that having complex initialization in the constructors of CoreOptions and PrivateScreen were the cause of this bug.

[Alan Griffiths (alan-griffiths)]

> Also regarding generated *Options, it is bad design to have so much activity
> in the constructor of any class (which happens to be the cause of this bug). I
> think separating initialization from construction was always going to be
> cleaner. I don't think any of this qualifies as a hack. In fact, this proposal
> removes hacks such as PrivateScreen::addScreenActions ()

There are a whole host of issues surrounding the initialisation process. But I don't agree that separating initialisation from construction is the right approach. A constructor should put a the instance into a *usable* state - neither more nor less. And that, in many cases implies "much activity".

The classes involved are neither coherent nor weakly coupled and that is the source of the problems. Separating out concerns is the right approach.

[Daniel van Vugt (vanvugt)]

I would be happy to review and approve any more elegant solution that is proposed...

One could also argue that the "screen" global variable is the cause of this bug. If the global variable didn't exist then this bug wouldn't either.

[Tim Penhey (thumper)]

Can we file a bug then for the next release to do this with separating out concerns?

=== modified file 'src/action.cpp'
--- src/action.cpp	2012-01-24 08:24:18 +0000
+++ src/action.cpp	2012-02-13 08:23:17 +0000
@@ -26,6 +26,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <cassert>
 
 #include <core/option.h>
 #include <core/action.h>
@@ -158,6 +159,8 @@
     size_t       pos, start = 0;
     KeySym	 keysym;
 
+    /* This assertion is a regression test for LP: #930412 */
+    assert (screen);
     if (!screen)
 	return false;
 

=== modified file 'src/privatescreen.h'
--- src/privatescreen.h	2012-02-08 15:07:30 +0000
+++ src/privatescreen.h	2012-02-13 08:23:17 +0000
@@ -370,8 +370,6 @@
 
 	void disableEdge (int edge);
 
-	void addScreenActions ();
-
 	CompWindow *
 	focusTopMostWindow ();
 

=== modified file 'src/screen.cpp'
--- src/screen.cpp	2012-02-09 15:03:04 +0000
+++ src/screen.cpp	2012-02-13 08:23:17 +0000
@@ -3257,24 +3257,10 @@
     }
 }
 
-/* add actions that should be automatically added as no screens
-   existed when they were initialized. */
-void
-PrivateScreen::addScreenActions ()
-{
-    foreach (CompOption &o, mOptions)
-    {
-	if (!o.isAction ())
-	    continue;
-
-	if (o.value ().action ().state () & CompAction::StateAutoGrab)
-	    screen->addAction (&o.value ().action ());
-    }
-}
-
 bool
 CompScreenImpl::addAction (CompAction *action)
 {
+    assert (priv->initialized);
     if (!priv->initialized)
 	return false;
 
@@ -4735,6 +4721,8 @@
 
     reshape (attrib.width, attrib.height);
 
+    initialized = true;
+    initOptions ();
     detectOutputDevices ();
     updateOutputDevices ();
 
@@ -4805,7 +4793,6 @@
     XSync (dpy, FALSE);
 
     /* Start initializing windows here */
-    initialized = true;
 
     for (unsigned int i = 0; i < nchildren; i++)
     {
@@ -4893,7 +4880,38 @@
 
     pingTimer.start ();
 
-    addScreenActions ();
+    optionSetCloseWindowKeyInitiate (CompScreenImpl::closeWin);
+    optionSetCloseWindowButtonInitiate (CompScreenImpl::closeWin);
+    optionSetRaiseWindowKeyInitiate (CompScreenImpl::raiseWin);
+    optionSetRaiseWindowButtonInitiate (CompScreenImpl::raiseWin);
+    optionSetLowerWindowKeyInitiate (CompScreenImpl::lowerWin);
+    optionSetLowerWindowButtonInitiate (CompScreenImpl::lowerWin);
+
+    optionSetUnmaximizeWindowKeyInitiate (CompScreenImpl::unmaximizeWin);
+
+    optionSetMinimizeWindowKeyInitiate (CompScreenImpl::minimizeWin);
+    optionSetMinimizeWindowButtonInitiate (CompScreenImpl::minimizeWin);
+    optionSetMaximizeWindowKeyInitiate (CompScreenImpl::maximizeWin);
+    optionSetMaximizeWindowHorizontallyKeyInitiate (
+	CompScreenImpl::maximizeWinHorizontally);
+    optionSetMaximizeWindowVerticallyKeyInitiate (
+	CompScreenImpl::maximizeWinVertically);
+
+    optionSetWindowMenuKeyInitiate (CompScreenImpl::windowMenu);
+    optionSetWindowMenuButtonInitiate (CompScreenImpl::windowMenu);
+
+    optionSetShowDesktopKeyInitiate (CompScreenImpl::showDesktop);
+    optionSetShowDesktopEdgeInitiate (CompScreenImpl::showDesktop);
+
+    optionSetToggleWindowMaximizedKeyInitiate (CompScreenImpl::toggleWinMaximized);
+    optionSetToggleWindowMaximizedButtonInitiate (CompScreenImpl::toggleWinMaximized);
+
+    optionSetToggleWindowMaximizedHorizontallyKeyInitiate (
+	CompScreenImpl::toggleWinMaximizedHorizontally);
+    optionSetToggleWindowMaximizedVerticallyKeyInitiate (
+	CompScreenImpl::toggleWinMaximizedVertically);
+
+    optionSetToggleWindowShadedKeyInitiate (CompScreenImpl::shadeWin);
 
     if (dirtyPluginList)
 	updatePlugins ();
@@ -4907,6 +4925,7 @@
 }
 
 PrivateScreen::PrivateScreen (CompScreen *screen) :
+    CoreOptions (false),
     source(0),
     timeout(0),
     fileWatch (0),
@@ -4970,39 +4989,6 @@
 
     memset (&history, 0, sizeof (Window) * ACTIVE_WINDOW_HISTORY_NUM);
 
-    optionSetCloseWindowKeyInitiate (CompScreenImpl::closeWin);
-    optionSetCloseWindowButtonInitiate (CompScreenImpl::closeWin);
-    optionSetRaiseWindowKeyInitiate (CompScreenImpl::raiseWin);
-    optionSetRaiseWindowButtonInitiate (CompScreenImpl::raiseWin);
-    optionSetLowerWindowKeyInitiate (CompScreenImpl::lowerWin);
-    optionSetLowerWindowButtonInitiate (CompScreenImpl::lowerWin);
-
-    optionSetUnmaximizeWindowKeyInitiate (CompScreenImpl::unmaximizeWin);
-
-    optionSetMinimizeWindowKeyInitiate (CompScreenImpl::minimizeWin);
-    optionSetMinimizeWindowButtonInitiate (CompScreenImpl::minimizeWin);
-    optionSetMaximizeWindowKeyInitiate (CompScreenImpl::maximizeWin);
-    optionSetMaximizeWindowHorizontallyKeyInitiate (
-	CompScreenImpl::maximizeWinHorizontally);
-    optionSetMaximizeWindowVerticallyKeyInitiate (
-	CompScreenImpl::maximizeWinVertically);
-
-    optionSetWindowMenuKeyInitiate (CompScreenImpl::windowMenu);
-    optionSetWindowMenuButtonInitiate (CompScreenImpl::windowMenu);
-
-    optionSetShowDesktopKeyInitiate (CompScreenImpl::showDesktop);
-    optionSetShowDesktopEdgeInitiate (CompScreenImpl::showDesktop);
-
-    optionSetToggleWindowMaximizedKeyInitiate (CompScreenImpl::toggleWinMaximized);
-    optionSetToggleWindowMaximizedButtonInitiate (CompScreenImpl::toggleWinMaximized);
-
-    optionSetToggleWindowMaximizedHorizontallyKeyInitiate (
-	CompScreenImpl::toggleWinMaximizedHorizontally);
-    optionSetToggleWindowMaximizedVerticallyKeyInitiate (
-	CompScreenImpl::toggleWinMaximizedVertically);
-
-    optionSetToggleWindowShadedKeyInitiate (CompScreenImpl::shadeWin);
-
     TimeoutHandler::SetDefault (dTimeoutHandler);
 }
 

=== modified file 'xslt/bcop.xslt'
--- xslt/bcop.xslt	2012-02-01 14:03:38 +0000
+++ xslt/bcop.xslt	2012-02-13 08:23:17 +0000
@@ -941,11 +941,13 @@
 
         </xsl:text>
 	<xsl:value-of select="$Plugin"/>
-        <xsl:text>Options ();
+        <xsl:text>Options (bool init = true);
         virtual ~</xsl:text>
 	<xsl:value-of select="$Plugin"/>
         <xsl:text>Options ();
 
+	void initOptions ();
+
         virtual CompOption::Vector &amp; getOptions ();
         virtual bool setOption (const CompString &amp;name, CompOption::Value &amp;value);
 
@@ -1090,7 +1092,7 @@
         <xsl:value-of select="$Plugin"/>
         <xsl:text>Options::</xsl:text>
 	<xsl:value-of select="$Plugin"/>
-        <xsl:text>Options () :
+        <xsl:text>Options (bool init /* = true */) :
     mOptions (</xsl:text>
         <xsl:value-of select="$Plugin"/>
         <xsl:text>Options::OptionNum),
@@ -1098,6 +1100,14 @@
         <xsl:value-of select="$Plugin"/>
         <xsl:text>Options::OptionNum)
 {
+    if (init)
+        initOptions ();
+}
+
+void
+</xsl:text>
+<xsl:value-of select="$Plugin"/><xsl:text>Options::initOptions ()
+{
 </xsl:text>
     <xsl:if test="/compiz/plugin[@name=$pName]/descendant-or-self::option[@type = 'action'] or
                   /compiz/plugin[@name=$pName]/descendant-or-self::option[@type = 'key'] or