glue-viz / glue

Linked Data Visualizations Across Multiple Files
http://glueviz.org
Other
740 stars 153 forks source link

"Fill markers" button doesn't work in "Writing a custom viewer for glue with Qt" tutorial #2388

Closed sergiopasra closed 1 year ago

sergiopasra commented 1 year ago

Describe the bug I'm following the "Writing a custom viewer for glue with Qt" tutorial. I have copied the TutorialViewer classes and viewer_state.ui. I load a csv file and I mostly obtain what is expected with one exception. The "Fill markers" toggle button does nothing. The fill property is never updated and the method _on_fill_changeis never called.

If I add a callback to self.checkbox, than I can get the desired behavior, but connect_checkable_button is not working correctly.

To Reproduce Steps to reproduce the behavior such as:

  1. Copy the code in "Writing a custom viewer for glue with Qt"
  2. Load a CSV file
  3. Drag to plot
  4. Select "Tutorial Viewer"
  5. Click on "Fill markers"

Expected behavior Markers on the plot are filled.

Details:

Carifio24 commented 1 year ago

Hi @sergiopasra, there is indeed a bug in the code given on the tutorial page. In line 132 of the code (inside the initializer for TutorialLayerStateWidget, replace

connect_checkable_button(self.layer_state, 'fill', self.checkbox)

with

self._connection = connect_checkable_button(self.layer_state, 'fill', self.checkbox)

The problem with the current code is that no reference to the created connection is retained, so it doesn't exist anymore when the callback property is modified. The containers that store the callback functions (echo.CallbackContainer) only store weak references to instance methods, and when the instance is garbage collected, the callback will be removed as well.

sergiopasra commented 1 year ago

Thank you! Now code like this makes more sense:

self._connections = autoconnect_callbacks_to_qt(self.viewer_state, self.ui)

So the value returned by autoconnect_callbacks_to_qt is retained.

How do I store more than one callback? Looks like autoconnect_callbacks_to_qt returns a dictionary.

Carifio24 commented 1 year ago

It shouldn't really matter how they're stored - you could have a member value (of the TutorialLayerStateWidget) that's a list of connections, or a dictionary with the connections as values (like autoconnect_callbacks_to_qt), or just have each connection as a separate member value. As long as the connection is still retained somewhere, the callback setup will work correctly.

If you end up having a decent number of connections, note that you could also use the same pattern for the layer state as for the viewer state - create a .ui file that follows echo's autoconnect naming conventions and call autoconnect_callbacks_to_qt to create the connections for you.