GNOME Bugzilla – Bug 749883
Design regressions
Last modified: 2015-06-16 14:46:38 UTC
Created attachment 303982 [details] Screenshot of the current state There are 2 design regression in the 'New printer dialog': The first one is that after detection of available printers, the search entry is expanded over the space previously occupied by the spinner on the right side. There should be an empty space instead of the spinner. The second one is that there is missing border around the 'No printers detected' text when no printers are detected in the dialog. The first regression was introduced in 3.16, the second one in 3.14.2.
Created attachment 303983 [details] [review] Don't expand search entry This patch fixes the first regression by placing the GtkSpinner into a GtkFixed.
Created attachment 303984 [details] [review] Show border around 'No printers detected' text This patch fixes the second regression by placing the GtkLabel into a GtkViewport.
Created attachment 303985 [details] Screenshot of the fixed dialog
(In reply to Marek Kašík from comment #3) > Created attachment 303985 [details] > Screenshot of the fixed dialog Seems like an obvious improvement to me.
Review of attachment 303983 [details] [review]: Sure.
Review of attachment 303984 [details] [review]: ::: panels/printers/new-printer-dialog.ui @@ +147,3 @@ </child> <child> + <object class="GtkViewport" id="viewport1"> Why do you use a viewport here?
(In reply to Bastien Nocera from comment #6) > Review of attachment 303984 [details] [review] [review]: > > ::: panels/printers/new-printer-dialog.ui > @@ +147,3 @@ > </child> > <child> > + <object class="GtkViewport" id="viewport1"> > > Why do you use a viewport here? I use it because it has a visible border and I can place the label into it.
(In reply to Marek Kašík from comment #7) > (In reply to Bastien Nocera from comment #6) > > Review of attachment 303984 [details] [review] [review] [review]: > > > > ::: panels/printers/new-printer-dialog.ui > > @@ +147,3 @@ > > </child> > > <child> > > + <object class="GtkViewport" id="viewport1"> > > > > Why do you use a viewport here? > > I use it because it has a visible border and I can place the label into it. The point of GtkViewport is to be able to put scrollable content inside it. You probably want GtkFrame instead.
Created attachment 305400 [details] [review] Show border around 'No printers detected' text (In reply to Bastien Nocera from comment #8) > (In reply to Marek Kašík from comment #7) > > (In reply to Bastien Nocera from comment #6) > > > Review of attachment 303984 [details] [review] [review] [review] [review]: > > > > > > ::: panels/printers/new-printer-dialog.ui > > > @@ +147,3 @@ > > > </child> > > > <child> > > > + <object class="GtkViewport" id="viewport1"> > > > > > > Why do you use a viewport here? > > > > I use it because it has a visible border and I can place the label into it. > > The point of GtkViewport is to be able to put scrollable content inside it. > You probably want GtkFrame instead. You are right, GtkFrame is enough here. Attached is modified version of the patch.
Review of attachment 305400 [details] [review]: Looks good.
Thank you for the reviews. I've pushed the first patch into 3.16 and master. The second one into 3.14, 3.16 and master.