Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix regression for strange values of slider using precision property #74571

Closed

Conversation

Amitpatil215
Copy link
Contributor

@Amitpatil215 Amitpatil215 commented Jan 23, 2021

Description

Added precision property to handle decimal places of slider's value.

Root cause of the issue

lets say,

min: 0,
max: 100,
division:100,
value: 0.07 // value sent to _lerp()

double _lerp(double value) {
assert(value >= 0.0);
assert(value <= 1.0);
return value * (widget.max - widget.min) + widget.min;

According to above method we end up having

0.07 * (100 - 0) + 0

expected value: 7.0
actual value : 7.000000000000001

Screenshot (107)

And this value is shown to user, that's why we having regression in slider's label.

Refer issue on Dart-lang repo

dart-lang/sdk#23315

Related issue

fixes #73787

Test

added test Slider respects precision value

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jan 23, 2021
@google-cla google-cla bot added the cla: yes label Jan 23, 2021
@Amitpatil215 Amitpatil215 changed the title precision property added Fix regression for strange values of slider using precision property Jan 23, 2021
@Amitpatil215 Amitpatil215 marked this pull request as ready for review January 24, 2021 07:41
Comment on lines -539 to +555
final double lerpValue = _lerp(value);
double lerpValue = _lerp(value);
if (widget.precision != null) {
lerpValue = _preciseLerp(lerpValue);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if precision is null? will we still face the same issue?

I suggest to fix the issue directly instead of introduce a new property. FYI, this was already working without any additional property so it still should be posible to fix the issue without any additional property.

Maybe just round the resulting value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @pedromassango , Thanks for the review!

Maybe just round the resulting value?

Example
Slider(
	value: value,
	label: '$value',
	min: 0,
	max: 10,
	divisions: 20,
	onChanged: (v) => setState(() {
		value = v;
		print(value);
		}),
 	),

User wants 20 divisions of 10 that means 0.5 as a single division. If we round the value it end up giving us values 0, 1.0, 2.0

What if precision is null? will we still face the same issue?

Thanks for pointing out this. We could set precision to 0 (Which is equivalent to round) as a default. and modify assertion to

assert(precision != null && (precision >= 0 && precision <= 20))

This approach may affect many users as it is a breaking change. (That's the only reason I made it null by default).

Why I chosen to add precision property workaround?

instead of rounding it up internally to certain precision to 2,3, or foo. Cause we never know what exact precision user wants. So I tried to give it a bit customizable approach.

If I'm wrong anywhere please point it out to me.

Meanwhile I try to figure out some other workarounds to fix this issue without introducing new property.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @pedromassango, it seems possible to fix this without introducing a new parameter, since it used to print with less precision (that math will always result in a rounding error in the mantissa for IEEE doubles, so it had to be printing in lower precision before).

Maybe figure out why the regression happened, and see if that sheds some light? Probably you could fix the formatting code in the label, rather than modifying the value.

Also, even if you end up with this method, it's probably a lot faster to do something like this, instead of creating strings and parsing them.

import 'dart:math' as math;
// ...
final double factor = math.pow(10, precision);
final double lerpValue = (factor * _lerp(value)).roundToDouble() / factor;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry, I wrote the above comment months ago, I just hadn't submitted it)

@HansMuller
Copy link
Contributor

CC @JoseAlba @clocksmith

@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@gspencergoog
Copy link
Contributor

@Amitpatil215 Are you still planning to work on this PR? If not, could you please close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flutter Slider Label showing strange numbers
4 participants