Comment on attachment 216401 [details] [review]
patch

[Patch => setting "patch" flag and correct mimetype]

Review of attachment 216401 [details] [review]:

Sounds good to me in theory, but we should make sure no other widget gets an unwanted scaling behavior.
The part I'm worried about is that GtkImage documentation doesn't seem to imply its pixel-size and icon-size properties should work when a GdkPixbuf was set on it, and this patch would change that; the same is also true for at least GtkCellRendererPixbuf (for the stock-size property).
You could either ensure this by checking for storage_type != pixbuf in each of the callers, or by having a force_resize flag or similar on the icon helper, not sure what's best.

::: gtk/gtkiconhelper.c
@@ +291,3 @@
+      self->priv->icon_size != GTK_ICON_SIZE_INVALID)
+    {
+

Can you move these at the top of the function to make it consistent with the other similar ensure_* helpers?

(In reply to comment #2)
> Review of attachment 216401 [details] [review]:
> 
> Sounds good to me in theory, but we should make sure no other widget gets an
> unwanted scaling behavior.

Right, this would also play odd with GtkStatusIcon where it does the rescaling itself

> The part I'm worried about is that GtkImage documentation doesn't seem to imply
> its pixel-size and icon-size properties should work when a GdkPixbuf was set on
> it, and this patch would change that; the same is also true for at least
> GtkCellRendererPixbuf (for the stock-size property).
> You could either ensure this by checking for storage_type != pixbuf in each of
> the callers, or by having a force_resize flag or similar on the icon helper,
> not sure what's best.

How about a gboolean force_resize argument on _new()? it's at least GtkEntry and GtkStatusIcon which would benefit from this.

> 
> ::: gtk/gtkiconhelper.c
> @@ +291,3 @@
> +      self->priv->icon_size != GTK_ICON_SIZE_INVALID)
> +    {
> +
> 
> Can you move these at the top of the function to make it consistent with the
> other similar ensure_* helpers?

The fallback part of the function still needs to be hit though, right?

+  if (!self->priv->rendered_pixbuf)
+    self->priv->rendered_pixbuf = g_object_ref (self->priv->orig_pixbuf);

(In reply to comment #3)

> How about a gboolean force_resize argument on _new()? it's at least GtkEntry
> and GtkStatusIcon which would benefit from this.

On second thought maybe what we really need for this is simply a flag that determines whether pixbufs should be scaled as well, defaulting to FALSE.

_gtk_icon_helper_set/get_force_scale_pixbuf()

All the other storage type cases shouldn't modify their behavior is this flag is TRUE...GtkStatusIcon could then use it to force a pixel size and avoid doing the scaling itself.

> > ::: gtk/gtkiconhelper.c
> > @@ +291,3 @@
> > +      self->priv->icon_size != GTK_ICON_SIZE_INVALID)
> > +    {
> > +
> > 
> > Can you move these at the top of the function to make it consistent with the
> > other similar ensure_* helpers?
> 
> The fallback part of the function still needs to be hit though, right?

Sorry, git-bz made it hard to understand what I was referring to here...I was just asking to move the gint width, height variable declarations at the top of the function :)

Created attachment 218726 [details] [review]
patch v2, GtkIconHelper API addition

Created attachment 218727 [details] [review]
use iconhelper api in entries

Created attachment 218728 [details] [review]
use iconhelper api in statusicon

Review of attachment 218726 [details] [review]:

Looks good, except for the following comment

::: gtk/gtkiconhelper.c
@@ +608,3 @@
+                                         gboolean       force_scale)
+{
+}

Should probably check if self->priv->force_scale != force_scale here, and call invalidate() if it is.

Review of attachment 218727 [details] [review]:

Sure

Review of attachment 218728 [details] [review]:

Looks good

I've applied the suggested modification and committed all 3 patches, thanks!