Created attachment 22217 [details] [review]
proposed patch

I noticed that the above patch is not good for translators since I
used the markup in the translatable strings.

Here is an updated patch: since gedit already depends on eel I used
the recently introduced eel_alert_dialog.

Created attachment 22617 [details] [review]
updated patch

Thanks for the patch.
I have just committed it to CVS HEAD.

<paolo> La stringa per la "close-confirmation dialog" forse contiene
un errore di inglese, l'ho notato ieri facendo il commit della tua patch
<pbor> dove? Cmq c'e' un paragrafo del' HIG che contiene la frase
esatta da usare
<paolo> Si dice: "to the document "Untitled 1"" o "to document
"Untitled 1"" ?
<pbor> con il "the" mi suonava bene, ma non sono sicuro al 100%
<pbor> cmq qui c'e' l'hig :)
http://developer.gnome.org/projects/gup/hig/draft_hig_new/windows-alert.html#alerts-confirmation
<pbor> hai ragione, niente "the"
<paolo> hmmm... potremmo rendere la confirmation dialog più HIG
compliant ancora :-)
<paolo> non dovrebbe essere difficile... hai voglia?
<paolo> Nel cose si stia chiudendo gedit potremmo usare "Quit without
saving"? Cosa ne pensi
<pbor> ok
<paolo> A dire il vero nel caso di "Close All" e "Quit" mi piacerebbe
avere una dialog completamente nuova con la lista di tutti i documenti
da salvare
<paolo> al posto di dover rispondere file per file
<pbor> "The following documents were not saved"
<pbor> lista
<pbor> "Save all" come bottone?
<paolo> qualcosa del genere, in cui e' possibile selzionare i
documenti da salvare come in JBuilder
<paolo> ma un po' più gnome-like
<pbor> una treeview con check buttons?
<paolo> esatto
* pbor provera'
<paolo> con un bottone per selezionare o deselezionare tutti
<pbor> tanto ogni scusa e' buona per non studiare ;)
<paolo> ti faccio uno screen-shot di JBuilder?
* paolo dovrebbe lavorare, ma anche per lui ogni scusa e' buona :-)
<pbor> ok grazie, allegalo al bugreport della vecchia patch
eventualmente riaprendo il bug
<paolo> ok
<paolo> attacco anche il log IRC
<pbor> ok

Created attachment 22772 [details]
JBuilder screenshot

See also bug #130237

still not done (sorry)... in the mean time it would be ok to factor
out a gedit_document_get_deleted() function like in the following
patch, since it should be used also in the yet-to-be-written multi-doc
dialog?

While at it I'd like also to ask why the logic for a readonly file is
different... I'm probably missing something... Even if it was
readonly, what happens if the file was removed anyway?


-		if (gedit_document_is_readonly (doc))
-			deleted = FALSE;
-		else
-			deleted = !gedit_utils_uri_exists (raw_uri);

Created attachment 23015 [details] [review]
factor out gedit_document_get_deleted

I have done a preliminary version of the dialog... but the hard thing
is hookin it up at the right place doing the right thing.

Btw, is quitting the only way to trigger this? Was there a "close all"
menu/keyboard shortcut before?

Is it only my impression or are the codepaths for quitting pressing
the "X" button and quitting using the "Quit" menu separate? Why?

Anyway here is a preliminary screenshot: note I didn't include a
general toggle button as IMHO is not necessary since all files
defaults to "save" and there is a "Save None" button...

Created attachment 23018 [details]
sshot

Please, commit your last patch.

Well, I can think about a couple of reson why in the current code the
logic for readonly files is different, but they are all due to the
weird way gedit manages them.
Since I will rewrite the way gedit manages readonly files in the next
major release (2.8), I think we should not change the current code for
the moment.

Created attachment 23019 [details]
Close All dialog mockup

> is quitting the only way to trigger this? Was there a "close all"
> menu/keyboard shortcut before?

No, you can trigger it by using View->Close All or by closing a single
gedit toplevel window. In the last case you will only close the
document managed by the window you are closing.

> Is it only my impression or are the codepaths for quitting pressing
> the "X" button and quitting using the "Quit" menu separate? Why?

Yes, they are different due to the way bonobo-mdi works.

Attached is another irc log discussing details of both the
implementation and the UI.

This time in english for the happiness of the audience :-)


