[Nautilus-list] Review of patch round 2 (was Re: Integration of gmc and nautilus desktop directories.)

[Darin Adler <darin eazel com>]

on 4/12/01 12:45 PM, Miguel de Icaza at miguel ximian com wrote:

> Patch, Take 2.

Thanks!

I'm hoping for a new patch (take 3?) that includes the other thing you did
(copying from the old Nautilus desktop directory).

>> Nautilus coding style requires braces for even one-line if statements.
> 
> Fixed all of these

Most. (See below.)

>> 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.
> 
> I am just trying to speed up the path by avoiding doing expensive
> operations.  It does not really matter correctness wise, as the only
> case where these things will happen is the ~/.gnome-desktop directory,
> and I am not expecting people to have thousands of files there.

This doesn't really speed up the path much at all. And it's extra code. I
still think you should take them out. In any case, you have places that are
not checking for NULL paths on the output of
gnome_vfs_get_local_path_from_uri, and that is more important.

>> Nautilus coding style requires all declarations at the top of the function.
> 
> Fixed all of these as well.  I am not sure this is a great policy, it
> seems like it might lead to bugs.

Either way can lead to bugs. I've seen many examples where the variable
inside braces shadowed another with the same name, and someone thought it
was the same one. (Usually found with -Wuninitialized.) The best approach is
to avoid functions that are so big that this matters.

>> 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?
> 
> Did not fix this, as it is a private define in
> nautilus-file-utilities.c
> 
> I can move it to a header if you think thats ok

No need.

>> 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.
> 
> It is not that important, it is a test used to not impact performance
> on the most common case.  Ie, it will turn out always ok and not use
> any expensive operations in its process.

I'm pretty sure it doesn't help speed. The other tests will also fail fairly
quickly. But in the end, it's OK, as long as you also have the NULL checks
for paths. I really hate to have the extra code though, and I think it can
confuse future programmers.

>> Nautilus coding style requires braces for even one-line if statements and
>> "path != NULL" rather than "path".
> 
> Fixed all of these as well.

Missed a few, see below.

>> 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?
> 
> Not really, but I can not think of other ways of testing this
> (actually, for correctness, the gmc-url handler should not depend on
> the contents of the file for fetching the URL, the data was always
> stored on the metadata.db).

This "url"-prefix problem will last forever!

>> 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.
> 
> It is just a quick way of separating the good from the bad.  A more
> accurate test follows for false positives.  It wont hurt, it is just
> faster. 

Right, it won't hurt as long as there is a NULL check in the path stuff
below.

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

> +        if (path) {

path != NULL

> +    if (!nautilus_file_is_local (file))
> +        return FALSE;
> +    
> +    if (strcmp (file->details->relative_uri, "Trash.gmc") == 0)
> +        return TRUE;
> +
> +    if (nautilus_file_is_symbolic_link (file)) {
> +        /* 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.
> +         */
> +        uri = nautilus_file_get_uri (file);
> +        path = gnome_vfs_get_local_path_from_uri (uri);

This code assumes that path will be non-NULL, but that's not guaranteed by
any of the tests above. You should really get rid of the
nautilus_file_is_local check, because the check for the name Trash.gmc is
fine as far as speed is concerned, and a check for NULL here will be quick
enough for all those Trash.gmc files you see on servers.

> +        if (!home_dir) {

home_dir == NULL

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

home_dir != NULL

> +        if (home_dir) {

home_dir != NULL

> +    if (strcmp (key, NAUTILUS_METADATA_KEY_CUSTOM_ICON) == 0) {
> +        if (nautilus_file_is_in_desktop (file) &&
> +            nautilus_file_is_local (file)) {
> +
> +            local_uri = nautilus_file_get_uri (file);
> +            local_path = gnome_vfs_get_local_path_from_uri (local_uri);
> +            icon_path = gnome_vfs_get_local_path_from_uri (metadata);

You need to check local_path != NULL and icon_path != NULL here. Maybe just
a theoretical problem, but nautilus_file_is_local is not a guarantee that
gnome_vfs_get_local_path_from_uri will return non-NULL.

> +        if (path) {

path != NULL

> +        if (path) {

path != NULL

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

desktop_directory == NULL

Need braces.

> +        if (directory && strcmp (directory, desktop_directory) == 0) {

directory != NULL

> +            if (file_path) {

file_path != NULL;

> +#if TRANSITIONAL_NAUTILUS

You used ifdef elsewhere, so use ifdef here, please.

> -    gnome_druid_set_page (druid, GNOME_DRUID_PAGE
(pages[GMC_TRANSITION_PAGE]));
> +    gnome_druid_set_page (druid, GNOME_DRUID_PAGE (pages[USER_LEVEL_PAGE]));

This needs to be done with #ifdef TRANSITIONAL_NAUTILUS instead of
unconditionally.

> -#if 0
> +#ifdef TRANSITIONAL_NAUTILUS

This particular #if 0 is *not* a TRANSITIONAL_NAUTILUS one, right?

> +#endif

You added an endif, but the #if 0 was already there, so I think this
unbalances endifs!?

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

Need braces.

> +    if (!desktop_directory) {

desktop_directory == NULL

> +            if (path) {

path != NULL

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

Thanks for all your hard work on this, Miguel. I think it's getting close to
the point where we'll roll it in.

    -- Darin