Re: desktop item editing

[Dave Camp <dave ximian com>]

This is sweet, thanks.  A few comments:

On Tue, 2004-05-04 at 16:25 -0500, James Willcox wrote:
>                 for (l = properties_window->details->original_files;
> l != NULL; l = l->next) {
>                         file = NAUTILUS_FILE (l->data);
> +                       file_uri = nautilus_file_get_uri (file);
> +                       ditem = gnome_desktop_item_new_from_uri
> (file_uri,
> +
> GNOME_DESKTOP_ITEM_LOAD_ONLY_IF_EXISTS,
> +
> NULL);
> +                       if (ditem != NULL) {

You should probably only try to open a desktop item if it is of the
right mime type.


> 
>                 gboolean localized,
>                 gboolean filename)
> {
>         ItemEntry *entry = g_malloc (sizeof (ItemEntry));
>         entry->field = field;

We generally use g_new0() for this.


> 
>         ItemEntry *item_entry = g_object_get_data (G_OBJECT (entry),
> "item_entry");
>         const char *val = gtk_entry_get_text (entry);

We don't initialize variables in the declaration.


> fm_ditem_page_exec_drag_data_received (GtkWidget *widget,
> GdkDragContext *context,
>                                        int x, int y,
>                                        GtkSelectionData
> *selection_data,
>                                        guint info, guint time,
>                                        GtkEntry *entry)
> {
>         char **uris;
>         gboolean exactly_one;
>         GnomeDesktopItem *item;
>         
>         uris = g_strsplit (selection_data->data, "\r\n", 0);
>         exactly_one = uris[0] != NULL && (uris[1] == NULL ||
> uris[1][0] == '\0');
> 
>         if (!exactly_one) {
>                 g_strfreev (uris);
>                 return;
>         }
> 
>         item = gnome_desktop_item_new_from_uri (uris[0],
>                                                 GNOME_DESKTOP_ITEM_LOAD_ONLY_IF_EXISTS,
>                                                 NULL);
>         if (item != NULL &&
>             gnome_desktop_item_get_entry_type (item) ==
> GNOME_DESKTOP_ITEM_TYPE_APPLICATION) {
>                 gtk_entry_set_text (entry,
>                                     gnome_desktop_item_get_string
> (item,
> 
> GNOME_DESKTOP_ITEM_EXEC));
>                 gnome_desktop_item_unref (item);
>         }
>         
>         g_strfreev (uris);
> }

This bit needs some work.  You should probably check the mime type
before you try to read the desktop entry.  This will require getting a
NautilusFile and waiting for the mime type to be read
(nautilus_file_call_when_ready()).  gnome_desktop_item_new() doesn't
seem to be too smart about bailing out - I dragged a large binary to the
field and it took awhile to try to read it.

If the URI is not a desktop entry, you probably just want to put the
local path in intact (for people dragging binaries, for example).


> 
>                 entries = g_list_prepend (entries,
>                                           item_entry_new
> (GNOME_DESKTOP_ITEM_COMMENT,
> 
> _("Comment"), TRUE, FALSE));
> 
> 
> 
>                 entries = g_list_prepend (entries,
>                                           item_entry_new
> (GNOME_DESKTOP_ITEM_COMMENT,
> 
> _("Comments"), TRUE, FALSE));

Why are these different?

>         if (item == NULL)
>                 return;

Brackets on all if statements please.

> 
>         g_signal_connect (box, "unrealize", G_CALLBACK
> (box_unrealize_cb), item);

We usually apply changes on activate and focus_out for the various
entries.  For freeing the desktop file, it'd be better to use a weak ref
than unrealize.


>         item_type = gnome_desktop_item_get_entry_type (item);
>         if (item_type == GNOME_DESKTOP_ITEM_TYPE_APPLICATION) {
>                 gtk_label_set_text (label, _("Launcher"));
>                 create_page (item, box, FALSE);
>         } else if (item_type == GNOME_DESKTOP_ITEM_TYPE_LINK) {
>                 gtk_label_set_text (label, _("Link"));
>                 create_page (item, box, TRUE);
>         } else {
>                 return;
>         }

You leak the item if the page isn't created, and it's a little weird
that you're giving the ref to create_page.  Maybe better would be:

GtkWidget *page;

if (item_type == GNOME_DESKTOP_ITEM_TYPE_APPLICATION) {
	page = create_page ();
} else if (item_type == ...) {
	page = create_page ();
}

if (page) {
	g_object_weak_ref (page, item);
} else {
	gnome_desktop_item_unref (item);
}

>         if (g_list_length (files) > 1)
>                 return NULL;

Probably best to just do if (!files || files->next) to avoid excessive
counting. 

>         info = NAUTILUS_FILE_INFO (files->data);
>         mime = nautilus_file_info_get_mime_type (info);
> 
>         if (strcmp (mime, "application/x-desktop") != 0) {
>                 g_free (mime);
>                 return NULL;
>         }

You can just use nautilus_file_info_is_mime_type
("application/x-desktop");

-dave