GNOME Bugzilla – Bug 373117
Proposal: Change how ESource stores colors
Last modified: 2013-09-14 16:49:24 UTC
I'd like to propose a change to the ESource API in libedataserver. ESource currently stores colors as 32-bit integers, which is cumbersome when dealing with GdkColor structures. The upper eight bits of GdkColor's red, green, and blue components must each be bit-masked and shifted, and then assembled into an RGB integer for passing to e_source_set_color(). Vice versa for building a GdkColor from an RGB integer obtained from e_source_get_color(). Internally, ESource encodes the RGB integer as a hexadecimal string of the form "#rrggbb" before storing the value in GConf. Vice versa when reading a color value from GConf. It would be simpler for ESource to store the color as a textual specification that can be written directly to GConf, with the restriction that the specification must be parsable by gdk_color_parse() [1]. Specifically, I propose changing the ESource API as follows [2]: (1) Change the prototype of e_source_set_color() to: void e_source_set_color (ESource *source, const gchar *color_spec) Passing a NULL color_spec unsets the source's color. (2) Change the prototype of e_source_get_color() to: const gchar *e_source_get_color (ESource *source); A NULL return value indicates the source's color is not set. (3) Remove e_source_unset_color() since e_source_set_color() now provides a way to unset the color (see (1)). This proposal should be evaluated in parallel with bug #373116. [1] http://developer.gnome.org/doc/API/2.0/gdk/gdk-Colormaps-and-Colors.html#gdk-color-parse [2] Maintaining backward-compatibility proved infeasible since the current e_source_get_color() function would require gdk_color_parse().
Created attachment 76298 [details] [review] Proposed patch
Created attachment 76392 [details] [review] Revised patch Couple changes from the previous version: (1) The current ESource code generates color strings of the form 'rrggbb', not '#rrggbb' as I originally thought. The latter form is parsable by gdk_color_parse(), the former is not. So I've added a migration path that converts 'rrggbb' strings to '#rrggbb'. (2) Renamed e_source_get_color() to e_source_peek_color() to be more consistent with the rest of the API.
Matthew : Great patch :-) and I cannot agree more on the need for a consistent format for handling colors within the code and while interfacing with GConf/Gtk libraries. Unfortunately, Evo/EDS are still supported on Distros using Gtk+ < 2.6 and the dependency on libgnomeui may stay around for a little longer than we might like it to. So, I would prefer to extend the ESource API, adding the interfaces handling text specifications for color to the existing ones, which would be marked deprecated and can be discarded after a specified time (1.12?) We can ifdef the code based on the Gtk+ version we link to, so only those who need backward compatibility need use the existing code. I can write this patch if we agree on this. Thoughts ?
Harish, that sounds okay to me. Let me know if I can help. Also, I would really appreciate your thoughts on bug #380534, which I'm making this and similar bugs I've filed depend on. It seeks clarification of Evolution's library requirements / support commitments. This patch exemplifies the need for a clear policy.
Harish, might I suggest we start tagging deprecated interfaces like so: #ifndef EDS_DISABLE_DEPRECATED /* ... deprecated declarations ... */ #endif This is similar to what other GNOME libraries do for deprecated symbols. It makes the deprecated symbols easy to track, they can be disabled at compile time, and GTK-Doc has a hook for automatically generating deprecation warnings in the documentation when this method is used.
Created attachment 78448 [details] [review] Revised patch This revision implements the suggestions in comment #3 and comment #5 as follows: (1) Extends the ESource API and by introducing e_source_peek_color_spec() and e_source_set_color_spec() which take textual color specifications instead of integer values. Tags the other color functions as deprecated using EDS_DISABLE_DEPRECATED. (2) The deprecated color functions are now just wrappers around the new color functions, but applications should either use the new functions or the deprecated ones. Not both. There is no guarantee that e_source_color_get() can successfully parse a color string set by e_source_set_color_spec().
Harish? Any news on this?
Harish: Ping
Harish or Srini: Could one of you review this and bug #373116 please?
This looks fine to me.
Thanks for reviewing, Srini. Committed to Subversion trunk, revision 7680.