-
-
Notifications
You must be signed in to change notification settings - Fork 674
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
[widget audit] Slider #1708
[widget audit] Slider #1708
Conversation
8d2a068
to
ae8df25
Compare
ae8df25
to
37b6e1f
Compare
I've rebased this PR onto #1707 in order to get the macOS probe classes and the exit status line. Once that PR is merged, the diff for this PR should become a lot smaller. |
def get_value(self): | ||
actual_value = self.native.getProgress() | ||
actual_max = self.native.getMax() | ||
minimum, maximum = self.interface.range | ||
if self.interface.tick_count is not None and self.interface.tick_count <= 1: | ||
return minimum | ||
toga_tick_count = self.interface.tick_count or DEFAULT_NUMBER_OF_TICKS | ||
android_slider_max = toga_tick_count - 1 | ||
tick_factor = (maximum - minimum) / android_slider_max | ||
progress_scaled = self.native.getProgress() * tick_factor | ||
result = progress_scaled + minimum | ||
return result | ||
span = maximum - minimum | ||
return actual_value / actual_max * span + minimum |
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.
Replaced get_value
and set_value
with simpler implementations based on the WIndows backend. There's no need to check for tick_count
being less than 2, because the interface layer now prevents that.
@freakboy3742: Since #1707 is stabilizing, I think it would be worth you reviewing this PR now. You can see the diff against the upstream branch by selecting only the last few commits like this. |
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.
The core of the Android implementation makes sense; however, the examples/slider demo shows some gaps in the Android implementation. The second last slider doesn't appear to report correctly for values <0 or >9. The last slider doesn't work at all - I'm guessing that's missing functionality, though.
I'm not sure I'm following why the sync_values
change is needed, though. Some of the code that has been promoted to the interface layer makes sense (like the "tick count < 2" checks), but the rest of the sync approach seems really roundabout - an event occurs on the impl, which tells the interface to sync, which asks the impl for it's value. The int->float conversion is still needed on each backend, so there's no avoidance of duplication there; and it doesn't seem to impact on the float vs double distinction with iOS (although I'm not sure that's a distinction we need to care about - is anyone going to be able set a slider with precision where the difference between double and float will matter?) From what I can tell, the sync suppression stuff is only required because of this round trip as well. What am I missing here.
Some other things I noticed:
- The tests only have 32% coverage of the winforms slider module; 82% on macOS. I'm guessing that means coverage is low on Android as well, but we're not reporting on that at present. It looks like at least some of this is because of on_press/on_release not being tested
- The sync_value changes break the iOS implementation of slider (possibly a moot point depending on where we land on making this change)
…3742/audit-button into test-slider-windows-android
c14ba00
to
fdb306e
Compare
This was actually a problem with the Label: see #1289 (comment). |
6d1d52f
to
eb62795
Compare
There's a failure in test_flex_horizontal_widget_size on a couple of platforms which I'll look at tomorrow, but everything else is now ready for review. |
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've dropped a bunch of comments, but they're almost entirely cosmetic - the broad strokes look really solid. A good chunk of the comments are about introducing redraws into the testbed tests; plus a handful of requests for explanatory docs; but in terms of core functionality, there's not much to fault 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.
Looks good.
I've made a small tweak to the slider __init__
docstring, and merged with main to make sure that the progress bar changes didn't break anything; but otherwise - I think our long nightmare of slider testing is over :-)
Audit checklist