GNOME.org
 

Re: [Nautilus-list] Integration of gmc and nautilus desktop directories.

on 4/12/01 10:20 AM, Miguel de Icaza at miguel ximian com wrote:

> 
> This patch is the Nautilus side of things to share the same desktop
> directory as gmc does and allow the user to switch between the file
> managers (shared nfs, different fms running, different contraints,
> etc)
> 
> We are shipping this in the upcoming Ximian GNOME release, together
> with the gmc patches that accompany it (some gmc pieces are already on
> cvs).

I'd like to OK this patch for Nautilus, but there are some coding style
mistakes and other stuff I am not sure about.

Miguel, I know that your main purpose for this patch was to ship it in
Ximian GNOME. I'm hoping you'll also be willing to make the changes so it
can be applied to Nautilus. If not, I guess we can find someone to take care
of that.

Patch looks good overall, but there are some issues. Please read my
comments. A lot of them refer to the Nautilus coding style guidelines, which
include all the GNOME style guidelines, plus the additions in the Nautilus
docs/style-guide.html.

--------------------------------

> ===================================================================
> RCS file: 
> /cvs/gnome/nautilus/libnautilus-extensions/nautilus-directory-async.c,v
> retrieving revision 1.145
> diff -u -r1.145 nautilus-directory-async.c
> --- libnautilus-extensions/nautilus-directory-async.c    2001/03/02
> 18:18:13    1.145
> +++ libnautilus-extensions/nautilus-directory-async.c    2001/04/12 17:20:12
> @@ -3159,7 +3159,7 @@
> }
> 
> g_free (file_contents);
> -    activation_uri_read_done (directory, uri);
> +    activation_uri_read_done (directory, uri ? uri + 5 : NULL);
> g_free (uri);
> }

Wow, glad you found this one. It sure needs to be fixed! If I was fixing it,
I would have also renamed the "uri" variable; the misleading name clearly
led to the bug.

