GNOME Bugzilla – Bug 772514
Dialog: use suggested-action (blue buttons) and destructive-action (red buttons) when necessary
Last modified: 2016-10-14 00:24:19 UTC
Created attachment 337075 [details] [review] patch proposal v1 While investigating bug 747627, I discovered that all Geary's popup buttons are standard buttons, even destructive ones (delete mail, etc.). If that patch is merged (I hope so!), the next step would probably be to remove ErrorDialog and use embedded notification instead (as gnome-boxes does)?
Created attachment 337076 [details] gnome-boxes notificationbar popup mecanism
Review of attachment 337075 [details] [review]: Hey, this is a nice bit of polish. Ta. I'd be pretty happy to accept the dialog suggested-action parts as-is, but I'd want to review and commit the text changes such as "They will be lost definitively" and the Close/Save button in a separate patch. I would pretty happy to change the Save/Close text ordering, but the other additions are somewhat redundant with the existing text. Since people tend to not read dialog text, I'd me more inclined to leave the other texts alone, especially if the translations will change yet again when we do convert to in-app notification like Boxes. PS: patch subject/description is missing the bug number, too.
Created attachment 337165 [details] [review] patch proposal v2 OK, I'm fine with leaving the extra sentence, please find v2. However I kept one dialog string change: > - _("Do you want to discard this message?"), null, Stock._DISCARD); > + _("This message cannot be saved. Do you want to discard it definitively?"), null, Stock. _DISCARD, "destructive-action"); Because in my opinion it is not obvious that "Close and save" can fail when clicking on it. Is this fine or should I still remove it as well?
Review of attachment 337165 [details] [review]: Yeah, I'm mostly happy with this. Using "permanently" would be better than "definitively" however it will match other existing text (e.g. when deleting a message), and definitive has a slightly different meaning in English - more like "authoritative" than "not being able to be undone".
Created attachment 337177 [details] [review] patch proposal v3 OK, modified!
Review of attachment 337177 [details] [review]: Err, but the text is still the same? Also, I spotted another issue. ::: src/client/composer/composer-widget.vala @@ +1171,3 @@ if (try_to_save) { + dialog = new ConfirmationDialog(container.top_window, + _("Do you want to discard this message?"), null, Stock._DISCARD, "destructive-action"); Oh, also, what what was the reason behind this change? If we can try to save we should give the user the option, however this removes that option. @@ +1174,3 @@ } else { dialog = new ConfirmationDialog(container.top_window, + _("This message cannot be saved. Do you want to discard it definitively?"), null, Stock._DISCARD, "destructive-action"); Still says "definitively" rather than "permanently" --^
Created attachment 337251 [details] [review] patch proposal v4 > Oh, also, what what was the reason behind this change? If we can try to save we should give the user the option, however this removes that option. Since user clicked the "Close and Delete" button, I do not expect a "Save" button here otherwise user should have click the other button. Basically the "Keep" label is not very clear: what does "keep" exactly mean? (obviously it means "save it to the draft", but that is not explained to the user). I think having only a "Cancel" button is clearer for the user: if he wants to save his draft, he must use the "save and close" button instead. > Still says "definitively" rather than "permanently" --^ Oops, I reuploaded the same patch; wrong directory sorry.
Also you may reach the popup when closing the composer; so in that case it might be problematic. I think this should be handled differently here (eg "By closing the composer you will lose your draft. Do you want to save it?" and "Save" / "Discard" / "Cancel" buttons are fine here). I really dislike the "Keep" label actually.
(In reply to Gautier Pelloux-Prayer from comment #8) > Also you may reach the popup when closing the composer; so in that case it > might be problematic. Yep, that's the case I had in mind - I didn't realise it was also invoked for Close and Delete, however. So there's a number of actions that the user could choose: - Close and Discard - Close and Save - Close And for all three we also have the possibility of saving the draft, or not. Ideally, we should just perform the action and let the user undo it if they made the mistake, but all of this is getting beyond the scope of this particular issue, which is about adding hints to the dialog buttons. So, I'd suggest the following: - Keep this bug and hence patch just about the action hints, and otherwise leave the type, text and buttons as-is. - Open a new bug for reviewing the design of how this works, so we can address the issues above properly. That may include switching to an in-app notification a'la Boxes, or just tweaking what's in place for now.
Created attachment 337295 [details] [review] patch proposal v5 Seems good indeed, will do that.
Review of attachment 337295 [details] [review]: I just noticed the Ternary dialog's hint is wrong. There's maybe one or two small tweaks as well. ::: src/client/application/geary-controller.vala @@ -956,1 @@ if (security_dialog.run() != Gtk.ResponseType.OK) Even though they have no other choice we don't want to suggest they should continue to not use TLS. Destructive action might be more appropriate, but maybe over the top. Otherwise just leave it unhinted. ::: src/client/composer/composer-widget.vala @@ +1171,3 @@ if (try_to_save) { dialog = new TernaryConfirmationDialog(container.top_window, + _("Do you want to discard this message?"), null, Stock._KEEP, Stock._DISCARD, Gtk.ResponseType.CLOSE, "destructive-action"); This label applies to keep, so should be a suggested action instead, right? @@ +1177,1 @@ } This string change should be reverted as well.
Created attachment 337563 [details] [review] patch proposal v6 OK I though I had to keep the last text change, my bad. I'll make these changes for bug https://bugzilla.gnome.org/show_bug.cgi?id=747627 instead!
Review of attachment 337563 [details] [review]: This patch has some training whitespace, but it's probably not worth going round yet again. Committed as 2b77986 to master. Ta!