Re: GtkColorPickerButton

[Matthias Clasen <maclas gmx de>]

On Mon, 2003-03-17 at 22:50, Owen Taylor wrote:
> On Mon, 2003-03-17 at 15:38, Matthias Clasen wrote:
> > Hmm, so it seems that this widget doesn't need any discussion...
> > I have attached my latest version to
> > http://bugzilla.gnome.org/show_bug.cgi?id=107597
> > The only change wrt to the proposal posted last week is a name change to
> > GtkColorSelectionButton, in order to keep it more in line with
> > GtkColorSelection and GtkColorSelectionDialog. 
> > 
> > I'd like to commit this soon, if no further comments arrive.
> 
> Just to raise the issue ... perhaps simply 'GtkColorButton' would
> be better? I don't have any strong objection to GtkColorSelectionButton,
> but it seems a little long.
> 
> A few things from studying the headers and scanning the
> C file:
> 
> - it is still using GTK_CHECK_CAST, etc.
> 
> - It would be nice to switch it to allocating the private chunk  
>   as instance-private-data, now that's in GLib. (Though would
>   require doing the version number bump for glib.) I don't
>   think you need to get rid of the ->priv member in the 
>   instance... just make it point to the object data chunk. (*)
> 
>   Not urgent though, since we'll need to do a pass over all
>   the current widgets anyways.
> 
> - I think making the size dependent on the font size as was
>   proposed earlier is a good idea... If it doesn't make it
>   to small it might be nice if the total height of the
>   button was the same as that of a button with a string in it.
>   
>   Could be done after committing to CVS.
> 
> - The indentation in the C file seems to be a little uneven
>   (set_property/get_property in particular) 
> 
> - The state_changed() handler probably also needs to check
>   REALIZED. To avoid the possibility of excessive renders
>   I think it would be better if style_set() and state_changed()
>   simply cleared priv->pixbuf unconditionally, and the render()
>   was only called by expose().
> 
> - It would be a good cleanup to never to store the color
>   as doubles internally, and always keep that as integers.
>   Prevents possible roundoff errors, removes a bunch of 
>   code, etc.
> 
> - The keypress connection on the dialog is no longer necessary ..
>   GtkDialog handles that automatically.
> 

Thanks for the feedback. I've done all except for the private chunk; see
the latest patch in bugzilla.

Matthias