From 4f7797966ef9bc8caa11ae02bcd15371fa8b0b03 Mon Sep 17 00:00:00 2001 From: Daan Vanden Bosch Date: Fri, 25 Sep 2020 22:39:31 +0200 Subject: [PATCH] Fixed bug in FlatMappedProperty that resulted in undo stack switching not working correctly. --- .../observable/property/DependentProperty.ts | 11 +++-- .../property/FlatMappedProperty.test.ts | 26 ++++++++++ .../observable/property/FlatMappedProperty.ts | 19 ++++--- src/core/undo/SimpleUndo.ts | 4 +- src/core/undo/UndoManager.ts | 49 +++++++++++-------- 5 files changed, 73 insertions(+), 36 deletions(-) create mode 100644 src/core/observable/property/FlatMappedProperty.test.ts diff --git a/src/core/observable/property/DependentProperty.ts b/src/core/observable/property/DependentProperty.ts index dc269e15..e15f6dbb 100644 --- a/src/core/observable/property/DependentProperty.ts +++ b/src/core/observable/property/DependentProperty.ts @@ -11,7 +11,8 @@ import { ChangeEvent } from "../Observable"; */ export abstract class DependentProperty extends AbstractMinimalProperty { private dependency_disposer = new Disposer(); - private _val?: T; + + protected _val?: T; get val(): T { return this.get_val(); @@ -19,7 +20,7 @@ export abstract class DependentProperty extends AbstractMinimalProperty { get_val(): T { if (this.should_recompute()) { - this._val = this.compute_value(); + this._val = this.compute_value(this.observers.length > 0); } return this._val as T; @@ -34,13 +35,13 @@ export abstract class DependentProperty extends AbstractMinimalProperty { options?: { call_now?: boolean }, ): Disposable { if (this.dependency_disposer.length === 0) { - this._val = this.compute_value(); + this._val = this.compute_value(true); this.dependency_disposer.add_all( ...this.dependencies.map(dependency => dependency.observe(() => { const old_value = this._val!; - this._val = this.compute_value(); + this._val = this.compute_value(true); if (this._val !== old_value) { this.emit(); @@ -67,5 +68,5 @@ export abstract class DependentProperty extends AbstractMinimalProperty { return this.dependency_disposer.length === 0; } - protected abstract compute_value(): T; + protected abstract compute_value(has_observers: boolean): T; } diff --git a/src/core/observable/property/FlatMappedProperty.test.ts b/src/core/observable/property/FlatMappedProperty.test.ts new file mode 100644 index 00000000..dbab8c6a --- /dev/null +++ b/src/core/observable/property/FlatMappedProperty.test.ts @@ -0,0 +1,26 @@ +import { FlatMappedProperty } from "./FlatMappedProperty"; +import { SimpleProperty } from "./SimpleProperty"; +import { with_disposable } from "../../../../test/src/core/observables/disposable_helpers"; + +// This is a regression test, it's important that this exact sequence of statements stays the same. +test(`It should emit a change when its direct property dependency changes.`, () => { + // p is the direct property dependency. + const p = new SimpleProperty(new SimpleProperty(7)); + const fp = new FlatMappedProperty([p], () => p.val); + let v: number | undefined; + + with_disposable( + fp.observe(({ value }) => (v = value)), + () => { + expect(v).toBeUndefined(); + + p.val.val = 99; + + expect(v).toBe(99); + + p.val = new SimpleProperty(7); + + expect(v).toBe(7); + }, + ); +}); diff --git a/src/core/observable/property/FlatMappedProperty.ts b/src/core/observable/property/FlatMappedProperty.ts index aeed8de9..d21a4ebc 100644 --- a/src/core/observable/property/FlatMappedProperty.ts +++ b/src/core/observable/property/FlatMappedProperty.ts @@ -9,10 +9,10 @@ export class FlatMappedProperty extends DependentProperty { private computed_disposable?: Disposable; get_val(): T { - if (this.should_recompute() || !this.computed_property) { + if (this.should_recompute()) { return super.get_val(); } else { - return this.computed_property.val; + return this.computed_property!.val; } } @@ -50,14 +50,17 @@ export class FlatMappedProperty extends DependentProperty { return new FlatMappedProperty([this], () => transform(this.val)); } - protected compute_value(): T { - this.computed_disposable?.dispose(); - + protected compute_value(has_observers: boolean): T { this.computed_property = this.compute(); - this.computed_disposable = this.computed_property.observe(() => { - this.emit(); - }); + this.computed_disposable?.dispose(); + + if (has_observers) { + this.computed_disposable = this.computed_property.observe(() => { + this._val = this.computed_property!.val; + this.emit(); + }); + } return this.computed_property.val; } diff --git a/src/core/undo/SimpleUndo.ts b/src/core/undo/SimpleUndo.ts index 8ce02aef..f39c50dc 100644 --- a/src/core/undo/SimpleUndo.ts +++ b/src/core/undo/SimpleUndo.ts @@ -31,9 +31,9 @@ export class SimpleUndo implements Undo { undo_manager.current.val = this; } - readonly can_undo = property(false); + readonly can_undo: WritableProperty = property(false); - readonly can_redo = property(false); + readonly can_redo: WritableProperty = property(false); readonly first_undo: Property; diff --git a/src/core/undo/UndoManager.ts b/src/core/undo/UndoManager.ts index b98ddf40..0bfd09d9 100644 --- a/src/core/undo/UndoManager.ts +++ b/src/core/undo/UndoManager.ts @@ -1,39 +1,46 @@ import { property } from "../observable"; import { Undo } from "./Undo"; +import { Property } from "../observable/property/Property"; +import { Action } from "./Action"; +import { WritableProperty } from "../observable/property/WritableProperty"; -const NOOP_UNDO: Undo = { - can_redo: property(false), - can_undo: property(false), - first_redo: property(undefined), - first_undo: property(undefined), +class NoopUndo implements Undo { + readonly can_redo = property(false); + readonly can_undo = property(false); + readonly first_redo = property(undefined); + readonly first_undo = property(undefined); - make_current() { - undo_manager.current.val = this; - }, + constructor(private readonly manager: UndoManager) {} - redo() { + make_current(): void { + this.manager.current.val = this; + } + + redo(): boolean { return false; - }, + } - reset() { + reset(): void { // Do nothing. - }, + } - undo() { + undo(): boolean { return false; - }, -}; + } +} export class UndoManager { - readonly current = property(NOOP_UNDO); + private readonly noop_undo = new NoopUndo(this); - can_undo = this.current.flat_map(c => c.can_undo); + readonly current: WritableProperty = property(this.noop_undo); - can_redo = this.current.flat_map(c => c.can_redo); + readonly can_undo: Property = this.current.flat_map(c => c.can_undo); - first_undo = this.current.flat_map(c => c.first_undo); + readonly can_redo: Property = this.current.flat_map(c => c.can_redo); - first_redo = this.current.flat_map(c => c.first_redo); + readonly first_undo: Property = this.current.flat_map(c => c.first_undo); + + readonly first_redo: Property = this.current.flat_map(c => c.first_redo); undo(): boolean { return this.current.val.undo(); @@ -44,7 +51,7 @@ export class UndoManager { } make_noop_current(): void { - undo_manager.current.val = NOOP_UNDO; + this.current.val = this.noop_undo; } }