Add TrackedEvents for better event cleanup #2617
Conversation
| # # Compat only | ||
| # function Base.getproperty(e::Events, field::Symbol) | ||
| # if field === :mousebuttons | ||
| # error("`events.mousebuttons` is deprecated. Use `events.mousebutton` to react to `MouseButtonEvent`s instead.") | ||
| # elseif field === :keyboardbuttons | ||
| # error("`events.keyboardbuttons` is deprecated. Use `events.keyboardbutton` to react to `KeyEvent`s instead.") | ||
| # elseif field === :mousedrag | ||
| # error("`events.mousedrag` is deprecated. Use `events.mousebutton` or a mouse state machine (`addmouseevents!`) instead.") | ||
| # else | ||
| # return getfield(e, field) | ||
| # end | ||
| # end |
There was a problem hiding this comment.
This was just there to give people a useful error message when updating from an older version of the Events struct. I think it's fine to delete this now?
There was a problem hiding this comment.
2 years since those fieldnames were changed, 8 months since the warning was turned into an error
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))
|
|
I think this is not necessary anymore with the changes in #2731 @SimonDanisch? You introduce some kind of tracking there, too. |
|
I don't really like the dependence on a new Observable type (it's somewhat breaking because we haven't really used on(plot, :projection) do projection
...
end
on(plot, :mouseposition) do position
endWhich will then gets only connected when displayed on a certain scene... |
Description
Eventsare hard to clean up because every child scene has the same event struct, i.e. the same observables, containing all callbacks.This is an attempt to solve this with two wrappers
TrackedObservableandTrackedEvents. Each child scene gets its ownTrackedEvents(parent.events)which mirrorsEventsexcept that every observable is wrapped in aTrackedObservble. When callingonetc on any of those, the registered callback is tracked and added to the shared event observable. This means that every child scenes keeps track of its "local" event callbacks, which can then be cleaned up onempty!(scene).I tested this with the same Menu deleting code from https://discourse.julialang.org/t/removing-row-from-gridlayout/93196 and it seems to do its job.
See also: #2614
Type of change
Checklist