GNOME Bugzilla – Bug 677892
Add a GtkStrengthBar widget
Last modified: 2012-11-20 15:23:35 UTC
To be used, among others, for password strength and power charging indicator as seen in gnome-control-center.
Created attachment 216144 [details] [review] strength-bar: introduce GtkStrengthBar Similar to CcStrengthBar from gnome-control-center, but more generic and with thorough CSS styling support.
Created attachment 216145 [details] [review] visuals: add a visual demo for GtkStrengthBar
Review of attachment 216144 [details] [review]: ::: gtk/gtkstrengthbar.c @@ +75,3 @@ +gtk_strength_bar_get_min_block_size (GtkStrengthBar *self, + gint *block_width, + gint *block_height) Function parameter alignment is off in the entire file @@ +261,3 @@ + 0, 0, width, height); + gtk_render_frame (context, cr, + 0, 0, width, height); Can probably get these on a single line without problems @@ +370,3 @@ + (g_strcmp0 (pspec->name, "weak-fraction") == 0)) + gtk_strength_bar_update_style_classes (GTK_STRENGTH_BAR (obj)); +} I think it is slightly nicer to put this in the setters than overriding notify for this kind of thing. @@ +485,3 @@ + P_("Fraction level above which the bar is treated as strong"), + 0.0, 1.0, 0.75, + G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS); What happens if strong-fraction < weak-fraction ? @@ +493,3 @@ + * is -1 (the default), the number of blocks will be calculated automatically + * to fit in the allocated space, respecting the value of the + * #GtkStrengthBar:min-num-blocks property. I don't think min-num-blocks exists ? @@ +555,3 @@ + * + * Returns: a fraction from 0.0 to 1.0. + */ Doc comments miss Since: 3.6 throughout ::: gtk/gtkstrengthbar.h @@ +75,3 @@ + gint num_blocks); +gint gtk_strength_bar_get_num_blocks (GtkStrengthBar *self); + Need GDK_AVAILABLE_IN_3_6 here
Review of attachment 216144 [details] [review]: for the API, I'd use NSLevelIndicator inside AppKit as an inspiration: http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Classes/NSLevelIndicator_Class/Reference/Reference.html#//apple_ref/doc/uid/TP40004058 which is a tad more generic that just strong/weak; for instance, if you set min=0 and max=5 on NSLevelIndicator and set the style to NSRatingsIndicatorStyle you get a classic ratings widget, whereas if you use a NSDiscreteCapacityIndicatorStyle you get a bar. without going completely insane, I think that the API should be a bit more generic, with: min/critical/warning/max states, and the ability to control the segment style to be continuous, bar, and eventually stars. ::: gtk/gtkstrengthbar.c @@ +369,3 @@ + (g_strcmp0 (pspec->name, "strong-fraction") == 0) || + (g_strcmp0 (pspec->name, "weak-fraction") == 0)) + gtk_strength_bar_update_style_classes (GTK_STRENGTH_BAR (obj)); if you're thinking of coalescing multiple changes inside a single style class update, then you probably want to override dispatch_notify instead. @@ +542,3 @@ +} + +GtkWidget * missing documentation block on the constructor ::: gtk/gtkstrengthbar.h @@ +49,3 @@ + +struct _GtkStrengthBarClass { + GtkWidgetClass parent_class; missing the /*< private >*/ trigraph ::: gtk/gtkstylecontext.h @@ +705,3 @@ + * GTK_STYLE_CLASS_STRONG: + * + * This is used by #GtkStrengthBar. if these classes are going to be used by anything else than the strength bar then they should probably be more informative; if they are going to be used only by the strength bar then they should probably be internal-only, or refer to the strength.
Created attachment 216347 [details] [review] strength-bar: introduce GtkStrengthBar Similar to CcStrengthBar from gnome-control-center, but more generic and with thorough CSS styling support.
Created attachment 216348 [details] [review] visuals: add a visual demo for GtkStrengthBar
Thanks Matthias and Emmanuele for the reviews. In this new version of the patch I addressed all the points raised on the code in the reviews and I followed Emmanuele's advice of changing the API to be more similar to NSLevelIndicator. Now the bar has two styles: continuous and discrete (rating and others could be added after landing if we want). When the bar is continuous, it just draws a trough and a fill with style classes set according to the current value and the defined boundaries. When the discrete style is used, a number of blocks equal to the units composing the value interval is drawn by the bar - so I ended up completely removing the num-blocks API in favor of this value-based approach. I also added an additional critical state and API to set the min/max values of the interval.
Review of attachment 216347 [details] [review]: ::: docs/reference/gtk/objects_grouped.sgml @@ -93,3 @@ <link linkend="GtkHRuler">GtkHRuler</link> <link linkend="GtkVRuler">GtkVRuler</link> <link linkend="GtkHScrollbar">GtkHScrollbar</link> Is this file actually under source control ? I don't recall ever editing it... ::: gtk/gtkenums.h @@ +943,3 @@ + GTK_STRENGTH_BAR_STYLE_CONTINUOUS, + GTK_STRENGTH_BAR_STYLE_DISCRETE +} GtkStrengthBarStyle; Should this explicitly be documented as extensible ? If you want to add stars or somesuch later... ::: gtk/gtkstrengthbar.c @@ +38,3 @@ + * modify the interval using #gtk_strength_bar_set_min_value and + * #gtk_strength_bar_set_max_value. The value will be always drawn in proportion to + * the ammissible interval, i.e. a value of 15 with a specified interval between admissible @@ +79,3 @@ + PROP_WARNING_VALUE, + PROP_STRONG_VALUE, + PROP_MAX_VALUE, Looks to me like this is somewhat assymmetrical now ? We have two levels below 'average' but only one level above. Maybe that is not a problem...
Created attachment 218198 [details] [review] level-bar: introduce GtkLevelBar --- Another approach to the API
Created attachment 218199 [details] [review] visuals: add a visual demo for GtkLevelBar
The latest patch uses another approach to the API for this widget...talking with Paolo on IRC convinced me to explore this and I think it's actually better and more flexible * the widget was renamed from GtkStrengthBar to GtkLevelBar to make its meaning more broad/generic * the hardcoded set of critical/warning/optimum level properties has been replaced by a named offset API. You can add an offset using gtk_level_bar_add_offset_value(), and that will also specify the style class to be used when rendering the corresponding blocks. GtkLevelBar will fill two offset values by default (low and high). This has the advantage of keeping the API compact while at the same time giving clients all the flexibility they need * I also added support for specifying such offsets using GtkBuilder directly * the bar style enum has been renamed to GtkLevelBarMode, since using "style" can be confused with style properties Comments?
Review of attachment 218198 [details] [review]: Cosimo, thanks for tackling this, I look forward trying it out in baobab ::: gtk/gtklevelbar.c @@ +108,3 @@ + gdouble cur_value; + + GList *offsets; if one wants to be picky, this could be a SList since afaic you never need ->prev ::: gtk/gtklevelbar.h @@ +66,3 @@ + +GDK_AVAILABLE_IN_3_6 +GtkWidget *gtk_level_bar_new (void); maybe a constructor which takes max/min could be a handy util? @@ +70,3 @@ +GDK_AVAILABLE_IN_3_6 +void gtk_level_bar_set_bar_mode (GtkLevelBar *self, + GtkLevelBarMode bar_mode); why not simply set/get_mode() ?
> if one wants to be picky, this could be a SList since afaic you never need > ->prev Ignore this, I did only look at g_list_* calls but you use ->prev
I had a look over the API and it looks good, but I though I should comment on one use-case to check this might be suitable. I have observed many people mis?using GtkProgressBar as a gauge/level indicator widget in a science/engineering context. The common problem with this, back in the GTK2 days, was the ugly animation. Many people applied custom gtkrc styles to the widget to get it to draw a solid colour. It would be cool if there was some sort of simple way, enforced by the API and not really through the CSS styling, to ensure this was drawn with a flat single color fill (or a gradient fill from min to max, but that might be out of scope considering the existence of _DISCRETE). Does GTK_LEVEL_BAR_MODE_CONTINUOUS go far enough to enforce this? BTW, the max/min/scale parts of the API are very nice, and I think this will actually be very suitable for such uses.
Created attachment 218338 [details] [review] level-bar: introduce GtkLevelBar Similar to CcStrengthBar from gnome-control-center, but more generic and with thorough CSS styling support.
Created attachment 218339 [details] [review] visuals: add a visual demo for GtkLevelBar
(In reply to comment #12) > ::: gtk/gtklevelbar.h > @@ +66,3 @@ > + > +GDK_AVAILABLE_IN_3_6 > +GtkWidget *gtk_level_bar_new (void); > > maybe a constructor which takes max/min could be a handy util? Added this now. > @@ +70,3 @@ > +GDK_AVAILABLE_IN_3_6 > +void gtk_level_bar_set_bar_mode (GtkLevelBar *self, > + GtkLevelBarMode bar_mode); > > why not simply set/get_mode() ? Good point, I renamed these and the property now.
(In reply to comment #14) > I have observed many people mis?using GtkProgressBar as a gauge/level indicator > widget in a science/engineering context. The common problem with this, back in > the GTK2 days, was the ugly animation. Many people applied custom gtkrc styles > to the widget to get it to draw a solid colour. > > It would be cool if there was some sort of simple way, enforced by the API and > not really through the CSS styling, to ensure this was drawn with a flat single > color fill (or a gradient fill from min to max, but that might be out of scope > considering the existence of _DISCRETE). > > Does GTK_LEVEL_BAR_MODE_CONTINUOUS go far enough to enforce this? We're not really able to influence theme behavior like that from the API (and I don't think it's the right thing to do). When the continuous mode is used, in this case, the gauge/level indicator will always be drawn in a single block, and the theme will be able to apply a CSS background to it - with default theming it will be a solid color. > BTW, the max/min/scale parts of the API are very nice, and I think this will > actually be very suitable for such uses. Thanks for your feedback!
> > We're not really able to influence theme behavior like that from the API (and I > don't think it's the right thing to do). > When the continuous mode is used, in this case, the gauge/level indicator will > always be drawn in a single block, and the theme will be able to apply a CSS > background to it - with default theming it will be a solid color. OK, that sounds fine. It might be useful to include an example in gtk-demo or some such that shows the -gtk-gradient CSS magic needed to override the solid background, drawing a gradient from color A to color B.... .... such and example would let me replace a handful of stupid custom hand draw cairo widgets that I use to do this in some lab software :-)
Review of attachment 218338 [details] [review]: The one thing I don't like here is that the bars get rendered without any filling with current Raleigh or Adwaita. ::: gtk/gtklevelbar.c @@ +25,3 @@ + * + * The #GtkLevelBar is a bar widget that can be used + * as a level indicator. Typical use cases are displaying the level 'strength of a password', maybe ? @@ +28,3 @@ + * of a password, or showing the charge level of a battery. + * + * Use #gtk_level_bar_set_value to set the current value, and gtk_level_bar_set_value(), not #... @@ +29,3 @@ + * + * Use #gtk_level_bar_set_value to set the current value, and + * #gtk_level_bar_add_offset_value to set the value offsets at which Same here @@ +114,3 @@ + +static void gtk_level_bar_set_value_internal (GtkLevelBar *self, + gdouble value); A bit of a wide gap, there. I usually try to keep these to a single space, in .c files where there's no need to line them up with anything else. @@ +564,3 @@ +#define OFFSET_ELEMENT "offset" +#define OFFSET_NAME "name" +#define OFFSET_VALUE "value" I must say that I prefer such strings as literals in the code. @@ +577,3 @@ + const gchar **values, + gpointer user_data, + GError **error) Please line up the parameters here @@ +624,3 @@ + GMarkupParser *parser, + gpointer *data) +{ and here @@ +648,3 @@ + GObject *child, + const gchar *tagname, + gpointer user_data) here too @@ +656,3 @@ + + if (strcmp (tagname, OFFSETS_ELEMENT) != 0) + return; You leak the parser_data in the error case @@ +781,3 @@ + signals[SIGNAL_OFFSET_CHANGED] = + g_signal_new ("offset-changed", + GTK_TYPE_LEVEL_BAR, Should be documented ? In particular since it has an interesting detail @@ +801,3 @@ + P_("Currently filled value level"), + P_("Currently filled value level of the level bar"), + 0.0, G_MAXDOUBLE, 0.0, Out of interest, why are values restricted to be non-negative ? @@ +1193,3 @@ + * + * Returns: a value in the interval between + * #GtkLevelBar:min-value and #GtkLevelBar:max-value, or zero. Is zero a good fallback here, considering it is in the allowed range of offsets ? -1.0 might be better. Or maybe this should be gboolean gtk_level_bar_get_offset_value (GtkLevelBar *self, const gchar *name, gdouble *value) ?
Review of attachment 218339 [details] [review]: Can we also add a level bar example to gtk/tests/builder.c that tests custom attribute parsing ? And also having one in widget-factory might be nice, but we probably need to make that have some paging or scrolling first...
Created attachment 218632 [details] [review] level-bar: introduce GtkLevelBar --- Fixed Matthias' review comments
Created attachment 218633 [details] [review] level-bar: add default Raleigh theming for GtkLevelBar
Created attachment 218634 [details] [review] tests: add a test for GtkLevelBar GtkBuildable implementation
(In reply to comment #20) > @@ +801,3 @@ > + P_("Currently filled value level"), > + P_("Currently filled value level of the level bar"), > + 0.0, G_MAXDOUBLE, 0.0, > > Out of interest, why are values restricted to be non-negative ? I don't really there's really a lot of value in allowing negative values here, and it could add a bit of confusion and complexity about the order of offset values...the API doesn't really block adding negative values later if it proves to be a common request.
Review of attachment 218632 [details] [review]: Can't find much wrong with this. ::: gtk/gtklevelbar.c @@ +1133,3 @@ + * @self: a #GtkLevelBar + * + * Returns the value of the #GtkLevelBar:bar-mode property Missing . @@ +1152,3 @@ + * @mode: a #GtkLevelBarMode + * + * Sets the value of the #GtkLevelBar:bar-mode property Missing . @@ +1212,3 @@ + * when rendering the level bar fill. + * If another offset marker named @name exists, its value will be + * replaced by @value. I think it would be really good to have an example that shows how to add a custom offset and sets its color. Could go in the long desc
Review of attachment 218633 [details] [review]: ok
Review of attachment 218634 [details] [review]: ok
Attachment 218339 [details] pushed as 855088b - visuals: add a visual demo for GtkLevelBar Attachment 218632 [details] pushed as 68acf78 - level-bar: introduce GtkLevelBar Attachment 218633 [details] pushed as 6af40f9 - level-bar: add default Raleigh theming for GtkLevelBar Attachment 218634 [details] pushed as 5ee5912 - tests: add a test for GtkLevelBar GtkBuildable implementation Pushed with the suggested fixes, and a couple more documentation tweaks, thanks!
Hi, The commit 68acf78 (level-bar: introduce GtkLevelBar) broke the build on non-C99 compilers due to the usage of round() and strtof(). One can include the (gtk/)fallback-c89.c for the round() function, but there isn't currently a fallback for strtof(). So, just wondering, whether one can use strtod() in place of the strtof() call? With blessings, thank you!
Hi, I moved this issue to bug 680651 - in the future, please open a separate report instead of reopening the old one.
*** Bug 677857 has been marked as a duplicate of this bug. ***