From 1c320a9613356e9b7915ba785f08986f6256885b Mon Sep 17 00:00:00 2001 From: Benoit Bovy Date: Fri, 19 May 2023 09:38:17 +0200 Subject: [PATCH] Refactor schedule observer (#118) * refactor ScheduleObserver jsdlink: avoid syncing with the backend and update the target widget directly in the front-end. * fix schedule unlink / unobserve with transport * fix schedule_dlink * fix/update tests * update release notes --- docs/release_notes.rst | 7 ++++ ipytone/observe.py | 37 +++++++++++++++-- ipytone/tests/test_observe.py | 10 ++++- src/widget_observe.ts | 75 ++++++++++++++++++++++++++--------- 4 files changed, 104 insertions(+), 25 deletions(-) diff --git a/docs/release_notes.rst b/docs/release_notes.rst index 3001cb2..15aee98 100644 --- a/docs/release_notes.rst +++ b/docs/release_notes.rst @@ -8,6 +8,13 @@ Release Notes v0.6.0 (unreleased) ------------------- +Bug fixes +~~~~~~~~~ + +- Fixed ``schedule_jsdlink`` so it doesn't synchronize state with the backend + and fix ``unlink`` / ``unobserve`` with ``transport=True`` (:issue:`116`, + :pull:`118`). + Maintenance ~~~~~~~~~~~ diff --git a/ipytone/observe.py b/ipytone/observe.py index 931f152..84146c0 100644 --- a/ipytone/observe.py +++ b/ipytone/observe.py @@ -1,5 +1,5 @@ import ipywidgets -from ipywidgets import widget_serialization +from ipywidgets import Widget, widget_serialization from traitlets import ( Bool, Dict, @@ -19,6 +19,11 @@ from .serialization import data_array_serialization +class DummyLink: + def unlink(self): + pass + + class ToneDirectionalLink: def __init__(self, observer, link): self.observer = observer @@ -64,6 +69,11 @@ class ScheduleObserver(ToneWidgetBase): sync=True ) + target_widget = Instance(Widget, help="target widget instance (jsdlink)", allow_none=True).tag( + sync=True, **widget_serialization + ) + target_trait = Unicode(allow_none=True).tag(sync=True) + time = Float(0.0, help="current observed time", read_only=True).tag(sync=True) value = Union( @@ -145,7 +155,9 @@ def schedule_dlink(self, target, update_interval, transport, js=False): self.schedule_repeat(update_interval, transport, draw=draw) if js: - link = ipywidgets.jsdlink((self, self.observed_trait), target) + # bypass ipywidget.jsdlink (target is updated directly by this + # widget in the front-end) + link = DummyLink() else: link = ipywidgets.dlink((self, self.observed_trait), target) @@ -253,7 +265,7 @@ def schedule_observe( ) observer = ScheduleObserver( - observed_widget=self, observed_trait=name, observe_time=observe_time + observed_widget=self, observed_trait=name, observe_time=observe_time, target_trait=name ) observer.schedule_observe(handler, update_interval, transport) @@ -274,7 +286,24 @@ def schedule_unobserve(self, handler): def _schedule_dlink(self, target, update_interval, transport, name, js=False): name = self._validate_trait_name(name) - observer = ScheduleObserver(observed_widget=self, observed_trait=name) + if not isinstance(target, tuple) or len(target) != 2: + raise ValueError("target must be a (widget_obj, trait_name) 2-length tuple") + + if js: + target_widget, target_trait = target + else: + # the target won't be used directly ; instead the observer instance + # created below will sync with the backend and be linked with the + # target. + target_widget = None + target_trait = name + + observer = ScheduleObserver( + observed_widget=self, + observed_trait=name, + target_widget=target_widget, + target_trait=target_trait, + ) return observer.schedule_dlink(target, update_interval, transport, js=js) def schedule_dlink(self, target, update_interval=1, transport=False, name=None): diff --git a/ipytone/tests/test_observe.py b/ipytone/tests/test_observe.py index f535eb3..2ea7827 100644 --- a/ipytone/tests/test_observe.py +++ b/ipytone/tests/test_observe.py @@ -67,6 +67,7 @@ def test_schedule_observe(widget, trait_name, handler): observer = widget._observers[key] assert observer.observed_widget is widget assert observer.observed_trait == trait_name + assert observer.target_widget is None # test observe already observed with same handler with pytest.raises(ValueError, match="this handler is already used"): @@ -97,6 +98,12 @@ def test_schedule_dlink(widget, trait_name, target_widget, mocker): assert link.observer.observed_widget is widget assert link.observer.observed_trait == trait_name + if method is widget.schedule_dlink: + assert link.observer.target_widget is None + elif method is widget.schedule_jsdlink: + assert link.observer.target_widget is target_widget + assert link.observer.target_trait == trait_name + # test unlink mocker.patch.object(link.link, "unlink") mocker.patch.object(link.observer, "schedule_cancel") @@ -111,6 +118,7 @@ def test_schedule_observer(): assert observer.observed_widget is sig assert observer.observe_time is False + assert observer.target_widget is None assert observer.time == 0.0 assert observer.value == 0 assert observer.state == "stopped" @@ -149,7 +157,6 @@ def test_schedule_observer_schedule_observe(mocker, handler): def test_schedule_observer_schedule_link(mocker, target_widget): dlink_patch = mocker.patch("ipywidgets.dlink") - jsdlink_patch = mocker.patch("ipywidgets.jsdlink") sig = Signal() observer = ScheduleObserver(observed_widget=sig) @@ -168,4 +175,3 @@ def test_schedule_observer_schedule_link(mocker, target_widget): observer.send.assert_called_with( {"event": "scheduleRepeat", "update_interval": 1, "transport": True, "draw": True} ) - jsdlink_patch.assert_called_with((observer, "value"), (target_widget, "value")) diff --git a/src/widget_observe.ts b/src/widget_observe.ts index e09f0e0..a3861c1 100644 --- a/src/widget_observe.ts +++ b/src/widget_observe.ts @@ -1,4 +1,8 @@ -import { ISerializers, unpack_models } from '@jupyter-widgets/base'; +import { + ISerializers, + unpack_models, + WidgetModel, +} from '@jupyter-widgets/base'; import * as tone from 'tone'; @@ -39,6 +43,8 @@ export class ScheduleObserverModel extends ToneWidgetModel { observed_widget: null, observed_trait: 'value', observe_time: false, + target_widget: null, + target_trait: 'value', time: 0.0, value: 0.0, state: 'stopped', @@ -52,6 +58,8 @@ export class ScheduleObserverModel extends ToneWidgetModel { } private event: ObserveEvent; + private _observedWidget: ObservableModel; + private _targetWidget: WidgetModel; initialize( attributes: Backbone.ObjectHash, @@ -60,10 +68,21 @@ export class ScheduleObserverModel extends ToneWidgetModel { super.initialize(attributes, options); this.event = { id: 0, transport: false }; + + // cache unserialized widget models for fast access in schedule loop + this._observedWidget = this.get('observed_widget'); + this._targetWidget = this.get('target_widget'); + if (this._targetWidget === null) { + this._targetWidget = this; + } } + /* + * Returns the observed ipytone widget model (e.g., audio node, signal, param + * or transport) + */ get observedWidget(): ObservableModel { - return this.get('observed_widget'); + return this._observedWidget; } get observedTrait(): string { @@ -74,35 +93,52 @@ export class ScheduleObserverModel extends ToneWidgetModel { return this.get('observe_time'); } - private setObservedTrait(time: tone.Unit.Seconds, transport: boolean): void { - const model = this.observedWidget; - const traitName = this.observedTrait; - let traitValue: TraitValue = 0; + /* + * Returns either this observer model instance (observe or dlink) or another + * widget model instance (jsdlink) + */ + get targetWidget(): WidgetModel { + return this._targetWidget; + } - if (traitName === 'array') { + get targetTrait(): string { + return this.get('target_trait'); + } + + private setTargetTrait(time: tone.Unit.Seconds, transport: boolean): void { + const observedWidget = this.observedWidget; + const observedTrait = this.observedTrait; + const targetWidget = this.targetWidget; + const targetTrait = this.targetTrait; + let value: TraitValue = 0; + + if (observedTrait === 'array') { // bug when array elements aren't changing, then it's never // synced again even if it gets updated again later? // -> reset array value silently - this.set('array', new Float32Array([0]), { silent: true }); + targetWidget.set('array', new Float32Array([0]), { silent: true }); } - if (traitName === 'time') { - traitValue = time; + if (observedTrait === 'time') { + value = time; } else { if (transport) { - traitValue = model.getValueAtTime(traitName, time); + value = observedWidget.getValueAtTime(observedTrait, time); } else { - traitValue = model.getValue(traitName); + value = observedWidget.getValue(observedTrait); } } if (this.observeTime) { - this.set('time_value', [time, traitValue]); + targetWidget.set('time_value', [time, value]); } else { - this.set(traitName, traitValue); + targetWidget.set(targetTrait, value); } - this.save_changes(); + if (targetWidget === this) { + // sync with backend unless target is another widget (jsdlink). + targetWidget.save_changes(); + } } private scheduleRepeat( @@ -116,16 +152,16 @@ export class ScheduleObserverModel extends ToneWidgetModel { eid = tone.Transport.scheduleRepeat((time) => { if (draw) { tone.Draw.schedule(() => { - this.setObservedTrait(time, transport); + this.setTargetTrait(time, transport); }, time); } else { - this.setObservedTrait(time, transport); + this.setTargetTrait(time, transport); } }, updateInterval); } else { eid = setInterval( () => { - this.setObservedTrait(tone.now(), transport); + this.setTargetTrait(tone.now(), transport); }, (updateInterval as number) * 1000, ); @@ -136,7 +172,7 @@ export class ScheduleObserverModel extends ToneWidgetModel { private scheduleCancel(): void { if (this.event.transport) { - tone.Transport.cancel(this.event.id as number); + tone.Transport.clear(this.event.id as number); } else { clearInterval(this.event.id as ReturnType); } @@ -161,6 +197,7 @@ export class ScheduleObserverModel extends ToneWidgetModel { static serializers: ISerializers = { ...ToneWidgetModel.serializers, observed_widget: { deserialize: unpack_models as any }, + target_widget: { deserialize: unpack_models as any }, array: dataarray_serialization, };