Re: gtkstock fixes and other comments

[Havoc Pennington <hp redhat com>]

Paolo Molaro <lupus lettere unipd it> writes:
> I think there is a bug in the gtkstock implementation:
> the stock label is translated at each lookup of the stock
> item (unnecessary, IMHO, and could well get called on the
> already translated string)

It couldn't, the GtkStockItem is defined to contain the untranslated
string. Though I don't think there's any problem with the optimization
of calling it when the item is inserted in the hash, if you want to
apply that patch it should be fine.

> and later the result of dgettext()
> is passed to g_free() (while managing to leak some memory:-).
>

Yes, the problem is that the stock hash calls gtk_stock_item_free()
even on the static items if you overwrite an item, right. Good catch.

> There is still one problem, though: the default items
> are added to the hash table and if you want to override them
> later, they are g_free()ed. We have two options: copy them
> (bleah) or add a flag telling they are really static entries.
> 
> I'd go for the second option. And while we are changing the 
> GtkStockItem structure, why not add a GtkIconSet* as well?

GtkStockItem is supposed to be usable in a static table, it's only by
coincidence an internal data structure. So if something is only of
interest internally (e.g. the static flag) it should be internal, not
something the user has to type in.

I would solve the problem by having the stock hash actually be a hash
from stock ID to some internal struct, that contains a GtkStockItem*
and a flag whether the GtkStockItem should be freed. i.e.:

struct StockHashEntry
{
  gboolean should_free;
  GtkStockItem *item;
};

Or alternatively, maybe it would be more efficient to store the stock
items in two hash tables, one containing the static items and one
containing the allocated items. This would complicate the stock code a
bit though, and I guess avoiding the StockHashEntry allocation would
be negated by having to allocate two tables.

> Right now we have two hash tables (one static in gtkstock.c
> and another in the default instance of GtkIconFactory): both
> of them have stock_ids as keys and need to be kept in sync.
> Keeping the icon source and the stock label in the same
> structure will also allow to set a standard 'property'
> on GtkImage widgets created from stock ids that holds
> a description of the image (a screen reader such as gspeech
> could make use of that info).
> What is the reason for having different GtkIconFactory objects,
> anyway? I see no reason for an application to have different
> stock items in different windows, for example (and if you
> need that you'd need also different stock labels).
> So my plan would be to remove the GtkItemFactory stuff and
> move the functionality for stock icons in gtkstock.c.
> Comments?
> 

The icon factory is separate so that you can theme your icons.  Like
all themeable widget properties, icons can be changed per-widget.

A screen reader could just look up an image's stock ID in the stock
system to get the stock info, there's no need to store it in the same
place for that to be possible.

Havoc