GNOME Bugzilla – Bug 592997
Confirmation needed when there are unapplied changes on an account
Last modified: 2009-08-31 14:08:59 UTC
When there are changes not applied in the account dialog, and the dialog is closed or another account is selected, a confirmation dialog should popup otherwise changes are lost. Maybe we could have also a dialog that ask to reconnect modified accounts so the new settings are also applied on the current connection. For example when SSL setting is changed, it is important to reconnect the account...
Marking this bug as a 2.28 as people who are not used of the new interface could be confused by this.
> Maybe we could have also a dialog that ask to reconnect modified accounts so > the new settings are also applied on the current connection. For example when > SSL setting is changed, it is important to reconnect the account... The reconnection is already being done automatically and has been improved in Bug 593158.
Created attachment 141870 [details] [review] Branch containing the proposed fix: http://git.collabora.co.uk/?p=user/jtellier/empathy.git;a=shortlog;h=refs/heads/confirm-lose-accounts-settings libempathy-gtk/empathy-account-widget.c | 52 ++++--- libempathy-gtk/empathy-account-widget.h | 5 + libempathy-gtk/empathy-ui-utils.c | 28 ++++ libempathy-gtk/empathy-ui-utils.h | 6 + src/empathy-accounts-dialog.c | 244 +++++++++++++++++++++++++++++-- 5 files changed, 299 insertions(+), 36 deletions(-)
Comment on attachment 141870 [details] [review] Branch containing the proposed fix: http://git.collabora.co.uk/?p=user/jtellier/empathy.git;a=shortlog;h=refs/heads/confirm-lose-accounts-settings Some comments on the branch: >- g_signal_connect (G_OBJECT (priv->enabled_checkbox), "toggled", >- G_CALLBACK (account_widget_enabled_toggled_cb), self); >+ g_signal_connect (G_OBJECT (priv->enabled_checkbox), "released", >+ G_CALLBACK (account_widget_enabled_released_cb), self); > } Why is this needed? >+void empathy_show_yes_no_question_dialog (GtkWindow *parent, >+ gchar *message, These dialogs do not have the right look; you should build the message dialog with two message, and set the second with gtk_message_dialog_format_secondary_text(). This way, the primary becomes bold like in the dialog that comes up when you try to remove an account. Also, IMHO the buttons labels (and icons accordingly) should be changed to a more intuitive metaphor, like "Discard" and "Cancel" or "Continue" "Back", as the text in the dialog is long and "Yes" and "No" do not have any immediate meaning without reading the whole text. >+static gboolean >+accounts_dialog_has_pending_change (EmpathyAccountsDialog *dialog, >+ model = gtk_tree_view_get_model (GTK_TREE_VIEW (priv->treeview)); >+ settings = accounts_dialog_model_get_selected_settings (dialog); >+ >+ if (accounts_dialog_get_settings_iter (dialog, settings, &iter)) >+ gtk_tree_model_get (model, &iter, COL_ACCOUNT_POINTER, account, -1); Couldn't you just call gtk_tree_selection_get_selected (gtk_tree_view_get_selection(), NULL, &iter) to get the right tree iter instead? >+ message = g_strdup_printf ( >+ _("There are unsaved modification regarding your %s account.\n" >+ "You are about to create a new account, which will discard\n" >+ "your changes. Are you sure you want to proceed?"), >+ empathy_account_get_display_name (account)); See the comment about the dialogs: the phrase before the first "\n" should probably be the primary message. >+ if (gtk_tree_model_get_iter_from_string (model, >+ &iter, priv->destination_path)) >+ priv->destination_path = gtk_tree_path_to_string (path); It's probably better to use a GtkTreeRowReference instead of a string to save the destination path.
Created attachment 141937 [details] [review] Updated patch >>- g_signal_connect (G_OBJECT (priv->enabled_checkbox), "toggled", >>- G_CALLBACK (account_widget_enabled_toggled_cb), self); >>+ g_signal_connect (G_OBJECT (priv->enabled_checkbox), "released", >>+ G_CALLBACK (account_widget_enabled_released_cb), self); >> } > >Why is this needed? When the state of the "Enabled" checkbox was changed (with a "toggled" event), a change was registered by the EmpathyAccountWidget and it would then report that the account contained pending changes. The problem is that when an account gets enabled or disabled, the "Enabled" checkbox gets automatically checked or uncheck without any intervention of the user. A user could then have been warned that changes were about to be lost, even though he had made no changes to the account. By using the "released" signal we can make sure that the modification was done by the user. I've corrected the other points you've mentioned. libempathy-gtk/empathy-account-widget.c | 52 +++-- libempathy-gtk/empathy-account-widget.h | 5 + src/empathy-accounts-dialog.c | 324 +++++++++++++++++++++++++++---- 3 files changed, 321 insertions(+), 60 deletions(-)
Comment on attachment 141937 [details] [review] Updated patch >+ gchar *question_dialog_primary_text = g_strdup_printf ( >+ PENDING_CHANGES_QUESTION_PRIMARY_TEXT, >+ empathy_account_get_display_name (account)); PENDING_CHANGES_QUESTION_PRIMARY_TEXT should be marked for translation. >+ accounts_dialog_show_question_dialog (dialog, question_dialog_primary_text, >+ "You are about to create a new account, which will discard\n" >+ "your changes. Are you sure you want to proceed?", This should also be marked for translation. Also, something's wrong with the indentation of this whole block. >+ question_dialog_primary_text = g_strdup_printf ( >+ PENDING_CHANGES_QUESTION_PRIMARY_TEXT, Should be marked for translation. >+ accounts_dialog_show_question_dialog (dialog, question_dialog_primary_text, >+ "You are about to select another account, which will discard\n" >+ "your changes. Are you sure you want to proceed?", Ditto. >+static void > accounts_dialog_response_cb (GtkWidget *widget, >+ question_dialog_primary_text = g_strdup_printf ( >+ PENDING_CHANGES_QUESTION_PRIMARY_TEXT, >+ empathy_account_get_display_name (account)); >+ >+ accounts_dialog_show_question_dialog (dialog, question_dialog_primary_text, >+ "You are about to close the window, which will discard\n" >+ "your changes. Are you sure you want to proceed?", Same here.
Created attachment 141955 [details] [review] Fixed patch libempathy-gtk/empathy-account-widget.c | 52 +++-- libempathy-gtk/empathy-account-widget.h | 5 + src/empathy-accounts-dialog.c | 335 +++++++++++++++++++++++++++---- 3 files changed, 330 insertions(+), 62 deletions(-)
Comment on attachment 141955 [details] [review] Fixed patch Looks good to commit after we get the necessary acks for the string freeze break.
Comment on attachment 141955 [details] [review] Fixed patch Merged to master after i18n approval.