GNOME Bugzilla – Bug 742521
Put the panel switches in a list
Last modified: 2015-01-20 14:20:24 UTC
The "Show Pop Up Banners" and "Show in Lock Screen" labels are quite a long way from the corresponding switches. Placing them in a list would make the UI easier to read: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/notifications/notifications-wireframes.png
Created attachment 294675 [details] [review] Move switches to a GtkListBox This patch moves the switches on the notifications panel to a GtkListBox.
Created attachment 294676 [details] [review] Add label Applications This patch adds the Application label from the mockup.
Created attachment 294677 [details] [review] Change switch labels This patch changes labels of the switches on the notifications panel as done in the mockup.
Created attachment 294678 [details] [review] Place switches into GtkListBox This patch rewrites the edit dialog of notifications panel so that the switches are in a GtkListBox and have the behaviour written on the mockup. It uses ui file now. You need to have the patch from the bug #742520 applied before these.
Created attachment 294685 [details] screenshots of the result
I should mention that the switches which are off and insensitive because of a superior switch don't actually change corresponding gsetting values. I think that user would like to keep what he set before once he enables it again. Tell me if I'm wrong.
Thanks for working on this, Marek! It looks really good to me. I've tried the patches and have a few minor changes to suggest: * The panel list boxes are missing a visible border * border-width on edit-dialog-vbos should be 0 * margin-start on Applications label should be 3, not 6 Otherwise, it looks excellent.
(In reply to comment #6) > I should mention that the switches which are off and insensitive because of a > superior switch don't actually change corresponding gsetting values. > I think that user would like to keep what he set before once he enables it > again. Tell me if I'm wrong. That sounds correct. Thanks again!
Review of attachment 294675 [details] [review]: ok
Review of attachment 294676 [details] [review]: ++
Review of attachment 294677 [details] [review]: ok
Review of attachment 294678 [details] [review]: otherwise looks good ::: panels/notifications/cc-edit-dialog.c @@ +324,3 @@ + g_warning ("Could not load ui: %s", error->message); + g_error_free (error); + return; the builder object is leaked on this path
Created attachment 294973 [details] [review] Move switches to a GtkListBox (In reply to comment #7) > Thanks for working on this, Marek! It looks really good to me. I've tried the > patches and have a few minor changes to suggest: > > * The panel list boxes are missing a visible border Done.
Created attachment 294974 [details] [review] Add label Applications (In reply to comment #7) > Thanks for working on this, Marek! It looks really good to me. I've tried the > patches and have a few minor changes to suggest: > > * margin-start on Applications label should be 3, not 6 Done.
Created attachment 294975 [details] [review] Place switches into GtkListBox (In reply to comment #7) > Thanks for working on this, Marek! It looks really good to me. I've tried the > patches and have a few minor changes to suggest: > > * border-width on edit-dialog-vbos should be 0 Done. (In reply to comment #12) > Review of attachment 294678 [details] [review]: > > otherwise looks good > > ::: panels/notifications/cc-edit-dialog.c > @@ +324,3 @@ > + g_warning ("Could not load ui: %s", error->message); > + g_error_free (error); > + return; > > the builder object is leaked on this path Fixed.
Review of attachment 294973 [details] [review]: looks good otherwise ::: panels/notifications/notifications.ui @@ +7,3 @@ <property name="can_focus">True</property> <property name="hscrollbar_policy">never</property> + <property name="shadow_type">in</property> "in" crept in again, should be none
Review of attachment 294974 [details] [review]: ++
Review of attachment 294975 [details] [review]: yep
Comment on attachment 294973 [details] [review] Move switches to a GtkListBox (In reply to comment #16) > Review of attachment 294973 [details] [review]: > > looks good otherwise > > ::: panels/notifications/notifications.ui > @@ +7,3 @@ > <property name="can_focus">True</property> > <property name="hscrollbar_policy">never</property> > + <property name="shadow_type">in</property> > > "in" crept in again, should be none I'm sorry about that. It somehow got back during handling those 6 patches.
Thank you all for your help. The patches have been pushed to master.