Merge lp:~bilalakhtar/unity/software-center-integration-for-o into lp:unity

Proposed by Bilal Akhtar on 2011-08-17
Status: Superseded
Proposed branch: lp:~bilalakhtar/unity/software-center-integration-for-o
Merge into: lp:unity
Diff against target: 330 lines (+223/-4)
6 files modified
plugins/unityshell/src/Launcher.cpp (+3/-3)
plugins/unityshell/src/Launcher.h (+1/-0)
plugins/unityshell/src/LauncherController.cpp (+55/-1)
plugins/unityshell/src/LauncherController.h (+2/-0)
plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp (+102/-0)
plugins/unityshell/src/SoftwareCenterLauncherIcon.h (+60/-0)
To merge this branch: bzr merge lp:~bilalakhtar/unity/software-center-integration-for-o
Reviewer Review Type Date Requested Status
Mikkel Kamstrup Erlandsen (community) 2012-01-15 Needs Fixing on 2012-01-17
Neil J. Patel (community) 2011-08-17 Needs Fixing on 2011-08-19
Unity Team 2012-01-17 Pending
Review via email: mp+71905@code.launchpad.net

This proposal has been superseded by a proposal from 2012-01-20.

Description of the change

This branch contains the first phase of the implementation of software-center integration with Unity. The complete spec on how it should be implemented, is located at:
https://wiki.ubuntu.com/SoftwareCenter#Learning_how_to_launch_an_application

This branch brings the following changes:
1) An app being installed is shown in the launcher with a tooltip "Waiting to install"
2) A progress bar on the launcher item displays the download/install progress.
3) When the app gets installed, the launcher items becomes usable, and the tooltip gets changed to the app name.

Things which are still to be implemented, in the next phases:
1) Animate the movement of the icon from the USC window to the Unity launcher.
2) Make the launcher icon wiggle when installation is complete
3) Stop the launcher icon from blinking when clicked in "waiting to install" state.

In the meantime, you can merge this branch into Unity while I implement the remaining aspects

Branch Testing instructions:
1) Make sure Unity built from my branch is running.
2) Get software-center from bzr branch lp:~gary-lasker/software-center/launcher-integration-lp761851
3) In the software center branch dir, run PYTHONPATH=. python ./software-center
4) Make sure that in the software center View menu, "New Applications In Launcher" is CHECKED.
5) Install a package using software-center that contains a desktop file (like gnome-color-chooser, gnome-mplayer or could be any package with a .desktop file)
6) Watch the Unity launcher :-)

To post a comment you must log in.
1358. By Bilal Akhtar on 2011-08-17

MEH. Fix conflicts

Neil J. Patel (njpatel) wrote :

Hey Bilal, great work so far! I have some points which will hopefully reduce the code and make it a bit nicer to merge:

+ We try and not do anything _sync() in Unity, as it effects startup time and, if done after startup, painting time of Compiz. I see that you are connecting to the apt daemon via D-Bus, to make the code cleaner and automatically async, I suggest you use unity::glib::DBusProxy in <UnityCore/GLibDBusProxy.h> I added, which wraps the GIO DBusProxy bits in a C++ wrapper that:

  - Connects to your chosen bus and object asynchronously
  - Let's you use the Connect() method that allows you to connect to signals from the proxy individually [instead of
    having a if (signal_name == foo) else if () etc etc], and also allows you to use a sigc::mem_fun instead of a
    static function, which should make the code nicer.

You also forgot to store the proxy pointer and unref it on destruction (as well as disconnecting the signal). Using the glib::DBusProxy will handle all this for you internally, so you'll only need to delete the proxy instance once your done with it.

+ You shouldn't be g_variant_unref'ing the "params" value, as it is passed as a function argument and not expected to be unref'd. I think you wanted to unref "property_value".

+ When you start using glib::DBusProxy, you should be able to incorporate initialize_tooltip_text(); into your constructor as the constructor won't be as large anymore.

+ Instead of if (!app), do if (!BAMF_IS_APPLICATION(app))

