Refactor Slider and IntervalSlider for dragging logic
#3231
Conversation
Compile Times benchmarkNote, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running: using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(display(fig))
|
jkrumbiegel
requested review from
SimonDanisch and
ffreyer
and removed request for
SimonDanisch
| onmouseleftdoubleclick(mouseevents) do event | ||
| isl.dragging[] = false |
There was a problem hiding this comment.
This causes an update like this:
IntervalSlider drag true
IntervalSlider drag false
IntervalSlider interval (0.0, 10.0)
So if you react to dragging you won't catch the new interval here. It would be better if you did. Maybe in general the dragging = false update should happen after the interval update?
| sliderfractions[] = sliderfractions[] | ||
| notify(isl.selected_indices) |
There was a problem hiding this comment.
This just duplicates updates
IntervalSlider interval (1.18, 10.0)
IntervalSlider indices (119, 1001)
IntervalSlider drag false
IntervalSlider interval (1.18, 10.0)
IntervalSlider indices (119, 1001)
so I think we should just skip the notify here.
| onmouseleftclick(mouseevents) do event | ||
| isl.dragging[] = false | ||
| notify(isl.selected_indices) | ||
| return Consume(true) | ||
| end |
There was a problem hiding this comment.
Clicks also trigger a drag start so you end up with a duplicate update here too:
IntervalSlider drag true
IntervalSlider interval (0.43, 10.0)
IntervalSlider indices (44, 1001)
[ Info: click start
IntervalSlider drag false
IntervalSlider interval (0.43, 10.0)
IntervalSlider indices (44, 1001)
[ Info: click end
There is an exception to this her though: If you do not move the slider through a click you do not get an interval update through the drag start. So then you only get the click update, which however does not change the values. I'm not sure if that's an update worth pushing out.
| onmouseleftdragstop(mouseevents) do event | ||
| dragging[] = false | ||
| sl.dragging[] = false | ||
| # adjust slider to closest legal value | ||
| sliderfraction[] = sliderfraction[] | ||
| fraction = get_fraction(event) | ||
| newindex = closest_fractionindex(sliderrange[], fraction) | ||
| selected_index[] = newindex | ||
|
|
||
| linecolors[] = [sl.color_active_dimmed[], sl.color_inactive[]] | ||
| return Consume(true) | ||
| end | ||
|
|
||
| onmouseleftclick(mouseevents) do event | ||
| sl.dragging[] = false | ||
| # adjust slider to closest legal value | ||
| sliderfraction[] = sliderfraction[] | ||
| fraction = get_fraction(event) | ||
| newindex = closest_fractionindex(sliderrange[], fraction) | ||
| selected_index[] = newindex | ||
|
|
||
| linecolors[] = [sl.color_active_dimmed[], sl.color_inactive[]] | ||
| return Consume(true) | ||
| end |
There was a problem hiding this comment.
This has the same update duplication as IntervalSlider
| onmouseleftdoubleclick(mouseevents) do event | ||
| sl.dragging[] = false | ||
| selected_index[] = closest_index(sliderrange[], sl.startvalue[]) | ||
| return Consume(true) | ||
| end |
There was a problem hiding this comment.
Same update order problem as IntervalSlider
|
@jkrumbiegel I've tried this specific slider dragging feature in a dev build before and found that it works quite well in its current state. Is there still a plan to continue working on this and eventually merge it or has it been abandoned? |
|
I never took the time to follow up on @ffreyer's review comments, but in principle I'd still like to get equivalent changes in. |
|
Hi! Thanks for your efforts already! I just wanted to express interest in this; it would be really cool to have for uses when you don't want to trigger countless calls of a long-running function as you drag. In my use case I'd use the slider to flip through satellite images taken at different times and only want to read the target images from disk. |
Description
Supersedes #2228
I thought about it some more and now settled on the idea that the user can use the
draggingstatus to decide what they want to do with a changed slider value. This is a bit more flexible than having a second output observable, and allows further custom logic depending on the slider drag state change. This approach also showed some smaller problems with the implementation which I therefore refactored a bit.Delete options that do not apply:
Checklist