Re: GtkColorPickerButton

[Owen Taylor <otaylor redhat com>]

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.

Regards,
                                         Owen


(*) Though we do need to come up with a policy on this time/space
    tradeoff.