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

[Frederic Crozat <fcrozat mandrakesoft com>]

le mer 03-10-2001 à 18:43, Darin Adler a écrit :
> 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.

No problem with that.. I'll try to keep this feature working in our
Mandrake package until we get to a version good enough to be merged in
"official" Nautilus source..

> 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.

This was my first idea (since I discovered Desktop on Volume are
created/removed using this system) but when I found the merged directory
class in libnautilus-private, I though it would be cleaner than
synchronizing ~/.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.

How did you discovered that ? :))))))
You got it right, this patch was developped rather quickly and it is not
very clean because it was my first real big development in Nautilus and
I hadn't time to do much better :((

I'm going to fix the patch for our Mandrake cooker users but I'd like to
have a clean design first before rewriting it from scratch..

In fact, I'm wondering which design is the "best" between :
-using a method similar to new gnome-vfs desktop-method  (favorites:,
programs: ...) by adding a desktop: URI
-using a NautilusMonitor object to monitor "common" desktop directory
and synchronize it like Volume icons (the desktop: URI would not be
needed..)

-extending my original patch into a real nautilus class (similar to
Nautilus Trash Directory) and other fixes in nautilus/eel..

> 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.

Agreed.. But let's choose good design first :)

> > -        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!

Agreed.. I did that because I was in a rush..
> 
> > -        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.

Same excuse :) 

> > +        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.

I didn't want to patch eel when I was doing this patch.. 
> 
> > +        } 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.

After discussing with our ergonomic guy, I decided to remove "can't
write" emblem from desktop since it would confuse users.. In fact, we
plan to add a new emblem to these special icons, since they are supposed
to represent icons to hotplugged devices

> > +    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.

Agreed..

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

Well, it shows that nautilus code is quite robust :))
> 
> > +        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.

Agreed. This patch was supposed to provide a basic functionaliy. Now, we
have to decide which design is the best and I'll try to implement it.
-- 
Frédéric Crozat
MandrakeSoft