Forgot to mention: this could be stuff for ProjectRidley

I think the first task here would be to identify what is missing from the
GtkImage animation support to make it suitable as a spinner.

The big thing is probably that you can't have animated PNGs, and the artists
don't want to work within GIF's limitations. (The default gnome-spinner has
an alpha channel for the drop shadow, etc.) Maybe this could be addressed by
having a GdkPixbufAnimation subclass that does the image slicing-and-dicing
that EphySpinner/NautilusThrobber currently do.

The other thing is that in some themes, the "resting" image is different from
any of the animation frames, so we probably need to keep the convention of
using two separate images.

Googling cvs.gnome.org for "gnome-spinner" turns up nautilus, epiphany,
galeon, drivel, blam, and eggiconchooser (most of which use a cut+pasted
version of EphySpinner, which was itself copied from NautilusThrobber).
It would be nice to do something here for Project Ridley. I'm willing to do
the work.

gdk-pixbuf supports constructing an animation from a series of frames now...

OK. That's part of it. The other two "hard" things EphySpinner does are
slicing up the gnome-spinner image into its component pixbufs, and loading
new spinner images if the theme changes. The slicing could be done via
a new GdkPixbufSimpleAnim constructor (or two: one that takes a filename and
one that takes a pixbuf). The theme-watching is the same as what GtkImage
already does for icon_name images, but we'd need to add a new
"animated_icon_name" type or something. Then you'd be able to do:

    gtk_image_set_animated_icon_name (spinner, "gnome-spinner");

    /* do slow stuff */

    gtk_image_set_icon_name (spinner, "gnome-spinner-rest");

(except presumably with a new icon name like "stock-spinner").

This might suck though, because GtkImage would have to rebuild the
spinner animation every time you set it, unlike EphySpinner, which
caches it.

Rodney Dawes added an 'Animation' context to icon naming specs (online at
http://standards.freedesktop.org/icon-naming-spec/, but not yet updated to
latest version ).


Animation context:  Animated images used to represent loading web sites, or
other background processing which may be less suited to more verbose progress
reporting in the user itnerface.
-----

Standard Animation Icon

process-working:  This is the standard spinner animation for web browsers and
file managers to show that the location is loading. This image should be a
multi-frame PNG with the frames as the size that the directory containing the
image, is specified to be in. The first frame of the animation should be used
for the resting state of the animation.

It could be smart follow this ;-)

thats not going to work with the icon theme spec in its current version, since 
that allows only xpm, png and svg as image formats

Do you think could be useful add a link to this bug on live.gnome.org/ProjectRideley adding a "Unresourced" (or "Nice to have") section?

Well, nobody explained to me yet what GtkImage is missing, really. Do you think that will happen in the wiki ?

from comment #5:

1) Slicing "comic strip"-style icons into frames -- that probably goes into
   GdkPixbufAnimSimple rather than GtkImage

2) Setting the spinner icon by name rather than via the pixbuf-animation
   property, so that GtkImage can deal with reloading it on theme changes.

3) Caching both the spinning and idle pixbufs regardless of which is
   currently shown. (Optional, but probably a good thing.)

> 3) Caching both the spinning and idle pixbufs regardless of which is
>    currently shown. (Optional, but probably a good thing.)

If GTK+ will follow the icon naming spec standard, you will have a single pixbuf to load image (frame) for rest and animation (frames) for working. See comment #6

> Well, nobody explained to me yet what GtkImage is missing, really. Do you think that will happen in the wiki ?

No, but maybe more people can see it. ;-)

No, the icon theme spec does not support animated icons in themes. 
Nobody proposed this extension on xdg-list yet. 
We will not just sneak this into GTK+ without icon theme spec amendmend.

Matthias: of course, animation from user point of view, not "animated" images (i.e. GIF or MNG). Icon Naming Spec suggests to use a simple image like

    A  B  C 
 1  *  -  \ 
 2  |  /  -
 3  \  |  /

Where A1 is the image used for spinner on rest, B1 C1 A2 B2 C2 A3 B3 C3 are "frames" for spinner on working; if the spinner size in 16x16 pixels, this image should be 48x48 (but placed under 16x16 directory) and if the spinner  size is 22x22 this image should be 66x66 (but placed under 22x22 directory). Of course the toolkit should be able to manage also 64x48 images (A~D;1~3) with 1 rest image and 11 "frames" and so on.

