chaco icon indicating copy to clipboard operation
chaco copied to clipboard

DataRange2D set_bounds method fires multiple events

Open ievacerny opened this issue 4 years ago • 1 comments

Problem Description The goal of set_bounds is to update all the bounds simultaneously under one event. Current implementation fires multiple events (depending on how many values have changed).

Reproduction Steps: Checked by writing a unittest for DataRange2D

    def test_set_bounds_events(self):
        with self.subTest(msg="DataRange2D"):
            r = DataRange2D()
            with self.assertTraitChanges(r, 'updated', count=1):
                r.set_bounds((-1, -2), (3, 4))

        with self.subTest(msg="DataRange2D.x_range"):
            r = DataRange2D()
            with self.assertTraitChanges(r.x_range, 'updated', count=1):
                r.set_bounds((-1, -2), (3, 4))

        with self.subTest(msg="DataRange2D.y_range"):
            r = DataRange2D()
            with self.assertTraitChanges(r.y_range, 'updated', count=1):
                r.set_bounds((-1, -2), (3, 4))

Output from each subtest:

FAIL: test_set_bounds_events (chaco.tests.test_datarange_2d.DataRange2DTestCase) [DataRange2D]
...
AssertionError: Change event for updated was fired 4 times instead of 1

FAIL: test_set_bounds_events (chaco.tests.test_datarange_2d.DataRange2DTestCase) [DataRange2D.x_range]
...
AssertionError: Change event for updated was fired 2 times instead of 1

FAIL: test_set_bounds_events (chaco.tests.test_datarange_2d.DataRange2DTestCase) [DataRange2D.y_range]
...
AssertionError: Change event for updated was fired 2 times instead of 1

Expected behavior:

The call to this method should fire only one event on DataRange2D and associated DataRange1D instances.

ievacerny avatar Mar 30 '20 17:03 ievacerny

Hah, this is a fun one. For the first example, at least, the DataRange2D sets the bounds of x_range and y_range individually, and each of those generates an updated event. The weird thing is that it claims to do the update of the lower bounds at least without firing an event (via the fire_event=False parameter), but that parameter has no effect in the code. https://github.com/enthought/chaco/blob/192675d6cb579df56458b87988026d895e7a7a84/chaco/data_range_2d.py#L103

I think chaco could be more parsimonious with the number of events that it fires, but I don't know if changing this behaviour would have averse consequences elsewhere. I'd be interested in @corranwebster's take on this.

Nice work narrowing things down to a single (data) class!

jvkersch avatar Mar 31 '20 07:03 jvkersch