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

[Bilal Akhtar]

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 :-)

[Neil J. Patel (njpatel)]

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

[Bilal Akhtar (bilalakhtar)]

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 :(

[Mikkel Kamstrup Erlandsen (kamstrup)]

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)]

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

[Bilal Akhtar (bilalakhtar)]

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.

[Mikkel Kamstrup Erlandsen (kamstrup)]

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

[Bilal Akhtar (bilalakhtar)]

Okay, fixed, ready for a review again!

[Mikkel Kamstrup Erlandsen (kamstrup)]

Sorry :-)

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

[Mikkel Kamstrup Erlandsen (kamstrup)]

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

[Mikkel Kamstrup Erlandsen (kamstrup)]

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)]

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)]

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

[Bilal Akhtar (bilalakhtar)]

There, you can review it again :)

[Bilal Akhtar (bilalakhtar)]

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.

[Mikkel Kamstrup Erlandsen (kamstrup)]

Looks good now. Thanks Bilal!

review: Approve

[Gary Lasker (gary-lasker)]

Bilal, this is awesome!! I'm so excited that this is landed. Thank you for your great work, and thank you Mikkel for reviewing!

I'm am wondering if we need to coordinate releasing the USC side of this (currently being held in the branch lp:~gary-lasker/software-center/launcher-integration-lp761851, as you noted above) with the Unity release that contains this change. It definitely seems like we should release the USC side as soon as possible after the Unity update goes out. Please let me know and we can make sure that happens.

Thanks again!!! (/me ^5s Bilal)
Gary

[Bilal Akhtar (bilalakhtar)]

Yes, Gary's right, we'd surely need a co-ordinated upload.

I'm not sure about when the next Unity precise upload will be, otherwise I'd have acted as a mediator. Could someone from the Unity team point me/Gary at a roadmap OR poke me or Gary when Unity packages with this change get uploaded to the repos?

Thanks!

[Michal Hruby (mhr3)]

Hey Bilal, next unity release (5.2) should be next week (most likely Thursday).

[Michael Vogt (mvo)]

Once the new unity is uploaded we will upload a matching s-c as well.

[Bilal Akhtar (bilalakhtar)]

Second phase is complete, merge request: https://code.launchpad.net/~bilalakhtar/unity/sc-integration-phase2/+merge/95795

=== modified file 'plugins/unityshell/src/Launcher.cpp'
--- plugins/unityshell/src/Launcher.cpp	2012-01-18 11:16:38 +0000
+++ plugins/unityshell/src/Launcher.cpp	2012-01-20 00:43:27 +0000
@@ -112,8 +112,8 @@
   "  <interface name='com.canonical.Unity.Launcher'>"
   ""
   "    <method name='AddLauncherItemFromPosition'>"
+  "      <arg type='s' name='title' direction='in'/>"
   "      <arg type='s' name='icon' direction='in'/>"
-  "      <arg type='s' name='title' direction='in'/>"
   "      <arg type='i' name='icon_x' direction='in'/>"
   "      <arg type='i' name='icon_y' direction='in'/>"
   "      <arg type='i' name='icon_size' direction='in'/>"
@@ -3300,10 +3300,10 @@
     gchar*  desktop_file;
     gchar*  aptdaemon_task;
 
-    g_variant_get(parameters, "(ssiiiss)", &icon, &title, &icon_x, &icon_y, &icon_size, &desktop_file, &aptdaemon_task, NULL);
+    g_variant_get(parameters, "(ssiiiss)", &title, &icon, &icon_x, &icon_y, &icon_size, &desktop_file, &aptdaemon_task, NULL);
 
     Launcher* self = (Launcher*)user_data;
-    self->launcher_addrequest.emit(desktop_file, NULL);
+    self->launcher_addrequest_special.emit(desktop_file, NULL, aptdaemon_task, icon);
 
     g_dbus_method_invocation_return_value(invocation, NULL);
     g_free(icon);

=== modified file 'plugins/unityshell/src/Launcher.h'
--- plugins/unityshell/src/Launcher.h	2012-01-17 15:41:30 +0000
+++ plugins/unityshell/src/Launcher.h	2012-01-20 00:43:27 +0000
@@ -196,6 +196,7 @@
   void EnableCheckWindowOverLauncher(gboolean enabled);
 
   sigc::signal<void, char*, LauncherIcon*> launcher_addrequest;
+  sigc::signal<void, char*, LauncherIcon*, char*, char*> launcher_addrequest_special;
   sigc::signal<void, LauncherIcon*> launcher_removerequest;
   sigc::signal<void> selection_change;
   sigc::signal<void> hidden_changed;

=== modified file 'plugins/unityshell/src/LauncherController.cpp'
--- plugins/unityshell/src/LauncherController.cpp	2012-01-10 15:26:51 +0000
+++ plugins/unityshell/src/LauncherController.cpp	2012-01-20 00:43:27 +0000
@@ -38,12 +38,12 @@
 #include "LauncherEntryRemote.h"
 #include "LauncherEntryRemoteModel.h"
 #include "LauncherIcon.h"
