Re: [Nautilus-list] continued redhat merge

[Alex Larsson <alexl redhat com>]

On Mon, 17 Sep 2001, Darin Adler wrote:

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

Ok.
I'll go with eel-desktop-file-loader instead then.
 
> 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.

I'll make a pass over it.
 
> 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.

Ok. I'll move it to eel then.
 
> 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.

Using eel_read_entire_file sounds like a good idea.

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

I don't really know this code that well. Is it possible to move it to HEAD 
withouth the other changes without much work?
 
> 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.

I will add such a comment.
 
> What's the status on the code inside "#if ADDITIONAL_TEXT_DISABLED"? Lets
> remove this feature completely, or turn it back on.

It was disabled, because the text stored in the .desktop files was often 
really bad.

Stuff like:
Name=Emacs
Comment=Emacs
just made it look really bad.
 
> 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.

Yeah.

/ Alex