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

Proposed by Marco Trevisan (Treviño) on 2012-05-15
Status: Merged
Approved by: Marco Trevisan (Treviño) on 2012-05-16
Approved revision: 2359
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 on 2012-05-16
Thomi Richards (community) 2012-05-15 Approve on 2012-05-15
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.
2359. By Marco Trevisan (Treviño) on 2012-05-15

autopilot, fix the close_all_app to correctly use kill

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

review: Approve
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
1=== modified file 'launcher/BamfLauncherIcon.cpp'
2--- launcher/BamfLauncherIcon.cpp 2012-05-07 19:52:54 +0000
3+++ launcher/BamfLauncherIcon.cpp 2012-05-15 23:08:19 +0000
4@@ -55,7 +55,7 @@
5 , _window_moved_id(0)
6 {
7 g_object_set_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen"),
8- GINT_TO_POINTER(1));
9+ GUINT_TO_POINTER(1));
10 auto bamf_view = glib::object_cast<BamfView>(_bamf_app);
11
12 glib::String icon(bamf_view_get_icon(bamf_view));
13@@ -101,10 +101,17 @@
14 sig = new glib::Signal<void, BamfView*, gboolean>(bamf_view, "running-changed",
15 [&] (BamfView*, gboolean running) {
16 SetQuirk(QUIRK_RUNNING, running);
17+
18 if (running)
19 {
20 EnsureWindowState();
21 UpdateIconGeometries(GetCenters());
22+
23+ if (_remove_timeout_id)
24+ {
25+ g_source_remove(_remove_timeout_id);
26+ _remove_timeout_id = 0;
27+ }
28 }
29 });
30 _gsignals.Add(sig);
31@@ -119,7 +126,23 @@
32 sig = new glib::Signal<void, BamfView*>(bamf_view, "closed",
33 [&] (BamfView*) {
34 if (!IsSticky())
35- Remove();
36+ {
37+ /* Use a timeout to remove the icon, this avoids
38+ * that we remove an application that is going
39+ * to be reopened soon. So applications that
40+ * have a splash screen won't be removed from
41+ * the launcher while the splash is closed and
42+ * a new window is opened. */
43+ if (_remove_timeout_id)
44+ g_source_remove(_remove_timeout_id);
45+
46+ _remove_timeout_id = g_timeout_add_seconds(1, [] (gpointer data) -> gboolean {
47+ auto self = static_cast<BamfLauncherIcon*>(data);
48+ self->Remove();
49+ self->_remove_timeout_id = 0;
50+ return false;
51+ }, this);
52+ }
53 });
54 _gsignals.Add(sig);
55
56@@ -147,6 +170,9 @@
57 g_object_set_qdata(G_OBJECT(_bamf_app.RawPtr()),
58 g_quark_from_static_string("unity-seen"), nullptr);
59
60+ if (_remove_timeout_id != 0)
61+ g_source_remove(_remove_timeout_id);
62+
63 if (_fill_supported_types_id != 0)
64 g_source_remove(_fill_supported_types_id);
65
66@@ -160,6 +186,18 @@
67 g_source_remove(_dnd_hover_timer);
68 }
69
70+void BamfLauncherIcon::Remove()
71+{
72+ /* Removing the unity-seen flag to the wrapped bamf application, on remove
73+ * request we make sure that if the bamf application is re-opened while
74+ * the removal process is still ongoing, the application will be shown
75+ * on the launcher. */
76+ g_object_set_qdata(G_OBJECT(_bamf_app.RawPtr()),
77+ g_quark_from_static_string("unity-seen"), nullptr);
78+
79+ SimpleLauncherIcon::Remove();
80+}
81+
82 bool BamfLauncherIcon::IsSticky() const
83 {
84 return bamf_view_is_sticky(BAMF_VIEW(_bamf_app.RawPtr()));
85
86=== modified file 'launcher/BamfLauncherIcon.h'
87--- launcher/BamfLauncherIcon.h 2012-05-07 19:52:54 +0000
88+++ launcher/BamfLauncherIcon.h 2012-05-15 23:08:19 +0000
89@@ -66,6 +66,7 @@
90 std::string NameForWindow(Window window);
91
92 protected:
93+ void Remove();
94 void UpdateIconGeometries(std::vector<nux::Point3> center);
95 void OnCenterStabilized(std::vector<nux::Point3> center);
96 void AddProperties(GVariantBuilder* builder);
97@@ -124,6 +125,7 @@
98 guint _dnd_hover_timer;
99
100 bool _supported_types_filled;
101+ guint _remove_timeout_id;
102 guint _fill_supported_types_id;
103 guint _window_moved_id;
104 guint _quicklist_activated_id;
105
106=== modified file 'tests/autopilot/autopilot/tests/__init__.py'
107--- tests/autopilot/autopilot/tests/__init__.py 2012-05-04 17:33:59 +0000
108+++ tests/autopilot/autopilot/tests/__init__.py 2012-05-15 23:08:19 +0000
109@@ -274,8 +274,9 @@
110 def close_all_app(self, app_name):
111 """Close all instances of the app_name."""
112 app = self.KNOWN_APPS[app_name]
113- self.addCleanup(call, "kill `pidof %s`" % (app['process-name']), shell=True)
114- super(LoggedTestCase, self).tearDown()
115+ pids = check_output(["pidof", app['process-name']]).split()
116+ if len(pids):
117+ call(["kill"] + pids)
118
119 def get_app_instances(self, app_name):
120 """Get BamfApplication instances for app_name."""
121
122=== modified file 'tests/autopilot/autopilot/tests/test_launcher.py'
123--- tests/autopilot/autopilot/tests/test_launcher.py 2012-05-15 19:33:20 +0000
124+++ tests/autopilot/autopilot/tests/test_launcher.py 2012-05-15 23:08:19 +0000
125@@ -445,6 +445,20 @@
126 self.assertTrue(mah_win2.is_hidden)
127 self.assertVisibleWindowStack([mah_win1, calc_win])
128
129+ def test_icon_shows_on_quick_application_reopen(self):
130+ """Icons should stay on launcher when an application is quickly closed/reopened."""
131+ calc = self.start_app("Calculator")
132+ desktop_file = calc.desktop_file
133+ calc_icon = self.launcher.model.get_icon_by_desktop_id(desktop_file)
134+ self.assertThat(calc_icon.visible, Eventually(Equals(True)))
135+
136+ self.close_all_app("Calculator")
137+ calc = self.start_app("Calculator")
138+ sleep(2)
139+
140+ calc_icon = self.launcher.model.get_icon_by_desktop_id(desktop_file)
141+ self.assertThat(calc_icon, NotEquals(None))
142+ self.assertThat(calc_icon.visible, Eventually(Equals(True)))
143
144 class LauncherRevealTests(LauncherTestCase):
145 """Test the launcher reveal behavior when in autohide mode."""