+#include "SoftwareCenterLauncherIcon.h"
 #include "LauncherModel.h"
 #include "WindowManager.h"
 #include "TrashLauncherIcon.h"
 #include "BFBLauncherIcon.h"
 
-
 namespace unity
 {
 namespace launcher
@@ -68,6 +68,7 @@
   void OnIconRemoved(LauncherIcon* icon);
 
   void OnLauncherAddRequest(char* path, LauncherIcon* before);
+  void OnLauncherAddRequestSpecial(char* path, LauncherIcon* before, char* aptdaemon_trans_id, char* icon_path);
   void OnLauncherRemoveRequest(LauncherIcon* icon);
 
   void OnLauncherEntryRemoteAdded(LauncherEntryRemote* entry);
@@ -85,6 +86,8 @@
 
   LauncherIcon* CreateFavorite(const char* file_path);
 
+  SoftwareCenterLauncherIcon* CreateSCLauncherIcon(const char* file_path, const char* aptdaemon_trans_id, char* icon_path);
+
   void SetupBamf();
 
   void OnExpoActivated();
@@ -148,6 +151,7 @@
 
   launcher_->SetModel(model_.get());
   launcher_->launcher_addrequest.connect(sigc::mem_fun(this, &Impl::OnLauncherAddRequest));
+  launcher_->launcher_addrequest_special.connect(sigc::mem_fun(this, &Impl::OnLauncherAddRequestSpecial));
   launcher_->launcher_removerequest.connect(sigc::mem_fun(this, &Impl::OnLauncherRemoveRequest));
 
   device_section_ = new DeviceLauncherSection(raw_launcher);
@@ -236,6 +240,30 @@
   unity::FavoriteStore::GetDefault().SetFavorites(desktop_paths);
 }
 
+void
+Controller::Impl::OnLauncherAddRequestSpecial(char* path, LauncherIcon* before, char* aptdaemon_trans_id, char* icon_path)
+{
+  std::list<BamfLauncherIcon*> launchers;
+  std::list<BamfLauncherIcon*>::iterator it;
+
+  launchers = model_->GetSublist<BamfLauncherIcon> ();
+  for (it = launchers.begin(); it != launchers.end(); it++)
+  {
+    if (g_strcmp0(path, (*it)->DesktopFile()) == 0)
+      return;
+  }
+
+  SoftwareCenterLauncherIcon* result = CreateSCLauncherIcon(path, aptdaemon_trans_id, icon_path);
+  if (result)
+  {
+    RegisterIcon(result);
+
+    if (before)
+      model_->ReorderBefore(result, before, false);
+  }
+  Save();
+}
+
 void Controller::Impl::SortAndUpdate()
 {
   gint   shortcut = 1;
@@ -450,6 +478,32 @@
   return icon;
 }
 
