GNOME Bugzilla – Bug 778200
sensitivity of save buttons
Last modified: 2017-03-11 03:30:52 UTC
the save buttons on the edit page and the chef dialog should only turn blue once the content has been changed
I've done this for the chef dialog now. The edit page still needs to be done.
Created attachment 347586 [details] [review] Make save button sensitive or otherwise as needed
Review of attachment 347586 [details] [review]: You did quite a bit of work here, impressive. I've found only three places in the edit page that didn't turn the button blue as I would have expected: 1) changing the title of an ingredient list 2) removing an entire ingredient list 3) removing an ingredient My remaining review comments are mainly about formatting trivia. ::: src/gr-edit-page.c @@ +168,1 @@ I strongly prefer to have a space before an opening ( I might also suggest to name the function in a way that makes it clearer that it actually changes a flag. Maybe: mark_as_unsaved() or set_unsaved() ? @@ +239,3 @@ if (page->recipe) g_object_set (page->recipe, "default-image", index, NULL); + has_unsaved_changes(page); Also when calling functions @@ +1363,3 @@ + { + has_unsaved_changes(page); + } I don't like comparing to TRUE (that's the logician in me coming through). But I also don't understand the point of the comparison - GDK_EVENT_PROPAGATE is a fixed value - it is either always == TRUE or always != TRUE. Why do we need to call has_unsaved_changes here ? Also, our indentation style put the opening { on the same line as the if, and 8 space indent, please. @@ +1440,3 @@ + switch (prop_id) { + case PROP_UNSAVED: + self->unsaved=g_value_get_boolean(value); I would prefer spaces in this line: self->unsaved = g_value_get_boolean (value); @@ +1459,3 @@ + switch (prop_id) { + case PROP_UNSAVED: + g_value_set_boolean(value,self->unsaved); Same here: g_value_set_boolean (value, self->unsaved); @@ +1473,3 @@ + g_object_set(G_OBJECT(page),"unsaved",TRUE,NULL); + g_object_notify (G_OBJECT(page) ,"unsaved"); +} I didn't explain this, but if you use g_object_set(), the notification comes for free - g_object_set already does it for you, so you don't need to call g_object_notify() yourself here. And again, some formatting requests: the opening { of a function body always goes on a new line, and use spaces generously. ::: src/gr-window.c @@ +125,3 @@ + gtk_widget_set_sensitive(window->save_button,FALSE); + } +} Same formatting trivia complaints as before: - put function parameters on their own lines each and line them up nicely - spaces before ( and after , - { on the same line as the if - 8 space indents - just use: if (unsaved) { ... } else { } unsaved is a boolean, so it can only be TRUE or FALSE. no need for the second if check here. In fact, you can write this much more concisely as gtk_widget_set_sensitive (window->save_button, unsaved); no need for an if-else at all @@ +1345,3 @@ } + + Try to avoid redundant whitespace changes like these extra newlines at the end of the file - they just tend to clutter up the diffs and hide the actual changes. ::: src/gr-window.ui @@ +128,3 @@ <property name="visible">1</property> <property name="use-underline">1</property> + <property name="sensitive">0</property> Not sure we really need this change, now that we set the sensitivity of the button explicitly to FALSE in the signal handler, but it doesn't hurt.
Created attachment 347628 [details] [review] Make save button sensitive or otherwise as needed Patch with formatted code and other tiny tweaks needed
Almost perfect now. I don't think the few remaining formatting details are worth holding this patch up over. Two minor issues I noticed: When dealing with ingredients lists, the button goes blue as soon as I move the keyboard focus to one of the list title entries. It should probably stay gray until I actually make a change in the entry. On the new recipe page, the save button is initially gray, but I'm fine with that. It works out ok. What is not so great is that for some reason, the image viewer now shows the previous/next overlay buttons initially, even though there is nothing to switch to. That is probably not your fault, or only indirectly, since you didn't actually touch the image viewer code...
(In reply to Matthias Clasen from comment #5) > On the new recipe page, the save button is initially gray, but I'm fine with > that. It works out ok. What is not so great is that for some reason, the > image viewer now shows the previous/next overlay buttons initially, even > though there is nothing to switch to. That is probably not your fault, or > only indirectly, since you didn't actually touch the image viewer code... ...and now that I'm trying again, I can't reproduce this. So: nevermind. And lets treat the ingredients list title issue as a follow-up fix. I'll merge this now.
Lets use this bug for the follow-up fix of the ingredients titles as well. It turns out that the problem is caused by GSpell emitting GtkEntry::changed a bunch for its own purposes, thereby breaking it for our purpose. As a workaround, we can use notify::label instead.
Created attachment 347670 [details] [review] Make save sensitive with change in ingredients' list name