review: Needs Fixing
1359. By Bilal Akhtar on 2011-08-19

Merge from trunk

1360. By Bilal Akhtar on 2011-09-04

Fix some issues, thanks Neil! Still need to work out on GDBusProxy

1361. By Bilal Akhtar on 2011-09-05

Trunkify yet again

1362. By Bilal Akhtar on 2011-09-06

Use GLIbDBusProxy

1363. By Bilal Akhtar on 2011-09-06

Massive code cleanup, still not able to solve a memory issue :(

Bilal Akhtar (bilalakhtar) wrote :

Hi Neil,

I managed to get my file to use GLibDBusProxy. Remember the memory issue when adding the tooltip line suddenly made all the dbus calls fail mysteriously ? Moving to GLibDBusProxy has resurrected that weird issue, and now, I'm not able to find out the line which is causing it :(

Can you help me at it? I'll describe exactly what the issue is and where its happening:

1) The SCLauncherIcon class constructor is called, and both _aptdaemon_trans and _aptdaemon_trans_prop are initialized.

2) The Progress Get call is made. It fails, GLibDBusProxy returns an error that the proxy doesn't exist (while in fact it does, I've double-checked it with the aptdaemon log). Nothing comes in the log after this, except for point 3 below:

3) Mysteriously, the OnFinished callback gets called correctly when the transaction finishes (as expected), and I get the "Transaction Finished" message in the log at the right time.

Sure, C/C++ suck :(

Have you tried running a 'dbus-monitor > dbus-dump.txt' before doing a test run (and ctrl-c the command after). And then inspect the DBus traffic in that file carefully to make sure it looks like you expect?

Bilal Akhtar (bilalakhtar) wrote :

Yes, its exactly how its expected to be. Aptdaemon is creating the exactly same object which is being said by Unity to "not exist".

1364. By Bilal Akhtar on 2012-01-15

Merge from trunk

1365. By Bilal Akhtar on 2012-01-15

Fix build failure

1366. By Bilal Akhtar on 2012-01-15

Integrate SoftwareCenterLauncherIcon.* code back into the main Unity 5.0 source and fix build failures.

1367. By Bilal Akhtar on 2012-01-15

Major code cleanup, get progress bar to work

1368. By Bilal Akhtar on 2012-01-15

Fix persistance of launcher icons

Bilal Akhtar (bilalakhtar) wrote :

Okay, so this merge request is okay for a review again. Everything which I've mentioned in the description works, I've tested it with Gary's branch and all.

Do note that the animation hasn't been implemented yet, I'll do that later on. Most of the groundwork is complete, only the animation and the wiggle etc has to land, which I'll do next weekend.

This was originally supposed to land in Oneiric but due to my faults it was postponed to Precise., but I've had good progress so far in the Precise cycle and as per current state, expect all changes to have landed by Alpha 2.

1369. By Bilal Akhtar on 2012-01-16

Fix crash, icon of newly-created icon

Just some drive by comments - I am not sure I am the right guy to review this...

166 + * Copyright (C) 2010 Canonical Ltd

I believe we write 2012 these days ;-)

223 + _aptdaemon_trans->Connect("Finished", sigc::mem_fun(*this, &SoftwareCenterLauncherIcon::OnFinished));
224 + _aptdaemon_trans->Connect("PropertyChanged", sigc::mem_fun(*this, &SoftwareCenterLauncherIcon::OnPropertyChanged));

Are you sure you mean '*this' and not 'this'?

Generally; I don't like the 'if (!g_strcmp0 (variable, "XYZ"))' construction. Do it like how it's done elsewhere too, with == 0. It improves legibility. Humans doesn't not suck at negations ;-)

review: Needs Fixing
1370. By Bilal Akhtar on 2012-01-17

Make minor changes as pointed out by Mikkel, thanks!

Bilal Akhtar (bilalakhtar) wrote :

Okay, fixed, ready for a review again!

Sorry :-)

