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

Proposed by Lars Karlitski on 2012-02-23
Status: Merged
Approved by: Ted Gould on 2012-03-21
Approved revision: 253
Merged at revision: 259
Proposed branch: lp:~larsu/indicator-messages/fix-blacklist
Merge into: lp:indicator-messages/0.3
Diff against target: 99 lines (+22/-23)
1 file modified
src/messages-service.c (+22/-23)
To merge this branch: bzr merge lp:~larsu/indicator-messages/fix-blacklist
Reviewer Review Type Date Requested Status
Ted Gould (community) 2012-02-23 Approve on 2012-03-21
Review via email: mp+94424@code.launchpad.net

Description of the change

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?

To post a comment you must log in.
Ted Gould (ted) wrote :

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

Ted Gould (ted) wrote :

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.

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

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

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/messages-service.c'
2--- src/messages-service.c 2012-03-08 16:05:30 +0000
3+++ src/messages-service.c 2012-03-14 18:01:19 +0000
4@@ -327,20 +327,8 @@
5 blacklist_add (gpointer udata)
6 {
7 gchar * definition_file = (gchar *)udata;
8- /* Dump the file */
9- gchar * desktop;
10- g_file_get_contents(definition_file, &desktop, NULL, NULL);
11- if (desktop == NULL) {
12- g_warning("Couldn't get data out of: %s", definition_file);
13- return FALSE;
14- }
15-
16- /* Clean up the data */
17- gchar * trimdesktop = pango_trim_string(desktop);
18- g_free(desktop);
19-
20- blacklist_add_core(trimdesktop, definition_file);
21- g_free(trimdesktop);
22+
23+ blacklist_add_core(definition_file, definition_file);
24
25 return FALSE;
26 }
27@@ -352,8 +340,10 @@
28 static void
29 blacklist_add_core (gchar * desktop, gchar * definition)
30 {
31+ gchar *basename = g_path_get_basename(desktop);
32+
33 /* Check for conflicts */
34- gpointer data = g_hash_table_lookup(blacklist, desktop);
35+ gpointer data = g_hash_table_lookup(blacklist, basename);
36 if (data != NULL) {
37 gchar * oldfile = (gchar *)data;
38 if (!g_strcmp0(oldfile, definition)) {
39@@ -362,27 +352,31 @@
40 g_warning("Already have desktop file '%s' in blacklist file '%s' not adding from '%s'", desktop, oldfile, definition);
41 }
42
43+ g_free(basename);
44 return;
45 }
46
47 /* Actually blacklist this thing */
48- g_hash_table_insert(blacklist, g_strdup(desktop), g_strdup(definition));
49+ g_hash_table_insert(blacklist, g_strdup(basename), g_strdup(definition));
50 g_debug("Adding Blacklist item '%s' for desktop '%s'", definition, desktop);
51
52 /* Go through and eclipse folks */
53 GList * launcher;
54 for (launcher = launcherList; launcher != NULL; launcher = launcher->next) {
55 launcherList_t * item = (launcherList_t *)launcher->data;
56- if (!g_strcmp0(desktop, launcher_menu_item_get_desktop(item->menuitem))) {
57+ gchar * item_basename = g_path_get_basename(launcher_menu_item_get_desktop(item->menuitem));
58+ if (!g_strcmp0(basename, item_basename)) {
59 launcher_menu_item_set_eclipsed(item->menuitem, TRUE);
60 dbusmenu_menuitem_property_set_bool(item->separator, DBUSMENU_MENUITEM_PROP_VISIBLE, FALSE);
61 }
62+ g_free(item_basename);
63 }
64
65 check_hidden();
66 /* Shouldn't need a resort here as hiding shouldn't cause things to
67 move other than this item disappearing. */
68
69+ g_free(basename);
70 return;
71 }
72
73@@ -444,15 +438,20 @@
74 static gboolean
75 blacklist_check (const gchar * desktop_file)
76 {
77- g_debug("Checking blacklist for: %s", desktop_file);
78- if (blacklist == NULL) return FALSE;
79-
80- if (g_hash_table_lookup(blacklist, desktop_file)) {
81+ gchar *basename = g_path_get_basename(desktop_file);
82+ gboolean found;
83+
84+ g_debug("Checking blacklist for: %s", basename);
85+
86+ if (blacklist && g_hash_table_lookup(blacklist, basename)) {
87 g_debug("\tFound!");
88- return TRUE;
89+ found = TRUE;
90 }
91+ else
92+ found = FALSE;
93
94- return FALSE;
95+ g_free(basename);
96+ return found;
97 }
98
99 /* A callback everytime the blacklist directory changes

Subscribers

People subscribed via source and target branches