GNOME Bugzilla – Bug 658060
Improve user interaction during web application creation
Last modified: 2011-09-05 20:38:51 UTC
Two things are currently not handled nicely: - If a web application name is already used, the user is not warned and bad things happen. - If the web application saving process fails, there are no messages to the user. The two patches below fix this.
Created attachment 195484 [details] [review] Ask the user for confirmation before overwriting web applications Show a confirmation dialog and, in case the user confirms, delete the old application before saving a new one.
Created attachment 195485 [details] [review] Display a notification if the web application saving fails
Review of attachment 195484 [details] [review]: Looks good to my not so demanding eyes. ::: embed/ephy-web-view.c @@ +49,3 @@ #include "ephy-settings.h" #include "ephy-string.h" +#include "ephy-web-app-utils.h" used?
Review of attachment 195485 [details] [review]: Shouldn't this warning be inside Epiphany instead of in the shell? We are notifying of success in the shell because the web-app is a new application to all effects, but if it failed to be created I think that "conceptually" the action never left Epiphany's domain.
Review of attachment 195484 [details] [review]: You can commit if you at least change the Yes/No thing :) ::: embed/ephy-web-view.c @@ +49,3 @@ #include "ephy-settings.h" #include "ephy-string.h" +#include "ephy-web-app-utils.h" As Diego said this seems unused. ::: src/window-commands.c @@ +528,3 @@ + dialog = gtk_message_dialog_new (parent, 0, + GTK_MESSAGE_QUESTION, + GTK_BUTTONS_YES_NO, Yes/No dialogs are evil. Let's do Overwrite/Cancel ? @@ +560,3 @@ + ephy_delete_web_application (gtk_entry_get_text (GTK_ENTRY (data->entry))); + } else { + g_free (profile_dir); Nitpick: you can save the return value of g_file_test and free the string right after.
Review of attachment 195485 [details] [review]: I think it's fine to use a shell notification in both cases, to keep symmetry.
(In reply to comment #3) > Review of attachment 195484 [details] [review]: > > Looks good to my not so demanding eyes. > > ::: embed/ephy-web-view.c > @@ +49,3 @@ > #include "ephy-settings.h" > #include "ephy-string.h" > +#include "ephy-web-app-utils.h" > > used? Now it is :)
Thank you both for reviewing! Attachment 195484 [details] pushed as 7e16cb6 - Ask the user for confirmation before overwriting web applications Attachment 195485 [details] pushed as aada5fc - Display a notification if the web application saving fails