249 + if (!g_strcmp0 (property_name, "Progress")) {

I am taking the branch for a test spin - and one problem I've discovered so far is that if S-C hangs (because apt is broken typically) then you'll be perpetually stuck with a launcher icon with an empty progress bar on it.

I am not sure what to do in these situations. I guess the right thing is to simply have some sort of timeout. If there haven't been progress in 20s then silently remove the launcher icon. There are probably better ways though..?

review: Needs Fixing

More comments - mostly also noted in your merge request:

 - Clicking the icon when the root password dialog from USC is up, starts pulsing the icon as if launched. From the spec I gather the icon should be grayed out and unresponive.

 - Same for while the app is installing

 - With lots of apps in the launcher the new app is "folded away". Meaning that I can't see the progress bar or tell when the app is done installing. I'd suggest that we wiggle it/call for attention/set urgency when done installing.

I guess these are points 2) and 3) on your todo list - except that you should "graying out while installing" as well?

But with the g_strcmp0() fix I noted, I guess this branch is good enough to merge. It is at leats ways better than not having it.

Bilal Akhtar (bilalakhtar) wrote :

As for the grey out part, I chatted with MPT at UDS and he was ready to compromise on that part and instead get a progress bar implemented.So, we'll implement a progress bar for now and we'll see later what we can do about the graying part.

Bilal Akhtar (bilalakhtar) wrote :

I'll make the g_strcmp0 fix this evening and then you can go ahead with it for now.

1371. By Bilal Akhtar on 2012-01-17

Fix yet another g_strcmp0

Bilal Akhtar (bilalakhtar) wrote :

There, you can review it again :)

Bilal Akhtar (bilalakhtar) wrote :

As for your question on SC hanging, in theory a crashed SC shouldn't screw anything up, the progress bar will still be updated regularly as it queries progress status from aptdaemon and not software-center. And if aptdaemon crashes, the apt process itself would crash I guess. If Apt crashes and not aptdaemon, aptdaemon would send a "finished" signal which would clear up the progress bar and remove it from the launcher.

