After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 319607 - Add convenience API for GtkCellRendererSpinner
Add convenience API for GtkCellRendererSpinner
Status: RESOLVED OBSOLETE
Product: gtk+
Classification: Platform
Component: Widget: Other
2.91.x
Other All
: Normal normal
: Medium feature
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2005-10-24 14:09 UTC by Luca Ferretti
Modified: 2013-02-13 04:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-Bug-319607-Add-a-throbber-activity-widget-to-G.patch (13.81 KB, patch)
2009-07-21 01:19 UTC, Bastien Nocera
none Details | Review
Screenshot-GtkSpinner.png (9.53 KB, image/png)
2009-07-21 01:24 UTC, Bastien Nocera
  Details
0001-Bug-319607-Add-a-throbber-activity-widget-to-G.patch (13.70 KB, patch)
2009-07-21 16:32 UTC, Bastien Nocera
none Details | Review
Bug 319607 – Add a throbber (activity widget) to GTK+ (13.64 KB, patch)
2009-10-07 11:50 UTC, Bastien Nocera
none Details | Review
Bug 319607 – Add a throbber (activity widget) to GTK+ (44.91 KB, patch)
2009-10-09 11:00 UTC, Bastien Nocera
none Details | Review
Bug 319607 – Add a throbber (activity widget) to GTK+ (56.45 KB, patch)
2009-10-14 13:21 UTC, Bastien Nocera
none Details | Review
timeout should add/remove in map/unmap, not realize/unrealize (3.01 KB, patch)
2010-09-14 18:11 UTC, Javier Jardón (IRC: jjardon)
none Details | Review
timeout should add/remove in map/unmap, not realize/unrealize.v2 (2.79 KB, patch)
2010-09-15 01:25 UTC, Javier Jardón (IRC: jjardon)
committed Details | Review

Description Luca Ferretti 2005-10-24 14:09:15 UTC
A spinner is a control that shows an animation when a process is running. A
spinner can be defined as a "asynchronus progress indicator".

Examples are the one in Nautilus (browser) toolbar or Epiphany toolbar. Also
FireFox menubar.

I suppose a GTK+ developer knows what a spinner is, so I stop the description here.

 - Functions

I don't thing this kind ow widget needs too much control functions. Just
something to start the animation when the process start and to stop when the
process finish. No needs to get data from it or update while running (as well as
a progress bar).

 - UI

IMHO developers should be able to place a spinner in toolbar as well as in the
middle of a dialog (see the login on MacOSX or the spinner on tabs in Epiphany).
So a GtkSpinnerToolButton (for toolbars) and a GtkSpinner (elsewhere) widget
could be both useful.

 - Themability

The current implementation in Nautilus and Epiphany is using 2 images from
gnome-icon-theme: gnome-spinner-rest.png and gnome-spinner.png. The fist is used
when the spinner is stopped. Also gnome-spinner-rest.png (its size) is used to
count rows and columns in gnome-spinner.png.

I don't know if it's a good idea let the icon theme sets the spinner appearance.
Should instead the GTK+ engine to draw it? Or maybe the GtkSpinner should
support MNG animations? Or SVG animations too??? Dunno.

But please note that we need to render the spinner at different sizes: small in
Epiphany tabs, 24x24 on icons-only-toolbars (48x48 for LargePrint a11y themes)
and ??x?? (depending on font size) on icons-and-text-toolbars. Now that we
support cairo, is more simple draw it directly in theme engine, isn't it? Maybe
grabbing color from theme.

 - Default Appearance

