GNOME Bugzilla – Bug 771356
Delete actions button should have 'destructive-action' style class
Last modified: 2017-03-07 09:16:12 UTC
Created attachment 335436 [details] Nautilus Shift Delete confirmation (image) Delte buttons in confirmation dialog don't have any style classes. Adding 'destructive-action' would be nice.
good idea
Here's an example of how this can be implemented: https://git.gnome.org/browse/gnome-control-center/tree/panels/privacy/cc-privacy-panel.c#n1035
Created attachment 346271 [details] [review] Use 'destructive-action' style class Proposal of patch from GNOME hackaton @FIT.
Created attachment 346272 [details] [review] destructive-action: change button style when permanently deleting file In this commit we changed color of button in confirmation dialog when user permanently deletinf file. The problem is that native dialog hasn't any color warning on buttons which are dangerous with no way back. So we changed a color of delete button to inform users that it is dangerous one and it is on his own risk to click on it.
Review of attachment 346272 [details] [review]: There is a problem with deleting file. After shift+del it only adds a file to a pending list!
Created attachment 346280 [details] [review] destructive-action: change button style when permanently deleting file In this commit we changed color of button in confirmation dialog when user permanently deletinf file. The problem is that native dialog hasn't any color warning on buttons which are dangerous with no way back. So we changed a color of delete button to inform users that it is dangerous one and it is on his own risk to click on it.
Review of attachment 346271 [details] [review]: The code looks good. The commit message needs some work, as I explained in the hackathon and how is explained in here: https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines You can point out that this is mentioned in the HIG (Human Interface Guidelines) for buttons and dialogs in: https://developer.gnome.org/hig/stable/buttons.html.en (that's why the change)
Review of attachment 346280 [details] [review]: Hey good work on the patch! The code needs work, but the commit message is good. See the code review: ::: src/nautilus-file-operations.c @@ +1484,3 @@ + NULL); + + 0, it's better to use the API of GtkDialog rather than g_object_set, it's clearer. @@ +1485,3 @@ + + g_object_set(dialog, "text", primary_text, "secondary-text", secondary_text, NULL); + GTK_MESSAGE_WARNING, missing space after commas @@ +1486,3 @@ + g_object_set(dialog, "text", primary_text, "secondary-text", secondary_text, NULL); + gtk_dialog_add_button(GTK_DIALOG(dialog),_("_Cancel"),GTK_RESPONSE_CANCEL); + GTK_MESSAGE_WARNING, Here we are in a generic "run_warning" function, but this code does something more specific than that. We should either change the function name or make the code more generic. Since we already have other code that deals with this situation (the run_simple_dialog_va) we should modify that instead. @@ +1489,3 @@ + + gtk_dialog_set_default_response(GTK_DIALOG(dialog),FALSE); + NULL); you can actually get this button from the gtk_dialog_add_button call as a return value @@ +1497,2 @@ va_end (varargs); + return res == GTK_RESPONSE_OK; What's returned it's not a boolean, but the actual response_id, so you need to return "res" as before. @@ +1708,3 @@ NULL); + spurious style change
Created attachment 346367 [details] [review] Use 'destructive-action' style class for delete button. Edited commit message.
Review of attachment 346367 [details] [review]: Hey! Commit message is now good, just a small code change which I didn't realized before: ::: src/nautilus-file-operations.c @@ +1318,3 @@ gtk_dialog_set_default_response (GTK_DIALOG (dialog), response_id); + + if (!g_strcmp0(button_title, DELETE)) Just realized this thanks to Sadiq, can you use != 0? Here you are using a logic operand with a rational value, which is wrong.
(In reply to Carlos Soriano from comment #10) > Review of attachment 346367 [details] [review] [review]: > > Hey! Commit message is now good, just a small code change which I didn't > realized before: > > ::: src/nautilus-file-operations.c > @@ +1318,3 @@ > gtk_dialog_set_default_response (GTK_DIALOG (dialog), response_id); > + > + if (!g_strcmp0(button_title, DELETE)) > > Just realized this thanks to Sadiq, can you use != 0? Here you are using a > logic operand with a rational value, which is wrong. g_strcmp0(str1, str2) returns 0 if str1 == str2. https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strcmp0 I could use == 0, if you find that more readable, but technically it should be the same.
> > I could use == 0, if you find that more readable, but technically it should > be the same. Yes, it's about the correct semantics
Created attachment 346624 [details] [review] Use 'destructive-action' style class for delete button. fixed '!' -> == 0
Review of attachment 346624 [details] [review]: Now it looks good! Congrats for your first contribution!
Thank you, guys. I gained so many experiences through this bug fix. I appreciate a code review you gave me. I think I will continue in these contributions to open source. Thanks!
(In reply to Marek Tamaškovič from comment #15) > Thank you, guys. I gained so many experiences through this bug fix. I > appreciate a code review you gave me. I think I will continue in these > contributions to open source. > > Thanks! Really happy to hear that!