Re: [PATCH]: Application chooser for the Open With dialog

[Alexander Larsson <alexl redhat com>]

On Tue, 2005-01-18 at 18:04 +0100, Fernando Herrera wrote:
> 
> 		Hello
> 
> 	I wrote the attached patch as a live example during a mini gnome-love
> session.
> 
> 	It adds a chooser of known applications (got from libmenu) to the open
> with dialog for the nautilus file properties page. Most of the code is
> ripped from the Exec dialog from gnome-panel.
> 
> 	I didn't add the expander because I think most users would select it
> from there, and if the desired application is not present, or the user
> prefers to type it, that piece of UI is not very disturbing.
> 
> What do you think about it?

It looks good. I think this looks better than the other patch.

Comments:

You need to allow the window to be resized.

The way this works is very bizzare. You select a desktop file from the
menu, extract the exec line from it, remove stuff that is needed, and
then create a new desktop file with the modified exec line (and none
of the other information in the original desktop file) in the users
home directory (this is what eel_mime_add_application does). A better
solution would be to just put the name of the desktop file in the
entry when you select one, and then make the code handle desktop 
files. This should be easy, and there is even a comment about doing
this in check_application.

+#define sure_string(s)                    ((const char *)((s)!=NULL?(s):""))

Define this in the c file, not in the public header!

+	gboolean      long_operation = FALSE;

no initialization in definition (per the nautilus styleguide)

+	/* don't go back into the main loop if this wasn't very hard to do */
+	} while (!long_operation);

Why aren't loads via the icon theme long? They also read a file.

Anyway, this should use
nautilus_icon_factory_get_pixbuf_for_icon_force_size()
instead of manually loading icons, so that the results is cached.

+	/* Strip duplicates */
+	prev_name = NULL;
+	for (l = all_applications; l; l = next) {
+		MenuTreeEntry *entry = l->data;
+
+		next = l->next;
+
+		if (prev_name && strcmp (menu_tree_entry_get_name (entry), prev_name) == 0) {
+			menu_tree_entry_unref (entry);
+
+			all_applications = g_slist_delete_link (all_applications, l);
+		} else {
+			prev_name = menu_tree_entry_get_name (entry);
+		}
+	}

Just do the prev_name comparison in the list store append loop instead.
That means you don't have to call g_slist_delete_link() (which causes
this
loop to be O(n^2))

+		ditem = gnome_desktop_item_new_from_file (path,
+							  GNOME_DESKTOP_ITEM_LOAD_ONLY_IF_EXISTS,
+							  NULL /* error */);

This new code should probably use the new g_key_file_load_from_file()
api.
Also GNOME_DESKTOP_ITEM_LOAD_ONLY_IF_EXISTS doesn't do what you probably
think it
does, although it looks ok for this use. It only loads the file if the
file specified
in exec or tryexec exists.

+			/* Order is important here. We have to set the text first so that the
+			 * drag source is enabled, otherwise the drag icon can't be set by
+			 * panel_run_dialog_set_icon.
+			 */

Eh?

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a scrappy gay messiah in a wheelchair. She's a mistrustful communist 
politician with an incredible destiny. They fight crime!