GNOME Bugzilla – Bug 349808
GtkRange: add stream prebuffering indicator
Last modified: 2018-04-15 00:17:12 UTC
Attached patch from maemo-gtk adds a prebuffering indicator to GtkRange which can be used when the range is used for displaying playback position in a streamed media file. It's actually a esoteric thing, but the feature is pretty useful. It's also hard-to-impossible to implement in a subclass without reimplementing the entire range rendering. The patch works for all kinds of ranges, but still has proof-of-concept quality because I don't like the property names and API yet. Comments?
Created attachment 70140 [details] [review] Patch implementing the above
interesting. i'm pondering whether the notion of "fill level" would be more appropriate than "stream indicator", because it's a bit more genereic. in any case, can you please provide some example/testing code for testgtk or gtk-demo (preferred), so we actually get a chance to test and explore this new feature?
Yes I agree fill-indicator is more generic, will change the patch accordingly and look into a test case.
Created attachment 75370 [details] [review] Updated patch - changed properties/API to "fill-level" and "show-fill-level" - patch is more local and cleaner - rendering of fill level indicator is pixel-perfect now
Comment on attachment 75370 [details] [review] Updated patch >Index: gtk/gtkrange.c >=================================================================== >RCS file: /cvs/gnome/gtk+/gtk/gtkrange.c,v >retrieving revision 1.129 >diff -u -p -r1.129 gtkrange.c >--- gtk/gtkrange.c 8 Oct 2006 05:07:15 -0000 1.129 >+++ gtk/gtkrange.c 25 Oct 2006 13:46:44 -0000 >@@ -46,7 +46,9 @@ enum { > PROP_ADJUSTMENT, > PROP_INVERTED, > PROP_LOWER_STEPPER_SENSITIVITY, >- PROP_UPPER_STEPPER_SENSITIVITY >+ PROP_UPPER_STEPPER_SENSITIVITY, >+ PROP_SHOW_FILL_LEVEL, hm, do we have precedence for a second ::show_* property? alternatives that i see are: - don't display fill-level if it is set to something <0 - call the proeprty ::fille-level-display >+ PROP_FILL_LEVEL > }; > > enum { >@@ -351,6 +353,38 @@ gtk_range_class_init (GtkRangeClass *cla > GTK_SENSITIVITY_AUTO, > GTK_PARAM_READWRITE)); > >+ /** >+ * GtkRange:show-fill-level: >+ * >+ * Whether to display a fill level indicator graphics on trough. >+ * >+ * Since: 2.12 >+ **/ >+ g_object_class_install_property (gobject_class, >+ PROP_SHOW_FILL_LEVEL, >+ g_param_spec_boolean ("show-fill-level", >+ P_("Show Fill Level"), >+ P_("Whether to display a fill level indicator graphics on trough."), >+ FALSE, >+ GTK_PARAM_READWRITE)); >+ >+ /** >+ * GtkRange:fill-level: >+ * >+ * The fill level (e.g. prebuffering of a network stream). >+ * >+ * Since: 2.12 >+ **/ >+ g_object_class_install_property (gobject_class, >+ PROP_FILL_LEVEL, >+ g_param_spec_double ("fill-level", >+ P_("Fill Level"), >+ P_("The fill level."), >+ -G_MAXDOUBLE, >+ G_MAXDOUBLE, >+ 0.0, what exactly is the use case here for fille-level < 0, if it's not used to avoid drawing of fill-level alltogether? >+ GTK_PARAM_READWRITE)); >+ > gtk_widget_class_install_style_property (widget_class, > g_param_spec_int ("slider-width", > P_("Slider Width"), >@@ -472,6 +506,12 @@ gtk_range_set_property (GObject *ob > case PROP_UPPER_STEPPER_SENSITIVITY: > gtk_range_set_upper_stepper_sensitivity (range, g_value_get_enum (value)); > break; >+ case PROP_SHOW_FILL_LEVEL: >+ gtk_range_set_show_fill_level (range, g_value_get_boolean (value)); >+ break; >+ case PROP_FILL_LEVEL: >+ gtk_range_set_fill_level (range, g_value_get_double (value)); >+ break; > default: > G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); > break; >@@ -505,6 +545,12 @@ gtk_range_get_property (GObject *ob > case PROP_UPPER_STEPPER_SENSITIVITY: > g_value_set_enum (value, gtk_range_get_upper_stepper_sensitivity (range)); > break; >+ case PROP_SHOW_FILL_LEVEL: >+ g_value_set_boolean (value, gtk_range_get_show_fill_level (range)); >+ break; >+ case PROP_FILL_LEVEL: >+ g_value_set_double (value, gtk_range_get_fill_level (range)); >+ break; > default: > G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); > break; >@@ -848,6 +894,9 @@ gtk_range_set_range (GtkRange *range, > range->adjustment->lower, > (range->adjustment->upper - range->adjustment->page_size)); > >+ if (gtk_range_get_show_fill_level (range)) >+ value = MIN (value, gtk_range_get_fill_level (range)); >+ MIN() evaluates it's arguments more than once. so it'd be better to store fille-level into a variable here before using it in the macro. > gtk_adjustment_set_value (range->adjustment, value); > gtk_adjustment_changed (range->adjustment); > } >@@ -872,6 +921,9 @@ gtk_range_set_value (GtkRange *range, > value = CLAMP (value, range->adjustment->lower, > (range->adjustment->upper - range->adjustment->page_size)); > >+ if (gtk_range_get_show_fill_level (range)) >+ value = MIN (value, gtk_range_get_fill_level (range)); >+ dito. however i don't quite get why fill-level would affect the adjustement value at all. i thought the fill-level would be an indicator orthogonal to the adjustment value... > gtk_adjustment_set_value (range->adjustment, value); > } > >@@ -891,6 +943,118 @@ gtk_range_get_value (GtkRange *range) > return range->adjustment->value; > } > >+/** >+ * gtk_range_set_show_fill_level: >+ * @range: A #GtkRange >+ * @show_fill_level: Whether a fill level indicator graphics is shown and >+ * restricts the slider. >+ * >+ * Sets whether a graphical fill level is show on the trough and >+ * the slider is restricted to the filled area. >+ * >+ * Since: 2.12 >+ **/ >+void >+gtk_range_set_show_fill_level (GtkRange *range, >+ gboolean show_fill_level) >+{ >+ g_return_if_fail (GTK_IS_RANGE (range)); >+ >+ show_fill_level = show_fill_level ? TRUE : FALSE; >+ >+ if (show_fill_level != gtk_range_get_show_fill_level (range)) >+ { >+ g_object_set_data (G_OBJECT (range), "gtk-range-show-fill-level", >+ GINT_TO_POINTER (show_fill_level)); >+ g_object_notify (G_OBJECT (range), "show-fill-level"); >+ gtk_widget_queue_draw (GTK_WIDGET (range)); >+ >+ gtk_range_set_value (range, gtk_range_get_value (range)); this effectively means that toggling the ::show-fill-level property possibly changes the current adjustment->value..., right? i don't think that'd really be expected by applications (which may use adjustments as arbitrary models). >+ } >+} >+ >+/** >+ * gtk_range_get_show_fill_level: >+ * @range: A #GtkRange >+ * >+ * Return value: Whether GtkRange displays a fill level >+ * graphics and slider is restricted to the filled area hmmmm. i *kinda* see the point of restricting the slider. however i'd lean towards having the applicaiton implement this. as use case, let's say the fill-level indicator is not used as a download mark, but indicates the degree to which a randomly accessible WAV file has been compressed to OGG. in this case, it does still make sense to allow seek positions on the entire stream (adjustment->value) while not everything has been compressed (fill_level < adjustment->upper). so validly, adjustment->value > fill_level can occour here. >+ * >+ * Since: 2.12 >+ **/ >+gboolean >+gtk_range_get_show_fill_level (GtkRange *range) >+{ >+ g_return_val_if_fail (GTK_IS_RANGE (range), FALSE); >+ >+ return GPOINTER_TO_INT (g_object_get_data (G_OBJECT (range), >+ "gtk-range-show-fill-level")); >+} >+ >+/** >+ * gtk_range_set_fill_level: >+ * @range: A #GtkRange >+ * @positon: The new position of the fill level indicator >+ * >+ * Sets the new position of the fill level indicator. It is automatically >+ * clamped between lower and upper. Note that you need to enable >+ * show_fill_level before any fill-level functionality is active. >+ * >+ * Since: 2.12 >+ **/ >+void >+gtk_range_set_fill_level (GtkRange *range, >+ gdouble fill_level) >+{ >+ g_return_if_fail (GTK_IS_RANGE (range)); >+ >+ fill_level = CLAMP (fill_level, >+ range->adjustment->lower, >+ range->adjustment->upper - range->adjustment->page_size); >+ >+ if (fill_level != gtk_range_get_fill_level (range)) >+ { >+ gdouble *val = g_new (gdouble, 1); >+ >+ *val = fill_level; >+ >+ g_object_set_data_full (G_OBJECT (range), "gtk-range-fill-level", >+ val, (GDestroyNotify) g_free); >+ g_object_notify (G_OBJECT (range), "fill-level"); >+ >+ if (gtk_range_get_show_fill_level (range)) >+ { >+ gtk_widget_queue_draw (GTK_WIDGET (range)); >+ gtk_range_set_value (range, gtk_range_get_value (range)); >+ } >+ } >+} >+ >+/** >+ * gtk_range_get_fill_level: >+ * @range : A #GtkRange >+ * >+ * Return value: The current position of the fill level indicator. Note >+ * that this value is undefined when the fill-level indicator >+ * is not enabled. >+ * >+ * Since: 2.12 >+ **/ >+gdouble >+gtk_range_get_fill_level (GtkRange *range) >+{ >+ gdouble *val; >+ >+ g_return_val_if_fail (GTK_IS_RANGE (range), 0.0); >+ >+ val = g_object_get_data (G_OBJECT (range), "gtk-range-fill-level"); >+ >+ if (val) >+ return *val; >+ >+ return 0; >+} >+ > static gboolean > should_invert (GtkRange *range) > { [...] as requested in an earlier comment, can you please also provide a gtk-demo window that demonstrates this feature? e.g. by setting fill-level of a slider in a timeout.
>hm, do we have precedence for a second ::show_* property? >alternatives that i see are: >- don't display fill-level if it is set to something <0 >- call the proeprty ::fille-level-display There can't be a special value incicating anything, since fill-level must have the same possible range as adjustment->value. And yes, there are many show-foo properties in GTK+ >what exactly is the use case here for fille-level < 0, if it's not used to >avoid drawing of fill-level alltogether? See above. >MIN() evaluates it's arguments more than once. so it'd be better >to store fille-level into a variable here before using it in the macro. Right. >however i don't quite get why fill-level would affect the adjustement value at >all. >i thought the fill-level would be an indicator orthogonal to the adjustment >value... >this effectively means that toggling the ::show-fill-level property possibly >changes the current adjustment->value..., right? i don't think that'd really be >expected by applications (which may use adjustments as arbitrary models). >hmmmm. i *kinda* see the point of restricting the slider. however i'd lean >towards having the applicaiton implement this. >as use case, let's say the fill-level indicator is not used as a download mark, >but indicates the degree to which a randomly accessible WAV file has been >compressed to OGG. in this case, it does still make sense to allow seek >positions on the entire stream (adjustment->value) while not everything has >been compressed (fill_level < adjustment->upper). so validly, adjustment->value >> fill_level can occour here. Doing this in the adjustment's "value-changed" callback means that the user and the callback would be fighting for the value of the adjustment, causing possible jumping. But I see your point about use cases other than download prebuffering. Perhaps we simply need another property for this? "restrict-to-fill-level" or something. >as requested in an earlier comment, can you please also provide a gtk-demo >window that demonstrates this feature? >e.g. by setting fill-level of a slider in a timeout. Sure, but you can already easily test it by using the properties dialog in testgtk.
(In reply to comment #6) > >hm, do we have precedence for a second ::show_* property? > There can't be a special value incicating anything, since fill-level > must have the same possible range as adjustment->value. And yes, > there are many show-foo properties in GTK+ ok, i'll shut up then. > >hmmmm. i *kinda* see the point of restricting the slider. however i'd lean > >towards having the applicaiton implement this. > >as use case, let's say the fill-level indicator is not used as a download mark, > >but indicates the degree to which a randomly accessible WAV file has been > >compressed to OGG. in this case, it does still make sense to allow seek > >positions on the entire stream (adjustment->value) while not everything has > >been compressed (fill_level < adjustment->upper). so validly, adjustment->value > >> fill_level can occour here. > > Doing this in the adjustment's "value-changed" callback means that > the user and the callback would be fighting for the value of the > adjustment, causing possible jumping. no, adjustments are intentionally designed to work well in this case. actually (if you forgive some anecdotal remarks) RUN_NO_RECURSE signals were exactly implemented for the type of notification GtkAdjustemnt::value-changed presents. i.e. to allow multiple handlers to impose their constraints on a value and cause re-emissions of the assorted notification signal until no further changes are required. GtkRange is supposed to not jump during this "negotiation phase" and the current implementation also doesn't do it. > But I see your point about > use cases other than download prebuffering. Perhaps we simply need > another property for this? "restrict-to-fill-level" or something. this is certainly an alternative solution (and it would probably make sense to have restrict-to-fill-level default to TRUE because that can reasonably expected to be the common case). however, i'd say it is not strictly necessary and i think it wouldn't be too bad if the user had to implement this himself if desired via a signal handler. > >as requested in an earlier comment, can you please also provide a gtk-demo > >window that demonstrates this feature? > >e.g. by setting fill-level of a slider in a timeout. > > Sure, but you can already easily test it by using the properties dialog > in testgtk. maybe, but testgtk is know to contain bad example code and not be really user digestible. this is exactly what gtk-demo tries to fix, consequently it makes sense to demonstrate new features (and old features lacking presentation so far) in gtk-demo.
Created attachment 75453 [details] [review] Updated patch - Fixed double evaluation in MIN() - Adds "restrict-to-fill-level" property Test case follows.
hm, looking at your new patch, fill-level restriction is now bound to show-fill-level (which is arguably a GUI/look setting only) and restrict-to-fill-level (which makes sense, judging from the property name): + if (gtk_range_get_show_fill_level (range) && + gtk_range_get_restrict_to_fill_level (range)) API wise, i'd actually expect/prefer: 1) let ::show-fill-level default to OFF (covers common case) 2) let ::restrict-to-fill-level default to TRUE (common case) 3) let ::fill-level default to G_MAXDOUBLE (allows any adjustment->value per default) 4) restrict adjustment->value iff ::restrict-to-fill-level is TRUE. i.e. regardless of the current value of ::show-fill-level. of these, your patch dpes (1) and (2), but would need adaptions for (3) and (4).
I disargee. The fill level should IMHO never restrict the range if it is not visible. The actual feature to be added here is the visual indicator, the restricting is just an extra goodie that could be implemented in user callbacks.
Created attachment 75665 [details] [review] Next patch version - restrict-to-fill-level defaults to TRUE - fill-leve- defaults to G_MAXDOUBLE - restricting is controlled by restrict-to-fill-level only - converted GtkRangeLayout to a g_type private struct - store fill-level stuff there, instead of attaching it - make sure we clamp everything correctly
the patch looks mostly ok, except that: - fill_level should never be modified beyond the value set by the user. in particular, this line should be removed: gtk_range_set_fill_level (GtkRange *range, gdouble fill_level) { g_return_if_fail (GTK_IS_RANGE (range)); + fill_level = MAX (fill_level, range->adjustment->lower); the reson is that without fill_level being part of the adjustment and thus always getting properly clamped/readjusted etc if any party does GtkAdjustment::changed or ::value-changed, keeping the fill-level up to date will be very hard for the user. GtkRange doesn't need this restriction though, it can simply use value = MIN (adjustment->value, MAX (lower, fill_level)); and thus spare the user the programming and debugging work of always ensuring that gtk_range_set_fill_level() gets called after ::changed and ::value-changed has been emitted by anyone, and that it always gets called *after* those signals. (i.e. this simplification can essentially spare the user the need to set up any adjustment callbacks.) other than that, once we have the test case promised in Comment #8, i'd say it can go in ;)
Created attachment 76564 [details] [review] Implements the change in tim's last comment
I'm ok with the general concept of fill-level. Some comments on the patch: The docs seem to explain too little about the concept and many of the doc comments seem incomplete. It would be good to have a paragraph in the GtkRange long desc. explaining the general concept and maybe giving an example. Also, a cool new feature like this should have a cool demo in gtk-demo... Have you tried this with different themes, does it generally look ok ?
(In reply to comment #13) > Created an attachment (id=76564) [edit] > Implements the change in tim's last comment i think the patch looks technically good now. but i have to agree with Matthias comment that more conceptual documentaiton in the patch would be nice. other than that, it can go in, closing this bug requires a gtk-demo for fill level though.
Created attachment 76631 [details] [review] Adds and fixes documentation
comitted the last patch with minor doc changes. Leaving open for the demo case. 2006-11-15 Michael Natterer <mitch@imendio.com> * gtk/gtkrange.[ch]: added properties "fill-level", "show-fill-level" and "restrict-to-fill-level" and getters/setters for them. The "fill level" is an additional marker on the range's trough than can be e.g. used to indicate the amount of pre-buffering in a range showing the play position of streamed media. See the embedded API docs for details. Made GtkRangeLayout a GTypeInstance private struct and removed finalize() implementation. Fixes bug #349808 * gtk/gtk.symbols: added the new symbols.
We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.
As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla. If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab: https://gitlab.gnome.org/GNOME/gtk/issues/new