+SoftwareCenterLauncherIcon*
+Controller::Impl::CreateSCLauncherIcon(const char* file_path, const char* aptdaemon_trans_id, char* icon_path)
+{
+  BamfApplication* app;
+  SoftwareCenterLauncherIcon* icon;
+
+  app = bamf_matcher_get_application_for_desktop_file(matcher_, file_path, true);
+  if (!BAMF_IS_APPLICATION(app))
+    return NULL;
+
+  if (g_object_get_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen")))
+  {
+    bamf_view_set_sticky(BAMF_VIEW(app), true);
+    return 0;
+  }
+
+  g_object_set_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen"), GINT_TO_POINTER(1));
+
+  bamf_view_set_sticky(BAMF_VIEW(app), true);
+  icon = new SoftwareCenterLauncherIcon(launcher_.GetPointer(), app, (char*)aptdaemon_trans_id, icon_path);
+  icon->SetIconType(LauncherIcon::TYPE_APPLICATION);
+  icon->SetSortPriority(sort_priority_++);
+
+  return icon;
+}
+
 void Controller::Impl::SetupBamf()
 {
   GList* apps, *l;

=== modified file 'plugins/unityshell/src/LauncherController.h'
--- plugins/unityshell/src/LauncherController.h	2011-11-30 03:06:44 +0000
+++ plugins/unityshell/src/LauncherController.h	2012-01-20 00:43:27 +0000
@@ -26,6 +26,8 @@
 #include <sigc++/sigc++.h>
 #include <core/core.h>
 
+#include "SoftwareCenterLauncherIcon.h"
+
 namespace unity
 {
 namespace launcher

=== added file 'plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp'
--- plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp	1970-01-01 00:00:00 +0000
+++ plugins/unityshell/src/SoftwareCenterLauncherIcon.cpp	2012-01-20 00:43:27 +0000
@@ -0,0 +1,102 @@
+// -*- Mode: C++; indent-tabs-mode: nil; tab-width: 2 -*-
+/*
+ * Copyright (C) 2012 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Bilal Akhtar <[e-mail]>
+ */
+
+#include "Nux/Nux.h"
+#include "Nux/BaseWindow.h"
+
+#include "BamfLauncherIcon.h"
+#include "Launcher.h"
+#include "LauncherController.h"
+#include "PluginAdapter.h"
+#include "FavoriteStore.h"
+
+#include "ubus-server.h"
+#include "UBusMessages.h"
+
+#include <glib.h>
+#include <glib/gvariant.h>
+#include <glib/gi18n-lib.h>
+#include <gio/gio.h>
+#include <libindicator/indicator-desktop-shortcuts.h>
+#include <core/core.h>
+#include <core/atoms.h>
+
+#include "SoftwareCenterLauncherIcon.h"
+
+namespace unity
+{
+namespace launcher
+{
+
+SoftwareCenterLauncherIcon::SoftwareCenterLauncherIcon(Launcher* IconManager, BamfApplication* app, char* aptdaemon_trans_id, char* icon_path)
+: BamfLauncherIcon(IconManager, app)
+{
+    _aptdaemon_trans_id = aptdaemon_trans_id; 
+    
+    g_debug("Aptdaemon transaction ID: %s", _aptdaemon_trans_id);
+
+    _aptdaemon_trans = new unity::glib::DBusProxy("org.debian.apt",
+                                    _aptdaemon_trans_id,
+                                    "org.debian.apt.transaction",
+                                    G_BUS_TYPE_SYSTEM,
+                                    G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START);
+
+    _aptdaemon_trans->Connect("Finished", sigc::mem_fun(this, &SoftwareCenterLauncherIcon::OnFinished));
+    _aptdaemon_trans->Connect("PropertyChanged", sigc::mem_fun(this, &SoftwareCenterLauncherIcon::OnPropertyChanged));
+
+    icon_name = icon_path;
+}
+
+SoftwareCenterLauncherIcon::~SoftwareCenterLauncherIcon() {
+
+}
+
+void
+SoftwareCenterLauncherIcon::OnFinished(GVariant* params) {
+
+   tooltip_text = BamfName();
+
+   SetQuirk(LauncherIcon::QUIRK_PROGRESS, FALSE); 
+}
+
+void
+SoftwareCenterLauncherIcon::OnPropertyChanged(GVariant* params) {
+
+    gint32 progress;
+    gchar* property_name;
+    GVariant* property_value;
+
+    g_variant_get_child (params, 0, "s", &property_name);
+    if (g_strcmp0 (property_name, "Progress") == 0) {
+        g_variant_get_child (params,1,"v",&property_value);
+        g_variant_get (property_value, "i", &progress);
+
+        if (progress < 100) {
+            SetQuirk(LauncherIcon::QUIRK_PROGRESS, TRUE);
+            tooltip_text = _("Waiting to install");
+        }
+        SetProgress(((float)progress) / ((float)100));
+    }
+    g_variant_unref(property_value);
+    g_free(property_name);
+
+}
+
+}
+}

=== added file 'plugins/unityshell/src/SoftwareCenterLauncherIcon.h'
--- plugins/unityshell/src/SoftwareCenterLauncherIcon.h	1970-01-01 00:00:00 +0000
+++ plugins/unityshell/src/SoftwareCenterLauncherIcon.h	2012-01-20 00:43:27 +0000
@@ -0,0 +1,60 @@
+// -*- Mode: C++; indent-tabs-mode: nil; tab-width: 2 -*-
+/*
+ * Copyright (C) 2010 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Bilal Akhtar <[e-mail]>
+ */
+
+#ifndef SOFTWARECENTERLAUNCHERICON_H
+#define SOFTWARECENTERLAUNCHERICON_H
+
+#include "BamfLauncherIcon.h"
+#include <Nux/BaseWindow.h>
+#include <NuxCore/Math/MathInc.h>
+#include <core/core.h>
+#include <gio/gio.h>
+#include <glib.h>
+#include <glib/gvariant.h>
+#include <UnityCore/GLibDBusProxy.h>
+
+
+namespace unity
+{
+namespace launcher
+{
+
+
+class SoftwareCenterLauncherIcon : public BamfLauncherIcon
+{
+public:
+
+    SoftwareCenterLauncherIcon(Launcher* IconManager, BamfApplication* app, char* aptdaemon_trans_id, char* icon_path);
+    virtual ~SoftwareCenterLauncherIcon();
+
+    gchar* original_tooltip_text;
+
+private:
+    char* _aptdaemon_trans_id;
+    unity::glib::DBusProxy* _aptdaemon_trans;
+
+    void OnFinished(GVariant* params);
+
+    void OnPropertyChanged(GVariant* params);
+};
+
+}
+}
+
+#endif