Re: [Nautilus-list] continued redhat merge

[Darin Adler <darin bentspoon com>]

on 9/19/01 3:36 PM, Alex Larsson at alexl redhat com wrote:

> Ok. I did one iteration.

My comments:

> +/* desktop-file-loader.c

Still says the old name in the header.

> +        df = g_new (NautilusDesktopFile, 1);
> +
> +        df->lines = NULL;
> +        df->addition_list = NULL;
> +        df->main_section = NULL;
> +        df->section_hash = NULL;

We normally use g_new0 for this sort of thing.

> +        char *contents = NULL;

No need to initialize this.

> +       if (contents == NULL) {
> +               return GNOME_VFS_ERROR_WRONG_FORMAT;
> +       }.

No need to check for NULL, the routine (now) doesn't return NULL unless it
fails.

> +static GnomeVFSResult
> +write_all (GnomeVFSHandle *handle,
> +          gconstpointer buffer,
> +          GnomeVFSFileSize bytes)
> +{
> +       GnomeVFSFileSize bytes_written;
> +       GnomeVFSResult result;
> +
> +       while (bytes > 0) {
> +               result = gnome_vfs_write (handle, buffer,
> +                                         bytes, &bytes_written);
> +               if (result != GNOME_VFS_OK) {
> +                       return result;
> +               }
> +               bytes -= bytes_written;
> +       }
> +
> +       return GNOME_VFS_OK;
> +}

First of all, this function isn't really needed. I'm not sure why
gnome_vfs_write returns the count of bytes written, but it always writes all
the bytes you ask it to. I can understand if we don't want to code depending
on this, but I think it's a design flaw in gnome-vfs, putting undue burden
on the caller.

But if we decide this write_all function is needed, then we must fix it
because this writes the start of the buffer over and over again, reusing the
buffer pointer without moving it through the buffer.

We should really fix this in the gnome-vfs API.

> +        if (df->section_hash) {

For nautilus coding style, this needs != NULL.

I got sick of the minor coding style issues and just skipped most of them;
there are many places where variables are declared in blocks, which is
forbidden in the nautilus coding style. I think it's OK for now.

nautilus-desktop-file.c is missing an #include <config.h> at the top.

There are lots of if statements in nautilus-desktop-file.c with one-line
bodies without the braces that Nautilus coding style requires.

In nautilus_desktop_file_get_icon, it checks the first character of
icon_name for '/' before calling gnome_vfs_get_uri_from_local_path. This is
unnecessary, since that function is defined to return NULL for any strings
without a leading '/'.

In nautilus_link_desktop_file_local_create I would have used fputs instead
of all those fwrite calls with strlen.

I'd like to address the FIXMEs about missing operations for .desktop files
in nautilus-link.c soon, by either removing the operation entirely, or
implementing it for .desktop files. My guess is that if we can get away with
having a function unimplemented for .desktop files, we can eliminate it
entirely, but we want to visit all the callers to be sure by removing the
declaration from the header and recompiling.

    -- Darin