fury-gl / fury

FURY - Free Unified Rendering in pYthon.
https://fury.gl
Other
246 stars 182 forks source link

Possibly remove or rename on_change #74

Open Garyfallidis opened 5 years ago

Garyfallidis commented 5 years ago

Let's say you build a LineSlider2D

linesl = LineSlider2D()
def callback(slider):
    do something

linesl.on_change = callback

that should work. But if you want to connect to a specific event. You need to do

linesl.handle_events(volume_slider.handle.actor)
volume_slider.on_left_mouse_button_released = callback2

But now callback2 needs a different signature

def callback2(iren, obj, slider):
     do something
     iren.force_render()

It becomes quite misleading to have two types of supported callbacks. Furthermore, it is strange that the LineSlider needs to call handle_events first. Other UI components do not require that.

Finally, what on_change means for people? Is it click, is it move, is it dragged, is it all together?

MarcCote commented 5 years ago

I agree callbacks need to be re-think a bit. Here's my rationale behind how it is structured at the moment.

Regarding the specific event you mentioned (button pressed/released/dragged), you need to define what vtk object should observe the event. In your example, it seems you want to set the callback to the left mouse button is released event of the handle. Which translates to

lines1.handle.on_left_mouse_button_released = callback2

Note there's no need to call handle_events. That function should only be called internally actually and users should only use the provided hooks, i.e. on_ variables/methods.

That said, there's a small caveat here because that would override the default callback for that event :(. It would probably make more sense, to allow multiple callbacks per event.

Regarding the callback signatures, you are right. We should uniformize all of them that are exposed to the users.

As for the on_change events, my take on it is their associated callback should be called whenever the value is updated. So far, on_change is defined for the sliders, checkboxes and the ListBox2D (which seems to have its own signature as well). Also, it seems we don't have an on_change for the textbox!

As a suggestion (I think I already talked about that in a meeting), I think the users shouldn't be using the callbacks/events defined in the UI abstract class (e.g. on_left_mouse_button_released, ...). Instead, we should only expose callbacks/hooks that make sense to the users like the on_change and keep the signature to be lambda ui_component: None. That means, in your specific example, we would need a more abstract on_click which is going to fire up whenever the user clicks any part of the component (handle, slider, text, etc).

What do you guys think of that? Also, I'm all for looking at what already exists out there and re-use a system that has already proven itself (maybe recent versions of VTK are more user-friendly now?).

Garyfallidis commented 3 years ago

A note here to also check when global variables in callbacks are really needed and when not.