Hold sync during set_state + fix selection widgets flakiness

ipywidgets/widgets/tests/test_set_state.py

@@ -236,12 +236,35 @@ def _propagate_value(self, change):
     widget.set_state({'value': 42})
     assert widget.value == 42
 
-    # we expect first the {'value': 2.0} state to be send, followed by the {'value': 42.0} state
-    msg = {'method': 'update', 'state': {'value': 2.0}, 'buffer_paths': []}
-    call2 = mock.call(msg, buffers=[])
+    # we expect no new state to be sent
+    calls = []
+    widget._send.assert_has_calls(calls)
+
+def test_hold_sync():
+    # when this widget's value is set to 42, it sets the value to 2, and also sets a different trait value
+    class AnnoyingWidget(Widget):
+        value = Float().tag(sync=True)
+        other = Float().tag(sync=True)
+
+        @observe('value')
+        def _propagate_value(self, change):
+            print('_propagate_value', change.new)
+            if change.new == 42:
+                self.value = 2
+                self.other = 11
+
+    widget = AnnoyingWidget(value=1)
+    assert widget.value == 1
+
+    widget._send = mock.MagicMock()
+    # this mimics a value coming from the front end
+    widget.set_state({'value': 42})
+    assert widget.value == 2
+    assert widget.other == 11
 
-    msg = {'method': 'update', 'state': {'value': 42.0}, 'buffer_paths': []}
+    # we expect only single state to be sent, i.e. the {'value': 42.0} state
+    msg = {'method': 'update', 'state': {'value': 2.0, 'other': 11.0}, 'buffer_paths': []}
     call42 = mock.call(msg, buffers=[])
 
-    calls = [call2, call42]
+    calls = [call42]
     widget._send.assert_has_calls(calls)

ipywidgets/widgets/widget.py

@@ -506,7 +506,7 @@ def set_state(self, sync_data):
         # The order of these context managers is important. Properties must
         # be locked when the hold_trait_notification context manager is
         # released and notifications are fired.
