Merge lp:~3v1n0/unity/quick-app-reopen-icon-fix into lp:unity

Proposed by Marco Trevisan (Treviño)
Status: Merged
Approved by: Marco Trevisan (Treviño)
Approved revision: no longer in the source branch.
Merged at revision: 2358
Proposed branch: lp:~3v1n0/unity/quick-app-reopen-icon-fix
Merge into: lp:unity
Diff against target: 145 lines (+59/-4)
4 files modified
launcher/BamfLauncherIcon.cpp (+40/-2)
launcher/BamfLauncherIcon.h (+2/-0)
tests/autopilot/autopilot/tests/__init__.py (+3/-2)
tests/autopilot/autopilot/tests/test_launcher.py (+14/-0)
To merge this branch: bzr merge lp:~3v1n0/unity/quick-app-reopen-icon-fix
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
Thomi Richards (community) Approve
Review via email: mp+105873@code.launchpad.net

Commit message

BamfLauncherIcon: add a timeout on application close before removing the icon

This makes the application to correctly hide when closed, but avoids
that the launcher icon is removed when an application is quickly opened/closed

Also we override the Remove() method to make sure that once it's called
we remove the "unity-seen" flag from the BamfApplication object.
So an application icon could be re-added in the period between the Remove()
call and the ~BamfLauncherIcon call (that could take some time).

Description of the change

If an application (i.e. GIMP, Hugin ...) quickly opens and closes a window so that the application is considered closed for few milliseconds, unity doesn't handle correctly the application re-open.

This is caused by the fact that when SimpleLauncherIcon::Remove() is called, then unity takes few (milli)seconds to actually remove the icon from the launcher, and then to call ~BamfLauncherIcon that unsets the "unity-seen" flag.
This makes the checks in LauncherController not to re-add an icon for the just re-opened application (as the "unity-seen" guard check fails).

So, to fix this I've overridden the Remove() method to force it to unset the "unity-seen" flag, and added a timeout before calling Remove() on application's close signal, so that if it's quickly re-opened the remove call is cancelled.
This won't cause any delay on actually hiding the icon from the launcher, since that is done when the application is not anymore on running state.

Added autopilot test to check this.

A crash can happen in libbamf with this fix, lp:~3v1n0/bamf/fix-window-removed-crash fixes it.

To post a comment you must log in.
Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

+1 from me. Probably needs someone to look at the C++ stuff as well.

review: Approve
Revision history for this message
Tim Penhey (thumper) wrote :

