[Nautilus-list] nautilus-1.0.4-dynamic.patch

[Darin Adler <darin bentspoon com>]

I can see how it's useful to have a desktop that's a merge of two different
directories if you want to include some icons that are shared by all the
users.

But there are tons of details in making a merged directory like the trash
work properly. You have a reasonable start, but there's a long way to go to
get this 100%. I don't think I'm ready to take this patch for 1.0.5, because
I don't think we can get it in shape in time.

I'd like to consider adding this feature for a future release.

As a practical matter, I'd think you'd have a far easier time just making
code that keeps the two directories synchronized. It's not as elegant, but
it doesn't have all the issues of integration with the rest of Nautilus, and
it will work for other programs that look at ~/.gnome-desktop.

The patch itself has a number of problems, besides the fact that I suspect
there's a lot that still needs to be added to get it to 100%. It looks like
it was developed by trial and error, fixing things that didn't work as you
encountered them. We can do a better job by getting the model right first,
then we won't have to patch things up in so many places.

Lots of the code doesn't have the space that's needed between the function
name and the opening parenthesis.

> +        desktop_directory_uri = g_strdup("desktop:/");

This needs to use a constant, like TRASH_URI. But also, there's no reason to
g_strdup and g_free when you are using a constant. And if this function
always uses a constant, then the name
nautilus_desktop_window_update_directory isn't really appropriate anyway. We
need to rethink how we implement changing the underlying directory if we are
using a "desktop:" URI, so a bigger change is needed here.

> -        nautilus_desktop_window_update_directory
> -            (nautilus_application_desktop_window);
> +        /* ugly hack since merged directory seems to prevent correct refresh
*/
> +        nautilus_application_close_desktop ();
> +        nautilus_application_open_desktop (NAUTILUS_APPLICATION(user_data));

Yes, a change is needed here, but doing a close and reopen of the desktop
does not sound right to me. For one thing, it would have a side effect of
turning on the desktop even if it isn't already on, I think!

> -        result = eel_drag_items_local (container_uri_string, items);
> +        if (eel_istr_has_prefix (container_uri_string, "desktop:")) {
> +            result =
> nautilus_directory_contains_file(nautilus_directory_get("desktop:"),nautilus_f
> ile_get(((EelDragSelectionItem *)items->data)->uri));
> +        } else {
> +            result = eel_drag_items_local (container_uri_string, items);
> +        }

This only looks at the location of the first item, which I think is not
correct.

> +        if (eel_istr_has_prefix (drop_target, "desktop:")) {
> +            *default_action = GDK_ACTION_MOVE;
> +            *non_default_action = GDK_ACTION_MOVE;
> +        }

I don't think it's correct to say that we always want to move items to the
desktop, and I think this code is at the wrong level and should be inside
eel_drag_default_drop_action_for_icons.

> +        } else if (eel_istr_has_prefix(target_dir,"desktop:")) {
> +            target_dir_uri = gnome_vfs_uri_new
> (g_strconcat("file://",nautilus_get_desktop_directory(),NULL));

You can't turn a path into a URI by doing just concatenating it with
"file://". The correct way is to use gnome_vfs_get_uri_from_local_path.

> +    integer |= !nautilus_file_can_write (file) &&
> !nautilus_file_is_in_desktop(file);

I understand that you want to leave the "can't write" emblem off of anything
on the desktop. I'm not sure that's quite the right rule. We do need to get
rid of these extra emblems, but just saying that the emblem never shows up
on the desktop might not be the right cut. Red Hat also has a patch in this
area -- I'll need to consider the issue after looking at both.

> +    return ((strstr (file->details->directory->details->uri,
"/.gnome-desktop") 
> != NULL) 
> +        || (strstr (file->details->directory->details->uri,
> "/usr/share/gnome/desktop") != NULL)) ;

> +            
nautilus_merged_directory_add_real_directory(NAUTILUS_MERGED_DIRECTORY(dir
> ectory),nautilus_directory_get("file://usr/share/gnome/desktop"));

This hard codes a "/usr" path. Normally, gnome programs don't do that. They
use a prefix or some other way of finding out about special system paths.

Also, you left out a / in "file:///usr/share/gnome/desktop". I'm amazed that
it worked at all with that incorrect URI!

> +        if (eel_istr_has_prefix (uri, "desktop:")) {

I think it's incorrect to treat all strings with "desktop:" prefix as the
desktop itself. The approach should be more like eel_uri_is_trash where
there's one special URI that means the trash, and other URIs in the scheme
don't necessarily mean the trash.

    -- Darin