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 677892 - Add a GtkStrengthBar widget
Add a GtkStrengthBar widget
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
3.5.x
Other Linux
: Normal enhancement
: ---
Assigned To: gtk-bugs
gtk-bugs
: 677857 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-06-11 20:03 UTC by Cosimo Cecchi
Modified: 2012-11-20 15:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
strength-bar: introduce GtkStrengthBar (30.69 KB, patch)
2012-06-11 20:04 UTC, Cosimo Cecchi
reviewed Details | Review
visuals: add a visual demo for GtkStrengthBar (6.02 KB, patch)
2012-06-11 20:04 UTC, Cosimo Cecchi
none Details | Review
strength-bar: introduce GtkStrengthBar (49.45 KB, patch)
2012-06-13 20:53 UTC, Cosimo Cecchi
reviewed Details | Review
visuals: add a visual demo for GtkStrengthBar (7.02 KB, patch)
2012-06-13 20:53 UTC, Cosimo Cecchi
none Details | Review
level-bar: introduce GtkLevelBar (47.05 KB, patch)
2012-07-06 18:42 UTC, Cosimo Cecchi
none Details | Review
visuals: add a visual demo for GtkLevelBar (7.15 KB, patch)
2012-07-06 18:42 UTC, Cosimo Cecchi
none Details | Review
level-bar: introduce GtkLevelBar (47.60 KB, patch)
2012-07-09 14:03 UTC, Cosimo Cecchi
reviewed Details | Review
visuals: add a visual demo for GtkLevelBar (7.14 KB, patch)
2012-07-09 14:04 UTC, Cosimo Cecchi
committed Details | Review
level-bar: introduce GtkLevelBar (49.58 KB, patch)
2012-07-12 14:58 UTC, Cosimo Cecchi
committed Details | Review
level-bar: add default Raleigh theming for GtkLevelBar (1.25 KB, patch)
2012-07-12 14:58 UTC, Cosimo Cecchi
committed Details | Review
tests: add a test for GtkLevelBar GtkBuildable implementation (3.17 KB, patch)
2012-07-12 14:59 UTC, Cosimo Cecchi
committed Details | Review

Description Cosimo Cecchi 2012-06-11 20:03:27 UTC
To be used, among others, for password strength and power charging indicator as seen in gnome-control-center.
Comment 1 Cosimo Cecchi 2012-06-11 20:04:25 UTC
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.
Comment 2 Cosimo Cecchi 2012-06-11 20:04:27 UTC
Created attachment 216145 [details] [review]
visuals: add a visual demo for GtkStrengthBar
Comment 3 Matthias Clasen 2012-06-11 20:13:01 UTC
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
Comment 4 Emmanuele Bassi (:ebassi) 2012-06-11 20:38:00 UTC
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.
Comment 5 Cosimo Cecchi 2012-06-13 20:53:44 UTC
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.
Comment 6 Cosimo Cecchi 2012-06-13 20:53:51 UTC
Created attachment 216348 [details] [review]
visuals: add a visual demo for GtkStrengthBar
Comment 7 Cosimo Cecchi 2012-06-13 21:01:16 UTC
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.
Comment 8 Matthias Clasen 2012-06-13 21:13:46 UTC
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...
Comment 9 Cosimo Cecchi 2012-07-06 18:42:13 UTC
Created attachment 218198 [details] [review]
level-bar: introduce GtkLevelBar

---

Another approach to the API
Comment 10 Cosimo Cecchi 2012-07-06 18:42:22 UTC
Created attachment 218199 [details] [review]
visuals: add a visual demo for GtkLevelBar
Comment 11 Cosimo Cecchi 2012-07-06 18:49:20 UTC
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?
Comment 12 Paolo Borelli 2012-07-06 20:32:28 UTC
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() ?
Comment 13 Paolo Borelli 2012-07-06 20:36:30 UTC
> 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
Comment 14 John Stowers 2012-07-09 12:42:27 UTC
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.
Comment 15 Cosimo Cecchi 2012-07-09 14:03:56 UTC
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.
Comment 16 Cosimo Cecchi 2012-07-09 14:04:02 UTC
Created attachment 218339 [details] [review]
visuals: add a visual demo for GtkLevelBar
Comment 17 Cosimo Cecchi 2012-07-09 14:05:04 UTC
(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.
Comment 18 Cosimo Cecchi 2012-07-09 17:52:15 UTC
(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!
Comment 19 John Stowers 2012-07-09 18:54:10 UTC
> 
> 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 :-)
Comment 20 Matthias Clasen 2012-07-11 03:13:24 UTC
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) ?
Comment 21 Matthias Clasen 2012-07-11 03:15:03 UTC
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...
Comment 22 Cosimo Cecchi 2012-07-12 14:58:48 UTC
Created attachment 218632 [details] [review]
level-bar: introduce GtkLevelBar

---

Fixed Matthias' review comments
Comment 23 Cosimo Cecchi 2012-07-12 14:58:55 UTC
Created attachment 218633 [details] [review]
level-bar: add default Raleigh theming for GtkLevelBar
Comment 24 Cosimo Cecchi 2012-07-12 14:59:17 UTC
Created attachment 218634 [details] [review]
tests: add a test for GtkLevelBar GtkBuildable implementation
Comment 25 Cosimo Cecchi 2012-07-12 15:11:53 UTC
(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.
Comment 26 Matthias Clasen 2012-07-14 01:43:31 UTC
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
Comment 27 Matthias Clasen 2012-07-14 01:45:52 UTC
Review of attachment 218633 [details] [review]:

ok
Comment 28 Matthias Clasen 2012-07-14 01:47:08 UTC
Review of attachment 218634 [details] [review]:

ok
Comment 29 Cosimo Cecchi 2012-07-16 05:02:55 UTC
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!
Comment 30 Fan, Chun-wei 2012-07-26 13:37:59 UTC
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!
Comment 31 Cosimo Cecchi 2012-07-26 13:46:27 UTC
Hi, I moved this issue to bug 680651 - in the future, please open a separate report instead of reopening the old one.
Comment 32 Cosimo Cecchi 2012-11-20 15:23:35 UTC
*** Bug 677857 has been marked as a duplicate of this bug. ***