C++ stuff looks fine

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'launcher/BamfLauncherIcon.cpp'
--- launcher/BamfLauncherIcon.cpp 2012-05-07 19:52:54 +0000
+++ launcher/BamfLauncherIcon.cpp 2012-05-15 23:08:19 +0000
@@ -55,7 +55,7 @@
55 , _window_moved_id(0)55 , _window_moved_id(0)
56{56{
57 g_object_set_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen"),57 g_object_set_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen"),
58 GINT_TO_POINTER(1));58 GUINT_TO_POINTER(1));
59 auto bamf_view = glib::object_cast<BamfView>(_bamf_app);59 auto bamf_view = glib::object_cast<BamfView>(_bamf_app);
6060
61 glib::String icon(bamf_view_get_icon(bamf_view));61 glib::String icon(bamf_view_get_icon(bamf_view));
@@ -101,10 +101,17 @@
101 sig = new glib::Signal<void, BamfView*, gboolean>(bamf_view, "running-changed",101 sig = new glib::Signal<void, BamfView*, gboolean>(bamf_view, "running-changed",
102 [&] (BamfView*, gboolean running) {102 [&] (BamfView*, gboolean running) {
103 SetQuirk(QUIRK_RUNNING, running);103 SetQuirk(QUIRK_RUNNING, running);
104
104 if (running)105 if (running)
105 {106 {
106 EnsureWindowState();107 EnsureWindowState();
107 UpdateIconGeometries(GetCenters());108 UpdateIconGeometries(GetCenters());
109
110 if (_remove_timeout_id)
111 {
112 g_source_remove(_remove_timeout_id);
113 _remove_timeout_id = 0;
114 }
108 }115 }
109 });116 });
110 _gsignals.Add(sig);117 _gsignals.Add(sig);
@@ -119,7 +126,23 @@
119 sig = new glib::Signal<void, BamfView*>(bamf_view, "closed",126 sig = new glib::Signal<void, BamfView*>(bamf_view, "closed",
120 [&] (BamfView*) {127 [&] (BamfView*) {
121 if (!IsSticky())128 if (!IsSticky())
122 Remove();129 {
130 /* Use a timeout to remove the icon, this avoids
131 * that we remove an application that is going
132 * to be reopened soon. So applications that
133 * have a splash screen won't be removed from
134 * the launcher while the splash is closed and
135 * a new window is opened. */
136 if (_remove_timeout_id)
137 g_source_remove(_remove_timeout_id);
138
139 _remove_timeout_id = g_timeout_add_seconds(1, [] (gpointer data) -> gboolean {
140 auto self = static_cast<BamfLauncherIcon*>(data);
141 self->Remove();
142 self->_remove_timeout_id = 0;
143 return false;
144 }, this);
145 }
123 });146 });
124 _gsignals.Add(sig);147 _gsignals.Add(sig);
125148
@@ -147,6 +170,9 @@
147 g_object_set_qdata(G_OBJECT(_bamf_app.RawPtr()),170 g_object_set_qdata(G_OBJECT(_bamf_app.RawPtr()),
148 g_quark_from_static_string("unity-seen"), nullptr);171 g_quark_from_static_string("unity-seen"), nullptr);
149172
173 if (_remove_timeout_id != 0)
174 g_source_remove(_remove_timeout_id);
175
150 if (_fill_supported_types_id != 0)176 if (_fill_supported_types_id != 0)
151 g_source_remove(_fill_supported_types_id);177 g_source_remove(_fill_supported_types_id);
152178
@@ -160,6 +186,18 @@
160 g_source_remove(_dnd_hover_timer);186 g_source_remove(_dnd_hover_timer);
161}187}
162188
189void BamfLauncherIcon::Remove()
190{
191 /* Removing the unity-seen flag to the wrapped bamf application, on remove
192 * request we make sure that if the bamf application is re-opened while
193 * the removal process is still ongoing, the application will be shown
194 * on the launcher. */
195 g_object_set_qdata(G_OBJECT(_bamf_app.RawPtr()),
196 g_quark_from_static_string("unity-seen"), nullptr);
197
198 SimpleLauncherIcon::Remove();
199}
200
163bool BamfLauncherIcon::IsSticky() const201bool BamfLauncherIcon::IsSticky() const
164{202{
165 return bamf_view_is_sticky(BAMF_VIEW(_bamf_app.RawPtr()));203 return bamf_view_is_sticky(BAMF_VIEW(_bamf_app.RawPtr()));
166204
=== modified file 'launcher/BamfLauncherIcon.h'
--- launcher/BamfLauncherIcon.h 2012-05-07 19:52:54 +0000
+++ launcher/BamfLauncherIcon.h 2012-05-15 23:08:19 +0000
@@ -66,6 +66,7 @@
66 std::string NameForWindow(Window window);66 std::string NameForWindow(Window window);
6767
68protected:68protected:
69 void Remove();
69 void UpdateIconGeometries(std::vector<nux::Point3> center);70 void UpdateIconGeometries(std::vector<nux::Point3> center);
70 void OnCenterStabilized(std::vector<nux::Point3> center);71 void OnCenterStabilized(std::vector<nux::Point3> center);
71 void AddProperties(GVariantBuilder* builder);72 void AddProperties(GVariantBuilder* builder);
@@ -124,6 +125,7 @@
124 guint _dnd_hover_timer;125 guint _dnd_hover_timer;
125126
126 bool _supported_types_filled;127 bool _supported_types_filled;
128 guint _remove_timeout_id;
127 guint _fill_supported_types_id;129 guint _fill_supported_types_id;
128 guint _window_moved_id;130 guint _window_moved_id;
129 guint _quicklist_activated_id;131 guint _quicklist_activated_id;
130132
=== modified file 'tests/autopilot/autopilot/tests/__init__.py'
--- tests/autopilot/autopilot/tests/__init__.py 2012-05-04 17:33:59 +0000
+++ tests/autopilot/autopilot/tests/__init__.py 2012-05-15 23:08:19 +0000
@@ -274,8 +274,9 @@
274 def close_all_app(self, app_name):274 def close_all_app(self, app_name):
275 """Close all instances of the app_name."""275 """Close all instances of the app_name."""
276 app = self.KNOWN_APPS[app_name]276 app = self.KNOWN_APPS[app_name]
277 self.addCleanup(call, "kill `pidof %s`" % (app['process-name']), shell=True)277 pids = check_output(["pidof", app['process-name']]).split()
278 super(LoggedTestCase, self).tearDown()278 if len(pids):
279 call(["kill"] + pids)
279280
280 def get_app_instances(self, app_name):281 def get_app_instances(self, app_name):
281 """Get BamfApplication instances for app_name."""282 """Get BamfApplication instances for app_name."""
282283
=== modified file 'tests/autopilot/autopilot/tests/test_launcher.py'
--- tests/autopilot/autopilot/tests/test_launcher.py 2012-05-15 19:33:20 +0000
+++ tests/autopilot/autopilot/tests/test_launcher.py 2012-05-15 23:08:19 +0000
@@ -445,6 +445,20 @@
445 self.assertTrue(mah_win2.is_hidden)445 self.assertTrue(mah_win2.is_hidden)
446 self.assertVisibleWindowStack([mah_win1, calc_win])446 self.assertVisibleWindowStack([mah_win1, calc_win])
447447
448 def test_icon_shows_on_quick_application_reopen(self):
449 """Icons should stay on launcher when an application is quickly closed/reopened."""
450 calc = self.start_app("Calculator")
451 desktop_file = calc.desktop_file
452 calc_icon = self.launcher.model.get_icon_by_desktop_id(desktop_file)
453 self.assertThat(calc_icon.visible, Eventually(Equals(True)))
454
455 self.close_all_app("Calculator")
456 calc = self.start_app("Calculator")
457 sleep(2)
458
459 calc_icon = self.launcher.model.get_icon_by_desktop_id(desktop_file)
460 self.assertThat(calc_icon, NotEquals(None))
461 self.assertThat(calc_icon.visible, Eventually(Equals(True)))
448462
449class LauncherRevealTests(LauncherTestCase):463class LauncherRevealTests(LauncherTestCase):
450 """Test the launcher reveal behavior when in autohide mode."""464 """Test the launcher reveal behavior when in autohide mode."""