GNOME Bugzilla – Bug 358970
gtk_scale_set_digits does not cause value to be rounded if draw-value is false, or retroactively
Last modified: 2017-04-22 10:21:06 UTC
Please describe the problem: Documentation for gtk_scale_set_digits says that this function should round off the value of the adjustment, however this is not happening. Is this a missing feature or is it a documentation bug? Steps to reproduce: int main () { GtkScale *scale; GtkAdjustment *adj; gint digits; gdouble value; gdouble expected_value; gtk_init (0, NULL); adj = GTK_ADJUSTMENT (gtk_adjustment_new (10.0f, 0.0f, 100.0f, 1.0f, 10.0f, 10.0f)); scale = GTK_SCALE (gtk_hscale_new (adj)); value = 10.1294f; digits = 2; gtk_range_set_value (GTK_RANGE (scale), value); gtk_scale_set_digits (scale, digits); value = gtk_range_get_value (GTK_RANGE (scale)); expected_value = 10.13f; if (value != expected_value) g_print ("Setting digits to %d does not round off adjustment value properly: current value=%.5f, expected value=%.5f\n", digits, value, expected_value); else g_print ("Adjustment value rounded off properly\n"); gtk_widget_destroy (GTK_WIDGET (scale)); return 0; } Actual results: Setting digits to 2 does not round off adjustment value properly: current value=10,12940, expected value=10,13000 Expected results: Adjustment value rounded off properly Does this happen every time? yes Other information:
Created attachment 77899 [details] testapplication to show the rounding with or without draw_value on a scale. I don't know if this is the same problem, but I noticed it heavly depends on gtk_scale_set_draw_value(); When setting this to false, it doesn't round anything anymore, is this the same bug? Or do we need a new one. (Find attached a test app, call it with ./foo 0 and ./foo 1 to see the difference).
The current behaviour depends indeed on "draw-value". From gtkscale.c: 577: if (priv->draw_value) 578: range->round_digits = digits; Quoting gtk_scale_set_digits docs: "Sets the number of decimal places that are displayed in the value. Also causes the value of the adjustment to be rounded off to this number of digits, so the retrieved value matches the value the user saw." It doesn't mention that the adjustment is rounded off only if draw-value is on. So I think there are two choices, fix the docs to be less ambiguous or fix the code to round the adjustment value to "digits" even if draw-value is on. I'd really prefer the latter one as it allows to get values from a gtkscale at discrete step sizes without doing the rounding thing on the application. This would cause a difference though with the current behaviour because digits defaults to 1 so you will always get discrete steps (rounded at the first decimal place) while at the moment you get continuous ones if draw-value is off. So I'd say to default digits to 1 when draw-value is true and to the maximum (64) otherwise so that the rounding when draw-value is false only happens if you explicitly set the digits property. Another solution, that would have been probably the best way to implement this thing, is to round the adjustment value to the adjustment step_increment. I honestly assumed this was done this way and was a bit puzzled to discover that step_increment had no effect.
Hi, Can someone comment on the proposed resolutions above, and preferably implement the latter? Scales lacking drawn values should still round off to the number of digits they're told to. The fact that Scale's version of set_digits only calls Range's base version when it's going to be visually indicated is bizarre - because it hides a remaining functional difference (or makes users work around this by calling the base version themselves). (and if there were a good reason for including the "if (priv->draw_value)" condition, why would you not be consistent and wrap the following "gtk_scale_clear_value_layout (scale);" in that too?) Thanks
Created attachment 338309 [details] [review] scale: always sync ::digits to range::round-digits This ought to cover it. > The docs unconditionally claim gtk_scale_set_digits() causes the > adjustment to be rounded accordingly. But in reality, this was only > applied if scale::draw_value was true. This made the docs wrong and > meant something that should've just been cosmetic detail affected > functionality. Fix these issues by ensuring the number of digits is > always propagated along to gtk_range_set_round_digits().
The patch is against 3.22, btw. It would be great to see this get into stable. (In reply to Filippo Argiolas from comment #2) > This would cause a difference though with the current behaviour because > digits defaults to 1 so you will always get discrete steps (rounded at the > first decimal place) while at the moment you get continuous ones if > draw-value is off. True, it might change the functioning of programs that depended on this odd difference. I wonder whether that will be considered problematic. Then again, maybe they shouldn't have been relying on something that was contradicted by the docs. :D I dunno. > So I'd say to default digits to 1 when draw-value is true and to the maximum > (64) otherwise so that the rounding when draw-value is false only happens if > you explicitly set the digits property. This would probably be the most backwards-compatible compromise, though it seems more hassle, in a widget already jam-packed with conditions and surprises! > Another solution, that would have been probably the best way to implement > this thing, is to round the adjustment value to the adjustment > step_increment. > I honestly assumed this was done this way and was a bit puzzled to discover > that step_increment had no effect. This certainly has the benefit of being intuitive. Are there be legitimate cases for Scales where the arrow keys et al should make discrete jumps, but the user can set finer values using the pointer? That seems to introduces a hard dependency on a specific input device to get full functionality, so it doesn't sound like a benefit. But I might be overlooking some other use case for this.
Created attachment 341288 [details] [review] Scale: Always sync ::digits to Range::round-digits This one actually works! D'oh.
On reading the OP properly, there seem to be two different issues raised here: * Setting a smaller number of digits than required by the current value does not cause the value to be rounded down (and fire a changed signal if different). Rather, the number of digits is only intended to be applied when the value is changed (with the default handler). I checked this with Matthias on IRC. * The problem I knew about was that rounding on value change was only occurring if Scale::draw-value was true, which clearly contradicts the documentation. The documentation for Range makes this pretty clear by talking about rounding > when it [the value] changes However, Scale is still more ambiguous: > Also causes the value of the adjustment to be rounded off to this number of digits The fix to round even if draw-value is false is definitely correct, I'd say, and I'll be pushing that once I've tested it a bit more. But I think either a doc clarification or a code patch is necessary, depending on whether the new number of digits should round the pre-existing value if it's got too many digits. That is: if we set value to 3.14159, then set digits to 3, then get the value: should we get the original value back, or the rounded 3.142?
Btw, sorry OP that I ran with the tangent
...of the draw-value issue explained in the subsequent comments, rather than the retroactive rounding of the existing value that you focussed on. I came here with the former in my mind, so I was biased. :P But I think the other should be amended, one way or another.
Created attachment 343754 [details] [review] [PATCH 1/3] Remove executable bit from C source files in tests (This trivial patch is only applicable to gtk-3-22.)
Created attachment 343755 [details] [review] [PATCH 2/3] Scale: Always sync ::digits to Range::round-digits This cleans up the previous patch and (I think) improves the commit message. > The documents state that gtk_scale_set_digits() “causes the value of the > adjustment to be rounded off to this number of digits, so the retrieved > value matches the value the user saw.” Note the lack of any condition. > > But in reality, rounding on value change only occurred if ::draw-value > was true. This made the docs wrong and meant something that should’ve > just been a cosmetic detail altered the functionality of the widget. > > Fix by ensuring the number of digits set on Scale is always propagated > along to gtk_range_set_round_digits(), thus rounding to it in all cases > when the value changes, regardless of whether the value is displayed. > > This doesn’t address the other idea from Bugzilla: that changing the > number of digits should clamp the _existing_ value if it’s more precise. > This contradicts digits docs in the base Range, but the above from Scale > can be read as implying it’ll happen. For now, that’s an open question.
Created attachment 343756 [details] [review] [PATCH 3/3] scale: doc: set_digits doesn’t round retroactively > Whether it should is an open question, but for now, the documentation > should clearly indicate that currently rounding is only applied upon > changes to the value, not to the existing value when ::digits changes. > This is already clear in the doc for the underlying Range::round-digits.
(In reply to Daniel Boles from comment #7) > Rather, the number of digits is only intended to be applied when > the value is changed (with the default handler). I checked this with > Matthias on IRC. Haha, sorry. Half of this is nonsense. I did ask, but I haven't heard back. That bit of the message was a draft, on the assumption that I wouldn't post it until he commented... and that the inference in the Range docs (that this only applies on change) is intended to cover Scale too. I forgot to remove it before posting. What I _did_ get confirmation of is that the patch to always round _on value change_ makes sense and should go to both master and gtk-3-22, all tests being well. All the tests I can think of are fine after this, so I'll probably push the patch and a clarification to the documentation shortly.
Comment on attachment 343754 [details] [review] [PATCH 1/3] Remove executable bit from C source files in tests gtk-3-22: commit ccba2eaace19ace17cf9895e277bbe2dfde671dc
Comment on attachment 343755 [details] [review] [PATCH 2/3] Scale: Always sync ::digits to Range::round-digits gtk-3-22: commit 4a6bd134bdc192b4830a1c6228e27332d4629418 master : commit d9dd312752f69acdfcba05afaffb83316589d17c
Comment on attachment 343756 [details] [review] [PATCH 3/3] scale: doc: set_digits doesn’t round retroactively gtk-3-22: commit 837785012717c6487f3db23ccfdb08bb48b5282a master : commit 0776bd438bc9badf94116e3f296ddb176d899b1d
*** Bug 777858 has been marked as a duplicate of this bug. ***
I'm going to close this for now. The functionality has been fixed, in terms of making Scales with and without values act consistently. As for the question of whether setting a lower number of digits should round off a pre-existing value that required greater precision, I will mark that as resolved too, on the basis that the OP asked this question: > Is this a missing feature or is it a documentation bug? I lean towards the opinion that it was the latter: Omitting this functionality looks more clearly like a deliberate choice, compared to the with/without value difference. Accordingly, I amended the associated docs a bit, to make it clearer that rounding to the new number of digits only occurs when the value changes via UI interaction. To handle rounding of a pre-existing value in GtkScale itself would be more involved, raise some philosophical questions about signal emission, and I'm not sure it's worthwhile. Still, if anyone ever finds themselves with a real rationale for this, then feel free to reopen this ancient ticket. Hopefully you won't. ;-) Rounding an existing value when the number of digits changes can be trivially implemented in client code - where that client then has full control over whether a signal should be emitted if rounding did occur, etc. This seems far superior.
This may have caused some regression with GtkVolumeButton, see bug 781605