Merge lp:~3v1n0/unity/quicklist-hide-fix into lp:unity

[Marco Trevisan (Treviño)]

This branch includes various fixes for the QuickList handling such as:
 - Correct hiding of quicklist items and quicklist roots (including not showing unneeded separators, see more at bug #843425).
 - Support for monitoring .desktop file changes to dynamically update the "static" quicklist generated using the launcher .desktop file
 - Some memory optimizations and fix of memory leaks
 - Fixed a bug causing unity to crash if removing launcher with an open quicklist (bug #801413)

[Gord Allott (gordallott)]

+1 thanks!

[Gord Allott (gordallott)]

review: Approve

=== modified file 'plugins/unityshell/src/BamfLauncherIcon.cpp'
--- plugins/unityshell/src/BamfLauncherIcon.cpp	2011-08-30 16:48:12 +0000
+++ plugins/unityshell/src/BamfLauncherIcon.cpp	2011-09-07 09:42:19 +0000
@@ -144,6 +144,7 @@
   _dnd_hovered = false;
   _launcher = IconManager;
   _menu_desktop_shortcuts = NULL;
+  _on_desktop_file_changed_handler_id = 0;
   char* icon_name = bamf_view_get_icon(BAMF_VIEW(m_App));
 
   tooltip_text = BamfName();
@@ -173,17 +174,7 @@
   EnsureWindowState();
   UpdateMenus();
 
-  // add a file watch to the desktop file so that if/when the app is removed we can remove ourself from the launcher.
-  GFile* _desktop_file = g_file_new_for_path(DesktopFile());
-  _desktop_file_monitor = g_file_monitor_file(_desktop_file,
-                                              G_FILE_MONITOR_NONE,
-                                              NULL,
-                                              NULL);
-
-  _on_desktop_file_changed_handler_id = g_signal_connect(_desktop_file_monitor,
-                                                         "changed",
-                                                         G_CALLBACK(&BamfLauncherIcon::OnDesktopFileChanged),
-                                                         this);
+  UpdateDesktopFile();
 
   WindowManager::Default()->window_minimized.connect(sigc::mem_fun(this, &BamfLauncherIcon::OnWindowMinimized));
   WindowManager::Default()->compiz_screen_viewport_switch_ended.connect(sigc::mem_fun(this, &BamfLauncherIcon::OnViewPortSwitchEnded));
@@ -192,7 +183,7 @@
 
   // hack
   SetProgress(0.0f);
-  
+
   // Calls when there are no higher priority events pending to the default main loop.
   _fill_supported_types_id = g_idle_add((GSourceFunc)FillSupportedTypes, this);
 
@@ -203,22 +194,40 @@
   g_object_set_qdata(G_OBJECT(m_App), g_quark_from_static_string("unity-seen"), GINT_TO_POINTER(0));
 
   // We might not have created the menu items yet
+  for (auto it = _menu_clients.begin(); it != _menu_clients.end(); it++)
+  {
+    g_object_unref(G_OBJECT(it->second));
+  }
+
   if (_menu_items.find("Pin") != _menu_items.end())
   {
-    g_signal_handler_disconnect((gpointer) _menu_items["Pin"],
+    g_signal_handler_disconnect(G_OBJECT(_menu_items["Pin"]),
                                 _menu_callbacks["Pin"]);
   }
 
   if (_menu_items.find("Quit") != _menu_items.end())
   {
-    g_signal_handler_disconnect((gpointer) _menu_items["Quit"],
+    g_signal_handler_disconnect(G_OBJECT(_menu_items["Quit"]),
                                 _menu_callbacks["Quit"]);
   }
 
+  for (auto it = _menu_items.begin(); it != _menu_items.end(); it++)
+  {
+    g_object_unref(G_OBJECT(it->second));
+  }
+
+  for (auto it = _menu_items_extra.begin(); it != _menu_items_extra.end(); it++)
+  {
+    g_object_unref(G_OBJECT(it->second));
+  }
+
+  if (G_IS_OBJECT(_menu_desktop_shortcuts))
+    g_object_unref(G_OBJECT(_menu_desktop_shortcuts));
+
   if (_on_desktop_file_changed_handler_id != 0)
-    g_signal_handler_disconnect((gpointer) _desktop_file_monitor,
+    g_signal_handler_disconnect(G_OBJECT(_desktop_file_monitor),
                                 _on_desktop_file_changed_handler_id);
-                                
+
   if (_fill_supported_types_id != 0)
     g_source_remove(_fill_supported_types_id);
 
@@ -322,19 +331,40 @@
   return bamf_view_is_sticky(BAMF_VIEW(m_App));
 }
 
-const char* BamfLauncherIcon::DesktopFile()
+void BamfLauncherIcon::UpdateDesktopFile()
 {
   char* filename = NULL;
   filename = (char*) bamf_application_get_desktop_file(m_App);
 
-  if (filename != NULL)
+  if (filename != NULL && g_strcmp0(_cached_desktop_file, filename) != 0)
   {
     if (_cached_desktop_file != NULL)
       g_free(_cached_desktop_file);
 
     _cached_desktop_file = g_strdup(filename);
+
+    // add a file watch to the desktop file so that if/when the app is removed
+    // we can remove ourself from the launcher and when it's changed
+    // we can update the quicklist.
+
+    if (_on_desktop_file_changed_handler_id != 0)
+      g_signal_handler_disconnect(G_OBJECT(_desktop_file_monitor),
+                                  _on_desktop_file_changed_handler_id);
+
+    GFile* desktop_file = g_file_new_for_path(DesktopFile());
+    _desktop_file_monitor = g_file_monitor_file(desktop_file, G_FILE_MONITOR_NONE,
+                                                NULL, NULL);
+    g_file_monitor_set_rate_limit (_desktop_file_monitor, 1000);
+    _on_desktop_file_changed_handler_id = g_signal_connect(_desktop_file_monitor,
+                                                           "changed",
+                                                           G_CALLBACK(&BamfLauncherIcon::OnDesktopFileChanged),
+                                                           this);
   }
+}
 
+const char* BamfLauncherIcon::DesktopFile()
+{
+  UpdateDesktopFile();
   return _cached_desktop_file;
 }
 
@@ -357,7 +387,7 @@
 {
   LauncherIcon::AddProperties(builder);
 
-  g_variant_builder_add(builder, "{sv}", "desktop-file", g_variant_new_string(bamf_application_get_desktop_file(m_App)));
+  g_variant_builder_add(builder, "{sv}", "desktop-file", g_variant_new_string(DesktopFile()));
 
   GList* children, *l;
   BamfView* view;
@@ -414,7 +444,7 @@
   GDesktopAppInfo* appInfo;
   GError* error = NULL;
 
-  appInfo = g_desktop_app_info_new_from_filename(bamf_application_get_desktop_file(BAMF_APPLICATION(m_App)));
+  appInfo = g_desktop_app_info_new_from_filename(DesktopFile());
 
   if (g_app_info_supports_uris(G_APP_INFO(appInfo)))
   {
@@ -613,10 +643,83 @@
   self->EnsureWindowState();
 }
 
+void BamfLauncherIcon::UpdateDesktopQuickList()
+{
+  IndicatorDesktopShortcuts* desktop_shortcuts;
+  GKeyFile* keyfile;
+  GError* error = NULL;
+  const char *desktop_file;
+
+  desktop_file = DesktopFile();
+
+  if (!desktop_file || g_strcmp0(desktop_file, "") == 0)
+    return;
+
+  // check that we have the X-Ayatana-Desktop-Shortcuts flag
+  // not sure if we should do this or if libindicator should shut up
+  // and not report errors when it can't find the key.
+  // so FIXME when ted is around
+  keyfile = g_key_file_new();
+  g_key_file_load_from_file(keyfile, desktop_file, G_KEY_FILE_NONE, &error);
+
+  if (error != NULL)
+  {
+    g_warning("Could not load desktop file for: %s", desktop_file);
+    g_key_file_free(keyfile);
+    g_error_free(error);
+    return;
+  }
+
+  if (g_key_file_has_key(keyfile, G_KEY_FILE_DESKTOP_GROUP,
+                         "X-Ayatana-Desktop-Shortcuts", NULL))
+  {
+    DbusmenuMenuitem* root = dbusmenu_menuitem_new();
+    dbusmenu_menuitem_set_root(root, TRUE);
+    desktop_shortcuts = indicator_desktop_shortcuts_new(desktop_file, "Unity");
+    const gchar** nicks = indicator_desktop_shortcuts_get_nicks(desktop_shortcuts);
+
+    int index = 0;
+    if (nicks)
+    {
+      while (((gpointer*) nicks)[index])
+      {
+        gchar* name;
+        DbusmenuMenuitem* item;
+        name = indicator_desktop_shortcuts_nick_get_name(desktop_shortcuts,
+                                                         nicks[index]);
+        ShortcutData* data = g_slice_new0(ShortcutData);
+        data->self = this;
+        data->shortcuts = INDICATOR_DESKTOP_SHORTCUTS(g_object_ref(desktop_shortcuts));
+        data->nick = g_strdup(nicks[index]);
+
+        item = dbusmenu_menuitem_new();
+        dbusmenu_menuitem_property_set(item, DBUSMENU_MENUITEM_PROP_LABEL, name);
+        dbusmenu_menuitem_property_set_bool(item, DBUSMENU_MENUITEM_PROP_ENABLED, TRUE);
+        dbusmenu_menuitem_property_set_bool(item, DBUSMENU_MENUITEM_PROP_VISIBLE, TRUE);
+        g_signal_connect_data(item, "item-activated",
+                              (GCallback) shortcut_activated, (gpointer) data,
+                              (GClosureNotify) shortcut_data_destroy, (GConnectFlags)0);
+
+        dbusmenu_menuitem_child_append(root, item);
+
+        index++;
+
+        g_free(name);
+      }
+    }
+
+    if (G_IS_OBJECT(_menu_desktop_shortcuts))
+      g_object_unref(G_OBJECT(_menu_desktop_shortcuts));
+
+    _menu_desktop_shortcuts = root;
+  }
+
+  g_key_file_free(keyfile);
+}
+
 void BamfLauncherIcon::UpdateMenus()
 {
   GList* children, *l;
-  IndicatorDesktopShortcuts* desktop_shortcuts;
 
   children = bamf_view_get_children(BAMF_VIEW(m_App));
   for (l = children; l; l = l->next)
@@ -639,73 +742,15 @@
 
   // add dynamic quicklist
   if (_menuclient_dynamic_quicklist != NULL)
-    _menu_clients["dynamicquicklist"] = _menuclient_dynamic_quicklist;
+  {
+    auto menu_client = DBUSMENU_CLIENT(g_object_ref(_menuclient_dynamic_quicklist));
+    _menu_clients["dynamicquicklist"] = menu_client;
+  }
 
   // make a client for desktop file actions
-  if (!DBUSMENU_IS_MENUITEM(_menu_desktop_shortcuts) &&
-      g_strcmp0(DesktopFile(), """"))
+  if (!DBUSMENU_IS_MENUITEM(_menu_desktop_shortcuts))
   {
-    GKeyFile* keyfile;
-    GError* error = NULL;
-
-    // check that we have the X-Ayatana-Desktop-Shortcuts flag
-    // not sure if we should do this or if libindicator should shut up
-    // and not report errors when it can't find the key.
-    // so FIXME when ted is around
-    keyfile = g_key_file_new();
-    g_key_file_load_from_file(keyfile, DesktopFile(), G_KEY_FILE_NONE, &error);
-
-    if (error != NULL)
-    {
-      g_warning("Could not load desktop file for: %s" , DesktopFile());
-      g_error_free(error);
-      return;
-    }
-
-    if (g_key_file_has_key(keyfile, G_KEY_FILE_DESKTOP_GROUP,
-                           "X-Ayatana-Desktop-Shortcuts", NULL))
-    {
-      DbusmenuMenuitem* root = dbusmenu_menuitem_new();
-      dbusmenu_menuitem_set_root(root, TRUE);
-      desktop_shortcuts = indicator_desktop_shortcuts_new(bamf_application_get_desktop_file(m_App),
-                                                          "Unity");
-      const gchar** nicks = indicator_desktop_shortcuts_get_nicks(desktop_shortcuts);
-
-      int index = 0;
-      if (nicks)
-      {
-        while (((gpointer*) nicks)[index])
-        {
-          gchar* name;
-          DbusmenuMenuitem* item;
-          name = indicator_desktop_shortcuts_nick_get_name(desktop_shortcuts,
-                                                           nicks[index]);
-          ShortcutData* data = g_slice_new0(ShortcutData);
-          data->self = this;
-          data->shortcuts = INDICATOR_DESKTOP_SHORTCUTS(g_object_ref(desktop_shortcuts));
-          data->nick = g_strdup(nicks[index]);
-
-          item = dbusmenu_menuitem_new();
-          dbusmenu_menuitem_property_set(item, DBUSMENU_MENUITEM_PROP_LABEL, name);
-          dbusmenu_menuitem_property_set_bool(item, DBUSMENU_MENUITEM_PROP_ENABLED, TRUE);
-          dbusmenu_menuitem_property_set_bool(item, DBUSMENU_MENUITEM_PROP_VISIBLE, TRUE);
-          g_signal_connect_data(item, "item-activated",
-                                (GCallback) shortcut_activated, (gpointer) data,
-                                (GClosureNotify) shortcut_data_destroy, (GConnectFlags)0);
-
-          dbusmenu_menuitem_child_append(root, item);
-
-          index++;
-
-          g_free(name);
-        }
-      }
-
-      // TODO: check unref prior to assign, and add unref to destructor.
-      _menu_desktop_shortcuts = root;
-      g_key_file_free(keyfile);
-
-    }
+    UpdateDesktopQuickList();
   }
 
 }
@@ -738,7 +783,7 @@
   if (!bamf_view_is_sticky(view))
     return;
 
-  const gchar* desktop_file = bamf_application_get_desktop_file(this->m_App);
+  const gchar* desktop_file = DesktopFile();
   bamf_view_set_sticky(view, false);
   if (bamf_view_is_closed(view))
     this->Remove();
@@ -751,7 +796,7 @@
 {
   BamfView* view = BAMF_VIEW(self->m_App);
   bool sticky = bamf_view_is_sticky(view);
-  const gchar* desktop_file = bamf_application_get_desktop_file(self->m_App);
+  const gchar* desktop_file = self->DesktopFile();
 
   if (sticky)
   {
@@ -774,7 +819,6 @@
   if (_menu_items.find("Pin") == _menu_items.end())
   {
     menu_item = dbusmenu_menuitem_new();
-    g_object_ref(menu_item);
 
     dbusmenu_menuitem_property_set(menu_item, DBUSMENU_MENUITEM_PROP_TOGGLE_TYPE, DBUSMENU_MENUITEM_TOGGLE_CHECK);
     dbusmenu_menuitem_property_set(menu_item, DBUSMENU_MENUITEM_PROP_LABEL, _("Keep in launcher"));
@@ -797,7 +841,6 @@
   if (_menu_items.find("Quit") == _menu_items.end())
   {
     menu_item = dbusmenu_menuitem_new();
-    g_object_ref(menu_item);
 
     dbusmenu_menuitem_property_set(menu_item, DBUSMENU_MENUITEM_PROP_LABEL, _("Quit"));
     dbusmenu_menuitem_property_set_bool(menu_item, DBUSMENU_MENUITEM_PROP_ENABLED, true);
@@ -838,16 +881,22 @@
     DbusmenuClient* client = (*it).second;
     DbusmenuMenuitem* root = dbusmenu_client_get_root(client);
 
+    if (!root || !dbusmenu_menuitem_property_get_bool(root, DBUSMENU_MENUITEM_PROP_VISIBLE))
+      continue;
+
     for (child = dbusmenu_menuitem_get_children(root); child != NULL; child = g_list_next(child))
     {
       DbusmenuMenuitem* item = (DbusmenuMenuitem*) child->data;
 
-      if (!item)
+      if (!item || !DBUSMENU_IS_MENUITEM(item))
         continue;
 
-      first_separator_needed = true;
+      if (dbusmenu_menuitem_property_get_bool(item, DBUSMENU_MENUITEM_PROP_VISIBLE))
+      {
+        first_separator_needed = true;
 
-      result.push_back(item);
+        result.push_back(item);
+      }
     }
   }
 
@@ -872,31 +921,59 @@
 
   if (first_separator_needed)
   {
+    auto first_sep = _menu_items_extra.find("FirstSeparator");
+    if (first_sep != _menu_items_extra.end())
+    {
+      item = first_sep->second;
+    }
+    else
+    {
+      item = dbusmenu_menuitem_new();
+      dbusmenu_menuitem_property_set(item,
+                                     DBUSMENU_MENUITEM_PROP_TYPE,
+                                     DBUSMENU_CLIENT_TYPES_SEPARATOR);
+      _menu_items_extra["FirstSeparator"] = item;
+    }
+    result.push_back(item);
+  }
+
+  auto app_name_item = _menu_items_extra.find("AppName");
+  if (app_name_item != _menu_items_extra.end())
+  {
+    item = app_name_item->second;
+  }
+  else
+  {
+    gchar* app_name;
+    app_name = g_markup_escape_text(BamfName(), -1);
+
+    item = dbusmenu_menuitem_new();
+    dbusmenu_menuitem_property_set(item,
+                                   DBUSMENU_MENUITEM_PROP_LABEL,
+                                   app_name);
+    dbusmenu_menuitem_property_set_bool(item,
+                                        DBUSMENU_MENUITEM_PROP_ENABLED,
+                                        true);
+    g_signal_connect(item, "item-activated", (GCallback) OnAppLabelActivated, this);
+    g_free(app_name);
+
+    _menu_items_extra["AppName"] = item;
+  }
+  result.push_back(item);
+
+  auto second_sep = _menu_items_extra.find("SecondSeparator");
+  if (second_sep != _menu_items_extra.end())
+  {
+    item = second_sep->second;
+  }
+  else
+  {
     item = dbusmenu_menuitem_new();
     dbusmenu_menuitem_property_set(item,
                                    DBUSMENU_MENUITEM_PROP_TYPE,
                                    DBUSMENU_CLIENT_TYPES_SEPARATOR);
-    result.push_back(item);
+    _menu_items_extra["SecondSeparator"] = item;
   }
-
-  gchar* app_name;
-  app_name = g_markup_escape_text(BamfName(), -1);
-
-  item = dbusmenu_menuitem_new();
-  dbusmenu_menuitem_property_set(item,
-                                 DBUSMENU_MENUITEM_PROP_LABEL,
-                                 app_name);
-  dbusmenu_menuitem_property_set_bool(item,
-                                      DBUSMENU_MENUITEM_PROP_ENABLED,
-                                      true);
-  g_signal_connect(item, "item-activated", (GCallback) OnAppLabelActivated, this);
-  result.push_back(item);
-  g_free(app_name);
-
-  item = dbusmenu_menuitem_new();
-  dbusmenu_menuitem_property_set(item,
-                                 DBUSMENU_MENUITEM_PROP_TYPE,
-                                 DBUSMENU_CLIENT_TYPES_SEPARATOR);
   result.push_back(item);
 
   EnsureMenuItemsReady();
@@ -978,7 +1055,7 @@
 {
   if (!_remote_uri)
   {
-    const gchar* desktop_file = bamf_application_get_desktop_file(BAMF_APPLICATION(m_App));
+    const gchar* desktop_file = DesktopFile();
     gchar* basename =  g_path_get_basename(desktop_file);
 
     _remote_uri = g_strdup_printf("application://%s", basename);
@@ -1053,6 +1130,9 @@
     case G_FILE_MONITOR_EVENT_DELETED:
       self->UnStick();
       break;
+    case G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT:
+      self->UpdateDesktopQuickList();
+      break;
     default:
       break;
   }

=== modified file 'plugins/unityshell/src/BamfLauncherIcon.h'
--- plugins/unityshell/src/BamfLauncherIcon.h	2011-08-30 16:48:12 +0000
+++ plugins/unityshell/src/BamfLauncherIcon.h	2011-09-07 09:42:19 +0000
@@ -81,6 +81,7 @@
   Launcher* _launcher;
   std::map<std::string, DbusmenuClient*> _menu_clients;
   std::map<std::string, DbusmenuMenuitem*> _menu_items;
+  std::map<std::string, DbusmenuMenuitem*> _menu_items_extra;
   std::map<std::string, gulong> _menu_callbacks;
   DbusmenuMenuitem* _menu_desktop_shortcuts;
   gchar* _remote_uri;
@@ -92,14 +93,16 @@
 
   GFileMonitor* _desktop_file_monitor;
   gulong _on_desktop_file_changed_handler_id;
-  
+
   std::set<std::string> _supported_types;
   bool _supported_types_filled;
   guint _fill_supported_types_id;
 
   void EnsureWindowState();
 
+  void UpdateDesktopFile();
   void UpdateMenus();
+  void UpdateDesktopQuickList();
 
   void OpenInstanceWithUris(std::set<std::string> uris);
   void Focus();

=== modified file 'plugins/unityshell/src/LauncherIcon.cpp'
--- plugins/unityshell/src/LauncherIcon.cpp	2011-09-01 01:55:36 +0000
+++ plugins/unityshell/src/LauncherIcon.cpp	2011-09-07 09:42:19 +0000
@@ -763,6 +763,9 @@
 void
 LauncherIcon::Remove()
 {
+  if (_quicklist->IsVisible())
+      _quicklist->Hide();
+
   SetQuirk(QUIRK_VISIBLE, false);
   remove.emit(this);
 }