I really love the FireFox spinner. Other good templates are the wait mouse
cursor in Industrial theme (see jimmac webapages), the spinner in Bluecurve
theme, the MacOSX asynchronous progress indicator and the Nokia 770 spinner.
Comment 1 Luca Ferretti 2005-10-24 14:12:28 UTC
Forgot to mention: this could be stuff for ProjectRidley
Comment 2 Matthias Clasen 2005-10-25 18:14:42 UTC
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.
Comment 3 Dan Winship 2005-11-04 16:22:50 UTC
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.
Comment 4 Matthias Clasen 2005-11-04 16:27:08 UTC
gdk-pixbuf supports constructing an animation from a series of frames now...
Comment 5 Dan Winship 2005-11-04 19:17:07 UTC
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.
Comment 6 Luca Ferretti 2005-12-08 18:14:01 UTC
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 ;-)
Comment 7 Matthias Clasen 2005-12-08 23:56:26 UTC
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
Comment 8 Luca Ferretti 2006-01-20 13:30:45 UTC
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?
Comment 9 Matthias Clasen 2006-01-20 20:55:47 UTC
Well, nobody explained to me yet what GtkImage is missing, really. Do you think that will happen in the wiki ?
Comment 10 Dan Winship 2006-01-20 23:07:51 UTC
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.)
Comment 11 Luca Ferretti 2006-01-22 14:40:31 UTC
> 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. ;-)
Comment 12 Matthias Clasen 2006-01-23 13:47:05 UTC
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.
Comment 13 Luca Ferretti 2006-01-23 15:40:41 UTC
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.
Comment 14 Matthias Clasen 2006-01-23 16:00:28 UTC
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
Comment 15 Rodney Dawes 2006-07-29 12:06:16 UTC
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.
Comment 16 Matthias Clasen 2006-07-29 14:22:15 UTC
I have brought it up, and proposed a more capable mechanism than the hack currently in use.
Comment 17 Rodney Dawes 2006-07-29 15:47:26 UTC
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?
Comment 18 Matthias Clasen 2006-07-30 04:54:27 UTC
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
Comment 19 Bastien Nocera 2007-04-09 21:15:02 UTC
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.
Comment 20 Luca Ferretti 2007-11-20 21:42:42 UTC
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
Comment 21 Christian Dywan 2008-09-24 16:30:32 UTC
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".
Comment 22 Matthias Clasen 2008-09-24 16:34:23 UTC
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...
Comment 23 Christian Dywan 2008-09-24 16:55:28 UTC
(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.
Comment 24 Matthias Clasen 2008-09-25 04:08:20 UTC
Sorry for being terse. 
I was referring to GdkPixbufSimpleAnim
Comment 25 Bastien Nocera 2009-04-17 23:58:45 UTC
(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.
Comment 26 Matthias Clasen 2009-04-18 03:37:54 UTC
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 27 Bastien Nocera 2009-04-20 13:12:22 UTC
(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+?
Comment 28 Christian Dywan 2009-04-20 14:04:28 UTC
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.
Comment 29 Matthias Clasen 2009-04-20 14:24:32 UTC
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
Comment 30 Cody Russell 2009-04-26 18:30:09 UTC
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.
Comment 31 Bastien Nocera 2009-07-21 01:19:44 UTC
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.
Comment 32 Bastien Nocera 2009-07-21 01:24:15 UTC
Created attachment 138870 [details]
Screenshot-GtkSpinner.png

And this is what it looks like.
Comment 33 Paolo Borelli 2009-07-21 15:59:48 UTC
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)
Comment 34 Bastien Nocera 2009-07-21 16:32:19 UTC
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
Comment 35 Christian Dywan 2009-07-22 00:06:58 UTC
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.
Comment 36 Bastien Nocera 2009-07-22 00:11:11 UTC
(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.
Comment 37 Matthias Clasen 2009-07-22 00:25:48 UTC
My comment on this can be found further up in comment #29
Comment 38 Bastien Nocera 2009-07-22 00:53:27 UTC
(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?
Comment 39 Matthias Clasen 2009-07-22 02:04:35 UTC
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.

Comment 40 Debarshi Ray 2009-09-30 22:42:56 UTC
VMWare's libview library provides a Spinner written using Gtkmm: http://view.sourceforge.net/classes.php#spinner
Comment 41 Bastien Nocera 2009-10-07 11:50:21 UTC
Created attachment 144951 [details] [review]
Bug 319607 – Add a throbber (activity widget) to GTK+

Add GtkSpinner activity throbber.
Comment 42 Luca Ferretti 2009-10-07 12:37:55 UTC
(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?
Comment 43 Bastien Nocera 2009-10-08 15:14:12 UTC
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).
Comment 44 A. Walton 2009-10-08 17:28:48 UTC
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.
Comment 45 Bastien Nocera 2009-10-08 19:49:58 UTC
(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.
Comment 46 Pierre-Luc Beaudoin 2009-10-08 20:04:42 UTC
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.
Comment 47 Matthias Clasen 2009-10-08 21:25:21 UTC
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.
Comment 48 Bastien Nocera 2009-10-09 11:00:40 UTC
Created attachment 145118 [details] [review]
Bug 319607 – Add a throbber (activity widget) to GTK+

Add GtkSpinner activity throbber, as well as a cell renderer.
Comment 49 Bastien Nocera 2009-10-09 11:01:43 UTC
(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.
Comment 50 Christian Dywan 2009-10-10 15:33:15 UTC
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.
Comment 51 Bastien Nocera 2009-10-11 00:40:52 UTC
(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").
Comment 52 Matthias Clasen 2009-10-13 15:20:17 UTC
> 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...
Comment 53 Matthias Clasen 2009-10-13 15:22:14 UTC
> 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 ?
Comment 54 Bastien Nocera 2009-10-14 12:40:40 UTC
(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.
Comment 55 Bastien Nocera 2009-10-14 13:21:33 UTC
Created attachment 145420 [details] [review]
Bug 319607 – Add a throbber (activity widget) to GTK+

Add GtkSpinner activity throbber, as well as a cell renderer.
Comment 56 Bastien Nocera 2009-10-14 13:26:45 UTC
- Added docs reference
- Use guint for the pulse in the spinner
- Update demo for code changes
- Removed tabs when it made sense
Comment 57 Matthias Clasen 2009-10-14 13:40:22 UTC
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 *.
Comment 58 Bastien Nocera 2009-10-14 15:02:49 UTC
(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.
Comment 59 Havoc Pennington 2010-09-13 16:27:36 UTC
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.
Comment 60 Javier Jardón (IRC: jjardon) 2010-09-14 18:11:22 UTC
Created attachment 170267 [details] [review]
timeout should add/remove in map/unmap, not realize/unrealize
Comment 61 Javier Jardón (IRC: jjardon) 2010-09-14 18:12:49 UTC
(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
Comment 62 Havoc Pennington 2010-09-14 18:49:00 UTC
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.
Comment 63 Javier Jardón (IRC: jjardon) 2010-09-15 01:25:49 UTC
Created attachment 170311 [details] [review]
timeout should add/remove in map/unmap, not realize/unrealize.v2

Updated patch with your comments.
Comment 64 Matthias Clasen 2010-09-15 01:54:58 UTC
Review of attachment 170311 [details] [review]:

Looks good, please commit
Comment 65 Javier Jardón (IRC: jjardon) 2010-09-15 02:02:25 UTC
Comment on attachment 170311 [details] [review]
timeout should add/remove in map/unmap, not realize/unrealize.v2

commit f5a06df3dc0b21cd98afcec47bce085df6d7c4a3
Comment 66 Benjamin Otte (Company) 2010-09-15 02:07:28 UTC
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).
Comment 67 Tim Janik 2010-09-17 10:55:00 UTC
(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.
Comment 68 Havoc Pennington 2010-09-17 13:22:35 UTC
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
Comment 69 Havoc Pennington 2010-09-17 13:29:17 UTC
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 ;-)
Comment 70 Benjamin Otte (Company) 2010-09-17 14:03:21 UTC
unmap is guaranteed. If it isn't in GTK2, we'll guarantee it in GTK3. :)
Comment 71 Tim Janik 2010-09-17 14:19:35 UTC
(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.
Comment 72 Havoc Pennington 2010-09-17 14:34:57 UTC
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?
Comment 73 Tim Janik 2010-09-17 14:59:39 UTC
(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.
Comment 74 Matthias Clasen 2010-09-17 16:28:23 UTC
(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.
Comment 75 Javier Jardón (IRC: jjardon) 2010-12-01 14:08:39 UTC
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
Comment 76 Benjamin Otte (Company) 2010-12-01 15:49:08 UTC
Afaik that code was never used, because it's never called on NO_WINDOW widgets. It's why I removed it in master.