-
Notifications
You must be signed in to change notification settings - Fork 24
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
APP-3318 - Fix sticky motor slider #153
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I swear I've seen this "setPower
interacts weirdly with setState(() { this.power = power });
" in one of @njooma's old reviews. Don't have much context here but seems fine to me.
You're right @benjirewis it does sound familiar. I hope we're not punting different bugs back and forth by changing the order around. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a Q, but LGTM otherwise
lib/widgets/resources/motor.dart
Outdated
await widget.motor.setPower(0); | ||
await widget.motor.stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need both here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call out, probably merits a comment if we keep it in.
I added both as a stop gap in case this the motor was a module with stop()
unimplemented.
thinking through it now, it does seem unneeded. 🤔
On Plus/Max sized iphones, the call to set state after awaiting setPower would happen sometimes after the call to stop had gone through, causing the slider to stick after release, even when the toggle to stop after release was enabled.
Moving the setState to before the awaited method calls fixes this issue.
I considered making the calls to setPower and stop
unawaited()
- but then we'd lose our error handling from the try catch.We could also use the older convention of not using async/await and instead use the Future.onError() callback, to populate the error object. But I opted for using async and await because they are more idiomatic.