So I guess the current implementation is okay for now, we'll investigate later on if issues arise.

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/unityshell/src/Launcher.cpp'
2--- plugins/unityshell/src/Launcher.cpp 2012-01-14 13:05:16 +0000
3+++ plugins/unityshell/src/Launcher.cpp 2012-01-17 20:09:26 +0000
4@@ -112,8 +112,8 @@
5 " <interface name='com.canonical.Unity.Launcher'>"
6 ""
7 " <method name='AddLauncherItemFromPosition'>"
8+ " <arg type='s' name='title' direction='in'/>"
9 " <arg type='s' name='icon' direction='in'/>"
10- " <arg type='s' name='title' direction='in'/>"
11 " <arg type='i' name='icon_x' direction='in'/>"
12 " <arg type='i' name='icon_y' direction='in'/>"
13 " <arg type='i' name='icon_size' direction='in'/>"
14@@ -3313,10 +3313,10 @@
15 gchar* desktop_file;
16 gchar* aptdaemon_task;
17
18- g_variant_get(parameters, "(ssiiiss)", &icon, &title, &icon_x, &icon_y, &icon_size, &desktop_file, &aptdaemon_task, NULL);
19+ g_variant_get(parameters, "(ssiiiss)", &title, &icon, &icon_x, &icon_y, &icon_size, &desktop_file, &aptdaemon_task, NULL);
20
21 Launcher* self = (Launcher*)user_data;
22- self->launcher_addrequest.emit(desktop_file, NULL);
23+ self->launcher_addrequest_special.emit(desktop_file, NULL, aptdaemon_task, icon);
24
25 g_dbus_method_invocation_return_value(invocation, NULL);
26 g_free(icon);
27
28=== modified file 'plugins/unityshell/src/Launcher.h'
29--- plugins/unityshell/src/Launcher.h 2012-01-07 13:26:49 +0000
30+++ plugins/unityshell/src/Launcher.h 2012-01-17 20:09:26 +0000
31@@ -195,6 +195,7 @@
32 void EnableCheckWindowOverLauncher(gboolean enabled);
33
34 sigc::signal<void, char*, LauncherIcon*> launcher_addrequest;
35+ sigc::signal<void, char*, LauncherIcon*, char*, char*> launcher_addrequest_special;
36 sigc::signal<void, LauncherIcon*> launcher_removerequest;
37 sigc::signal<void> selection_change;
38 sigc::signal<void> hidden_changed;
39
40=== modified file 'plugins/unityshell/src/LauncherController.cpp'
41--- plugins/unityshell/src/LauncherController.cpp 2012-01-07 17:09:23 +0000
42+++ plugins/unityshell/src/LauncherController.cpp 2012-01-17 20:09:26 +0000
43@@ -38,12 +38,12 @@
44 #include "LauncherEntryRemote.h"
45 #include "LauncherEntryRemoteModel.h"
46 #include "LauncherIcon.h"
47+#include "SoftwareCenterLauncherIcon.h"
48 #include "LauncherModel.h"
49 #include "WindowManager.h"
50 #include "TrashLauncherIcon.h"
51 #include "BFBLauncherIcon.h"
52
53-
54 namespace unity
55 {
56 namespace launcher
57@@ -67,6 +67,7 @@
58 void OnIconAdded(LauncherIcon* icon);
59
60 void OnLauncherAddRequest(char* path, LauncherIcon* before);
61+ void OnLauncherAddRequestSpecial(char* path, LauncherIcon* before, char* aptdaemon_trans_id, char* icon_path);
62 void OnLauncherRemoveRequest(LauncherIcon* icon);
63
64 void OnLauncherEntryRemoteAdded(LauncherEntryRemote* entry);
65@@ -84,6 +85,8 @@
66
67 LauncherIcon* CreateFavorite(const char* file_path);
68
69+ SoftwareCenterLauncherIcon* CreateSCLauncherIcon(const char* file_path, const char* aptdaemon_trans_id, char* icon_path);
70+
71 void SetupBamf();
72
73 void OnExpoActivated();
74@@ -147,6 +150,7 @@
75
76 launcher_->SetModel(model_.get());
77 launcher_->launcher_addrequest.connect(sigc::mem_fun(this, &Impl::OnLauncherAddRequest));
78+ launcher_->launcher_addrequest_special.connect(sigc::mem_fun(this, &Impl::OnLauncherAddRequestSpecial));
79 launcher_->launcher_removerequest.connect(sigc::mem_fun(this, &Impl::OnLauncherRemoveRequest));
80
81 device_section_ = new DeviceLauncherSection(raw_launcher);
82@@ -235,6 +239,30 @@
83 unity::FavoriteStore::GetDefault().SetFavorites(desktop_paths);
84 }
85
86+void
87+Controller::Impl::OnLauncherAddRequestSpecial(char* path, LauncherIcon* before, char* aptdaemon_trans_id, char* icon_path)
88+{
89+ std::list<BamfLauncherIcon*> launchers;
90+ std::list<BamfLauncherIcon*>::iterator it;
91+
92+ launchers = model_->GetSublist<BamfLauncherIcon> ();
93+ for (it = launchers.begin(); it != launchers.end(); it++)
94+ {
95+ if (g_strcmp0(path, (*it)->DesktopFile()) == 0)
96+ return;
97+ }
98+
99+ SoftwareCenterLauncherIcon* result = CreateSCLauncherIcon(path, aptdaemon_trans_id, icon_path);
100+ if (result)
101+ {
102+ RegisterIcon(result);
103+
104+ if (before)
105+ model_->ReorderBefore(result, before, false);
106+ }
107+ Save();
108+}
109+
110 void Controller::Impl::SortAndUpdate()
111 {
112 gint shortcut = 1;
113@@ -444,6 +472,32 @@
114 return icon;
115 }
116
117+SoftwareCenterLauncherIcon*
118+Controller::Impl::CreateSCLauncherIcon(const char* file_path, const char* aptdaemon_trans_id, char* icon_path)
119+{
120+ BamfApplication* app;
121+ SoftwareCenterLauncherIcon* icon;
122+
123+ app = bamf_matcher_get_application_for_desktop_file(matcher_, file_path, true);
124+ if (!BAMF_IS_APPLICATION(app))
125+ return NULL;
126+
127+ if (g_object_get_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen")))
128+ {
129+ bamf_view_set_sticky(BAMF_VIEW(app), true);
130+ return 0;
131+ }
132+
133+ g_object_set_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen"), GINT_TO_POINTER(1));
134+
135+ bamf_view_set_sticky(BAMF_VIEW(app), true);
136+ icon = new SoftwareCenterLauncherIcon(launcher_.GetPointer(), app, (char*)aptdaemon_trans_id, icon_path);
137+ icon->SetIconType(LauncherIcon::TYPE_APPLICATION);
138+ icon->SetSortPriority(sort_priority_++);
139+
140+ return icon;
141+}
142+
143 void Controller::Impl::SetupBamf()
144 {
145 GList* apps, *l;
146
147=== modified file 'plugins/unityshell/src/LauncherController.h'
148--- plugins/unityshell/src/LauncherController.h 2011-11-30 03:06:44 +0000
149+++ plugins/unityshell/src/LauncherController.h 2012-01-17 20:09:26 +0000
150@@ -26,6 +26,8 @@
151 #include <sigc++/sigc++.h>
152 #include <core/core.h>
153
154+#include "SoftwareCenterLauncherIcon.h"
155+
156 namespace unity
157 {
158 namespace launcher
159
160=== added file 'plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp'
161--- plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp 1970-01-01 00:00:00 +0000
162+++ plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp 2012-01-17 20:09:26 +0000
163@@ -0,0 +1,102 @@
164+// -*- Mode: C++; indent-tabs-mode: nil; tab-width: 2 -*-
165+/*
166+ * Copyright (C) 2012 Canonical Ltd
167+ *
168+ * This program is free software: you can redistribute it and/or modify
169+ * it under the terms of the GNU General Public License version 3 as
170+ * published by the Free Software Foundation.
171+ *
172+ * This program is distributed in the hope that it will be useful,
173+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
174+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
175+ * GNU General Public License for more details.
176+ *
177+ * You should have received a copy of the GNU General Public License
178+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
179+ *
180+ * Authored by: Bilal Akhtar <bilalakhtar@ubuntu.com>
181+ */
182+
183+#include "Nux/Nux.h"
184+#include "Nux/BaseWindow.h"
185+
186+#include "BamfLauncherIcon.h"
187+#include "Launcher.h"
188+#include "LauncherController.h"
189+#include "PluginAdapter.h"
190+#include "FavoriteStore.h"
191+
192+#include "ubus-server.h"
193+#include "UBusMessages.h"
194+
195+#include <glib.h>
196+#include <glib/gvariant.h>
197+#include <glib/gi18n-lib.h>
198+#include <gio/gio.h>
199+#include <libindicator/indicator-desktop-shortcuts.h>
200+#include <core/core.h>
201+#include <core/atoms.h>
202+
203+#include "SoftwareCenterLauncherIcon.h"
204+
205+namespace unity
206+{
207+namespace launcher
208+{
209+
210+SoftwareCenterLauncherIcon::SoftwareCenterLauncherIcon(Launcher* IconManager, BamfApplication* app, char* aptdaemon_trans_id, char* icon_path)
211+: BamfLauncherIcon(IconManager, app)
212+{
213+ _aptdaemon_trans_id = aptdaemon_trans_id;
214+
215+ g_debug("Aptdaemon transaction ID: %s", _aptdaemon_trans_id);
216+
217+ _aptdaemon_trans = new unity::glib::DBusProxy("org.debian.apt",
218+ _aptdaemon_trans_id,
219+ "org.debian.apt.transaction",
220+ G_BUS_TYPE_SYSTEM,
221+ G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START);
222+
223+ _aptdaemon_trans->Connect("Finished", sigc::mem_fun(this, &SoftwareCenterLauncherIcon::OnFinished));
224+ _aptdaemon_trans->Connect("PropertyChanged", sigc::mem_fun(this, &SoftwareCenterLauncherIcon::OnPropertyChanged));
225+
226+ icon_name = icon_path;
227+}
228+
229+SoftwareCenterLauncherIcon::~SoftwareCenterLauncherIcon() {
230+
231+}
232+
233+void
234+SoftwareCenterLauncherIcon::OnFinished(GVariant* params) {
235+
236+ tooltip_text = BamfName();
237+
238+ SetQuirk(LauncherIcon::QUIRK_PROGRESS, FALSE);
239+}
240+
241+void
242+SoftwareCenterLauncherIcon::OnPropertyChanged(GVariant* params) {
243+
244+ gint32 progress;
245+ gchar* property_name;
246+ GVariant* property_value;
247+
248+ g_variant_get_child (params, 0, "s", &property_name);
249+ if (g_strcmp0 (property_name, "Progress") == 0) {
250+ g_variant_get_child (params,1,"v",&property_value);
251+ g_variant_get (property_value, "i", &progress);
252+
253+ if (progress < 100) {
254+ SetQuirk(LauncherIcon::QUIRK_PROGRESS, TRUE);
255+ tooltip_text = _("Waiting to install");
256+ }
257+ SetProgress(((float)progress) / ((float)100));
258+ }
259+ g_variant_unref(property_value);
260+ g_free(property_name);
261+
262+}
263+
264+}
265+}
266
267=== added file 'plugins/unityshell/src/SoftwareCenterLauncherIcon.h'
268--- plugins/unityshell/src/SoftwareCenterLauncherIcon.h 1970-01-01 00:00:00 +0000
269+++ plugins/unityshell/src/SoftwareCenterLauncherIcon.h 2012-01-17 20:09:26 +0000
270@@ -0,0 +1,60 @@
271+// -*- Mode: C++; indent-tabs-mode: nil; tab-width: 2 -*-
272+/*
273+ * Copyright (C) 2010 Canonical Ltd
274+ *
275+ * This program is free software: you can redistribute it and/or modify
276+ * it under the terms of the GNU General Public License version 3 as
277+ * published by the Free Software Foundation.
278+ *
279+ * This program is distributed in the hope that it will be useful,
280+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
281+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
282+ * GNU General Public License for more details.
283+ *
284+ * You should have received a copy of the GNU General Public License
285+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
286+ *
287+ * Authored by: Bilal Akhtar <bilalakhtar@ubuntu.com>
288+ */
289+
290+#ifndef SOFTWARECENTERLAUNCHERICON_H
291+#define SOFTWARECENTERLAUNCHERICON_H
292+
293+#include "BamfLauncherIcon.h"
294+#include <Nux/BaseWindow.h>
295+#include <NuxCore/Math/MathInc.h>
296+#include <core/core.h>
297+#include <gio/gio.h>
298+#include <glib.h>
299+#include <glib/gvariant.h>
300+#include <UnityCore/GLibDBusProxy.h>
301+
302+
303+namespace unity
304+{
305+namespace launcher
306+{
307+
308+
309+class SoftwareCenterLauncherIcon : public BamfLauncherIcon
310+{
311+public:
312+
313+ SoftwareCenterLauncherIcon(Launcher* IconManager, BamfApplication* app, char* aptdaemon_trans_id, char* icon_path);
314+ virtual ~SoftwareCenterLauncherIcon();
315+
316+ gchar* original_tooltip_text;
317+
318+private:
319+ char* _aptdaemon_trans_id;
320+ unity::glib::DBusProxy* _aptdaemon_trans;
321+
322+ void OnFinished(GVariant* params);
323+
324+ void OnPropertyChanged(GVariant* params);
325+};
326+
327+}
328+}
329+
330+#endif