Created attachment 150947 [details] [review]
impose a hard limit on the number of entries read from ~/.gtk-bookmarks

Review of attachment 150947 [details] [review]:

::: gnome-panel/panel-menu-items.c
@@ +375,3 @@
+
+	if (error) {
+		g_error_free (error);

Since you don't use the content of the error there, just pass NULL to g_io_channel_new, and check if io_channel is NULL or not.

@@ +387,3 @@
+		gsize     length;
+		gsize     terminator_pos;
+		GIOStatus status;

Small nitpick: length/terminator_pos/status should be aligned with contents (not *contents), so add a space ;-)

@@ +396,3 @@
+			break;
+		} else if (status != G_IO_STATUS_NORMAL)
+			break;

Just do:

if (status != G_IO_STATUS_NORMAL)
    break;

(which means you can pass NULL as error to g_io_channel_read_line)

@@ +403,3 @@
+		if (terminator_pos > 0)
+			contents[terminator_pos] = '\0';
+

Hrm, do we really need to do this?

@@ +404,3 @@
+			contents[terminator_pos] = '\0';
+
+		g_debug("contents: %s", contents);

Please remove the g_debug()

@@ +420,3 @@
 
+	for (l = lines; l; l = l->next) {
+		char *line = (char*)l->data;

Nitpick: "(char *) l->data;" (add a space after the cast)

@@ +462,3 @@
 		}
+
+		g_free (line);

Hrm, doesn't this break the g_hash_table_lookup() call in case you have the same string again? You shouldn't free the memory now, but after the loop (with a g_slist_foreach() call).

Created attachment 151089 [details] [review]
second version

Thanks for the review! Here's another version.

(In reply to comment #2)
> @@ +403,3 @@
> +        if (terminator_pos > 0)
> +            contents[terminator_pos] = '\0';
> +
> 
> Hrm, do we really need to do this?

The string read with g_io_channel_read_line contains the \n, which we do not want, so yeah.

Review of attachment 151089 [details] [review]:

Please commit with the three changes below, thanks!

::: gnome-panel/panel-menu-items.c
@@ +371,3 @@
 				     BOOKMARKS_FILENAME, NULL);
 
+	io_channel = g_io_channel_new_file (filename, "r", &error);

s/&error/NULL/ else you're leaking the error in case of error ;-)

@@ +391,3 @@
+		if (status != G_IO_STATUS_NORMAL)
+			break;
+		else if (length == 0)

Small nitpick: can you just remove the else here? It doesn't matter, but I find it a tiny bit more readable without it.

@@ +396,3 @@
+		/* Clear the line terminator, if any */
+		if (terminator_pos > 0)
+			contents[terminator_pos] = '\0';

Thanks for the explanation for this! Maybe clarify with the comment:
/* Clear the \n line terminator */

Comment on attachment 151089 [details] [review]
second version

OK! Pushed 5ff272ab5dbdfd3a0c259e333a9dfe006ed0e431 to master with the fixes. Thanks for the reviews!