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.

Sign up for GitHub

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

Jump to bottom

Refactor Slider and IntervalSlider for dragging logic #3231

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Refactor Slider and IntervalSlider for dragging logic #3231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7af1bc2
Refactor `Slider` and `IntervalSlider` for `dragging` logic
jkrumbiegel Sep 14, 2023
61c3586
add news
jkrumbiegel Sep 14, 2023
55e02b7
add reference to value/interval docs
jkrumbiegel Sep 14, 2023
d44ec32
Merge branch 'master' into jk/slider-dragging-attribute
jkrumbiegel Sep 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## master

- Added the `dragging` attribute to `Slider` and `IntervalSlider` which can be used, for example, to react to value changes only when a drag ends [3231](https://github.com/MakieOrg/Makie.jl/pull/3231).

## v0.19.9

- Allow arbitrary reversible scale functions through `ReversibleScale`.
Expand Down
51 changes: 23 additions & 28 deletions src/makielayout/blocks/intervalslider.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ function initialize_block!(isl::IntervalSlider)
end
end

dragging = Observable(false)

# what the slider actually displays currently (also during dragging when
# the slider position is in an "invalid" position given the slider's range)
displayed_sliderfractions = Observable((0.0, 0.0))
Expand All @@ -50,7 +48,7 @@ function initialize_block!(isl::IntervalSlider)
on(sliderfractions) do fracs
# only update displayed fraction through sliderfraction if not dragging
# dragging overrides the value so there is clear mouse interaction
if !dragging[]
if !isl.dragging[]
displayed_sliderfractions[] = fracs
end
end
Expand Down Expand Up @@ -87,7 +85,7 @@ function initialize_block!(isl::IntervalSlider)
[ci, ci]
end

endbuttons = scatter!(blockscene, endpoints, color = endbuttoncolors,
endbuttons = scatter!(blockscene, endpoints, color = endbuttoncolors, marker = Circle,
markersize = isl.linewidth, strokewidth = 0, inspectable = false)

linesegs = linesegments!(blockscene, linepoints, color = linecolors,
Expand All @@ -107,7 +105,7 @@ function initialize_block!(isl::IntervalSlider)
end
buttonsizes = @lift($(isl.linewidth) .* $button_magnifications)
buttons = scatter!(blockscene, middlepoints, color = isl.color_active, strokewidth = 0,
markersize = buttonsizes, inspectable = false)
marker = Circle, markersize = buttonsizes, inspectable = false)

mouseevents = addmouseevents!(blockscene, isl.layoutobservables.computedbbox)

Expand All @@ -118,15 +116,17 @@ function initialize_block!(isl::IntervalSlider)
start_disp_fractions = Ref((0.0, 0.0))
startindices = Ref((1, 1))

onmouseleftdrag(mouseevents) do event

dragging[] = true
function get_fraction(event)
fraction = if isl.horizontal[]
(event.px[1] - endpoints[][1][1]) / (endpoints[][2][1] - endpoints[][1][1])
else
(event.px[2] - endpoints[][1][2]) / (endpoints[][2][2] - endpoints[][1][2])
end
fraction = clamp(fraction, 0, 1)
end

function handle_event(event)
fraction = get_fraction(event)

if state[] in (:min, :max)
if isl.snap[]
Expand Down Expand Up @@ -178,47 +178,42 @@ function initialize_block!(isl::IntervalSlider)
isl.selected_indices[] = newindices
end
end
end

onmouseleftdrag(mouseevents) do event
handle_event(event)
return Consume(true)
end

onmouseleftdragstop(mouseevents) do event
dragging[] = false
isl.dragging[] = false
# adjust slider to closest legal value
sliderfractions[] = sliderfractions[]
notify(isl.selected_indices)
Comment on lines 191 to +192
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

return Consume(true)
end

onmouseleftdown(mouseevents) do event
isl.dragging[] = true

pos = event.px

dim = isl.horizontal[] ? 1 : 2
frac = clamp(
(pos[dim] - endpoints[][1][dim]) / (endpoints[][2][dim] - endpoints[][1][dim]),
0, 1
)

startfraction[] = frac
fraction = get_fraction(event)
startfraction[] = fraction
startindices[] = isl.selected_indices[]
start_disp_fractions[] = displayed_sliderfractions[]

if state[] in (:both, :none)
return Consume(true)
end
handle_event(event)

newindex = closest_fractionindex(isl.range[], frac)
if abs(newindex - isl.selected_indices[][1]) < abs(newindex - isl.selected_indices[][2])
isl.selected_indices[] = (newindex, isl.selected_indices[][2])
else
isl.selected_indices[] = (isl.selected_indices[][1], newindex)
end
# linecolors[] = [color_active[], color_inactive[]]
return Consume(true)
end

onmouseleftclick(mouseevents) do event
isl.dragging[] = false
notify(isl.selected_indices)
return Consume(true)
end
Comment on lines +209 to 213
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


onmouseleftdoubleclick(mouseevents) do event
isl.dragging[] = false
Comment on lines 215 to +216
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

isl.selected_indices[] = isl.selected_indices[] = if isl.startvalues[] === Makie.automatic
(1, lastindex(isl.range[]))
else
Expand Down
50 changes: 36 additions & 14 deletions src/makielayout/blocks/slider.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,16 +40,14 @@ function initialize_block!(sl::Slider)
(i - 1) / (length(r) - 1)
end

dragging = Observable(false)

# what the slider actually displays currently (also during dragging when
# the slider position is in an "invalid" position given the slider's range)
displayed_sliderfraction = Observable(0.0)

on(topscene, sliderfraction) do frac
# only update displayed fraction through sliderfraction if not dragging
# dragging overrides the value so there is clear mouse interaction
if !dragging[]
if !sl.dragging[]
displayed_sliderfraction[] = frac
end
end
Expand Down Expand Up @@ -91,15 +89,17 @@ function initialize_block!(sl::Slider)

mouseevents = addmouseevents!(topscene, sl.layoutobservables.computedbbox)

onmouseleftdrag(mouseevents) do event
dragging[] = true
dif = event.px - event.prev_px
fraction = clamp(if sl.horizontal[]
function get_fraction(event)
raw = if sl.horizontal[]
(event.px[1] - endpoints[][1][1]) / (endpoints[][2][1] - endpoints[][1][1])
else
(event.px[2] - endpoints[][1][2]) / (endpoints[][2][2] - endpoints[][1][2])
end, 0, 1)
end
return clamp(raw, 0, 1)
end

onmouseleftdrag(mouseevents) do event
fraction = get_fraction(event)
newindex = closest_fractionindex(sliderrange[], fraction)
if sl.snap[]
fraction = (newindex - 1) / (length(sliderrange[]) - 1)
Expand All @@ -114,23 +114,45 @@ function initialize_block!(sl::Slider)
end

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
Comment on lines 116 to 138
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has the same update duplication as IntervalSlider


onmouseleftdown(mouseevents) do event
pos = event.px
dim = sl.horizontal[] ? 1 : 2
frac = (pos[dim] - endpoints[][1][dim]) / (endpoints[][2][dim] - endpoints[][1][dim])
selected_index[] = closest_fractionindex(sliderrange[], frac)
# linecolors[] = [color_active[], color_inactive[]]
sl.dragging[] = true
fraction = get_fraction(event)
newindex = closest_fractionindex(sliderrange[], fraction)
if sl.snap[]
fraction = (newindex - 1) / (length(sliderrange[]) - 1)
end
displayed_sliderfraction[] = fraction
if selected_index[] != newindex
selected_index[] = newindex
end
return Consume(true)
end

onmouseleftdoubleclick(mouseevents) do event
sl.dragging[] = false
selected_index[] = closest_index(sliderrange[], sl.startvalue[])
return Consume(true)
end
Comment on lines 154 to 158
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same update order problem as IntervalSlider

Expand Down
46 changes: 44 additions & 2 deletions src/makielayout/types.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,7 +876,10 @@ end
tellheight::Bool = true
"The start value of the slider or the value that is closest in the slider range."
startvalue = 0
"The current value of the slider. Don't set this manually, use the function `set_close_to!`."
"""
The current value of the slider. Don't change this attribute manually, use the function `set_close_to!` instead.
To trigger other (possibly expensive) behavior only at the end of a drag interaction, update only when the `dragging` attribute is `false`.
"""
value = 0
"The width of the slider line"
linewidth::Float32 = 15
Expand All @@ -892,6 +895,24 @@ end
alignmode = Inside()
"Controls if the button snaps to valid positions or moves freely"
snap::Bool = true
"""
Set to `true` when the user starts dragging the slider and to `false` when
they stop.
This attribute should not be changed by the user.
The dragging status changes before the `value` observable changes, therefore
the first `value` in a new drag action has `dragging === true` and the last one `false`.
To ignore `value` change events happening during a drag (for example because
your update logic is expensive), you can check that `dragging === false`:

```
on(slider.value) do value
if !slider.dragging[]
# do expensive update with value
end
end
```
"""
dragging::Bool = false
end
end

Expand Down Expand Up @@ -940,7 +961,10 @@ end
tellheight::Bool = true
"The start values of the slider or the values that are closest in the slider range."
startvalues = Makie.automatic
"The current interval of the slider. Don't set this manually, use the function `set_close_to!`."
"""
The current interval of the slider. Don't change this attribute manually, use the function `set_close_to!` instead.
To trigger other (possibly expensive) behavior only at the end of a drag interaction, update only when the `dragging` attribute is `false`.
"""
interval = (0, 0)
"The width of the slider line"
linewidth::Float64 = 15.0
Expand All @@ -956,6 +980,24 @@ end
alignmode = Inside()
"Controls if the buttons snap to valid positions or move freely"
snap::Bool = true
"""
Set to `true` when the user starts dragging the slider and to `false` when
they stop.
This attribute should not be changed by the user.
The dragging status changes before the `interval` observable changes, therefore
the first `interval` in a new drag action has `dragging === true` and the last one `false`.
To ignore `interval` change events happening during a drag (for example because
your update logic is expensive), you can check that `dragging === false`:

```
on(slider.interval) do interval
if !slider.dragging[]
# do expensive update with interval
end
end
```
"""
dragging::Bool = false
end
end

Expand Down
Loading