GNOME Bugzilla – Bug 640664
Port to GtkStyleContext
Last modified: 2011-02-03 12:42:07 UTC
gtk3 branch needs to be ported to GtkStyleContext
s/gtk3 branch/master/ :-)
Created attachment 179405 [details] [review] tasklist: Port to GtkStyleContext
Created attachment 179424 [details] [review] selector: Port to GtkStyleContext
Created attachment 179443 [details] [review] pager: Port to GtkStyleContext
Created attachment 179817 [details] [review] tasklist: Port to GtkStyleContext Updated to current git master
Created attachment 179818 [details] [review] selector: Port to GtkStyleContext Updated to current git master
Created attachment 179819 [details] [review] pager: Port to GtkStyleContext Updated to current git master
Review of attachment 179818 [details] [review]: ::: libwnck/selector.c @@ +1221,3 @@ + "#gnome-panel-window-menu-menu-bar {\n" + " border-width: 0px; }", + -1, NULL); We're losing this part: GtkMenuBar::shadow-type = none GtkMenuBar::internal-padding = 0 Can't we use something like: -GtkMenuBar-shadow-type: none; -GtkMenuBar-internal-padding: 0; ? (Also, we were not using border-width before, is it needed?)
Review of attachment 179819 [details] [review]: ::: libwnck/pager.c @@ +735,3 @@ { + rect->x += padding.left + padding.right; + rect->y += padding.top + padding.bottom; That's wrong: we only want left and top there. @@ +859,3 @@ + + c1 = gtk_symbolic_color_new_literal (&bg); + c2 = gtk_symbolic_color_new_shade (c1, 1.3); Will 1.3 make it lighter enough? I didn't apply the patch, but does it look the same before & after the patch? @@ +1001,2 @@ + xthickness = focus_width + padding.left; + ythickness = focus_width + padding.top; I think we need to rework this function a bit more: xthickness is used later on in a place where we should use padding.right. Same for y/bottom. @@ +1089,3 @@ + c1 = gtk_symbolic_color_new_literal (&bg); + + c2 = gtk_symbolic_color_new_shade (c1, 0.7); Same question as above: is 0.7 dark enough? @@ +1118,3 @@ is_current = (space == wnck_screen_get_active_workspace (pager->priv->screen)); + state = 0; state = GTK_STATE_FLAG_NORMAL; (this is clearer)
Review of attachment 179817 [details] [review]: I don't know how the last part of the patch behaves as I'd need to try it out. But generally looks good. ::: libwnck/tasklist.c @@ +3849,3 @@ + + gtk_style_context_get_padding (context, gtk_widget_get_state_flags (widget), &padding); + state = (task->tasklist->priv->active_class_group == task) ? GTK_STATE_FLAG_ACTIVE : 0; Use GTK_STATE_FLAG_NORMAL instead of 0.
(In reply to comment #8) > Review of attachment 179818 [details] [review]: > > ::: libwnck/selector.c > @@ +1221,3 @@ > + "#gnome-panel-window-menu-menu-bar {\n" > + " border-width: 0px; }", > + -1, NULL); > > We're losing this part: > GtkMenuBar::shadow-type = none > GtkMenuBar::internal-padding = 0 > > Can't we use something like: > -GtkMenuBar-shadow-type: none; > -GtkMenuBar-internal-padding: 0; > > ? We don't need it, it breaks the background with transparent panels, I had to do the same in gnome-panel for the main menu applet. > (Also, we were not using border-width before, is it needed?) There wasn't border-width with GtkStyle I think
(In reply to comment #9) > Review of attachment 179819 [details] [review]: > > ::: libwnck/pager.c > @@ +735,3 @@ > { > + rect->x += padding.left + padding.right; > + rect->y += padding.top + padding.bottom; > > That's wrong: we only want left and top there. right > @@ +859,3 @@ > + > + c1 = gtk_symbolic_color_new_literal (&bg); > + c2 = gtk_symbolic_color_new_shade (c1, 1.3); > > Will 1.3 make it lighter enough? I didn't apply the patch, but does it look the > same before & after the patch? I'm using the same values of gtk+, so it should be the same than before when using style->light or style->dark, see: http://git.gnome.org/browse/gtk+/tree/gtk/gtkstyle.c#n71 http://git.gnome.org/browse/gtk+/tree/gtk/gtkstyle.c#n735 > @@ +1001,2 @@ > + xthickness = focus_width + padding.left; > + ythickness = focus_width + padding.top; > > I think we need to rework this function a bit more: xthickness is used later on > in a place where we should use padding.right. Same for y/bottom. Ok, I'll look at it. > @@ +1089,3 @@ > + c1 = gtk_symbolic_color_new_literal (&bg); > + > + c2 = gtk_symbolic_color_new_shade (c1, 0.7); > > Same question as above: is 0.7 dark enough? Same answer :-) > @@ +1118,3 @@ > is_current = (space == wnck_screen_get_active_workspace > (pager->priv->screen)); > > + state = 0; > > state = GTK_STATE_FLAG_NORMAL; > (this is clearer) Ok
(In reply to comment #12) > (In reply to comment #9) > > @@ +859,3 @@ > > + > > + c1 = gtk_symbolic_color_new_literal (&bg); > > + c2 = gtk_symbolic_color_new_shade (c1, 1.3); > > > > Will 1.3 make it lighter enough? I didn't apply the patch, but does it look the > > same before & after the patch? > > I'm using the same values of gtk+, so it should be the same than before when > using style->light or style->dark, see: > > http://git.gnome.org/browse/gtk+/tree/gtk/gtkstyle.c#n71 > http://git.gnome.org/browse/gtk+/tree/gtk/gtkstyle.c#n735 Yeah, I know, but I've been hit by this somewhere else, where the result was a tiny bit different different. Ah, but it could because it was style->mid... Anyway. I've filed bug 641228 to have gtk+ define some macros for those values.
Created attachment 179908 [details] [review] tasklist: Port to GtkStyleContext Updated patch according to review
Created attachment 179909 [details] [review] pager: Port to GtkStyleContext Updated patch according to review
Created attachment 179910 [details] [review] pager: Port to GtkStyleContext I forgot this comment, sorry. @@ +1001,2 @@ + xthickness = focus_width + padding.left; + ythickness = focus_width + padding.top; I think we need to rework this function a bit more: xthickness is used later on in a place where we should use padding.right. Same for y/bottom.
Thanks, I've committed everything -- with small fixes for tasklist.c