-        with self._lock_property(**sync_data), self.hold_trait_notifications():
+        with self.hold_sync(), self._lock_property(**sync_data), self.hold_trait_notifications():
             for name in sync_data:
                 if name in self.keys:
                     from_json = self.trait_metadata(name, 'from_json',
@@ -608,6 +608,8 @@ def _should_send_property(self, key, value):
             if (jsonloads(jsondumps(split_value[0])) == split_lock[0]
                 and split_value[1] == split_lock[1]
                 and _buffer_list_equal(split_value[2], split_lock[2])):
+                if self._holding_sync:
+                    self._states_to_send.discard(key)
                 return False
         if self._holding_sync:
             self._states_to_send.add(key)

packages/controls/src/widget_selection.ts

@@ -99,16 +99,6 @@ export class DropdownModel extends SelectionModel {
 // For the old code, see commit f68bfbc566f3a78a8f3350b438db8ed523ce3642
 
 export class DropdownView extends SelectionView {
-  /**
-   * Public constructor.
-   */
-  initialize(parameters: WidgetView.IInitializeParameters): void {
-    super.initialize(parameters);
-    this.listenTo(this.model, 'change:_options_labels', () =>
-      this._updateOptions()
-    );
-  }
-
   /**
    * Called when view is rendered.
    */
@@ -127,7 +117,15 @@ export class DropdownView extends SelectionView {
   /**
    * Update the contents of this view
    */
-  update(): void {
+  update(options?: { updated_view?: DropdownView }): void {
+    // Debounce set calls from ourselves:
+    if (options?.updated_view !== this) {
+      const optsChanged = this.model.hasChanged('_options_labels');
+      if (optsChanged) {
+        // Need to update options:
+        this._updateOptions();
+      }
+    }
     // Select the correct element
     const index = this.model.get('index');
     this.listbox.selectedIndex = index === null ? -1 : index;
@@ -159,7 +157,8 @@ export class DropdownView extends SelectionView {
   _handle_change(): void {
     this.model.set(
       'index',
-      this.listbox.selectedIndex === -1 ? null : this.listbox.selectedIndex
+      this.listbox.selectedIndex === -1 ? null : this.listbox.selectedIndex,
+      { updated_view: this }
     );
     this.touch();
   }
@@ -193,12 +192,6 @@ export class SelectView extends SelectionView {
    */
   initialize(parameters: WidgetView.IInitializeParameters): void {
     super.initialize(parameters);
-    this.listenTo(this.model, 'change:_options_labels', () =>
-      this._updateOptions()
-    );
-    this.listenTo(this.model, 'change:index', (model, value, options) =>
-      this.updateSelection(options)
-    );
     // Create listbox here so that subclasses can modify it before it is populated in render()
     this.listbox = document.createElement('select');
   }
@@ -220,7 +213,20 @@ export class SelectView extends SelectionView {
   /**
    * Update the contents of this view
    */
-  update(): void {
+  update(options?: { updated_view?: WidgetView }): void {
+    // Don't update options/index on set calls from ourselves:
+    if (options?.updated_view !== this) {
+      const optsChange = this.model.hasChanged('_options_labels');
+      const idxChange = this.model.hasChanged('index');
+      if (optsChange || idxChange) {
+        // Stash the index to guard against change events
+        const idx = this.model.get('index');
+        if (optsChange) {
+          this._updateOptions();
+        }
+        this.updateSelection(idx);
+      }
+    }
     super.update();
     let rows = this.model.get('rows');
     if (rows === null) {
@@ -229,11 +235,8 @@ export class SelectView extends SelectionView {
     this.listbox.setAttribute('size', rows);
   }
 
-  updateSelection(options: any = {}): void {
-    if (options.updated_view === this) {
-      return;
-    }
-    const index = this.model.get('index');
+  updateSelection(index?: null | number): void {
+    index = index || (this.model.get('index') as null | number);
     this.listbox.selectedIndex = index === null ? -1 : index;
   }
 
@@ -697,8 +700,8 @@ export class SelectionSliderView extends DescriptionView {
    * Called when the model is changed.  The model may have been
    * changed by another view or by a state update from the back-end.
    */
-  update(options?: any): void {
-    if (options === undefined || options.updated_view !== this) {
+  update(options?: { updated_view?: WidgetView }): void {
+    if (options?.updated_view !== this) {
       this.updateSliderOptions(this.model);
       const orientation = this.model.get('orientation');
       const disabled = this.model.get('disabled');
@@ -884,10 +887,7 @@ export class SelectMultipleView extends SelectView {
     this.el.classList.add('widget-select-multiple');
   }
 
-  updateSelection(options: any = {}): void {
-    if (options.updated_view === this) {
-      return;
-    }
+  updateSelection(): void {
     const selected = this.model.get('index') || [];
     const listboxOptions = this.listbox.options;
     // Clear the selection
@@ -934,8 +934,8 @@ export class SelectionRangeSliderView extends SelectionSliderView {
     super.render();
   }
 
-  updateSelection(): void {
-    const index = this.model.get('index');
+  updateSelection(index?: number[]): void {
+    index = index || (this.model.get('index') as number[]);
     this.updateReadout(index);
   }
 

packages/controls/test/src/index.ts

@@ -4,6 +4,7 @@
 import './widget_date_test';
 import './widget_datetime_test';
 import './widget_time_test';
+import './widget_selection_test';
 import './widget_string_test';
 import './widget_upload_test';
 import './lumino/currentselection_test';

packages/controls/test/src/widget_selection_test.ts

@@ -0,0 +1,141 @@
+// Copyright (c) Jupyter Development Team.
+// Distributed under the terms of the Modified BSD License.
+
+import { expect } from 'chai';
+
+import { createTestModel, createTestView } from './utils';
+
+import {
+  DropdownModel,
+  DropdownView,
+  SelectModel,
+  SelectView,
+  SelectMultipleModel,
+  SelectMultipleView,
+} from '../../lib';
+
+describe('Dropdown', () => {
+  describe('DropdownModel', () => {
+    it('should be createable', () => {
+      const model = createTestModel(DropdownModel);
+      expect(model).to.be.an.instanceof(DropdownModel);
+      expect(model.get('index')).to.equal('');
+      expect(model.get('_options_labels')).to.eql([]);
+    });
+
+    it('should be creatable with a state', () => {
+      const state = { _options_labels: ['A', 'B', 'C'], index: 1 };
+      const model = createTestModel(DropdownModel, state);
+      expect(model).to.be.an.instanceof(DropdownModel);
+      expect(model.get('index')).to.equal(1);
+      expect(model.get('_options_labels')).to.eql(['A', 'B', 'C']);
+    });
+  });
+
+  describe('DropdownView', () => {
+    it('should be createable', async () => {
+      const model = createTestModel(DropdownModel);
+      const view = await createTestView(model, DropdownView);
+      expect(view).to.be.an.instanceof(DropdownView);
+      expect(view.model).to.equal(model);
+    });
+
+    it('should handle a set independent of order', async () => {
+      const model = createTestModel(DropdownModel);
+      const view = await createTestView(model, DropdownView);
+      expect(view).to.be.an.instanceof(DropdownView);
+      model.set_state({ _options_labels: ['A', 'B', 'C'], index: 1 });
+      expect(view.listbox.selectedIndex).to.equal(1, 'order 1 failed');
+      model.set_state({ _options_labels: [], index: null });
+      expect(view.listbox.selectedIndex).to.equal(-1);
+      model.set_state({ index: 1, _options_labels: ['A', 'B', 'C'] });
+      expect(view.listbox.selectedIndex).to.equal(1, 'order 2 failed');
+    });
+  });
+});
+
+describe('Select', () => {
+  describe('SelectModel', () => {
+    it('should be createable', () => {
+      const model = createTestModel(SelectModel);
+      expect(model).to.be.an.instanceof(SelectModel);
+      expect(model.get('index')).to.equal('');
+      expect(model.get('_options_labels')).to.eql([]);
+    });
+
+    it('should be creatable with a state', () => {
+      const state = { _options_labels: ['A', 'B', 'C'], index: 1 };
+      const model = createTestModel(SelectModel, state);
+      expect(model).to.be.an.instanceof(SelectModel);
+      expect(model.get('index')).to.equal(1);
+      expect(model.get('_options_labels')).to.eql(['A', 'B', 'C']);
+    });
+  });
+
+  describe('SelectView', () => {
+    it('should be createable', async () => {
+      const model = createTestModel(SelectModel);
+      const view = await createTestView(model, SelectView);
+      expect(view).to.be.an.instanceof(SelectView);
+      expect(view.model).to.equal(model);
+    });
+
+    it('should handle a set independent of order', async () => {
+      const model = createTestModel(SelectModel);
+      const view = await createTestView(model, SelectView);
+      expect(view).to.be.an.instanceof(SelectView);
+      model.set_state({ _options_labels: ['A', 'B', 'C'], index: 1 });
+      expect(view.listbox.selectedIndex).to.equal(1, 'order 1 failed');
+      model.set_state({ _options_labels: [], index: null });
+      expect(view.listbox.selectedIndex).to.equal(-1);
+      model.set_state({ index: 1, _options_labels: ['A', 'B', 'C'] });
+      expect(view.listbox.selectedIndex).to.equal(1, 'order 2 failed');
+    });
+  });
+});
+
+describe('SelectMultiple', () => {
+  describe('SelectMultipleModel', () => {
+    it('should be createable', () => {
+      const model = createTestModel(SelectMultipleModel);
+      expect(model).to.be.an.instanceof(SelectMultipleModel);
+      expect(model.get('index')).to.equal('');
+      expect(model.get('_options_labels')).to.eql([]);
+    });
+
+    it('should be creatable with a state', () => {
+      const state = { _options_labels: ['A', 'B', 'C'], index: 1 };
+      const model = createTestModel(SelectMultipleModel, state);
+      expect(model).to.be.an.instanceof(SelectMultipleModel);
+      expect(model.get('index')).to.equal(1);
+      expect(model.get('_options_labels')).to.eql(['A', 'B', 'C']);
+    });
+  });
+
+  describe('SelectMultipleView', () => {
+    it('should be createable', async () => {
+      const model = createTestModel(SelectMultipleModel);
+      const view = await createTestView(model, SelectMultipleView);
+      expect(view).to.be.an.instanceof(SelectMultipleView);
+      expect(view.model).to.equal(model);
+    });
+
+    it('should handle a set independent of order', async () => {
+      const model = createTestModel(SelectMultipleModel);
+      const view = await createTestView(model, SelectMultipleView);
+      expect(view).to.be.an.instanceof(SelectMultipleView);
+      model.set_state({ _options_labels: ['A', 'B', 'C'], index: [1, 2] });
+      expect([...view.listbox.selectedOptions].map((o) => o.index)).to.eql(
+        [1, 2],
+        'order 1 failed'
+      );
+      model.set_state({ _options_labels: [], index: null });
+      expect([...view.listbox.selectedOptions].map((o) => o.index)).to.eql([]);
+      model.set_state({ index: [1, 2], _options_labels: ['A', 'B', 'C'] });
+      expect([...view.listbox.selectedOptions].map((o) => o.index)).to.eql(
+        [1, 2],
+        'order 2 failed'
+      );
+    });
+  });
+});