Merge lp:~smspillaz/unity/unity.fix_881190v2 into lp:unity

[Sam Spilsbury]

Fix LP #881190 #871801

    The transient for reader would report every window as a "transient" if it
    matched the w->transientFor () definition on the window - at destruction time
    this will be zero, so it will match every single window. That would cause
    all windows to be unminimized.

    However, it makes no sense to do the unminimize/minimize normally dance when
    the window has been destroyed.

[Daniel van Vugt (vanvugt)]

Conflicts with: https://code.launchpad.net/~vanvugt/unity/fix-918329-trunk/+merge/89391

Also, this proposal attempts to fix 2 separate bugs. I think you should remove the fix for 918360 which is redundant with my proposal and re-propose just a fix for bug 881190.

[Sam Spilsbury (smspillaz)]

On Fri, 20 Jan 2012, Daniel van Vugt wrote:

> Conflicts with: https://code.launchpad.net/~vanvugt/unity/fix-918329-trunk/+merge/89391
>
> Also, this proposal attempts to fix 2 separate bugs. I think you should remove the fix for 918360 which is redundant with my proposal and re-propose just a fix for bug 881190.

Ack, removing the cycle code and keeping the fix for the unminimize

> --
> https://code.launchpad.net/~smspillaz/unity/unity.fix_881190_918360/+merge/89181
> You are the owner of lp:~smspillaz/unity/unity.fix_881190_918360.
>

[Sam Spilsbury (smspillaz)]

(Updated)

[Tim Penhey (thumper)]

mMinimizeHandler should be a std::unique_ptr from <memory>.

Also there are places where you dereference a pointer without checking that it is non-null.

Also please use nullptr rather than NULL.

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

> mMinimizeHandler should be a std::unique_ptr from <memory>.

It can't be. It needs strong and weak references (eg, the static list of mMinimizeHandler is there because the minimize handlers need to know about other minimize handlers that are around in order to handle stuff like, eg, minimizing transient windows)

>
> Also there are places where you dereference a pointer without checking that it
> is non-null.

If you are talking about

167 if (actualType == XA_WINDOW && actualFormat == 32 && nLeft == 0 && nItems == 1)
168 {
169 - Window *data = (Window *) prop;
170 + Window *data = (Window *) prop;
171
172 - serverAncestor = *data;
173 + serverAncestor = *data;
174 }

data in this case is always guarunteed to be non-null, as because nItems tells you what is actually in data.

>
> Also please use nullptr rather than NULL.

ack

[Sam Spilsbury (smspillaz)]

This has been updated, please re-review

[Tim Penhey (thumper)]

To do with std::unique_ptr...

Don't assign, please use reset:
> w->mMinimizeHandler = std::unique_ptr <Type> (new Type (win));

  w->mMinimizeHandler.reset(new Type(win);

Assignment only works with a temporary, as unique_ptr is non-copyable,
but has move semantics. Better to use the explicit reset.

Since you are not caring aroub the return type, use reset, not release.

 > w->mMinimizeHandler.release();

 w->mMinimizeHandler.reset();

 > return mMinimizeHandler.get () != nullptr;

Please use the class's operator bool.

  return mMinimizeHandler;

review: Needs Fixing

=== modified file 'plugins/unityshell/src/compizminimizedwindowhandler.h'
--- plugins/unityshell/src/compizminimizedwindowhandler.h	2012-01-20 06:47:42 +0000
+++ plugins/unityshell/src/compizminimizedwindowhandler.h	2012-02-02 01:53:21 +0000
@@ -26,6 +26,7 @@
 #include <core/core.h>
 #include "minimizedwindowhandler.h"
 #include "comptransientfor.h"
+#include <memory>
 
 // Will be merged back into compiz
 namespace compiz {
@@ -67,7 +68,7 @@
   typedef std::list <Type *> List;
 protected:
 
-  virtual std::vector<unsigned int> getTransients ();
+  std::vector<unsigned int> getTransients ();
 
 private:
 
@@ -103,6 +104,15 @@
 template <typename Screen, typename Window>
 compiz::CompizMinimizedWindowHandler<Screen, Window>::~CompizMinimizedWindowHandler ()
 {
+  if (!priv->mWindow->destroyed ())
+  {
+    priv->mWindow->unminimize ();
+    priv->mWindow->focusSetEnabled (Window::get (priv->mWindow), false);
+    priv->mWindow->minimizeSetEnabled (Window::get (priv->mWindow), false);
+    priv->mWindow->unminimizeSetEnabled (Window::get (priv->mWindow), false);
+    priv->mWindow->minimize ();
+  }
+
   minimizedWindows.remove (this);
 }
 
@@ -158,7 +168,7 @@
     {
       Window *w = Window::get (win);
       if (!w->mMinimizeHandler)
-        w->mMinimizeHandler = new Type (win);
+	w->mMinimizeHandler = std::unique_ptr <Type> (new Type (win));
       w->mMinimizeHandler->minimize ();
     }
   }
@@ -241,8 +251,7 @@
       if (w && w->mMinimizeHandler)
       {
         w->mMinimizeHandler->unminimize ();
-        delete w->mMinimizeHandler;
-        w->mMinimizeHandler = NULL;
+	w->mMinimizeHandler.release ();
       }
     }
   }
