Merge lp:~azzar1/unity/fix-875467-2 into lp:unity

[Andrea Azzarone]

Use unity::IconLoader to load device launcher icon that will be used to show an OSD notification. If it's not able to load an icon it shows a OSD notification without an icon.

This merge propose comes from this one (https://code.launchpad.net/~andyrock/unity/fix-875467/+merge/79638). In that merge proposal I've created a mini (very very mini) icon loader implementation. Reading unity code I noticed that we already use two icon loader implementations (the dash icon loader and the launcher ones) so...

Because unity::IconLoader has already been tested and in a unit test we should not test an external module (osd notification library), i don't know if we need a unit test for this change. In the case let me know.

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

I was wondering, shouldn't be the notification, this case, be shown as a priority one?
I mean, making it to be positioned where the volume or brightness bubbles are shown, to exceed the queue of the all other applications that are firing notifications. The hint "x-canonical-private-synchronous" or the urgency should do that.

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

Despite the comment above (about which I'd suggest to contact the design team), the code looks good to me and works as expected, so I approve.

I agree about the fact that testing should not be needed for this one, as we only change how the UI is rendering through an external lib, but the core you're using is already in.

review: Approve

[Tim Penhey (thumper)]

notify_notification_new is called without any corresponding clean up.

If it uses g_object_unref, prehaps you chould use a glib::Object wrapper.

GdkPixbuf* pixbuf = NULL // should be nullptr

line 168: there are multiple calls to icon_name(). This returns a new
temporary each time, we should use a local variable.

I _think_ that I added equality checks for a property (just checked, I did).

so you should be able to go:
  self->empty_ = (self->icon_name == "user-trash");
(just removed the () from icon_name).

review: Needs Fixing

[Andrea Azzarone (azzar1)]

> notify_notification_new is called without any corresponding clean up.
>
> If it uses g_object_unref, prehaps you chould use a glib::Object wrapper.

Done.

> GdkPixbuf* pixbuf = NULL // should be nullptr

Done.

>
> line 168: there are multiple calls to icon_name(). This returns a new
> temporary each time, we should use a local variable.

Done.

>
> I _think_ that I added equality checks for a property (just checked, I did).
>
> so you should be able to go:
> self->empty_ = (self->icon_name == "user-trash");
> (just removed the () from icon_name).

Done.

[Tim Penhey (thumper)]

review: Approve

=== added file 'manual-tests/EjectNotificationIcon.txt'
--- manual-tests/EjectNotificationIcon.txt	1970-01-01 00:00:00 +0000
+++ manual-tests/EjectNotificationIcon.txt	2011-12-16 10:59:25 +0000
@@ -0,0 +1,27 @@
+Test Eject Notification Icon (USB)
+-------------------
+This test shows that the correct icon is shown in the OSD notification when a
+USB pen drive is ejected using drag and drop into the trash.
+
+#. Plug in a USB pen drive
+#. Wait for device icon to appear in the launcher
+#. Drag and drop it into the trash icon
+
+Outcome
+  The usb pen drive will be "ejected" and an OSD-notification will appear. The
+  icon in the notification is the same icon used for the launcher icon.
+  
+  
+  
+Test Eject Notification Icon (CD)
+-------------------
+This test shows that the correct icon is shown in the OSD notification when a
+CD is ejected using drag and drop into the trash.
+
+#. Insert a CD
+#. Wait for device icon to appear in the launcher
+#. Drag and drop it into the trash icon
+
+Outcome
+  The cd will be"ejected and an OSD-notification will appear. The
+  icon in the notification is the same icon used for the launcher icon.

=== modified file 'plugins/unityshell/src/BFBLauncherIcon.cpp'
--- plugins/unityshell/src/BFBLauncherIcon.cpp	2011-12-09 14:40:35 +0000
+++ plugins/unityshell/src/BFBLauncherIcon.cpp	2011-12-16 10:59:25 +0000
@@ -36,7 +36,7 @@
  : SimpleLauncherIcon(IconManager)
 {
   tooltip_text = _("Dash home");
-  SetIconName(PKGDATADIR"/launcher_bfb.png");
+  icon_name = PKGDATADIR"/launcher_bfb.png";
   SetQuirk(QUIRK_VISIBLE, true);
   SetQuirk(QUIRK_RUNNING, false);
   SetIconType(TYPE_HOME);

=== modified file 'plugins/unityshell/src/BamfLauncherIcon.cpp'
--- plugins/unityshell/src/BamfLauncherIcon.cpp	2011-12-13 08:58:59 +0000
+++ plugins/unityshell/src/BamfLauncherIcon.cpp	2011-12-16 10:59:25 +0000
@@ -175,10 +175,10 @@
   _menu_desktop_shortcuts = NULL;
   _on_desktop_file_changed_handler_id = 0;
   _window_moved_id = 0;
-  char* icon_name = bamf_view_get_icon(BAMF_VIEW(m_App));
+  glib::String icon(bamf_view_get_icon(BAMF_VIEW(m_App)));
 
   tooltip_text = BamfName();
-  SetIconName(icon_name);
+  icon_name = icon.Str();
   SetIconType(TYPE_APPLICATION);
 
   if (bamf_view_is_sticky(BAMF_VIEW(m_App)))
@@ -189,8 +189,6 @@
   SetQuirk(QUIRK_ACTIVE, bamf_view_is_active(BAMF_VIEW(m_App)));
   SetQuirk(QUIRK_RUNNING, bamf_view_is_running(BAMF_VIEW(m_App)));
 
-  g_free(icon_name);
-
   g_signal_connect(app, "child-removed", (GCallback) &BamfLauncherIcon::OnChildRemoved, this);
   g_signal_connect(app, "child-added", (GCallback) &BamfLauncherIcon::OnChildAdded, this);
   g_signal_connect(app, "urgent-changed", (GCallback) &BamfLauncherIcon::OnUrgentChanged, this);

=== modified file 'plugins/unityshell/src/DesktopLauncherIcon.cpp'
--- plugins/unityshell/src/DesktopLauncherIcon.cpp	2011-11-30 03:06:44 +0000
+++ plugins/unityshell/src/DesktopLauncherIcon.cpp	2011-12-16 10:59:25 +0000
@@ -32,7 +32,7 @@
   ,   show_in_switcher_(true)
 {
   tooltip_text = _("Show Desktop");
-  SetIconName("desktop");
+  icon_name = "desktop";
   SetQuirk(QUIRK_VISIBLE, true);
   SetQuirk(QUIRK_RUNNING, true);
   SetIconType(TYPE_BEGIN);

=== modified file 'plugins/unityshell/src/DeviceLauncherIcon.cpp'
--- plugins/unityshell/src/DeviceLauncherIcon.cpp	2011-12-13 08:31:21 +0000
+++ plugins/unityshell/src/DeviceLauncherIcon.cpp	2011-12-16 10:59:25 +0000
@@ -27,6 +27,7 @@
 #include <NuxCore/Logger.h>
 
 #include "DevicesSettings.h"
+#include "IconLoader.h"
 #include "ubus-server.h"
 #include "UBusMessages.h"
 
@@ -46,7 +47,7 @@
   : SimpleLauncherIcon(launcher)
   , volume_(volume)
   , device_file_(g_volume_get_identifier(volume_, G_VOLUME_IDENTIFIER_KIND_UNIX_DEVICE))
-  , gdu_device_(get_device_for_device_file(device_file_.Value()))
+  , gdu_device_(get_device_for_device_file(device_file_))
 {  
   DevicesSettings::GetDefault().changed.connect(sigc::mem_fun(this, &DeviceLauncherIcon::OnSettingsChanged));
 
@@ -67,8 +68,8 @@
   glib::Object<GIcon> icon(g_volume_get_icon(volume_));
   glib::String icon_string(g_icon_to_string(icon));
 
-  tooltip_text = name.Value();
-  SetIconName(icon_string.Value());
+  tooltip_text = name.Str();
+  icon_name = icon_string.Str();
   
   SetIconType(TYPE_DEVICE);
   SetQuirk(QUIRK_RUNNING, false);
@@ -291,18 +292,32 @@
                                       DeviceLauncherIcon* self)
 {
   if (g_volume_eject_with_operation_finish(self->volume_, result, NULL))
-  {
-    NotifyNotification* notification;
-    glib::String name(g_volume_get_name(self->volume_));
-
-    notification = notify_notification_new(name.Value(),
-                                           _("The drive has been successfully ejected"),
-                                           "drive-removable-media-usb");
-
-    notify_notification_show(notification, NULL);
+  {    
+    IconLoader::GetDefault().LoadFromGIconString(self->icon_name(), 48,
+                                                 sigc::mem_fun(self, &DeviceLauncherIcon::ShowNotification));
   }
 }
 
+void DeviceLauncherIcon::ShowNotification(std::string const& icon_name, 
+                                          unsigned size,
+                                          GdkPixbuf* pixbuf)
+{
+  
+  glib::String name(g_volume_get_name(volume_));
+  glib::Object<NotifyNotification> notification(notify_notification_new(name,
+                                                                        _("The drive has been successfully ejected"),
+                                                                        NULL));
+                                                                        
+  notify_notification_set_hint(notification,
+                               "x-canonical-private-synchronous",
+                               g_variant_new_boolean(TRUE));
+  
+  if(GDK_IS_PIXBUF(pixbuf))
+    notify_notification_set_image_from_pixbuf(notification, pixbuf);
+    
+  notify_notification_show(notification, NULL);
+}
+
 void DeviceLauncherIcon::Eject()
 {
   glib::Object<GMountOperation> mount_op(gtk_mount_operation_new(NULL));

=== modified file 'plugins/unityshell/src/DeviceLauncherIcon.h'
--- plugins/unityshell/src/DeviceLauncherIcon.h	2011-10-25 17:00:12 +0000
+++ plugins/unityshell/src/DeviceLauncherIcon.h	2011-12-16 10:59:25 +0000
@@ -65,6 +65,7 @@
   static void OnUnmountReady(GObject* object, GAsyncResult* result, DeviceLauncherIcon* self);
   static void OnDriveStop(DbusmenuMenuitem* item, int time, DeviceLauncherIcon* self);
   void OnSettingsChanged();
+  void ShowNotification(std::string const& icon_name, unsigned size, GdkPixbuf* pixbuf);
 
 private:
   GVolume* volume_;

=== modified file 'plugins/unityshell/src/LauncherController.cpp'
--- plugins/unityshell/src/LauncherController.cpp	2011-12-12 00:18:26 +0000
+++ plugins/unityshell/src/LauncherController.cpp	2011-12-16 10:59:25 +0000
@@ -343,7 +343,7 @@
   expo_icon_ = new SimpleLauncherIcon(launcher_.GetPointer());
 
   expo_icon_->tooltip_text = _("Workspace Switcher");
-  expo_icon_->SetIconName("workspace-switcher");
+  expo_icon_->icon_name = "workspace-switcher";
   expo_icon_->SetQuirk(LauncherIcon::QUIRK_VISIBLE, true);
   expo_icon_->SetQuirk(LauncherIcon::QUIRK_RUNNING, false);
   expo_icon_->SetIconType(LauncherIcon::TYPE_EXPO);

=== modified file 'plugins/unityshell/src/SimpleLauncherIcon.cpp'
--- plugins/unityshell/src/SimpleLauncherIcon.cpp	2011-10-06 04:18:36 +0000
+++ plugins/unityshell/src/SimpleLauncherIcon.cpp	2011-12-16 10:59:25 +0000
@@ -40,6 +40,7 @@
 
 SimpleLauncherIcon::SimpleLauncherIcon(Launcher* IconManager)
   : LauncherIcon(IconManager)
+  , icon_name("", sigc::mem_fun(this, &SimpleLauncherIcon::SetIconName))
   , theme_changed_id_(0)
 {
   LauncherIcon::mouse_down.connect(sigc::mem_fun(this, &SimpleLauncherIcon::OnMouseDown));
@@ -97,29 +98,27 @@
   if (texture_map[size] != 0)
     return texture_map[size];
 
-  if (icon_name_.empty())
+  std::string icon_string(icon_name());
+
+  if (icon_string.empty())
     return 0;
 
-  if (icon_name_[0] == '/')
-    texture_map[size] = TextureFromPath(icon_name_.c_str(), size);
+  if (icon_string[0] == '/')
+    texture_map[size] = TextureFromPath(icon_string.c_str(), size);
   else
-    texture_map[size] = TextureFromGtkTheme(icon_name_.c_str(), size);
+    texture_map[size] = TextureFromGtkTheme(icon_string.c_str(), size);
   return texture_map[size];
 }
 
-void SimpleLauncherIcon::SetIconName(const char* name)
+bool SimpleLauncherIcon::SetIconName(std::string& target, std::string const& value)
 {
-  if (name == NULL)
-  {
-    LOG_WARNING(logger) << "attempted to set NULL as IconName";
-    icon_name_.clear();
-  }
-  else
-  {
-    icon_name_ = name;
-  }
+  if (target == value)
+    return false;
 
+  target = value;
   ReloadIcon();
+  
+  return true;
 }
 
 void SimpleLauncherIcon::ReloadIcon()

=== modified file 'plugins/unityshell/src/SimpleLauncherIcon.h'
--- plugins/unityshell/src/SimpleLauncherIcon.h	2011-10-06 04:18:36 +0000
+++ plugins/unityshell/src/SimpleLauncherIcon.h	2011-12-16 10:59:25 +0000
@@ -17,8 +17,8 @@
  * Authored by: Jason Smith <[e-mail]>
  */
 
-#ifndef SIMPLELAUNCHERICON_H
-#define SIMPLELAUNCHERICON_H
+#ifndef UNITYSHELL_SIMPLELAUNCHERICON_H
+#define UNITYSHELL_SIMPLELAUNCHERICON_H
 
 #include "LauncherIcon.h"
 
@@ -35,11 +35,13 @@
   SimpleLauncherIcon(Launcher* IconManager);
   virtual ~SimpleLauncherIcon();
 
-  /* override */
+  // override
   nux::BaseTexture* GetTextureForSize(int size);
 
-  void SetIconName(const char* name);
-
+  // Properties
+  nux::Property<std::string> icon_name;
+  
+  // Signals
   sigc::signal<void> activate;
 
 protected:
@@ -53,9 +55,9 @@
 private:
   void ReloadIcon();
   static void OnIconThemeChanged(GtkIconTheme* icon_theme, gpointer data);
+  bool SetIconName(std::string& target, std::string const& value);
 
 private:
-  std::string icon_name_;
   guint32 theme_changed_id_;
 
   std::map<int, nux::BaseTexture*> texture_map;

=== modified file 'plugins/unityshell/src/TrashLauncherIcon.cpp'
--- plugins/unityshell/src/TrashLauncherIcon.cpp	2011-10-06 04:18:36 +0000
+++ plugins/unityshell/src/TrashLauncherIcon.cpp	2011-12-16 10:59:25 +0000
@@ -37,7 +37,7 @@
   , proxy_("org.gnome.Nautilus", "/org/gnome/Nautilus", "org.gnome.Nautilus.FileOperations")
 {
   tooltip_text = _("Trash");
-  SetIconName("user-trash");
+  icon_name = "user-trash";
   SetQuirk(QUIRK_VISIBLE, true);
   SetQuirk(QUIRK_RUNNING, false);
   SetIconType(TYPE_TRASH);
@@ -135,11 +135,11 @@
   if (info)
   {    
     glib::Object<GIcon> icon(g_file_info_get_icon(info));
-    glib::String icon_name(g_icon_to_string(icon));
+    glib::String icon_string(g_icon_to_string(icon));
     
-    self->SetIconName(icon_name.Value());
+    self->icon_name = icon_string.Str();
 
-    self->empty_ = g_strcmp0(icon_name.Value(), "user-trash") == 0;
+    self->empty_ = (self->icon_name == "user-trash");
   }
 }
 

=== modified file 'standalone-clients/CMakeLists.txt'
--- standalone-clients/CMakeLists.txt	2011-12-02 14:40:16 +0000
+++ standalone-clients/CMakeLists.txt	2011-12-16 10:59:25 +0000
@@ -217,6 +217,8 @@
                 ${UNITY_SRC}/FavoriteStore.h
                 ${UNITY_SRC}/FavoriteStoreGSettings.cpp
                 ${UNITY_SRC}/FavoriteStoreGSettings.h
+                ${UNITY_SRC}/IconLoader.cpp
+                ${UNITY_SRC}/IconLoader.h
                 ${UNITY_SRC}/LauncherEntryRemoteModel.cpp
                 ${UNITY_SRC}/LauncherEntryRemoteModel.h
                 ${UNITY_SRC}/LauncherEntryRemote.cpp
@@ -261,6 +263,8 @@
                 ${UNITY_SRC}/QuicklistView.h
                 ${UNITY_SRC}/QuicklistManager.cpp
                 ${UNITY_SRC}/QuicklistManager.h
+                ${UNITY_SRC}/Timer.cpp
+                ${UNITY_SRC}/Timer.h
                 ${UNITY_SRC}/UBusMessages.h
                 ${UNITY_SRC}/UBusWrapper.cpp
                 ${UNITY_SRC}/UBusWrapper.h