Honestly, I am not seeing hacks of this magnitude entering the GTK icon theme code any time soon. 
Again, support for animations in icon themes has to be brought up and discussed on xdg-list first

Icon animations is in the Icon Naming Specification, and nobody has brought up any opposition, concerns, or alternative suggestions to what it states, on the XDG list. If you have any, please feel free to mail that list, and start a discussion on the subject.

In an attempt to fix the issue where the EphySpinner and NautilusThrobber widgets use the smallest icon in the theme, rather than one of appropriate size, though, I will state that neither widget's code is fit for inclusion as a widget in GTK+. The implementations need to at least have that issue fixed, and then follow the recommendations in the Icon Naming spec for how to handle animations.

I have brought it up, and proposed a more capable mechanism than the hack currently in use.

Can you point me to the mail in the xdg list archives then? I searched my xdg folder just now for all mails from you, and all mails that contain "animation" in the body text, and no mails in my folder contained "animation." My XDG folder has mails dating back to 2004 April 25. Are you sure you mailed XDG about it?

Ah, sorry. I thought it was on xdg-list, but it was on desktop-devel-list:
http://mail.gnome.org/archives/desktop-devel-list/2006-February/msg00208.html

Just like Matthias, I don't see what a spinner widget really buys us.

IMO, this can be solved by either adding an MNG loader that matches the requirements of GdkPixbufAnimation (bug #71267), or a piece of code to load an agreed-upon all-in-one-PNG image into a GdkPixbufAnimation.

Just a note: it seems that John Stowers and Neil Patel wrote a simple (cairo only, no images, themes or other) spinner as part of its libbling (http://code.google.com/p/libbling/). The API allow you to create a new spinner, start and stop it.

http://libbling.googlecode.com/svn/trunk/libbling/bling-spinner.c
http://libbling.googlecode.com/svn/trunk/libbling/bling-spinner.h

The name of the new widget should preferrably not be 'spinner', since that causes confusion with the spin button which is often enough referred to as spinner.
Instead 'throbber' is a synonym that is unique and is widely used as well.

I'm renaming the bug since the widget we want is not meant to and not capable of displying progress. "Throbber (activity widget)" should be more appropriate here and also be good if someone isn't familiar with the term "throbber".

And I'm _still_ waiting for an explanation for whats missing from GtkImage + GdkPixbufAnimation. We even have a construct-from-frames variant of GdkPixbufAnimation now...

(In reply to comment #22)
> And I'm _still_ waiting for an explanation for whats missing from GtkImage +
> GdkPixbufAnimation. We even have a construct-from-frames variant of
> GdkPixbufAnimation now...

What are you referring to with construct-from-frames? I am not aware of a function by the name and grepping through my gtk+ tree for construct_from_frames doesn't bring any results.

Sorry for being terse. 
I was referring to GdkPixbufSimpleAnim

(In reply to comment #22)
> And I'm _still_ waiting for an explanation for whats missing from GtkImage +
> GdkPixbufAnimation. We even have a construct-from-frames variant of
> GdkPixbufAnimation now...

I wrote a spinner widget using GdkPixbufAnimation and the only missing thing is the ability to set the animation to loop.

If we are actually going to add a spinner widget to GTK+, I want it to be more abstract than just an animated icon. 

For the animated icon, we can easily add the missing things (e.g. looping) to GtkImage. 

For a spinner, I'd rather have an api that is just start() and stop(), and have some pie-chart-thingie drawn with cairo (or possibly themed).

(In reply to comment #26)
> If we are actually going to add a spinner widget to GTK+, I want it to be more
> abstract than just an animated icon. 
> 
> For the animated icon, we can easily add the missing things (e.g. looping) to
> GtkImage. 
> 
> For a spinner, I'd rather have an api that is just start() and stop(), and have
> some pie-chart-thingie drawn with cairo (or possibly themed).

Comment 20 mentions some cairo-based spinner which works pretty well. If it used GTK+ symbolic colours, instead of its own colour code, would it be acceptable for GTK+?

Virtually all spinners used by Gtk applications use a themed animation. I think it would either do only that or support both, otherwise nobody will use it.

My thoughts on this question:

1) whatever way of themeing we use, it needs to be usable for both the spinner widget and a spinner cell renderer

2) I personally really hate the 'tiled png' approach to animation. Lo-tech has its charme, but this is too lo-tech. Problems include: 
 - no metadata about tile size (this is particularly problematic when using 
   themed icons, since they have a tendency to get scaled, so there is no way 
   to derive the tile size from the image size)
 - no metadata about intended animation speed
 - the 'tile 0 is inactive' convention is totally undocumented 

3) cairo drawing may be ok, but I believe we really want theme engines to be able to implement their own spinner drawing

Here is a spinner cell renderer that I did this weekend for gnome-packagekit:
http://git.gnome.org/cgit/gnome-packagekit/commit/?id=b96d48a0756c3b67ac18ae6b66c99fef72fc4ccc

It would be nice to be able to control the speed of the spinner, as well as the direction.  For example, Chrome's spinner starts turning slowly counter-clockwise while it's waiting for a response from the server (which seems kind of counter-intuitive when you think about it, but in practice is actually nice for reasons I can't quite describe).. and then spins clockwise at 'normal' speeds when a page is loading.

Created attachment 138869 [details] [review]
0001-Bug-319607-Add-a-throbber-activity-widget-to-G.patch

A simple spinner, as used in gnome-bluetooth and some other parts.

No options, just stop/start functions.

Created attachment 138870 [details]
Screenshot-GtkSpinner.png

And this is what it looks like.

some minor comments:

 - it should use GET_PRIVATE just once and then use ->priv
 - the timout should be cancelled on destroy or on dispose not in finalize to avoid running on a disposed-but-not-finalized widget
 - it should use G_PI instead of M_PI
 - there are some minor codestyle issues (spaces before parenthesis etc)

Created attachment 138921 [details] [review]
0001-Bug-319607-Add-a-throbber-activity-widget-to-G.patch

> - it should use GET_PRIVATE just once and then use ->priv

Fixed

> - the timout should be cancelled on destroy or on dispose not in finalize to
> avoid running on a disposed-but-not-finalized widget

We just have a dispose now

> - it should use G_PI instead of M_PI

Fixed

> - there are some minor codestyle issues (spaces before parenthesis etc)

Fixed as well

I don't see a way to specify the size. Is this how it should be done?

gint width, height;
gtk_icon_size_lookup (GTK_ICON_SIZE_MENU, &width, &height);
gtk_widget_set_size_request (spinner, width, height);

As it is, throbbers are often found in menubars, toolbars and tab labels, so one would want to have a reliable size.

And I notice the code explicitly enforces a minimum of 12 pixels. That seems like a bad idea.

(In reply to comment #35)
> I don't see a way to specify the size. Is this how it should be done?
> 
> gint width, height;
> gtk_icon_size_lookup (GTK_ICON_SIZE_MENU, &width, &height);
> gtk_widget_set_size_request (spinner, width, height);

It uses the space that it's allocated.

> As it is, throbbers are often found in menubars, toolbars and tab labels, so
> one would want to have a reliable size.

It will use the space available. Having a set size doesn't help that. For the cases you mention, the spinner here will behave as expected.

> And I notice the code explicitly enforces a minimum of 12 pixels. That seems
> like a bad idea.

It's unrecognisable if smaller than 12 pixels. That seems like a fair decision to take.

My comment on this can be found further up in comment #29

(In reply to comment #37)
> My comment on this can be found further up in comment #29

Could you comment on the code being proposed instead of hypothetical code? Do we need to make the spinner in the GtkSpinner I proposed themable? Do I need to write a cell renderer before this patch would be accepted?

Yes, I think some form of themability is a precondition for this; the skeleton for a spinner cell renderer can be found in gnome-packagekit. It uses the old 'tiled png' approach to themed animation, which I don't really want here. In any case, the widget and the cell renderer should share the themed drawing code.

VMWare's libview library provides a Spinner written using Gtkmm: http://view.sourceforge.net/classes.php#spinner

Created attachment 144951 [details] [review]
Bug 319607 – Add a throbber (activity widget) to GTK+

Add GtkSpinner activity throbber.

(In reply to comment #41)
> Created an attachment (id=144951) [details]
> Bug 319607 – Add a throbber (activity widget) to GTK+
> 
> Add GtkSpinner activity throbber.

Bastien, what about a11y theming? How does it work with LargePrint and/or HighContrast stuff?

Patch at:
http://www.gnome.org/~hadess/0001-Bug-319607-Add-a-throbber-activity-widget-to-GTK.patch
because Bugzilla thinks my patch is empty.

- Adds a spinner widget, with a "num-steps" style
- Adds a spinner cell renderer, controlled by the pulse property
- Adds a draw_spinner vfunc to the GtkStyle class, as well as a default implementation based on the BlingSpinner
- Slight change to gtk_style_get_style_property() as the cell renderer would be checking the style property of GtkSpinner, and crashing if there wasn't a spinner instantiated (or a reference held to the class).
- test bits in gtk-demo for both widgets (in the "List Store" and "Spinner" demos)

The default implementation of the drawing is all cairo, so there's no image themes involved, and GTK+ themes can change the number of steps, as well as the implementation of it (and use images if they fancy).

GtkSpinner has no method to know if it's spinning or not (EphySpinner does, but I guess it could be argued its unnecessary, *shrug*), and has no API to set the spinner's icon-size (but the CellRenderer does have the property for it).

Nautilus also subclasses EphySpinner to add accessibility information (image dimensions, description) to it since the copy of the spinner in Epiphany doesn't have it; we could drop that subclass if it gets added in Gtk+.

Otherwise porting Nautilus (and, obviously Epiphany) is a fairly trivial patch. Thanks for the work on this Bastien.

(In reply to comment #44)
> GtkSpinner has no method to know if it's spinning or not (EphySpinner does, but
> I guess it could be argued its unnecessary, *shrug*)

What does Ephy use it for?

>, and has no API to set the
> spinner's icon-size (but the CellRenderer does have the property for it).

That's because the spinner will take the whole of the available size, like most other widgets.

> Nautilus also subclasses EphySpinner to add accessibility information (image
> dimensions, description) to it since the copy of the spinner in Epiphany
> doesn't have it; we could drop that subclass if it gets added in Gtk+.

There's no images used. What description would it need?

> Otherwise porting Nautilus (and, obviously Epiphany) is a fairly trivial patch.
> Thanks for the work on this Bastien.

Thanks for working on this Bastien,

Just my 2 cents, I think it'd be nice if we'd need to call gtk_spinner_stop as many time as we called gtk_spinner_start for the spinner to actually stop.  This would be useful in applications where multiple parts could signal that they are busy using the same spinner, which would only stop once all parts are done.

You current patch doesn't allow that AFAIK.

Building now. While I wait, some stylistic comments:


+  gtk_style_get_style_property (gtk_widget_get_style (window),
+				GTK_TYPE_SPINNER,
+				"num-steps",
+				&val);

This kinda highlights a problem with the style property, and with the restriction of pulse values to [0,num_steps]: It is not really clear where to get the style property from here, and one can only hope that the theme defined it with a meaningful value for the GtkWindow style. 


    P_("Pulse of the spinner, maximum value is GtkSpinner::num-steps"),

I don't think it is a good idea to have a property whose allowed range depends on the theme. Much better to allow any value and say that it is interpreted modulo ::num-steps, in my opinion.



+static void
+gtk_cell_renderer_spinner_set_property (GObject *object, guint param_id, const GValue *value, GParamSpec *pspec)

+static void
+gtk_cell_renderer_spinner_get_size (GtkCellRenderer *cellr,
+				    GtkWidget *widget,
+				    GdkRectangle *cell_area,
+				    gint *x_offset,
+				    gint *y_offset,
+				    gint *width,
+				    gint *height)

Please line those parameters up as we do elsewhere:

static void
gtk_cell_renderer_spinner_set_property (GObject      *object,
                                        guint         param_id, 
                                        const GValue *value, 
                                        GParamSpec   *pspec)

static void
gtk_cell_renderer_spinner_get_size (GtkCellRenderer *cellr,
				    GtkWidget       *widget,
				    GdkRectangle    *cell_area,
				    gint            *x_offset,
				    gint            *y_offset,
				    gint            *width,
				    gint            *height)



+  if ((width < 12) || (height <12))
+  {
+    gtk_widget_set_size_request (widget, 12, 12);
+  }

We omit {} in this case


+ priv->timeout = g_timeout_add (80, gtk_spinner_timeout, spinner);

Should use gdk_threads_add_timeout for automatic locking correctness.


> GtkSpinner has no method to know if it's spinning or not (EphySpinner does, but
> I guess it could be argued its unnecessary, *shrug*), and has no API to set the
> spinner's icon-size (but the CellRenderer does have the property for it).

I guess we could add an "active" property to go along with start/stop ?

And "active" property might also be useful for the cell renderer, together with the 'automatic animation' convenience api we discussed. 


+  if (priv->pulse < 0)
+    return;

I don't really like overloading pulse for controlling whether things get rendered or not, so again, "active" could be used here.


+  gtk_paint_spinner (widget->style,
+		     window,
+		     GTK_WIDGET_STATE (widget),


I think we probably need to do something more complicated involving the flags, to get the rendering right in all cases. See e.g. gtkcellrendererpixbuf.c


> Nautilus also subclasses EphySpinner to add accessibility information (image
> dimensions, description)

I guess it is a good idea to do the same thing nautilus does, and make GtkSpinner return an AtkImage as accessible.

Created attachment 145118 [details] [review]
Bug 319607 – Add a throbber (activity widget) to GTK+

Add GtkSpinner activity throbber, as well as a cell renderer.

(In reply to comment #47)
> Building now. While I wait, some stylistic comments:
> 
> 
> +  gtk_style_get_style_property (gtk_widget_get_style (window),
> +                GTK_TYPE_SPINNER,
> +                "num-steps",
> +                &val);
> 
> This kinda highlights a problem with the style property, and with the
> restriction of pulse values to [0,num_steps]: It is not really clear where to
> get the style property from here, and one can only hope that the theme defined
> it with a meaningful value for the GtkWindow style. 
> 
> 
>     P_("Pulse of the spinner, maximum value is GtkSpinner::num-steps"),
> 
> I don't think it is a good idea to have a property whose allowed range depends
> on the theme. Much better to allow any value and say that it is interpreted
> modulo ::num-steps, in my opinion.

The problem is that you'd be ever increasing the value, and at one point, you'll need to reset the number so it doesn't overflow. At that point, if you don't know the value of num-steps, you might miss some frame, and it'll look bad...

Other ideas?

> +static void
> +gtk_cell_renderer_spinner_set_property (GObject *object, guint param_id, const
> GValue *value, GParamSpec *pspec)
> 
> +static void
> +gtk_cell_renderer_spinner_get_size (GtkCellRenderer *cellr,
> +                    GtkWidget *widget,
> +                    GdkRectangle *cell_area,
> +                    gint *x_offset,
> +                    gint *y_offset,
> +                    gint *width,
> +                    gint *height)
> 
> Please line those parameters up as we do elsewhere:

I use 8-space tabs, which is also used in other parts of the sources. Does GTK+ use tabs of a different length? Want me to use spaces instead?

<snip>
> +  if ((width < 12) || (height <12))
> +  {
> +    gtk_widget_set_size_request (widget, 12, 12);
> +  }
> 
> We omit {} in this case

Done.

> + priv->timeout = g_timeout_add (80, gtk_spinner_timeout, spinner);
> 
> Should use gdk_threads_add_timeout for automatic locking correctness.

Done.

> > GtkSpinner has no method to know if it's spinning or not (EphySpinner does, but
> > I guess it could be argued its unnecessary, *shrug*), and has no API to set the
> > spinner's icon-size (but the CellRenderer does have the property for it).
> 
> I guess we could add an "active" property to go along with start/stop ?

Done for both.

> And "active" property might also be useful for the cell renderer, together with
> the 'automatic animation' convenience api we discussed. 
> 
> 
> +  if (priv->pulse < 0)
> +    return;
> 
> I don't really like overloading pulse for controlling whether things get
> rendered or not, so again, "active" could be used here.

Added.

> +  gtk_paint_spinner (widget->style,
> +             window,
> +             GTK_WIDGET_STATE (widget),
> 
> 
> I think we probably need to do something more complicated involving the flags,
> to get the rendering right in all cases. See e.g. gtkcellrendererpixbuf.c

Done.

Do we need to add a "follow-state" property like pixbuf?

> > Nautilus also subclasses EphySpinner to add accessibility information (image
> > dimensions, description)
> 
> I guess it is a good idea to do the same thing nautilus does, and make
> GtkSpinner return an AtkImage as accessible.

Done, although the code probably needs a look-over.

Did you consider letting the spinner show a pixbuf or a stock icon? I find this very useful in my own code since the spinner usually sits in a tab label but if there is no progress on-going, an icon is shown. I guess it should be easy enough to subclass and add that capability, just figured I'd mention it in case others find it useful as well.

(In reply to comment #50)
> Did you consider letting the spinner show a pixbuf or a stock icon? I find this
> very useful in my own code since the spinner usually sits in a tab label but if
> there is no progress on-going, an icon is shown. I guess it should be easy
> enough to subclass and add that capability, just figured I'd mention it in case
> others find it useful as well.

I'd rather this be done as a sub-class (would be easy with property binding, spinner "active" -> spinner "visible" & pixbuf "!visible").

> The problem is that you'd be ever increasing the value, and at one point,
> you'll need to reset the number so it doesn't overflow. At that point, if you
> don't know the value of num-steps, you might miss some frame, and it'll look
> bad...
>
> Other ideas?

A quick calculation shows that at 25 frames per second, a signed 32-bit counter will overflow after 994 days. I think one visual glitch every 3 years is very acceptable. You have to have really abysmal connectivity for your download spinner to spin for 3 years in the first place...

> I use 8-space tabs, which is also used in other parts of the sources. Does GTK+
> use tabs of a different length? Want me to use spaces instead?

While we are not religious about it, I prefer to avoid tab.

> Do we need to add a "follow-state" property like pixbuf?

I don't think so. Just always following the state seems like the right thing to do here. Do you disagree ?

(In reply to comment #52)
> > The problem is that you'd be ever increasing the value, and at one point,
> > you'll need to reset the number so it doesn't overflow. At that point, if you
> > don't know the value of num-steps, you might miss some frame, and it'll look
> > bad...
> >
> > Other ideas?
> 
> A quick calculation shows that at 25 frames per second, a signed 32-bit counter
> will overflow after 994 days. I think one visual glitch every 3 years is very
> acceptable. You have to have really abysmal connectivity for your download
> spinner to spin for 3 years in the first place...

Switched it to a guint now, which should double that (makes the list store code simpler too).

(In reply to comment #53)
> > I use 8-space tabs, which is also used in other parts of the sources. Does GTK+
> > use tabs of a different length? Want me to use spaces instead?
> 
> While we are not religious about it, I prefer to avoid tab.

OK.

> > Do we need to add a "follow-state" property like pixbuf?
> 
> I don't think so. Just always following the state seems like the right thing to
> do here. Do you disagree ?

Nope, just wondering.

Will upload new patch after a test run.

Created attachment 145420 [details] [review]
Bug 319607 – Add a throbber (activity widget) to GTK+

Add GtkSpinner activity throbber, as well as a cell renderer.

- Added docs reference
- Use guint for the pulse in the spinner
- Update demo for code changes
- Removed tabs when it made sense

The docs need some work, but we can handle that after the initial commit.
We can also add the cell renderer convenience api after the initial commit.

Please commit, then we can work out the remaining things:



+  /* GtkCellRendererSpinner::pulse:
+   *
+   * Pulse of the spinner.
+   *
+   * Since 2.20

Should probably say something about the need to increment this value yourself to animate the spinner, and the recommended rate. 


 * Returns a new cell renderer which will contain a spinner widget.

'contain a spinner widget' is somewhat unfortunate wording, since treeview cells don't contain widgets. How about 'which will render a 'spinner' animation to indicate activity. 

+  atk_object_set_name (accessible, _("spinner"));
+  atk_object_set_description (accessible, _("provides visual status"));

These should probably be capitalized ?

+  priv->timeout = gdk_threads_add_timeout (1000 / priv->num_steps, gtk_spinner_timeout, spinner)

The documentation for num-steps should probably mention that a full cycle is completed in one second - that might be important information for theme writers.


+ /**
+  * gtk_paint_spinner:

Please don't add doc comments for internal functions, it confuses gtk-doc. Just remove the second *.

(In reply to comment #57)
> The docs need some work, but we can handle that after the initial commit.
> We can also add the cell renderer convenience api after the initial commit.
> 
> Please commit, then we can work out the remaining things:
> 
> 
> 
> +  /* GtkCellRendererSpinner::pulse:
> +   *
> +   * Pulse of the spinner.
> +   *
> +   * Since 2.20
> 
> Should probably say something about the need to increment this value yourself
> to animate the spinner, and the recommended rate. 

Done.

>  * Returns a new cell renderer which will contain a spinner widget.
> 
> 'contain a spinner widget' is somewhat unfortunate wording, since treeview
> cells don't contain widgets. How about 'which will render a 'spinner' animation
> to indicate activity. 

Done.

> +  atk_object_set_name (accessible, _("spinner"));
> +  atk_object_set_description (accessible, _("provides visual status"));
> 
> These should probably be capitalized ?

Fixed.

> +  priv->timeout = gdk_threads_add_timeout (1000 / priv->num_steps,
> gtk_spinner_timeout, spinner)
> 
> The documentation for num-steps should probably mention that a full cycle is
> completed in one second - that might be important information for theme
> writers.

Done.

> + /**
> +  * gtk_paint_spinner:
> 
> Please don't add doc comments for internal functions, it confuses gtk-doc. Just
> remove the second *.

It's a public API.

Will leave open for someone to tackle a convenience API for the GtkCellRendererSpinner.

Some review of gtkspinner.c as it is on master right now:

* GtkSpinner shouldn't subclass DrawingArea, there's no point. All DrawingArea is, is a convenience widget that lets you draw something custom without having to write a subclass. Ergo, subclassing it is always wrong. In fact it looks likely that just s/DrawingArea/Widget/ in gtkspinner.[hc] would work without having to make any other changes.

* gtk_spinner_screen_changed () handler that switches to RGBA colormap? This is not normal GtkWidget behavior ... why would spinner do this when other widgets don't?

* timeout should add/remove in map/unmap, not realize/unrealize. Otherwise hiding the spinner won't stop it

* This code in expose event handler:
if ((width < 12) || (height <12))
    gtk_widget_set_size_request (widget, 12, 12);

Um, no. Just no. Implement GtkSizeRequest and set a min size of 12 there.

Created attachment 170267 [details] [review]
timeout should add/remove in map/unmap, not realize/unrealize

(In reply to comment #59)
> Some review of gtkspinner.c as it is on master right now:
> 
> * GtkSpinner shouldn't subclass DrawingArea, there's no point. All DrawingArea
> is, is a convenience widget that lets you draw something custom without having
> to write a subclass. Ergo, subclassing it is always wrong. In fact it looks
> likely that just s/DrawingArea/Widget/ in gtkspinner.[hc] would work without
> having to make any other changes.

Fixed in master

> * gtk_spinner_screen_changed () handler that switches to RGBA colormap? This is
> not normal GtkWidget behavior ... why would spinner do this when other widgets
> don't?

Still pending

> * timeout should add/remove in map/unmap, not realize/unrealize. Otherwise
> hiding the spinner won't stop it

Patch attached
 
> * This code in expose event handler:
> if ((width < 12) || (height <12))
>     gtk_widget_set_size_request (widget, 12, 12);
> 
> Um, no. Just no. Implement GtkSizeRequest and set a min size of 12 there.

Fixed in master

Review of attachment 170267 [details] [review]:

::: gtk/gtkspinner.c
@@ +328,1 @@
+  if (gtk_widget_get_realized (widget) && !gtk_widget_get_mapped (widget))

I don't think you need this check. GTK won't call map() unless you're realized and not yet mapped.

@@ +343,1 @@
+  if (gtk_widget_get_mapped (widget))

same here, not necessary.

Created attachment 170311 [details] [review]
timeout should add/remove in map/unmap, not realize/unrealize.v2

Updated patch with your comments.

Review of attachment 170311 [details] [review]:

Looks good, please commit

Comment on attachment 170311 [details] [review]
timeout should add/remove in map/unmap, not realize/unrealize.v2

commit f5a06df3dc0b21cd98afcec47bce085df6d7c4a3

The sreen_changed thing is fixed in http://git.gnome.org/browse/gtk+/commit/?id=c516765bc8665b32b6a7d5be8b2a95e9545a44e4 which is one of the fixes hiding in my rendering cleanup branch(es).

(In reply to comment #63)
> Created an attachment (id=170311) [details] [review]
> timeout should add/remove in map/unmap, not realize/unrealize.v2
> 
> Updated patch with your comments.

Note that a widget usually is created, added to a parent, realized, mapped, unrealized, removed and destroyed. I.e. there's usually *no* unmap on a widget, map/unmap are not guaranteed to occour in pairs on  widgets like realize/unrealize. So you most often want to setup visually important things in map and and tear them down in unrealize.

Tim the code looks like unmap is guaranteed to me. (Why wouldn't it be? If it isn't we should fix it, that's totally annoying.)

invariant 2) GTK_WIDGET_MAPPED (widget) => GTK_WIDGET_REALIZED (widget) from widget_system.txt implies that we must unmap to unrealize

top of real_unrealize does 
 if (gtk_widget_get_mapped (widget))
    gtk_widget_real_unmap (widget);

  gtk_widget_set_mapped (widget, FALSE);

I guess something could fail to chain up to that but then it's broken

Teardown in unrealize only wouldn't work anyhow of course, you'd still be sitting there running the spinner if gtk_widget_hide() were called

The very best solution would be a master clock in GTK as clutter and mozilla now have, which could be even smarter and shut down if the toplevel were minimized or on another desktop or if the spinner is hidden due to being on a non-current notebook tab. But I think we can safely leave that to another patch ;-)

unmap is guaranteed. If it isn't in GTK2, we'll guarantee it in GTK3. :)

(In reply to comment #68)
> Tim the code looks like unmap is guaranteed to me. (Why wouldn't it be? If it
> isn't we should fix it, that's totally annoying.)

This has been reconsidered a couple years ago already, back then federico raised it IIRC. 

> invariant 2) GTK_WIDGET_MAPPED (widget) => GTK_WIDGET_REALIZED (widget) from
> widget_system.txt implies that we must unmap to unrealize

The invariants cover just widget flags, and the flags are properly set here:
  static void
  gtk_widget_real_unrealize (GtkWidget *widget)
  {
    if (gtk_widget_get_mapped (widget))
      gtk_widget_real_unmap (widget);
  [...]

But as you can see, this is the real_unrealize handler, and not gtk_widget_unrealize() which actually emits signals. So there's no ::unmap signal emission here, just flag adjustments in the unrealize handler. That means e.g. button_real_unmap is *not* called while button_real_unrealize *is* called through a signal emission.

(In reply to comment #70)
> unmap is guaranteed. If it isn't in GTK2, we'll guarantee it in GTK3. :)

GTK+ has always skipped the ::unmap signal emission and when Federico pointed this out in a bug report last time, we left it like that because it was a significatn performance optimization.

Without digging up the prior discussion (mailing list or bugzilla?), surely with client side windows the performance optimization will have gone away? (What the heck was slow about unmap() anyhow? maybe we somehow had an X round trip?)
I guess we're getting a little off-topic. 

I opened bug 629923 for this topic

I guess the spinner needs some sort of fix in the meantime though I don't actually see how you'd fix it without continuing to spin while hidden. 
notify::mapped maybe?

(In reply to comment #72)
> Without digging up the prior discussion (mailing list or bugzilla?), surely
> with client side windows the performance optimization will have gone away?
> (What the heck was slow about unmap() anyhow? maybe we somehow had an X round
> trip?)

On a window hierarchy with n subwindows, emitting unmap recursively is sending n unmap request to the Xserver. Omitting this, you can just call unrealize (XDestroyWindow) once on the toplevel.

(In reply to comment #73)
> (In reply to comment #72)
> > Without digging up the prior discussion (mailing list or bugzilla?), surely
> > with client side windows the performance optimization will have gone away?
> > (What the heck was slow about unmap() anyhow? maybe we somehow had an X round
> > trip?)
> 
> On a window hierarchy with n subwindows, emitting unmap recursively is sending
> n unmap request to the Xserver. Omitting this, you can just call unrealize
> (XDestroyWindow) once on the toplevel.

So csw should indeed make this largely a non-issue, already.

So, seems the latest issue here is:

(In reply to comment #59)
> Some review of gtkspinner.c as it is on master right now:
> 
> * gtk_spinner_screen_changed () handler that switches to RGBA colormap? This is
> not normal GtkWidget behavior ... why would spinner do this when other widgets
> don't?

Any comments?

About always calling unmap() when unsetting MAPPED flag see bug #629923

Afaik that code was never used, because it's never called on NO_WINDOW widgets. It's why I removed it in master.