Yeah. Definitely Bad. :)

note from the source as to why it was modal to begin with:

	 * I chose to use a modal confirmation dialog here. 
	 * If we used a modeless dialog, we'd have to do work to
	 * make sure that no more than one appears on screen at
	 * a time. That one probably couldn't be parented, because
	 * otherwise you'd get into weird layer-shifting problems
	 * selecting "Empty Trash" from one window when there was
	 * already a modeless "Empty Trash" dialog parented on a 
	 * different window. And if the dialog were not parented, it
	 * might show up in some weird place since window manager
	 * placement is unreliable (i.e., sucks). So modal it is.

Thats a bad solution. A good solution would be to make it so the 
dialog can only be opened once. Choosing to empty the trash shouldn't 
disable use of the rest of the desktop. The complexity of this may 
render this a post gnome2 bug, but the ideal behavior is non-model 
imho.

Created attachment 8466 [details] [review]
Patch to make "Empty Trash?" confirmation dialog non-modal.

The attached patch makes the confirmation dialog non-modal.  It
introduces a new static global variable, confirm_empty_trash_dialog,
which holds the dialog (if open) and is tested to make sure multiple
windows aren't created.

It also makes the dialog window non-resizeable.

Simon, regarding your patch, if you close the parent window before you
dismiss the dialog the program will crash when you do try to empty the
trash (because do_empty_trash() needs the parent view).

It will probably be easiest to simply ensure that the dialog doesn't
outlive its parent window (i.e. destroy it with its parent window).

If it causes a crash I'm removing the PATCH keyword :) Still love to
see a better one, though, Simon.

Created attachment 8570 [details] [review]
Corrected patch to make "Empty Trash?" dialog non-modal

Thanks for finding that, Tomas; I should have caught it myself.

I've attached a new and improved patch, which does these two extra things:

1.) Marks the confirmation dialog to be deleted with its parent, and

2.) Catches the "destroy" signal sent from the dialog to make sure the
dialog pointer is always set to NULL when its widget (or its parent)
is destroyed.

I can't get this one to crash no matter what I try. :)

Alerts should be object modal.
The object here is the trash, which may be accessed in many ways.
This confirmation alert should prevent all interaction with the trash.

http://developer.gnome.org/projects/gup/hig/draft_hig/alert-windows.html

greg shouldn't it be only modal to the desktop/an open trash folder
window and not to all nautilus windows? or am i missing something here???

The mode should prevent other interaction with the trash.
Some examples:
  Dragging items to the trash should be disabled while the alert is
   showing. Attempting to do so should:
    1) Provide "no-drop" feedback when over the trash
    2) probably present (raise into view) the alert window.
  Menu items affecting the trash should be disabled everywhere.
  Maybe (not sure about this) removing items from the trash should
   also be disabled.

Scrolling the view of the trash folder should not be disabled,
that doesn't change anything on the system. (I'm not sure this
is possible now nor the difficulty.)

I'm not sure if opening a view of the trash should be allowed if
there isn't one already. This, and whether removing from trash
should be allowed, probably should be determined by user-testing.
However, there may well be an applicable hueristic of which I'm
unaware. User-testing may have been done for something similar
so some research could eliminate the need to actually do a test.

*** Bug 101690 has been marked as a duplicate of this bug. ***

So, greg, is this a bug in your opinion?

Greg?

The patch still looks like it's going to apply at least. Anyone?

I'm raising the priority to high since a patch is waiting here :)

Now the second hunk fails a line 2207 in
<file://cvs.gnome.org/cvs/gnome/nautilus/libnautilus-private/nautilus-file-operations.c>:

aew@duende:~/Projects/GNOME/CVS/nautilus/libnautilus-private$ patch <
/home/aew/showattachment.cgi
patching file nautilus-file-operations.c
Hunk #1 succeeded at 82 (offset 2 lines).
Hunk #2 FAILED at 2207.
1 out of 2 hunks FAILED -- saving rejects to file
nautilus-file-operations.c.rej

Contents of nautilus-file-operates.c.rej:

***************
*** 2204,2259 ****
        gnome_vfs_uri_list_free (trash_dir_list);
  }
   
- static gboolean
- confirm_empty_trash (GtkWidget *parent_view)
  {
-       GtkDialog *dialog;
-       GtkWindow *parent_window;
-       int response;
   
-       /* Just Say Yes if the preference says not to confirm. */
-       if (!eel_preferences_get_boolean
(NAUTILUS_PREFERENCES_CONFIRM_TRASH)) {
-               return TRUE;
        }
-
-       parent_window = GTK_WINDOW (gtk_widget_get_toplevel
(parent_view));
   
-       dialog = eel_show_yes_no_dialog (
-               _("Are you sure you want to permanently delete "
-                 "all of the items in the Trash?"),
-               _("Empty Trash?"),
-               _("Empty"),
-               GTK_STOCK_CANCEL,
-               parent_window);
   
-       gtk_dialog_set_default_response (dialog, GTK_RESPONSE_CANCEL);
   
-       response = gtk_dialog_run (dialog);
   
-       gtk_object_destroy (GTK_OBJECT (dialog));
   
-       return response == GTK_RESPONSE_YES;
- }
   
  void
  nautilus_file_operations_empty_trash (GtkWidget *parent_view)
  {
        g_return_if_fail (parent_view != NULL);
   
-       /*
-        * I chose to use a modal confirmation dialog here.
-        * If we used a modeless dialog, we'd have to do work to
-        * make sure that no more than one appears on screen at
-        * a time. That one probably couldn't be parented, because
-        * otherwise you'd get into weird layer-shifting problems
-        * selecting "Empty Trash" from one window when there was
-        * already a modeless "Empty Trash" dialog parented on a
-        * different window. And if the dialog were not parented, it
-        * might show up in some weird place since window manager
-        * placement is unreliable (i.e., sucks). So modal it is.
-        */
-       if (confirm_empty_trash (parent_view)) {
                do_empty_trash (parent_view);
        }
  }
   
