GNOME Bugzilla – Bug 636695
GdkColor should be deprecated
Last modified: 2014-05-22 13:11:08 UTC
GdkRGBA apparently replaces GdkColor, and there are replacement functions that use GdkRGBA instead of GdkColor. So shouldn't we deprecate GdkColor and any API that uses it?
There are still widget style properties using GDK_TYPE_COLOR, I'm sure many of them can be deprecated as well in favor of CSS color handling (for example treeview even/odd colors), but mostly need a one by one check as widgets switch to GtkStyleContext
Is this likely to happen before 3.0? It would really clean the API up.
I think the current consensus is that removing it will cause lots of stuff to no longer compile as there's too many people still using GdkColor and it would likely need weeks to get jhbuild compiling again. So we'll probably aim at deprecating it for 3.2 or so.
Well, it still seems wise to avoid its use internally and to ensure that their are GdkRGBA versions of any API that uses GdkColor.
I think we've got this covered pretty much? I didn't check it, but what I heard from IRC was that everything but GtkTextView is done by now.
Created attachment 186917 [details] [review] docs/tools/widgets.c: Use GdkRGBA instead GdkColor
Created attachment 186918 [details] [review] docs/reference/gtk/text_widget.sgml: Use GdkRGBA instead GdkColor
Review of attachment 186917 [details] [review]: Looks fine
Review of attachment 186918 [details] [review]: Looks fine, too
Comment on attachment 186917 [details] [review] docs/tools/widgets.c: Use GdkRGBA instead GdkColor commit e73c0d9800ca499994070cb9129f5ee868949348
Comment on attachment 186918 [details] [review] docs/reference/gtk/text_widget.sgml: Use GdkRGBA instead GdkColor commit 8e1fdaebe78d701853fafe58f03310c520f0eb36
Created attachment 187041 [details] [review] gtk/gtktrayicon-x11.c: Use GdkRGBA instead GdkColor
Created attachment 187042 [details] [review] gtk/gtktrayicon-x11.c: Use GdkRGBA instead GdkColor
Created attachment 187052 [details] [review] gdk/x11/gdkcursor-x11.c: Use GdkRGBA instead GdkColor
Review of attachment 187042 [details] [review]: ::: gtk/gtktrayicon-x11.c @@ +206,3 @@ + icon->priv->success_color.red = 0.3047; + icon->priv->success_color.green = 0.6016; + icon->priv->success_color.blue = 0.0234; You probably want to initialize the alpha too, here @@ +559,3 @@ + color.red = prop.prop[0] / 65535; + color.green = prop.prop[1] / 65535; + color.blue = prop.prop[2] / 65535; Not sure this works correctly wrt to type propagation, as both prop[0] and 65535 are integer types. Better to use 65535.0.
Review of attachment 187052 [details] [review]: Looks fine
Comment on attachment 187052 [details] [review] gdk/x11/gdkcursor-x11.c: Use GdkRGBA instead GdkColor commit 1fcfa91ee3289a321647f259c4efa62f52
Created attachment 187220 [details] [review] gtk/gtktrayicon-x11.c: Use GdkRGBA instead GdkColor.v2 Updated patch with your comments
Review of attachment 187220 [details] [review]: Looks ok now
Comment on attachment 187220 [details] [review] gtk/gtktrayicon-x11.c: Use GdkRGBA instead GdkColor.v2 commit 2f3e1fa3e43eb751e7d730d48d6a2787868cc8f4
Created attachment 189336 [details] [review] gail: use GdkRGBA
Created attachment 189337 [details] [review] tests: Use GdkRGBA
Review of attachment 189336 [details] [review]: This is all just really annoying conversion... but the patch looks fine.
Review of attachment 189337 [details] [review]: Looks ok, assuming you've tested it
Comment on attachment 189336 [details] [review] gail: use GdkRGBA commit 02285b232ccc18fe6db745346046c9690fb382eb
Comment on attachment 189337 [details] [review] tests: Use GdkRGBA commit 85747da97238dc101f92cd8241aa7aea44f35789
Created attachment 189791 [details] [review] gdkcreen-x11: Use GdkRGBA instead GdkColor
Created attachment 189792 [details] [review] gdkcreen-x11: Use GdkRGBA instead GdkColor.v2 There was a typo in the previuos patch
Review of attachment 189792 [details] [review]: Same as before: looks fine if you tested it.
Comment on attachment 189792 [details] [review] gdkcreen-x11: Use GdkRGBA instead GdkColor.v2 commit 522c305c12c28d68b636ca8e601de22e260cb9dc
Created attachment 203872 [details] [review] gtkcellview
Created attachment 203873 [details] [review] gtktrayicon-x11
Created attachment 203874 [details] [review] testgtk
Created attachment 203875 [details] [review] testcombo
Created attachment 203877 [details] [review] Deprecate public API that is using GdkColor
Review of attachment 203872 [details] [review]: ::: gtk/gtkcellview.c @@ +450,3 @@ g_warning ("Don't know color `%s'", g_value_get_string (value)); + + g_object_notify (object, "background-rgba"); You should probably notify both background-rgba and background-gdk here ?
Review of attachment 203873 [details] [review]: ::: gtk/gtktrayicon-x11.c @@ +962,3 @@ { /* Set a transparent background */ + GdkRGBA transparent = { 0.0, 0.0, 0.0, 0.0 }; /* Only pixel=0 matters */ The comment is misleading now and should be removed. There is no pixel in gdkrgba.
Review of attachment 203874 [details] [review]: ::: tests/testgtk.c @@ +4822,3 @@ + gtk_color_selection_get_current_rgba (GTK_COLOR_SELECTION (colorsel), &rgba); + gtk_color_selection_set_current_rgba (GTK_COLOR_SELECTION (colorsel), &rgba); I wonder - what is the point of this code ?!
Review of attachment 203875 [details] [review]: ::: tests/testcombo.c @@ +1290,1 @@ color.green = 0; This leaves color.alpha uninitialized.
Review of attachment 203877 [details] [review]: You need to add Deprecated:3.4: Use ... instead notes to all the doc comments as well - gtk-doc does not know how to pick up the deprecation annotations (and it wouldn't pick up the version number there anyway).
Comment on attachment 203874 [details] [review] testgtk Reject patch as Its deatch code
Comment on attachment 203872 [details] [review] gtkcellview commited with your comments: 0d6af75bfe2ca794153f9523158acbc90bcebbe6
Comment on attachment 203873 [details] [review] gtktrayicon-x11 committed with your comments: e053590ae579463820159301f6934083dbf5ce8b
Comment on attachment 203875 [details] [review] testcombo committed with your comments: 14112534d64b8f05a5eb3321093c3da7a9070113
Created attachment 203976 [details] [review] Deprecate public API that is using GdkColor.v2
Review of attachment 203976 [details] [review]: Looks good now.
Comment on attachment 203976 [details] [review] Deprecate public API that is using GdkColor.v2 commit a3abc188585156e4a6ba309e26c6d123cfe4f39d
There's a bunch of deprecation warnings when compiling GTK now. Those need to be fixed still.
Created attachment 204578 [details] [review] gtksettings: Do not use GdkColor API
Created attachment 204579 [details] [review] gtkcolorsel: Do not use GdkColor API
Created attachment 204580 [details] [review] Deprecate GdkColor
Created attachment 204581 [details] [review] Move GdkColor to deprecated directory
There are some warnings because the link-color and the visited-link-color style properties. Should we add a GdkRGBA version of these properties?
Review of attachment 204578 [details] [review]: Looks good
Review of attachment 204579 [details] [review]: Looks ok. I don't think the clamping is really necessary though.
Review of attachment 204580 [details] [review]: ok
Review of attachment 204581 [details] [review]: Looks ok. Did you check that it survives make dist ?
Comment on attachment 204578 [details] [review] gtksettings: Do not use GdkColor API commit baef3e5f2470cb727407e45a655cec59ebd4a9e5
Comment on attachment 204579 [details] [review] gtkcolorsel: Do not use GdkColor API commit e77ffa6f871c0b0fd61da789ca4fc14d8e7a90ac
Review of attachment 204579 [details] [review]: Shouldn't this deprecate gtk_color_selection_palette_from_string() too, and maybe add a replacement? /** * gtk_color_selection_palette_from_string: * @str: a string encoding a color palette * @colors: (out) (array length=n_colors): return location for * allocated array of #GdkColor * @n_colors: return location for length of array * * Parses a color palette string; the string is a colon-separated * list of color names readable by gdk_color_parse(). * * Return value: %TRUE if a palette was successfully parsed */ gboolean gtk_color_selection_palette_from_string (const gchar *str, GdkColor **colors, gint *n_colors)
Javier: Could you answer Murray's question & commit your two attached patches?
gtk_color_selection_palette_from_string() is deprecatred, so Its nor worth to have a replacement. About apply my patches, I still have to find a way to avoid all the warnings before applying them
(In reply to comment #64) > gtk_color_selection_palette_from_string() is deprecatred, Ah, yes, all of GtkColorSelection is now deprecated.
(In reply to comment #55) > There are some warnings because the link-color and the visited-link-color style > properties. Yes, these are the warnings: gtkaboutdialog.c: In function 'follow_if_link': gtkaboutdialog.c:1912:15: warning: 'gdk_color_free' is deprecated (declared at ../gdk/gdkcolor.h:68): Use 'gdk_rgba_free' instead [-Wdeprecated-declarations] gtkaboutdialog.c: In function 'text_view_new': gtkaboutdialog.c:2113:7: warning: 'gdk_color_free' is deprecated (declared at ../gdk/gdkcolor.h:68): Use 'gdk_rgba_free' instead [-Wdeprecated-declarations] gtkaboutdialog.c:2121:7: warning: 'gdk_color_free' is deprecated (declared at ../gdk/gdkcolor.h:68): Use 'gdk_rgba_free' instead [-Wdeprecated-declarations] > Should we add a GdkRGBA version of these properties? By the way, thes style properties are here: http://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c#n3083 Obviously we cannot just change that to use GdkRGBA because style properties are public API, when used from C code. However, the theme's .css files do not seem to need to know that it's a GdkColor. For instance: http://git.gnome.org/browse/gnome-themes-standard/tree/themes/Adwaita/gtk-3.0/gtk-widgets.css#n31 which uses the color defined here: http://git.gnome.org/browse/gnome-themes-standard/tree/themes/Adwaita/gtk-3.0/gtk-main.css#n46 So, I wonder how we can keep the .css the same, but let that same CSS value be retrieved by both link-color and link-rgba style properties. Maybe gtk_widget_class_install_style_property_parser() would let us do this?
Review of attachment 204580 [details] [review]: Ok, so turns out we need to deal with the GdkColor-typed style properties first
Review of attachment 204581 [details] [review]: This too
Attachment 204580 [details] pushed as 49cf514 - Deprecate GdkColor