Re: [Nautilus-list] [patch] - hierarchical script menus - still incomplete

[Darin Adler <darin bentspoon com>]

Overall the patch is looking good. Here are some specific comments.

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

> 1) if script directories are moved out of
> the top level script directory (or deleted) then the associated
> callbacks should be removed and the directory object dereferenced.
> ...
> I have put a FIXME in the code to show what needs to be done to fix 1).

I think the current code will already do the right thing, and no 
additional work is needed here except for some testing. If a directory is 
moved or deleted or even renamed, the "files_changed" signal for its 
parent directory will be called. This will eventually cause 
reset_scripts_menu to run, which rebuilds the entire scripts menu.

> 2) I need to set the max menu depth to deal with recursive sym-links.
> ...
> 2 should also be easy enough, but I currently don't know how to do it.

A simple way to set the maximum menu depth would be to add code to 
add_directory_to_scripts_directory_list that uses 
view->details->scripts_directory_length and counts the number of "/" 
characters. The call can then do nothing if there are too many levels to 
the path name.

> +	g_assert (g_list_find(view->details->scripts_directory_list, 
> directory));

Nautilus coding style requires that you use a != NULL in this assert

> -	nautilus_directory_file_monitor_add (view->details->scripts_directory,
> -					     &view->details->scripts_directory,
> +	nautilus_directory_file_monitor_add (directory, &directory,
>  					     FALSE, FALSE, NULL);

&directory is not an acceptable value for the "client" parameter of 
nautilus_directory_file_monitor_add. The requirement for the client 
parameter is that it identify "who is doing the monitoring", and the same 
client value must be passed in to remove the monitor. In this case, 
"&directory" is the address of a parameter on the stack, and won't be 
available next time.

We need a value that's used here, and not in any other code. A good choice 
is probably &view->details->scripts_directory_list, since no other code 
will use that as a client pointer.

> +	gtk_signal_connect (GTK_OBJECT (directory),
> +			    "files_added",
> +			    scripts_added_or_changed_callback,
> +			    view);
> +
> +	gtk_signal_connect (GTK_OBJECT (directory),
> +			    "files_changed",
> +			    scripts_added_or_changed_callback,
> +			    view);

If we want to notice the directory being moved or destroyed, we also need 
to connect to the "changed" handler of the directory's corresponding file.

> +	view->details->scripts_directory_list = NULL;

This line of code is unnecessary.

> +		scripts_directory_uri = 
> nautilus_directory_get_uri(scripts_directory);
> +		view->details->scripts_directory_length = 
> strlen(scripts_directory_uri);

Don't forget the space before parentheses in function calls.

> +add_script_to_script_menus (FMDirectoryView *directory_view,
> +			  NautilusFile *file,
> +			  int index,
> +			  char *menu_path,
> +			  char *popup_path)

These parameters should be const char *, not just char *.

> +	g_assert(nautilus_file_is_directory(file));

Don't forget the space before the parenthesis. Also, an assert for this 
could be a problem. If a directory is removed and replaced by a file, we 
might get a core dump. So perhaps this code should just return and do 
nothing if the file is not a directory.

> +	uri = nautilus_file_get_uri (file);
> +	directory = nautilus_directory_get (uri);
> +	g_free (uri);

Ideally we should add a nautilus_file_get_corresponding_directory call 
instead of doing it by hand like this.

> +	if (!g_list_find(view->details->scripts_directory_list, directory)) {

Nautilus code uses g_list_find == NULL rather than !g_list_find.

> +static void
> +disconnect_scripts_directory_handler (FMDirectoryView *view, 
> NautilusDirectory *directory);

Nautilus code puts all forward declarations at the top of the file, not in 
the middle like this.

> +/* FIXME: Hook this callback up... */
> +static void
> +remove_directory_from_scripts_directory_list(FMDirectoryView *view, 
> NautilusDirectory *directory)
> +{
> +	g_assert(g_list_find(view->details->scripts_directory_list, 
> directory));
> +
> +	g_list_remove(view->details->scripts_directory_list, directory);
> +	disconnect_scripts_directory_handler(view, directory);
> +	nautilus_directory_unref (directory);
> +}

I don't think you'll need the above function at all.

> if (!all_files) {

Nautilus code uses "all_files == NULL" for this.

> +			add_directory_to_scripts_directory_list(view, file);

Don't forget the space before the parenthesis.

> +	g_assert (g_list_find(view->details->scripts_directory_list, 
> directory));

Nautilus code uses g_list_find != NULL.

> +	gtk_signal_disconnect_by_func(GTK_OBJECT (directory), 
> scripts_added_or_changed_callback, directory);

Don't forget the space before the parenthesis.

> +	nautilus_directory_file_monitor_remove (directory, &directory);

This call won't successfully remove the monitor, because the client 
pointer is just a pointer to the directory parameter on the stack. It 
should be changed to match what we used for monitor_add above.

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

Since you're very close, I'm sad to hear that you don't have more time for 
this right now.

    -- Darin