--- 2207,2285 ----
        gnome_vfs_uri_list_free (trash_dir_list);
  }
   
+ static void
+ confirm_empty_trash_dialog_response_callback (GtkDialog *dialog,
+                                             int response,
+                                             GtkWidget *parent_view)
  {
+       g_assert (dialog == confirm_empty_trash_dialog);
+       g_assert (confirm_empty_trash_dialog != NULL);
   
+       if (response == GTK_RESPONSE_YES) {
+               do_empty_trash(parent_view);
        }
   
+       gtk_widget_destroy (GTK_WIDGET (dialog));
+ }
   
+ static void
+ confirm_empty_trash_dialog_destroy_callback (GtkDialog *dialog,
+                                            gpointer data)
+ {
+       g_assert (dialog == confirm_empty_trash_dialog);
+       g_assert (confirm_empty_trash_dialog != NULL);
   
+       confirm_empty_trash_dialog = NULL;
+ }
   
+ static void
+ confirm_empty_trash (GtkWidget *parent_view)
+ {
+       GtkWindow *parent_window;
   
+       parent_window = GTK_WINDOW (gtk_widget_get_toplevel
(parent_view));
   
+       /* if a (non-modal) "Empty Trash?" confirmation dialog is
already open,
+        * do not create another */
+       if (confirm_empty_trash_dialog != NULL) {
+               gtk_window_set_transient_for (GTK_WINDOW
(confirm_empty_trash_dialog),
+                                             parent_window);
+               gtk_window_present (GTK_WINDOW
(confirm_empty_trash_dialog));
+       } else {
+               confirm_empty_trash_dialog = eel_show_yes_no_dialog (
+                       _("Are you sure you want to permanently delete "
+                         "all of the items in the Trash?"),
+                       _("Empty Trash?"),
+                       _("Empty"),
+                       GTK_STOCK_CANCEL,
+                       parent_window);
+
+               gtk_window_set_destroy_with_parent (GTK_WINDOW
(confirm_empty_trash_dialog), TRUE);
+               gtk_window_set_resizable (GTK_WINDOW
(confirm_empty_trash_dialog), FALSE);
+               gtk_dialog_set_default_response
(confirm_empty_trash_dialog, GTK_RESPONSE_CANCEL);
+
+               g_signal_connect (confirm_empty_trash_dialog,
+                                 "response",
+                                 G_CALLBACK
(confirm_empty_trash_dialog_response_callback),
+                                 parent_view);
+
+               g_signal_connect (confirm_empty_trash_dialog,
+                                 "destroy",
+                                 G_CALLBACK
(confirm_empty_trash_dialog_destroy_callback),
+                                 NULL);
+       }
+ }
+
  void
  nautilus_file_operations_empty_trash (GtkWidget *parent_view)
  {
        g_return_if_fail (parent_view != NULL);
   
+       /* just empty the Trash if the preference says not to confirm */
+       if (!eel_preferences_get_boolean
(NAUTILUS_PREFERENCES_CONFIRM_TRASH)) {
                do_empty_trash (parent_view);
+       } else {
+               confirm_empty_trash (parent_view);
        }
  }
   

As time has passed and I've been looking at other aspects of the
interaction model, I've come to believe that an object-modal alert
as I described is untenable. Such a mode would be like an open
field with randomly placed transparent walls; it would look like
something you could run straight through, but then you'd feel
very foolish upon running into one of the walls. (As one who has
walked into glass patio doors at parties while sober, I can testify
to feeling quite foolish.)

This is not to say that the alert shouldn't be modal. Failing to
prevent user error would be like sending the user running across
a minefield instead of a transparent-wall-field. It seems that this
is one of those few alerts that should in fact be system modal,
except that for now we lack a way to create accessible system modes.
(The screen lock alert and main logout confirmation alert are the
only other system modal alerts. They should be so, but today's
implementation is poor.)

So, I'd say the real bug here is that the alert mode doesn't prevent
enough. If it is fixed, then a new bug report should be opened for the
accessibility problem that will be created, and that report should
depend on whichever report is tracking the general problem of
inaccessible system modes. (Is there one? I don't recall seeing it.)

bug 129443 is interesting if we decide to keep it modal

*** Bug 129443 has been marked as a duplicate of this bug. ***

I discussed this with dave and seth, and we decided to keep the
modality as is. However, I made the dialog transient for the whole
window group instead of the parent window as suggested in the other bug.