MakieOrg / Makie.jl

Interactive data visualizations and plotting in Julia
https://docs.makie.org/stable
MIT License
2.41k stars 312 forks source link

Lift from single Observable #1662

Open fbanning opened 2 years ago

fbanning commented 2 years ago

Sometimes when I play around with lifting and I refactor some of my code, I end up trying to lift a single Observable but doing x = @lift($var) throws

https://github.com/JuliaPlots/Makie.jl/blob/bac1f40315d5b2ee170d467842910dead853a28a/src/interaction/liftmacro.jl#L29

A user can circumvent this by doing either x = lift(var) do v; v; end (arguably crappy to read) or just x = var which in the end do almost the same thing. The big difference is that the former also registers x as a listener of var.

In the end that's also what I would intuitively expect the lift macro to do as well when only a single Observable is passed. So maybe these two options could be added to the error message to make it clearer to the user why lifting from a single Observable does not work in the macro.

Or (and I would quite frankly prefer this), we could even make the hacky lift-do solution from above the default behaviour of @lift instead of it throwing an error? Probably there are good reasons against it, so I'm happy to learn something here. :)

jkrumbiegel commented 2 years ago

I mean I guess we could remove the error, I don't remember why I disallowed it at the time. We can just use identity, that's the same as your do

fbanning commented 2 years ago

We can just use identity, that's the same as your do

I'm not sure if it's really the same.

julia> a = Observable(1)
Observable{Int64} with 0 listeners. Value:
1

julia> b = identity(a)
Observable{Int64} with 0 listeners. Value:
1

julia> a
Observable{Int64} with 0 listeners. Value:
1

julia> a[] = 2
2

julia> b
Observable{Int64} with 0 listeners. Value:
2

This updates b as expected but doesn't add b as a listener to a. Using identity is therefore the same as doing b = a but not the same as the do-block in my example above.

I think it would be nice if user can rely on the @lift macro to add new variables as listeners to the existing Observable. This is important for example if a user wants to make a distinction between a[] and a.val (e.g. for delayed updating of lifted variables). Therefore I think the do solution would be better than just using identity. Would you agree?

jkrumbiegel commented 2 years ago

Sorry I meant lift(identity, obs)

fbanning commented 2 years ago

Ah yes, that works as expected. I'll try to prepare a PR for this, if you don't mind.

jkrumbiegel commented 2 years ago

Probably just need to handle that case here https://github.com/JuliaPlots/Makie.jl/blob/bac1f40315d5b2ee170d467842910dead853a28a/src/interaction/liftmacro.jl#L28-L30, maybe even just not error and handle like usual, I didn't check