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

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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]