Merge lp:~3v1n0/ubuntu/oneiric/bamf/libreoffice-fixes into lp:ubuntu/oneiric/bamf

[Marco Trevisan (Treviño)]

Fix for bug #842566 that caused libreoffice windows not to be mapped on unity launcher or alt+tab.

[lp:~3v1n0/ubuntu/oneiric/bamf/libreoffice-fixes]

55. By Marco Trevisan (Treviño) on 2011-12-09

BamfLegacyWindow: remove the double-check.

[Omer Akram (om26er)]

You need to replace oneiric with oneiric-proposed in debian/changelog file since you are uploading to a stable series for SRU. Also please change version number from 0.2.104-0ubuntu2 to 0.2.104-0ubuntu1.1 for SRU

review: Needs Fixing

[Marco Trevisan (Treviño) (3v1n0)]

Done, here you are the fix.

[lp:~3v1n0/ubuntu/oneiric/bamf/libreoffice-fixes]

56. By Marco Trevisan (Treviño) on 2011-12-11

Changelog: updated with the correct versioning.

[Omer Akram (om26er)]

looks good ;-)

review: Approve

[Ken VanDine (ken-vandine)]

Thanks for fixing this, lots of people will be very happy :)

You updated lp:~3v1n0/bamf/libreoffice-fixes based on feedback from kamstrup, can you also merge those fixes into this packaging branch? Also, we need to upload the fix to precise before doing an SRU for oneiric, please resbumit for lp:ubuntu/bamf then backpack port to the oneiric branch.

review: Needs Fixing

[Marco Trevisan (Treviño) (3v1n0)]

Ok. But I was wondering: should I just backport this "lighter" version for SRU, or the better fixed one (the one that will go in trunk)?
As I've been told that SRU should only contain small patches...

[Ken VanDine (ken-vandine)]

Best would be to backport what is needed to fix the bug and avoid large refactoring or feature changes. So yes, minimal change is what we really want for an SRU, but we do want the bug fixed. Hopefully that helps :)

[Marco Trevisan (Treviño) (3v1n0)]

Ok... I'll do that work tonight.

However, both the patches fix the issue. But this one don't remaps windows (i.e. when you go from writer to startcenter and to startcenter to calc), the final one does that.
So, the choice it's up to you! :)

[Marco Trevisan (Treviño) (3v1n0)]

Ken, I've placed the merge request for precise at http://is.gd/1IX7PO let me know if you want me to backport to this one the more recent (and improved) fixes.

[David Barth (dbarth)]

To prepare the fix for an SRU upload:

1. Can you highlight the test case that would accompany this sru fix? Most probably there is already a comment in the bug report; it should go into a manual-test/openoffice.txt text file describing the steps to reproduce the issue and verify that the problem is solved with the fix

2. More importantly, when looking at the start of the patch; can you think of a few regressions test case, to verify that even if a non-LO window is managed by the new code, then the code in line 23-29 of your patch will still do the right thing

review: Needs Information

[Marco Trevisan (Treviño) (3v1n0)]

As discussed on IRC:

1. unfortunately defining a test case is a little bit hard since this bug is mostly related to a race in BamfDaemon, that when libreoffice is opened and shows its splash-screen, and quickly closes it, causes the new opening libreoffice window to be considered as closed as well making our maching code not to work.
So, for sure a test case would be:

1) Enable the splash screen in libreoffice
2) Launch libreoffice double-cliking on a lo file
3) Repeat this action multiple times and the bug should manifest.

2. The first lines of this patch changes handle_window_closed() in bamf-legacy-window.c, that function is called every time that a window is closed, but the only field I'm in fact changing is the private member is_closed. This value can only be read and is only used by the bamf_legacy_window_is_closed() function, and this function is actually used in BAMF only by the matcher code that is used only for LO windows.
So, the bug is clear, the code I'm fixing is only affecting and used by LO windows, so we should be safe about regressions.

[Omer Akram (om26er)]

I have added SRU testcase to the report. Please upload the package so we can get the much needed fix SRUed

[Didier Roche-Tolomelli (didrocks)]

Discussed directly on IRC, this should be backported on a bamf-oneiric branch, then using the packaging branch for handling the upgrade (directions directly given on IRC) :)

Put resubmit on it then!
(also debian/changelog: LP: #<bugnumber>)

review: Needs Resubmitting

[Colin Watson (cjwatson)]

Marked as merged by request of seb128.

=== modified file 'debian/changelog'
--- debian/changelog	2011-09-26 13:51:22 +0000
+++ debian/changelog	2011-12-11 19:46:26 +0000
@@ -1,3 +1,10 @@
+bamf (0.2.104-0ubuntu1.1) oneiric-proposed; urgency=low
+
+  * Correctly signal a closed BamfLegacyWindow, fixes libreoffice and unity
+    integration bug (LP: 842566)
+
+ -- Marco Trevisan (Treviño) <[e-mail]>  Fri, 09 Dec 2011 23:01:30 +0100
+
 bamf (0.2.104-0ubuntu1) oneiric; urgency=low
 
   * New upstream release.

=== modified file 'src/bamf-legacy-window.c'
--- src/bamf-legacy-window.c	2011-09-26 13:51:22 +0000
+++ src/bamf-legacy-window.c	2011-12-11 19:46:26 +0000
@@ -325,11 +325,10 @@
 {
   g_return_if_fail (BAMF_IS_LEGACY_WINDOW (self));
   g_return_if_fail (WNCK_IS_WINDOW (window));
-  
-  self->priv->is_closed = TRUE;
 
-  if (window == self->priv->legacy_window)
+  if (self->priv->legacy_window == window)
     {
+      self->priv->is_closed = TRUE;
       g_signal_emit (self, legacy_window_signals[CLOSED], 0);
     }
 }
@@ -378,7 +377,7 @@
   screen = wnck_screen_get_default ();
 
   priv->closed_id = g_signal_connect (G_OBJECT (screen), "window-closed",
-    		                       (GCallback) handle_window_closed, self);
+                                      (GCallback) handle_window_closed, self);
 }
 
 static void

=== modified file 'src/bamf-matcher.c'
--- src/bamf-matcher.c	2011-09-26 13:51:22 +0000
+++ src/bamf-matcher.c	2011-12-11 19:46:26 +0000
@@ -1695,14 +1695,16 @@
   if (bamf_legacy_window_is_closed (args->window))
   {
     g_object_unref (args->window);
+    g_free (args);
     return FALSE;
   }
 
   args->count++;
-  if (args->count > 20 || get_open_office_window_hint (args->matcher, args->window))  
+  if (args->count > 50 || get_open_office_window_hint (args->matcher, args->window))  
     {
       g_object_unref (args->window);
       handle_raw_window (args->matcher, args->window);
+      g_free (args);
       return FALSE;
     }