Merge lp:~larsu/indicator-messages/fix-blacklist into lp:indicator-messages/0.3

[Lars Karlitski]

The blacklist hash map used desktop file contents as keys, but checked for file paths when checking whether an etnry should be hidden. This patch makes it use the link target, which should be something in /usr/share/applications in the majority of cases.

I'm not sure about this approach though. Maybe someone with more insight into i-messages can tell me whether this makes sense at all.

Also, hidden menuitems are still visible for me (only the label is gone). A bug in libindicator?

[Ted Gould (ted)]

I think that instead of using the symlink targets we should probably use basename's to match the desktop file names. I think that some people might have done a copy of the desktop files instead of linking them, while I think linking would be better, I don't think we can use that alone.

review: Needs Fixing

[Lars Karlitski (larsu)]

To be honest, I think this whole approach is kind of clunky and error-prone. Can't we just use a gsetting for that (like unity-panel-service does for the system tray whitelist)? This would make the i-messages code a lot cleaner and also make it easier for users to edit the blacklist.

[Ted Gould (ted)]

On Fri, 2012-03-09 at 10:22 +0000, Lars Uebernickel wrote:
> To be honest, I think this whole approach is kind of clunky and
> error-prone.. Can't we just use a gsetting for that (like
> unity-panel-service does for the system tray whitelist)? This would
> make the i-messages code a lot cleaner and also make it easier for
> users to edit the blacklist.

The problem with using lists in GSettings is that it's impossible to
upgrade them. If a user ever sets that list, the default is
unreachable. By using directories we get this "merging" feature for
free. It's on the slate to make it into DConf at some point, but we
haven't budgeted the time for that yet.

Also, when this code was written GSettings didn't exist :-)

I think if we use the directory listing like this "list of keys" we're
almost doing the same thing, although not as cleanly for sure.

[lp:~larsu/indicator-messages/fix-blacklist]

253. By Lars Karlitski on 2012-03-14

Use basename of the desktop file as key in the blacklist hash table

This is superior to using symlinks, as this also allows copying the desktop
files into the blacklist directory. Copying is the default when dragging and
dropping an application into that folder.

[Lars Karlitski (larsu)]

Pushed a fix that uses basenames instead of symlink targets.

By the way, applications that are running are always displayed, even if they are in the blacklist. Is that correct or also a bug? (Or did I introduce this with my patch?)

[Ted Gould (ted)]

Nope, that's been there. I kinda feel if the application is putting itself there, you either need to disable that in the app, but it's not our responsibility to not show it. Looks good!

review: Approve

=== modified file 'src/messages-service.c'
--- src/messages-service.c	2012-03-08 16:05:30 +0000
+++ src/messages-service.c	2012-03-14 18:01:19 +0000
@@ -327,20 +327,8 @@
 blacklist_add (gpointer udata)
 {
 	gchar * definition_file = (gchar *)udata;
-	/* Dump the file */
-	gchar * desktop;
-	g_file_get_contents(definition_file, &desktop, NULL, NULL);
-	if (desktop == NULL) {
-		g_warning("Couldn't get data out of: %s", definition_file);
-		return FALSE;
-	}
-
-	/* Clean up the data */
-	gchar * trimdesktop = pango_trim_string(desktop);
-	g_free(desktop);
-
-	blacklist_add_core(trimdesktop, definition_file);
-	g_free(trimdesktop);
+
+	blacklist_add_core(definition_file, definition_file);
 
 	return FALSE;
 }
@@ -352,8 +340,10 @@
 static void
 blacklist_add_core (gchar * desktop, gchar * definition)
 {
+	gchar *basename = g_path_get_basename(desktop);
+
 	/* Check for conflicts */
-	gpointer data = g_hash_table_lookup(blacklist, desktop);
+	gpointer data = g_hash_table_lookup(blacklist, basename);
 	if (data != NULL) {
 		gchar * oldfile = (gchar *)data;
 		if (!g_strcmp0(oldfile, definition)) {
@@ -362,27 +352,31 @@
 			g_warning("Already have desktop file '%s' in blacklist file '%s' not adding from '%s'", desktop, oldfile, definition);
 		}
 
+		g_free(basename);
 		return;
 	}
 
 	/* Actually blacklist this thing */
-	g_hash_table_insert(blacklist, g_strdup(desktop), g_strdup(definition));
+	g_hash_table_insert(blacklist, g_strdup(basename), g_strdup(definition));
 	g_debug("Adding Blacklist item '%s' for desktop '%s'", definition, desktop);
 
 	/* Go through and eclipse folks */
 	GList * launcher;
 	for (launcher = launcherList; launcher != NULL; launcher = launcher->next) {
 		launcherList_t * item = (launcherList_t *)launcher->data;
-		if (!g_strcmp0(desktop, launcher_menu_item_get_desktop(item->menuitem))) {
+		gchar * item_basename = g_path_get_basename(launcher_menu_item_get_desktop(item->menuitem));
+		if (!g_strcmp0(basename, item_basename)) {
 			launcher_menu_item_set_eclipsed(item->menuitem, TRUE);
 			dbusmenu_menuitem_property_set_bool(item->separator, DBUSMENU_MENUITEM_PROP_VISIBLE, FALSE);
 		}
+		g_free(item_basename);
 	}
 
 	check_hidden();
 	/* Shouldn't need a resort here as hiding shouldn't cause things to
 	   move other than this item disappearing. */
 
+	g_free(basename);
 	return;
 }
 
@@ -444,15 +438,20 @@
 static gboolean
 blacklist_check (const gchar * desktop_file)
 {
-	g_debug("Checking blacklist for: %s", desktop_file);
-	if (blacklist == NULL) return FALSE;
-
-	if (g_hash_table_lookup(blacklist, desktop_file)) {
+	gchar *basename = g_path_get_basename(desktop_file);
+	gboolean found;
+
+	g_debug("Checking blacklist for: %s", basename);
+
+	if (blacklist && g_hash_table_lookup(blacklist, basename)) {
 		g_debug("\tFound!");
-		return TRUE;
+		found = TRUE;
 	}
+	else
+		found = FALSE;
 
-	return FALSE;
+	g_free(basename);
+	return found;
 }
 
 /* A callback everytime the blacklist directory changes