Re: [Nautilus-list] continued redhat merge

[Darin Adler <darin bentspoon com>]

on 9/12/01 3:45 PM, Alex Larsson at alexl redhat com wrote:

> This patch is a lot more raw than the previous... Let the reviewing begin!

I'm not comfortable with the nautilus-link-impl.h and
nautilus-link-impl-desktop.h file names. I'd prefer
nautilus-link-historical.h and nautilus-link-desktop-file.h. And also
nautilus-desktop-file-loader.h. Unless the need to prefix everything with
nautilus was a mistake, in which case we could rename a lot of the files in
Nautilus to shorter names!

A lot of this code doesn't follow Nautilus coding style. Single line if
statements without braces, if (x) where x is a pointer, declarations inside
blocks, etc. I have no idea what to do about this. I have no interest in
picking it over line by line before someone at least tries to make it follow
the conventions. Or we can loosen or change the conventions.

The code in desktop-file-loader.c does not follow the Nautilus conventions.
It uses struct _X, which we avoid. It is not in the "nautilus_" namespace.
It calls isspace on chars, which is a bug. It needs to say "isspace((guchar)
x)" everywhere it calls isspace. Many of the private functions are named
g_xxx, which seems like it will make them conflict with the names of public
functions of similar names added to glib. That doesn't bother me for
g_strdupv, but g_key_equal seems like a foolish name to use. I'm not sure
what to think about this code. I'd like to see it added to Eel rather than
libnautilus-private, and I'd like to see some tests of it in isolation. I'd
put test .desktop files into eel and run tests on this code with a test
harness in there.

The slurp_uri_contents function doesn't do any reality check on the size of
the file. That means that Nautilus could crash if someone made a huge
desktop file to sabotage it. The slurp_uri_contents allocates a buffer
that's one byte longer than the file it reads, but it doesn't zero that
extra byte, although presumably desktop_file_from_string then assumes that
NULL-terminating byte is there. Also, slurp_uri_contents is almost identical
to eel_read_entire_file -- I'd prefer to enhance the eel call rather than
add yet another "read entire file" function.

The make_dot_directory_uri function needs to check for NULL when it calls
gnome_vfs_uri_new, because gnome-vfs will return NULL if the URI is one it
can't handle.

The code in nautilus-directory-async.c looks very good overall, but it's
very hard for me to spot problems just be reading it (especially in diff
form). I think we should put the changes into nautilus HEAD, but then we
need some testing before we can do a 1.0.5 release. And I'll probably have
to read it over again and think about possible simplifications. I don't
think that we need both "add_file_to_work_queue" and a separate
"async_state_changed". To fully deploy the new model, I'd like to see more
of the code to support the old model deleted.

Since nautilus_file_is_nautilus_link is changing function, I'd ideally like
to see it change its name too, but then again that would also mean changing
the names of everything in nautilus-link* files so I guess we can leave it
alone as long as we make a nice comment to explain that "Nautilus link"
means ".desktop file or historical Nautilus link file". I don't see such a
comment anywhere in the diff.

The nautilus_icon_container_remove function still has a stray FIXME in there
that should be removed.

In nautilus_icon_factory_get_icon_for_file, there's a code change to make it
only look up the icon_name if the uri is NULL. But even when you have a URI
for a custom icon, you might need the icon name to fall back on if you can't
load a custom icon at that URI. So I think that change is wrong.

The nautilus-link-impl-desktop.c contains another copy of the
slurp_uri_contents function! Yuck! But this one at least doesn't have the
bug where it forgets to add the 0 byte at the end. Perhaps
desktop-file-loader.c is unused code?

In nautilus_link_impl_desktop_local_create, the code makes the standard
mistake of using a path (not a URI) with gnome-vfs calls. And since this
code works only locally, it doesn't need to use gnome-vfs at all. It should
just use fopen and not bother with gnome_vfs_create at all.

The comment at the top of nautilus-link-impl.c says nautilus-link.c.

I think that nautilus_file_get_name probably should be left with its
original, pre-desktop hack meaning, and a new nautilus_file_get_display_name
or something along those lines should be added that returns the name taking
the custom name for display in the icon view into account. I'm pretty sure
there are bugs where one is gotten and the other is what's wanted. Perhaps
CUSTOM_NAME can be changed to DISPLAY_NAME too, to reflect this connection?

What's the status on the code inside "#if ADDITIONAL_TEXT_DISABLED"? Lets
remove this feature completely, or turn it back on.

That's all my comments. This change to use the directory async. machinery
instead of doing all that sync code in nautilus_get_name is a boon to
mankind and should get into Nautilus as soon as possible.

    -- Darin