GNOME Bugzilla – Bug 637761
ui: Port to GtkStyleContext
Last modified: 2011-01-13 19:25:30 UTC
See patch.
Created attachment 176842 [details] [review] ui: Port to GtkStyleContext GtkStyle has been deprecated in favor of GtkStyleContext. A full port would involve replacing GdkColor with GdkRGBA - leave this out for the time being. Bump the required version of GTK+.
Is this still waiting on the GtkIconView port-over for the final version, or has that been done now?
I'm not sure what the status of GtkIconView is - some changes I would have expected after a conversation with garnacho haven't landed, but honestly I don't know if it's going to happen or if I misinterpreted something, so I'd say it's good to review as-is (the code in question is 2 lines code + 4 lines comment, so ...)
Review of attachment 176842 [details] [review]: Just a few misc comments ::: src/ui/metaaccellabel.c @@ +307,3 @@ + gtk_style_context_save (style); + gtk_style_context_set_state (style, + gtk_widget_get_state_flags (widget)); The need to do this is pretty crackful, but not your issue. (A widget's style context should just normally have the right state, I complained to garnacho) ::: src/ui/testgradient.c @@ +232,2 @@ + gtk_style_context_save (style); + gtk_style_context_set_state (style, 0); The original code obeyed the state of the widget, probably doesn't really matter, but if we don't actually care at all, it seems like explicitly setting to 0 is a bit of overkill. ::: src/ui/theme-viewer.c @@ +860,3 @@ gtk_widget_realize (window); + style = gtk_widget_get_style_context (window); + font_desc = (PangoFontDescription *)gtk_style_context_get_font (style, 0); Why not make the variable const rather than use the cast? @@ +936,3 @@ + + style = gtk_widget_get_style_context (widget); + font_desc = (PangoFontDescription *)gtk_style_context_get_font (style, 0); Same question here ::: src/ui/ui.c @@ +718,3 @@ + gtk_style_context_get (style, 0, + GTK_STYLE_PROPERTY_FONT, &font_desc, + NULL); If style_context_get() works like other valist gets, this will copy and leak the font desc. Any reason not to use gtk_style_context_get_font()?
(In reply to comment #4) > ::: src/ui/testgradient.c > @@ +232,2 @@ > + gtk_style_context_save (style); > + gtk_style_context_set_state (style, 0); > > The original code obeyed the state of the widget Fixed. > ::: src/ui/theme-viewer.c > @@ +860,3 @@ > gtk_widget_realize (window); > + style = gtk_widget_get_style_context (window); > + font_desc = (PangoFontDescription *)gtk_style_context_get_font (style, 0); > > Why not make the variable const rather than use the cast? Possible, but than we'd need the case when passing it to preview_collection() - is that preferable? > ::: src/ui/ui.c > @@ +718,3 @@ > + gtk_style_context_get (style, 0, > + GTK_STYLE_PROPERTY_FONT, &font_desc, > + NULL); > > If style_context_get() works like other valist gets, this will copy and leak > the font desc. Right, I'll fix that. > Any reason not to use gtk_style_context_get_font()? The style is unref'ed before the font description is used - I have changed the code to GtkStyleContext *style = NULL; if (!font_desc) { style = gtk_style_context_new (); font_desc = gtk_style_context_get_font (); } /* do stuff */ if (style != NULL) g_object_unref (style);
(In reply to comment #5) > (In reply to comment #4) > > ::: src/ui/testgradient.c > > @@ +232,2 @@ > > + gtk_style_context_save (style); > > + gtk_style_context_set_state (style, 0); > > > > The original code obeyed the state of the widget > > Fixed. > > > > ::: src/ui/theme-viewer.c > > @@ +860,3 @@ > > gtk_widget_realize (window); > > + style = gtk_widget_get_style_context (window); > > + font_desc = (PangoFontDescription *)gtk_style_context_get_font (style, 0); > > > > Why not make the variable const rather than use the cast? > > Possible, but than we'd need the case when passing it to preview_collection() - > is that preferable? Better to make preview_collection take a const - if you can keep from chasing that over dozens of source files. > > > Any reason not to use gtk_style_context_get_font()? > > The style is unref'ed before the font description is used - I have changed the > code to > > GtkStyleContext *style = NULL; > > if (!font_desc) > { > style = gtk_style_context_new (); > font_desc = gtk_style_context_get_font (); > } > > /* do stuff */ > > if (style != NULL) > g_object_unref (style); That seems fine, refing the font returned from get_font() would be possible too.
Created attachment 178248 [details] [review] ui: Port to GtkStyleContext (In reply to comment #6) > Better to make preview_collection take a const - if you can keep from chasing > that over dozens of source files. Done. > That seems fine, refing the font returned from get_font() would be possible > too. Yeah, but trickier, as font_desc wouldn't need unref'ing when returned by meta_prefs_get_titlebar_font().
Review of attachment 178248 [details] [review]: Looks good to me
Attachment 178248 [details] pushed as 565f002 - ui: Port to GtkStyleContext From IRC: <fmuellner> owen: ^^^should I leave the bug open for GdkColor -> GdkRGBA porting? <owen> fmuellner: only if you think it's a good use of your time in the next few months to do the porting It's not, so closing now :-)