After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 778200 - sensitivity of save buttons
sensitivity of save buttons
Status: RESOLVED FIXED
Product: recipes
Classification: Other
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Recipes maintainer(s)
Recipes maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2017-02-05 11:38 UTC by Matthias Clasen
Modified: 2017-03-11 03:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Make save button sensitive or otherwise as needed (18.14 KB, patch)
2017-03-09 22:47 UTC, Ekta Nandwani
none Details | Review
Make save button sensitive or otherwise as needed (18.31 KB, patch)
2017-03-10 13:02 UTC, Ekta Nandwani
committed Details | Review
Make save sensitive with change in ingredients' list name (1.32 KB, patch)
2017-03-10 22:06 UTC, Ekta Nandwani
committed Details | Review

Description Matthias Clasen 2017-02-05 11:38:36 UTC
the save buttons on the edit page and the chef dialog should only turn blue once the content has been changed
Comment 1 Matthias Clasen 2017-02-08 03:06:44 UTC
I've done this for the chef dialog now. The edit page still needs to be done.
Comment 2 Ekta Nandwani 2017-03-09 22:47:25 UTC
Created attachment 347586 [details] [review]
Make save button sensitive or otherwise as needed
Comment 3 Matthias Clasen 2017-03-10 04:00:29 UTC
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.
Comment 4 Ekta Nandwani 2017-03-10 13:02:09 UTC
Created attachment 347628 [details] [review]
Make save button sensitive or otherwise as needed

Patch with formatted code and other tiny tweaks needed
Comment 5 Matthias Clasen 2017-03-10 18:52:44 UTC
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...
Comment 6 Matthias Clasen 2017-03-10 18:54:25 UTC
(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.
Comment 7 Matthias Clasen 2017-03-10 19:20:00 UTC
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.
Comment 8 Ekta Nandwani 2017-03-10 22:06:06 UTC
Created attachment 347670 [details] [review]
Make save sensitive with change in ingredients' list name