Re: GtkFontPickerButton

[Owen Taylor <otaylor redhat com>]

On Mon, 2003-03-17 at 15:41, Matthias Clasen wrote:
> I have attached my latest version of this widget to 
> http://bugzilla.gnome.org/show_bug.cgi?id=107682
> The changes wrt to the proposal posted last week are the option to
> display the font style in the button and a name change to
> GtkFontSelectionButton, in order to keep it more in line with
> GtkFontSelection and GtkFontSelectionDialog. 
> 
> I'd like to commit this soon, if no further comments arrive.

Many of the same comments as for GtkColorSelectionButton:

 - GtkColorButton?

 - GTK_CHECK_CAST

 - Identation (also notice here that you have struct {
   rather than struct\n{)
  
==
  g_object_class_install_property
    (gobject_class, 
==

   Ugh.

- I'd like to see "label_font_size" separated out from 
   set_use_font_in_label, so that you can use the
   default value. Or maybe removed altogether... 
   it's a acessibility/themability trap, since if you
   set it it won't follow the theme's font size.

    ... in fact, it doesn't follow the theme's
   font size by default either now!

 - If label_font_size is left: a) it should be in
   Pango units or a double (GtkTextTag::size, 
   GtkTextTag::size_points.) b) I don't see a point
   in the pretty strict limits that are put on the
   property values. c) maybe it should be a scale
   factor compared to the normal size.  

 - Does the ::font_set signal duplicate notify::font_name?

   (Yes, I know they behave differently... but the 
   question is whether a callback that only happens
   when the user changes the button is needed; it saves
   the typical initial-setup-reentrancy problems, but
   the programmer is presumably already dealing with
   that for the rest of their program.)

 - Does the 'preview-text' property really make sense?
   Under what circumstances would an application want
   to change it?

 - I'm a little skeptical of the 'show-style' 'show-size'
   options. I think it's basically wrong to have 'Helvetica'
   show if the font is 'Helvetica Bold'. I tend to 
   think the button should always show the name
   including the style, with 'Normal' suppressed.

   If we do want to allow applications to let a user
   pick:

   - A family without a style
   - A family+style without a size

   Then we need to add options to the font selection dialog
   to allow suppressing the relevant parts of the dialog.

 - VISIBLE => REALIZED for a toplevel, so the

    else if (priv->font_dialog->window) 

   check in gtk_font_selection_button_clicked() isn't needed.

 - The logic where ::destroy updates the values, and
   ::cancel/delete_event restores them back is 
   incredibroken. OK should simply propagate the values
   from the dialog to the button, and the code 
   in gtk_font_selection_button_get_font_name() that 
   fills in priv->font_name should be removed.

   (I guess the idea here is perhaps instant update, 
   except that it doesn't work ... it's instant update if 
   something happens to query the button while the dialog 
   is up...)
 
 - Doesn't look to me like notification on font_name
   and preview_text works correctly now.

 - if (desc == NULL) 
    {
      /* FIXME: eek does this ever happen? probably
       * so we should handle it somehow*/
      g_warning ("eeek!");
      return;
    }

   Needs to be fixed. pango_font_description_from_string()
   is not documented to have a failure mode and never fails,
   so it could be omitted.

   If you want to be paranoid,just return quietly instead
   of printing the "eeek!" since "eeek!" message in 
   .xsession-errors does nobody good.

Anyways, that's what I saw looking through the patches.

Regards,
                                    Owen