Re: gtk_window_set_icons

[Owen Taylor <otaylor redhat com>]

Havoc Pennington <hp redhat com> writes:

> Hi,
> 
> I attached a patch to http://bugzilla.gnome.org/show_bug.cgi?id=59240
> 
> The API docs and prototype are appended. I decided to allow passing in
> three standard sizes, rather than just one icon, in case people want
> to hand-draw the small one.
> 
> It should perhaps be called gtk_window_set_icon() even though it takes
> three args, since it's one icon, just in three sizes.
> 
> I don't have a getter because it would get some generated/scaled
> icons, it would be kind of strange.
> 
> I don't have a property because I'm not sure how it would work. (A
> separate property for each size?)

getters and setters and properties all need to give back what was set.
If you go with a list (see below), then the behavior is obvious,
though limitations of our current set of types probably make the
property uninteresting.
 
> I just went ahead and required three stock sizes rather than allowing
> an arbitrary list to be passed in, because I think this is simpler to
> use and will result in more consistency. I know KDE sets 16x16 and
> 32x32 for _NET_WM_ICON, and 48x48 is the size of most GNOME icons and
> should be large enough for windowmaker/afterstep. So we provide those
> three in _NET_WM_ICON.

> void
> gtk_window_set_icons (GtkWindow  *window,
>                       GdkPixbuf  *icon_16_by_16,
>                       GdkPixbuf  *icon_32_by_32,
>                       GdkPixbuf  *icon_48_by_48)

Ugh. What an awful interface. Look how much text it took you took explain
it in the docs!

"Pick 1,2, or 3 icons, of sizes close to the sizes of the parameter names,
then match them up with the parameters" is just too painful.

I think a list + a recommendation in the docs: "a good choice for the sizes
to include is 16x16 and 48x48" would be much nicer. 

Also, I think GTK+ should give the window manager the icons in the
sizes that the app provides and let the window manager / user worry
about scaling. Remember, scaling twice provides considerably worse
results than scaling once, and scaling even by a small amount reduces
quality.

> I wasn't sure what size to use for the WM_NORMAL_HINTS pixmap though.
> 
> Also, this patch changes gdk_window_set_icon_list() to unconditionally
> set the icons, rationale from ChangeLog:
> 
>         * gdk/x11/gdkwindow-x11.c (gdk_window_set_icon_list): don't
>           check whether the hint is supported, just always set the
>           icon. A task list might want to use it even if the WM
>           doesn't, and the WM may change over time. Also,
>           XDeleteProperty() if list == NULL.

This is fine.


one obvious problem in your patch is:

  gdk_pixbuf_render_pixmap_and_mask (info->icon_32,
                                     &info->icon_pixmap,
                                     &info->icon_mask,
                                     128);

While violatation of the ICCCM for this is common, you _at least_ need
to use the system default colormap/visual for this, not the GdkRGB
colormap/visual.

Regards,
                                        Owen