Re: [Nautilus-list] [PATCH] creating launchers with Nautilus

[Darin Adler <darin bentspoon com>]

This patch needs a lot of work.

And I think that this is probably not the right way to go -- please talk to
George Lebl about this before pursuing it further.

The source files need copyrights and licenses.

You need a G_END_DECLS at the end of the .h file.

> void
> launcher_dialog ( char * path, GtkWindow * parent, char * old_launcher );

Public functions use the namespace prefix, Nautilus. Also, the spacing here
and line breaks are wrong. In header files it's like this:

    void nautilus_present_launcher_dialog (const char *path,
                                           GtkWindow  *parent,
                                           const char *old_launcher);

Please look at other header files and imitate them.

The includes are wrong. In header files you include what you need for the
things defined in that header. In this case, it's:

    #include <gtk/gtkwindow.h>

In the .c files, the includes must begin with:

    #include <config.h>

And then the include of the corresponding .h file must be next, before any
other includes.

These:

> #include <libgnome/libgnome.h>
> #include <libgnomeui/libgnomeui.h>
> #include <libgnomevfs/gnome-vfs.h>

Are large catch-all includes that include many different files. They should
not be used in Nautilus code. Instead, include the specific headers you
actually need.

> char * current_path = NULL;

Things private to the file need to be marked "static".

> gboolean 
> launcher_save_to_file (GtkWidget *dialog, gpointer data);

Things private to the file need to be marked "static". You don't want a
separate prototype for each function just before the function. Marking them
static will take care of the warning you're getting.

>   GnomeDItemEdit *dedit = GNOME_DITEM_EDIT (data);

Declarations need to be separate from initialization. This is covered in the
Nautilus style guide.

>  filename = "No name";
> gnome_desktop_item_set_string (item, GNOME_DESKTOP_ITEM_NAME, _(filename));

The _() has to be around the constant string "No name". Also, we're past the
"no new localizable strings" milestone, so you have to get an OK from the
release team or translators to add this.

>   g_free (current_path);

This needs to NULL out current_path afterward too.

> (char *) _("Error creating launcher")
> (char *) _("No help")
> (char *) _("Error opening launcher")

No casts here.

>     nautilus_directory_force_reload (nautilus_directory_get (location));

This line leaks a NautilusDirectory object. Also, I don't think a
force_reload is needed if you have file monitoring, so it's bad to do it
unconditionally.

> current_path = g_malloc(strlen(path) + 1);
> strcpy(current_path, path);
> g_free (path);

You want to use g_strdup here. Also you don't want to free the passed-in
path.



    -- Darin