@@ -324,7 +333,7 @@
     if (w)
     {
       Window *pw = Window::get (w);
-      Type *compizMinimizeHandler = pw->mMinimizeHandler;
+      Type *compizMinimizeHandler = pw->mMinimizeHandler.get ();
 
       /* Restore and re-save input shape and remove */
       if (compizMinimizeHandler)

=== modified file 'plugins/unityshell/src/comptransientfor.cpp'
--- plugins/unityshell/src/comptransientfor.cpp	2011-08-23 14:47:38 +0000
+++ plugins/unityshell/src/comptransientfor.cpp	2012-02-02 01:53:21 +0000
@@ -55,13 +55,18 @@
 bool
 compiz::CompTransientForReader::isTransientFor (unsigned int ancestor)
 {
+  if (!ancestor ||
+      !priv->mWindow->id ())
+      return false;
+
   return priv->mWindow->transientFor () == ancestor;
 }
 
 bool
 compiz::CompTransientForReader::isGroupTransientFor (unsigned int clientLeader)
 {
-  if (!clientLeader)
+  if (!clientLeader ||
+      !priv->mWindow->id ())
     return false;
 
   if (priv->mWindow->transientFor () == None || priv->mWindow->transientFor () == screen->root ())

=== modified file 'plugins/unityshell/src/transientfor.cpp'
--- plugins/unityshell/src/transientfor.cpp	2011-08-26 10:37:35 +0000
+++ plugins/unityshell/src/transientfor.cpp	2012-02-02 01:53:21 +0000
@@ -51,9 +51,9 @@
   {
     if (actualType == XA_WINDOW && actualFormat == 32 && nLeft == 0 && nItems == 1)
     {
-	    Window *data = (Window *) prop;
+      Window *data = (Window *) prop;
 
-	    serverAncestor = *data;
+      serverAncestor = *data;
     }
 
     XFree (prop);
@@ -65,6 +65,10 @@
 bool
 compiz::X11TransientForReader::isTransientFor (unsigned int ancestor)
 {
+  if (!ancestor ||
+      !priv->mXid)
+    return false;
+
   return ancestor == getAncestor ();
 }
 
@@ -80,14 +84,17 @@
   std::vector<std::string> strings;
   std::list<Atom>   atoms;
 
+  if (!clientLeader ||
+      !priv->mXid)
+
   if (XGetWindowProperty (priv->mDpy, priv->mXid, wmClientLeader, 0L, 2L, false,
                           XA_WINDOW, &actualType, &actualFormat, &nItems, &nLeft, (unsigned char **)&prop) == Success)
   {
     if (actualType == XA_WINDOW && actualFormat == 32 && nLeft == 0 && nItems == 1)
     {
-	    Window *data = (Window *) prop;
+      Window *data = (Window *) prop;
 
-	    serverClientLeader = *data;
+      serverClientLeader = *data;
     }
 
     XFree (prop);

=== modified file 'plugins/unityshell/src/unityshell.cpp'
--- plugins/unityshell/src/unityshell.cpp	2012-02-01 00:06:29 +0000
+++ plugins/unityshell/src/unityshell.cpp	2012-02-02 01:53:21 +0000
@@ -1964,7 +1964,7 @@
 
   if (!mMinimizeHandler)
   {
-    mMinimizeHandler = new UnityMinimizedHandler (window);
+    mMinimizeHandler = std::unique_ptr <UnityMinimizedHandler> (new UnityMinimizedHandler (window));
     mMinimizeHandler->minimize ();
   }
 }
@@ -1975,8 +1975,7 @@
   if (mMinimizeHandler)
   {
     mMinimizeHandler->unminimize ();
-    delete mMinimizeHandler;
-    mMinimizeHandler = nullptr;
+    mMinimizeHandler.release ();
   }
 }
 
@@ -2015,7 +2014,7 @@
 bool
 UnityWindow::minimized ()
 {
-  return mMinimizeHandler != nullptr;
+  return mMinimizeHandler.get () != nullptr;
 }
 
 gboolean
@@ -2571,7 +2570,7 @@
   , PluginClassHandler<UnityWindow, CompWindow>(window)
   , window(window)
   , gWindow(GLWindow::get(window))
-  , mMinimizeHandler(nullptr)
+  , mMinimizeHandler()
   , mShowdesktopHandler(nullptr)
   , focusdesktop_handle_(0)
 {
@@ -2629,19 +2628,6 @@
 
   UnityShowdesktopHandler::animating_windows.remove (window);
 
-  if (mMinimizeHandler)
-  {
-    unminimize ();
-    window->focusSetEnabled (this, false);
-    window->minimizeSetEnabled (this, false);
-    window->unminimizeSetEnabled (this, false);
-    window->minimizedSetEnabled (this, false);
-    window->minimize ();
-
-    delete mMinimizeHandler;
-    mMinimizeHandler = nullptr;
-  }
-
   if (mShowdesktopHandler)
     delete mShowdesktopHandler;
     

=== modified file 'plugins/unityshell/src/unityshell.h'
--- plugins/unityshell/src/unityshell.h	2012-01-29 06:08:20 +0000
+++ plugins/unityshell/src/unityshell.h	2012-02-02 01:53:21 +0000
@@ -393,7 +393,7 @@
 
   typedef compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>
           UnityMinimizedHandler;
-  UnityMinimizedHandler *mMinimizeHandler;
+  std::unique_ptr <UnityMinimizedHandler> mMinimizeHandler;
 
   UnityShowdesktopHandler             *mShowdesktopHandler;