I'm also CC'ing usability paeople to get opinins both on the whole
concept of the new dialog and (in the case it's ok) on the details.

Created attachment 23020 [details]
discussion on ui and implementation

Created attachment 23021 [details]
Glade2 file of the mockup

attaching a quick and dirty patch to show a proposed api... it still
uses my dialog instead of the proper libgladified dialog, puts the
declarations in gedit-dialogs.h instead of a proper .h file and saving
doesn't work.

The dialog is hooked up in gedit_file_close_all just to trigger it for
display, not because it's the right place.

Created attachment 23061 [details]
to put in gedit/dialogs

doh not a good day... wrong file...
right file and glue patch follow

Created attachment 23062 [details]
right file hopefully

Created attachment 23063 [details] [review]
some glue

Here it is an updated patch. This time it uses libglade, toggling in
the "save?" column works and it actually saves files.

Note that the glade file is slightly simpler than the one by Paolo
(e.g.  the string doesn't contain the number of unsaved files) to make
my life easier. It can be tweaked later.

I also added a gedit_mdi_get_unsaved_children function.

The .c , .glade and patch follow. as above the patch contains some
cruft just intended to test the dialog using close all.

Created attachment 23097 [details]
.c file to put in dialogs

Created attachment 23098 [details] [review]
glade file

Created attachment 23099 [details] [review]
glue patch

Hi Paolo,
 thanks for the patch.

I have some concerns about the interface you are using since it adds a
circular dependency (dialog <--> GeditMDI).

I'd really prefer to have a dependency graph like GeditMDI <-- dialog.
This means that the dialog should not know anything about GeditMDI.

To do this I still think that the best interface is the one I proposed
on IRC a few days ago.

GtkWidget *gedit_close_confirmation_dialog_new (const GSList
*usaved_docs);
gboolean gedit_close_confirmation_dialog_run (GtkWidget *dialog);
GSList *gedit_close_confirmation_dialog_get_selected_documents
(GtkWidget *dialog);

Where gedit_close_confirmation_dialog_run is only an utility function
that returns TRUE if "Save" or "Save None" is pressed and FALSE if
"Cancel" is pressed.

So the normal use case will be:

dialog = gedit_close_confirmation_dialog_new (usaved_docs);
save = gedit_close_confirmation_dialog_run (dialog);

if (save)
{
  list = gedit_close_confirmation_dialog_get_selected_documents (diallog);
  while (list != NULL)
  {
     save (list->data);
  }
}

gtk_widget_detroy (dialog);


Ciao,
Paolo 

I was looking at removung the dep on gedit-mdi.h

Removing it from close_all_confirmation_dialog is simply done (and for
what is worth is orthogonal to the interface), the problem comes from
the following code in the confirmation dialog for a single document:

	view = GTK_WIDGET (g_list_nth_data (bonobo_mdi_child_get_views
(BONOBO_MDI_CHILD (child)), 0));
	if (view)
	{
		GtkWindow *window;

		window = GTK_WINDOW (bonobo_mdi_get_window_from_view (view));
		gtk_window_present (window);
			
		bonobo_mdi_set_active_view (BONOBO_MDI (mdi), view);
	}

passing only the GeditDocument to the function, how can I retrieve the
child/view/window?

In particular note that the single-doc dialog may be called from
inside the multi-doc dialog function, so I can't simply add a "view"
argument, because when calling it from the multi-doc dialog (which
only knows a list of unsaved GeditDocuments) I can't know the view
corresponding to the unsaved doc.


Probably I'm missing something supersimple... I should not fill
bugzilla stuff after midnight :) bear with me...

This piece of code well be used by the caller, not by the dialog
itself, so it is not a problem.
It was useful to change the current active window before displaying
the dialog so that the user could see document document she is closing.
It the new code it will probably unuseful and I will probably remove it.

Fixed in CVS HEAD.

Usability guys: could you please review the new close all dialog?

You can see it at
http://www.gnome.org/~paolo/screenshots/close_all_dlg.png

Thanks.

We discussed a bit on this dialog during ui-review, the log follows.

The discussion was in particular about:

* what happens when you have more than 1 file never saved? How does
the user know which of the unsaved docs he is Saving As?

* the buttons: maybe "Discard All" (and to a less extent "Save
Selected") are clearer

bottom line: we still need review from the "experts".

Created attachment 23288 [details]
ui review irc log

pbor: Any concrete plans about this bug? Can we close it?

We didn't receive any complaints with the new close confirmation dialog afaik,
so I think this can be closed.

Umh... I noticed now that we still use "definitively lost": I'm pretty sure that
we discussed with some english speaker that "permanently lost" was a lot better,
but we missed the string freeze for 2.6.

I'm changing it now before we miss the freeze again, I hope that's ok.

That sounds fine, thanks for catching that.