> Index: libnautilus-extensions/nautilus-file-utilities.c
> ===================================================================
> RCS file: 
> /cvs/gnome/nautilus/libnautilus-extensions/nautilus-file-utilities.c,v
> retrieving revision 1.90.2.1
> diff -u -r1.90.2.1 nautilus-file-utilities.c
> --- libnautilus-extensions/nautilus-file-utilities.c    2001/03/10
> 02:24:15    1.90.2.1
> +++ libnautilus-extensions/nautilus-file-utilities.c    2001/04/12 17:20:13
> @@ -50,7 +50,7 @@
> #define NAUTILUS_USER_DIRECTORY_NAME ".nautilus"
> #define DEFAULT_NAUTILUS_DIRECTORY_MODE (0755)
> 
> -#define DESKTOP_DIRECTORY_NAME "desktop"
> +#define DESKTOP_DIRECTORY_NAME ".gnome-desktop"
> #define DEFAULT_DESKTOP_DIRECTORY_MODE (0755)
> 
> #define NAUTILUS_USER_MAIN_DIRECTORY_NAME "Nautilus"
> @@ -930,7 +930,7 @@
> char *desktop_directory, *user_directory;
> 
> user_directory = nautilus_get_user_directory ();
> -    desktop_directory = nautilus_make_path (user_directory,
> DESKTOP_DIRECTORY_NAME);
> +    desktop_directory = nautilus_make_path (g_get_home_dir (),
> DESKTOP_DIRECTORY_NAME);
> g_free (user_directory);
> 
> if (!g_file_exists (desktop_directory)) {

This change by itself will cause people who switch from Ximian's Nautilus to
a non-Ximian Nautilus or vice versa to lose all their desktop icons, because
there's no code to move files between the desktop directories. I think this
is a recipe for disaster.

This does make the experience for existing gmc users deluxe, but we
introduce incompatibility between one company's Nautilus, the other versions
of Nautilus that have already been released, and future versions of
Nautilus.

Can we discuss how to deal with this? I have no great ideas, but I am
worried about it. I wish we had made this change before widely releasing
Nautilus, but now we have to deal with the existing Nautilus users, I think.

A number of Nautilus users have already requested that the desktop go into a
directory with a simpler name. ~/.gnome-desktop is shorter than
~/.nautilus/desktop, but I was hoping for something even better. (For
example, tigert wants ~ to just "be the desktop".)

> +    if (nautilus_file_is_mime_type (file, "application/x-gnome-app-info"))
> +        return FALSE;

Nautilus coding style requires braces for even one-line if statements.

(We tried to use the same coding style as the rest of GNOME, but I guess we
didn't get it right.)

> +    if (nautilus_file_is_local && nautilus_file_is_gmc_url (file)){

Nautilus coding style requires a space between the ")" and the "{".

There's a coding mistake here. You meant to call nautilus_file_is_local on
file. I think nautilus_file_is_local is probably not right anyway.

It's a coding mistake to assume that nautilus_file_is_local is only true for
"real" files in the local file system. "is_local" can be returned true from
any gnome-vfs module, and can be used to indicate other more-subtle forms of
"being local". Instead, the correct check is to do
gnome_vfs_get_local_path_from_uri, and reject files where the result is
NULL.

> +        char *uri, *path;

Nautilus coding style requires all declarations at the top of the function.

> +        if (path){

Nautilus coding style requires a space between the ")" and the "{".

> +    if (!nautilus_file_is_local (file))
> +        return FALSE;

Nautilus coding style requires braces for even one-line if statements.

As discussed above, this check should be removed and a NULL check should be
added after the call to get_local_path_from_uri.

> +    if (strcmp (file->details->relative_uri, "Trash.gmc") == 0)
> +        return TRUE;

Nautilus coding style requires braces for even one-line if statements.

> +    if (nautilus_file_is_symbolic_link (file)){

Nautilus coding style requires a space between the ")" and the "{".

> +        /*
> +         * You would think that
> +         * nautilus_file_get_symbolic_link_target_path would
> +         * be useful here, but you would be wrong.  The
> +         * information kept around by NautilusFile is not
> +         * available right now, and I I have no clue how to
> +         * fix this.  On top of that, inode/device are not
> +         * stored, so it is not possible to see if a link is a
> +         * symlink to the home directory.  sigh.
> +         */

Nautilus comment style matches GTK, so there should not be a first line with
a "/*" by itself..

> +        char *uri, *path;
> +        int s;

Nautilus coding style requires all declarations at the top of the function.

> +        if (s == -1)
> +            return FALSE;

Nautilus coding style requires braces for even one-line if statements.

> +        if (!home_dir){

Nautilus coding style requires a space between the ")" and the "{". Nautilus
coding style requires "home_dir == NULL" rather than "!home_dir".

> +            if (home_dir && home_dir [home_dir_len-1] == '/'){

Nautilus coding style requires a space between the ")" and the "{". Nautilus
coding style requires "home_dir == NULL" rather than "!home_dir".

> +        if (home_dir){

Nautilus coding style requires a space between the ")" and the "{". Nautilus
coding style requires "home_dir != NULL" rather than "home_dir".

> +            if (strncmp (home_dir, buffer, home_dir_len) == 0)
> +                return TRUE;

Nautilus coding style requires braces for even one-line if statements.

> +    /*
> +     * This handles visiting other people's desktops, but it can arguably
> +     * be said that this might break and that we should lookup the passwd
table.
> +     */

Nautilus comment style matches GTK, so there should not be a first line with
a "/*" by itself..

> +    return strstr (file->details->directory->details->uri, "/.gnome-desktop")

This should use DESKTOP_DIRECTORY_NAME. Perhaps it could also be changed so
that it doesn't get fooled by files whose name are prefixed with
".gnome-desktop", but maybe that's not a real issue?

> +    if (strcmp (key, NAUTILUS_METADATA_KEY_CUSTOM_ICON) == 0){

Nautilus coding style requires a space between the ")" and the "{".

> +        if (nautilus_file_is_in_desktop (file) &&
> +            nautilus_file_is_local (file)){

Nautilus coding style requires a space between the ")" and the "{". Also, we
normally put the && at the beginning of the second line, but it's not strict
rule.

As discussed above, nautilus_file_is_local is probably the wrong check here.
Instead, a check for NULL in the result of gnome_vfs_get_local_path_from_uri
is probably the better choice.

> +            char *icon_path;
> +            char *local_path;
> +            char *local_uri;

Nautilus coding style requires all declarations at the top of the function.

> +    if (nautilus_file_is_mime_type (file, "application/x-gnome-app-info")){

Nautilus coding style requires a space between the ")" and the "{".

> +        GnomeDesktopEntry *entry;
> +        char *path, *uri;

Nautilus coding style requires all declarations at the top of the function.

> +        uri = nautilus_file_get_uri (file);
> +        path = gnome_vfs_get_local_path_from_uri (uri);
> +
> +        if (path){

Nautilus coding style requires a space between the ")" and the "{", and
"path != NULL" rather than "path".

> +            if (entry){

Nautilus coding style requires a space between the ")" and the "{", and
"entry != NULL" rather than "path".

> +    if (nautilus_file_is_gmc_url (file)){

Nautilus coding style requires a space between the ")" and the "{".

> +        char *path, *uri, *caption;
> +        int size, res;

Nautilus coding style requires all declarations at the top of the function.

> +        if (path)

Nautilus coding style requires braces for even one-line if statements and
"path != NULL" rather than "path".

> +        if (res == 0 && caption)

I'm surprised this compiles without warnings, because res is uninitialized
if path is NULL. You need to set res to 0 in the path == NULL case.

Nautilus coding style requires braces for even one-line if statements and
"caption != NULL" rather than "caption".

> +/**
> + * nautilus_file_is_gmc_url
> + * 
> + * Check if this file is a gmc url
> + * @file: NautilusFile representing the file in question.
> + * 
> + * Returns: True if the file is a gmc url
> + * 
> + **/
> +gboolean
> +nautilus_file_is_gmc_url (NautilusFile *file)
> +{
> +    if ((strncmp (file->details->relative_uri, "url", 3) == 0) &&
> +        nautilus_file_is_in_desktop (file))
> +        return TRUE;
> +    return FALSE;
> }

This function worries me. What will prevent people from naming their files
with the letters "url" at the start, and putting them onto the desktop? Is
there a reason this is OK?

Oh, right, and Nautilus coding style requires braces for even one-line if
statements.

> +    if (uri == NULL){

Nautilus coding style requires a space between the ")" and the "{".

> +        char *directory, *directory_uri;

Nautilus coding style requires all declarations at the top of the function.

> +        /*
> +         * Do we have to check the gnome metadata?
> +         *
> +         * Do this only for the ~/.gnome-desktop directory, as it was
> +         * the only place where GMC used it (since everywhere else we could
> +         * not do it because of the imlib leaks).
> +         */

Nautilus comment style matches GTK, so there should not be a first line with
a "/*" by itself..

> +        directory_uri = nautilus_file_get_parent_uri (file);
> +        directory = gnome_vfs_get_local_path_from_uri (directory_uri);
> +        if (directory && strcmp (directory, desktop_directory) == 0){

Nautilus coding style requires a space between the ")" and the "{" and
"directory != NULL" rather than "directory". Variables named directory in
Nautilus are usually NautilusDirectory objects, so directory_path would be a
more natural name.

> +            char *file_path, *buf;
> +            int size, res;

Nautilus coding style requires all declarations at the top of the function.

> +            if (file_path)
> +                res = gnome_metadata_get (file_path, "icon-filename", &size,
&buf);

Nautilus coding style requires braces for even one-line if statements.

> +            if (res == 0 && buf){

I'm surprised this compiles without warnings, because res is uninitialized
if path is NULL. You need to set res to 0 in the path == NULL case.

Nautilus coding style requires a space between the ")" and the "{" and "buf
!= NULL" rather than "buf".

> +    if (uri == NULL && nautilus_file_is_mime_type (file,
> "application/x-gnome-app-info")){

Nautilus coding style requires a space between the ")" and the "{".

> +        char *file_path = gnome_vfs_get_local_path_from_uri (file_uri);
> +        GnomeDesktopEntry *entry;

Nautilus coding style requires all declarations at the top of the function
and prohibits initializing local variables in the declaration.

> +        if (entry){

Nautilus coding style requires a space between the ")" and the "{" and
"entry != NULL" rather than "entry".

> +            if (entry->icon)

Nautilus coding style requires braces for even one-line if statements and
"entry->icon != NULL" rather than "entry->icon".

> +static void
> +migrate (char *old, char *new, char *key)
> +{
> +    int size;
> +    char *buf;
> +
> +}

Please remove this empty unused function.

> +/*
> + * Find ~/.gnome-desktop/Trash and rename it to ~/.gnome-desktop/Trash-gmc
> + * Only if it is a directory
> + */

Nautilus comment style matches GTK, so there should not be a first line with
a "/*" by itself.

> +void
> +migrate_trashcan (void)

This local private function should be a static.

> +    char *dp = nautilus_get_desktop_directory ();
> +    char *trash_dir = g_strconcat (dp, "/", "Trash", NULL);
> +    char *dest = g_strconcat (dp, "/", "Trash.gmc", NULL);

Nautilus coding style prohibits initializing local variables in the
declaration.

> +    if (stat (trash_dir, &buf) == 0 && S_ISDIR (buf.st_mode)){

Nautilus coding style requires a space between the ")" and the "{".

> Index: src/nautilus-first-time-druid.c

Not sure what to do about the changes to the druid. At the very least, the
patch should either remove everything cleanly, or use an ifdef on some
symbol. The current patch has both a change or two and #if 0 blocks.

> Index: src/nautilus-first-time-druid.h
> ===================================================================
> RCS file: /cvs/gnome/nautilus/src/nautilus-first-time-druid.h,v
> retrieving revision 1.3
> diff -u -r1.3 nautilus-first-time-druid.h
> --- src/nautilus-first-time-druid.h    2000/11/07 19:26:37    1.3
> +++ src/nautilus-first-time-druid.h    2001/04/12 17:20:15
> @@ -35,5 +35,4 @@
> GtkWidget *nautilus_first_time_druid_show (NautilusApplication *application,
>   gboolean manage_desktop,
>   const char *urls[]);
> -
> #endif /* NAUTILUS_FIRST_TIME_DRUID_H */

This should be removed from the patch!

> ===================================================================

> +
> +    /*
> +     * For the desktop rescanning
> +     */

Nautilus comment style matches GTK, so there should not be a first line with
a "/*" by itself.

> +    gint reload_desktop_timeout;
> +    gint delayed_init_signal;

Nautilus coding style says use int, not gint, but these should be guint
anyway!

> +    /*
> +     * Remove hack of love timeout
> +     */

Nautilus comment style matches GTK, so there should not be a first line with
a "/*" by itself. And I bet you can come up with an even-better comment that
will help people later on (or remove this one I guess).

> +    FMDesktopIconView *desktop_icon_view = FM_DESKTOP_ICON_VIEW (data);

Nautilus coding style prohibits initializing local variables in the
declaration.

> +    if (desktop_icon_view->details->pending_rescan)
> +        return TRUE;

Nautilus coding style requires braces for even one-line if statements.

> +    if (stat (desktop_directory, &buf) == -1){
> +        return TRUE;
> +    }

Nautilus coding style requires a space between the ")" and the "{".

> +    if (buf.st_ctime == desktop_dir_modify_time)
> +        return TRUE;

Nautilus coding style requires braces for even one-line if statements.

> +    if (stat (desktop_directory, &buf) == -1)
> +        return;

Nautilus coding style requires braces for even one-line if statements.

> +#define RESCAN_TIMEOUT 4000

Nautilus coding style says that defined constants go at the top of the file.

> +/*
> + * This funcion is used because the NautilusDirectory
> + * model does not exist always in the desktop_icon_view,
> + * so we want until it has been instantiated
> + */

Nautilus comment style matches GTK, so there should not be a first line with
a "/*" by itself. Also, there's a typo: "funcion".

> +    /*
> +     * Keep track of the load time
> +     */

Nautilus comment style matches GTK, so there should not be a first line with
a "/*" by itself.

> +    /*
> +     * Hack of Love.  No abstractions, just plain working code.
> +     */

Nautilus comment style matches GTK, so there should not be a first line with
a "/*" by itself. Not sure what the "no abstractions" remark means. I just
find it confusing.

> +    desktop_icon_view->details->delayed_init_signal = -1;

We normally use 0 for this, not -1.

> +    if (!desktop_directory)
> +        desktop_directory = nautilus_get_desktop_directory ();

Nautilus coding style requires braces for even one-line if statements and
"desktop_directory == NULL" rather than "!desktop_directory".

> +    /*
> +     * If it is the desktop directory, maybe gnome metadata has information
> about it
> +     */

Nautilus comment style matches GTK so there should not be a first line with
a "/*" by itself. I also think that for context this should be called "the
old gnome metadata that gmc uses" rather than "gnome metadata" so that
future people reading this code have an easier time understanding it.

> +    if (!position_good){

Nautilus coding style requires a space between the ")" and the "{".

> +        if (nautilus_file_is_local (file) && nautilus_file_is_in_desktop
(file)){

Nautilus coding style requires a space between the ")" and the "{".

Also, the nautilus_file_is_local check isn't needed and should be removed.
gnome_vfs_get_local_path_from_uri will return NULL and everything will be
all right for the non-local case.

> +            char *path, *uri;
> +            int res, size;
> +            char *buf;

Nautilus coding style prohibits initializing local variables in the
declaration.

> +            uri = nautilus_file_get_uri (file);
> +            path = gnome_vfs_get_local_path_from_uri (uri);
> +
> +            if (path){

Nautilus coding style requires a space between the ")" and the "{" and "path
!= NULL" rather than "path".


> +                if (res == 0){
> +                    if (sscanf (buf, "%d%d", &position->x, &position->y) ==
2){

Nautilus coding style requires a space between the ")" and the "{".

> +        if (nautilus_file_is_local (file) && nautilus_file_is_in_desktop
(file)){

Nautilus coding style requires a space between the ")" and the "{".

> +            char *uri, *path;

Nautilus coding style requires all declarations at the top of the function.

> +            if (path){

Nautilus coding style requires a space between the ")" and the "{" and "path
!= NULL" rather than "path".

> +                char buf [128];

Nautilus coding style requires all declarations at the top of the function.

--------------------------------

    -- Darin


_______________________________________________
gnome-hackers mailing list
gnome-hackers gnome org
http://mail.gnome.org/mailman/listinfo/gnome-hackers
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]