From 1572991a894c3dd84910ab4cfaa4459c04fb6f5a Mon Sep 17 00:00:00 2001 From: Ben Gotow Date: Sat, 8 Jul 2017 12:33:41 -0700 Subject: [PATCH] Very basic implementation of undo/redo where redo data is built in JS --- .../spec/stores/undo-redo-store-spec.es6 | 22 ++++---- .../spec/tasks/change-mail-task-spec.coffee | 6 +-- .../spec/tasks/syncback-model-task-spec.es6 | 4 +- .../flux/attributes/attribute-collection.es6 | 12 +++-- .../src/flux/attributes/attribute-object.es6 | 9 ++-- .../src/flux/stores/undo-redo-store.es6 | 25 +++++---- .../src/flux/tasks/change-folder-task.es6 | 22 ++++++++ .../src/flux/tasks/change-labels-task.es6 | 19 +++++-- .../src/flux/tasks/change-mail-task.es6 | 54 ++++++------------- .../src/flux/tasks/change-starred-task.es6 | 21 ++++++-- .../src/flux/tasks/change-unread-task.es6 | 28 +++++----- .../src/flux/tasks/destroy-model-task.es6 | 3 -- .../src/flux/tasks/syncback-model-task.es6 | 4 -- packages/client-app/src/flux/tasks/task.es6 | 47 ++++------------ 14 files changed, 131 insertions(+), 145 deletions(-) diff --git a/packages/client-app/spec/stores/undo-redo-store-spec.es6 b/packages/client-app/spec/stores/undo-redo-store-spec.es6 index b2d381689..5377289b9 100644 --- a/packages/client-app/spec/stores/undo-redo-store-spec.es6 +++ b/packages/client-app/spec/stores/undo-redo-store-spec.es6 @@ -1,20 +1,16 @@ import {Actions, Task, UndoRedoStore} from 'nylas-exports' class Undoable extends Task { - canBeUndone() { - return true - } - - createIdenticalTask() { - const t = new Undoable() - t.id = this.id - return t + constructor() { + super(); + this.canBeUndone = true; } } class PermanentTask extends Task { - canBeUndone() { - return false + constructor() { + super(); + this.canBeUndone = false; } } @@ -73,7 +69,7 @@ describe("UndoRedoStore", function undoRedoStoreSpec() { it("doesn't refresh redo if our task is itself a redo task", () => { UndoRedoStore._redo = [[this.t1, this.t2], [this.t3]] const tr = new Undoable() - tr.isRedoTask = true + tr.source = 'redo' Actions.queueTask(tr) expect(UndoRedoStore._redo).toEqual([[this.t1, this.t2], [this.t3]]) expect(UndoRedoStore._undo).toEqual([[tr]]) @@ -112,8 +108,8 @@ describe("UndoRedoStore", function undoRedoStoreSpec() { UndoRedoStore._undo = [[this.t3], [this.t1, this.t2]] UndoRedoStore.undo() UndoRedoStore.redo() - expect(Actions.queueTasks.calls[0].args[0][0].isRedoTask).toBe(true) - expect(Actions.queueTasks.calls[0].args[0][1].isRedoTask).toBe(true) + expect(Actions.queueTasks.calls[0].args[0][0].source).toEqual('redo') + expect(Actions.queueTasks.calls[0].args[0][1].source).toEqual('redo') }); it("correctly follows the undo redo sequence of events", () => { diff --git a/packages/client-app/spec/tasks/change-mail-task-spec.coffee b/packages/client-app/spec/tasks/change-mail-task-spec.coffee index 2595c9621..ea3684800 100644 --- a/packages/client-app/spec/tasks/change-mail-task-spec.coffee +++ b/packages/client-app/spec/tasks/change-mail-task-spec.coffee @@ -80,18 +80,18 @@ xdescribe "ChangeMailTask", -> expect(clone.threads).toEqual([@threadA.id, @threadB.id]) describe "createUndoTask", -> - it "should return a task initialized with _isUndoTask and _restoreValues", -> + it "should return a task initialized with isUndo and _restoreValues", -> task = new ChangeMailTask() task.messages = [@threadAMesage1, @threadAMesage2] task._restoreValues = {'A': 'bla'} undo = task.createUndoTask() expect(undo.messages).toEqual([@threadAMesage1.id, @threadAMesage2.id]) expect(undo._restoreValues).toBe(task._restoreValues) - expect(undo._isUndoTask).toBe(true) + expect(undo.isUndo).toBe(true) it "should throw if you try to make an undo task of an undo task", -> task = new ChangeMailTask() - task._isUndoTask = true + task.isUndo = true expect( -> task.createUndoTask()).toThrow() it "should throw if _restoreValues are not availble", -> diff --git a/packages/client-app/spec/tasks/syncback-model-task-spec.es6 b/packages/client-app/spec/tasks/syncback-model-task-spec.es6 index 6bd2f6831..0bf5aa7f7 100644 --- a/packages/client-app/spec/tasks/syncback-model-task-spec.es6 +++ b/packages/client-app/spec/tasks/syncback-model-task-spec.es6 @@ -199,11 +199,11 @@ xdescribe('SyncbackModelTask', function syncbackModelTask() { describe("undo/redo", () => { it("cant be undone", () => { - expect(this.task.canBeUndone()).toBe(false) + expect(this.task.canBeUndone).toBe(false) }); it("isn't an undo task", () => { - expect(this.task.isUndo()).toBe(false) + expect(this.task.isUndo).toBe(false) }); }); }); diff --git a/packages/client-app/src/flux/attributes/attribute-collection.es6 b/packages/client-app/src/flux/attributes/attribute-collection.es6 index c7a96c5a3..59c6f6cd6 100644 --- a/packages/client-app/src/flux/attributes/attribute-collection.es6 +++ b/packages/client-app/src/flux/attributes/attribute-collection.es6 @@ -32,7 +32,7 @@ Section: Database export default class AttributeCollection extends Attribute { constructor({modelKey, jsonKey, itemClass, joinOnField, joinQueryableBy, queryable}) { super({modelKey, jsonKey, queryable}); - this.ItemClass = this.itemClass = itemClass; + this.itemClass = itemClass; this.joinOnField = joinOnField; this.joinQueryableBy = joinQueryableBy || []; } @@ -47,22 +47,24 @@ export default class AttributeCollection extends Attribute { } return vals.map((val) => { - if (this.ItemClass && !(val instanceof this.ItemClass)) { - throw new Error(`AttributeCollection::toJSON: Value \`${val}\` in ${this.modelKey} is not an ${this.ItemClass.name}`); + if (this.itemClass && !(val instanceof this.itemClass)) { + throw new Error(`AttributeCollection::toJSON: Value \`${val}\` in ${this.modelKey} is not an ${this.itemClass.name}`); } return (val.toJSON !== undefined) ? val.toJSON() : val; }); } fromJSON(json) { + const Klass = this.itemClass; + if (!json || !(json instanceof Array)) { return []; } return json.map((objJSON) => { - if (!objJSON || !this.ItemClass || objJSON instanceof this.ItemClass) { + if (!objJSON || !Klass || objJSON instanceof Klass) { return objJSON; } - return new this.ItemClass(objJSON); + return new Klass(objJSON); }); } diff --git a/packages/client-app/src/flux/attributes/attribute-object.es6 b/packages/client-app/src/flux/attributes/attribute-object.es6 index e35370dce..fbe5ee607 100644 --- a/packages/client-app/src/flux/attributes/attribute-object.es6 +++ b/packages/client-app/src/flux/attributes/attribute-object.es6 @@ -8,7 +8,7 @@ Section: Database export default class AttributeObject extends Attribute { constructor({modelKey, jsonKey, itemClass, queryable}) { super({modelKey, jsonKey, queryable}); - this.ItemClass = itemClass; + this.itemClass = itemClass; } toJSON(val) { @@ -16,11 +16,12 @@ export default class AttributeObject extends Attribute { } fromJSON(val) { - if (!val || (this.ItemClass && val instanceof this.ItemClass)) { + const Klass = this.itemClass; + if (!val || (Klass && val instanceof Klass)) { return val; } - if (this.ItemClass) { - return new this.ItemClass(val); + if (Klass) { + return new Klass(val); } if (val.__cls) { return Utils.convertToModel(val); diff --git a/packages/client-app/src/flux/stores/undo-redo-store.es6 b/packages/client-app/src/flux/stores/undo-redo-store.es6 index 6be6a35d5..e43a7384d 100644 --- a/packages/client-app/src/flux/stores/undo-redo-store.es6 +++ b/packages/client-app/src/flux/stores/undo-redo-store.es6 @@ -1,8 +1,8 @@ -import _ from 'underscore'; import NylasStore from 'nylas-store'; - import Actions from '../actions'; +const TASK_SOURCE_REDO = 'redo'; + class UndoRedoStore extends NylasStore { constructor() { @@ -19,16 +19,15 @@ class UndoRedoStore extends NylasStore { NylasEnv.commands.add(document.body, {'core:redo': this.redo }); } - _onQueue = (taskArg) => { - if (!taskArg) { return; } - let tasks = taskArg; - if (!(tasks instanceof Array)) { tasks = [tasks]; } - if (tasks.length <= 0) { return; } - const undoable = _.every(tasks, t => t.canBeUndone()); - const isRedoTask = _.every(tasks, t => t.isRedoTask); + _onQueue = (taskOrTasks) => { + const tasks = taskOrTasks instanceof Array ? taskOrTasks : [taskOrTasks]; + if (tasks.length === 0) { return; } - if (undoable) { - if (!isRedoTask) { this._redo = []; } + const isUndoableAndNotUndo = tasks.every(t => t.canBeUndone && !t.isUndo); + const isRedo = tasks.every(t => t.source === TASK_SOURCE_REDO); + + if (isUndoableAndNotUndo) { + if (!isRedo) { this._redo = []; } this._undo.push(tasks); this._mostRecentTasks = tasks; this.trigger(); @@ -43,12 +42,12 @@ class UndoRedoStore extends NylasStore { this.trigger(); for (const task of topTasks) { - Actions.queueTask(task.createUndoTask()); + Actions.queueTask(task.createUndoTask()) } const redoTasks = topTasks.map((t) => { const redoTask = t.createIdenticalTask(); - redoTask.isRedoTask = true; + redoTask.source = TASK_SOURCE_REDO; return redoTask; }); this._redo.push(redoTasks); diff --git a/packages/client-app/src/flux/tasks/change-folder-task.es6 b/packages/client-app/src/flux/tasks/change-folder-task.es6 index 9e169dcab..b8c5c91f4 100644 --- a/packages/client-app/src/flux/tasks/change-folder-task.es6 +++ b/packages/client-app/src/flux/tasks/change-folder-task.es6 @@ -16,6 +16,10 @@ import Folder from '../models/folder'; export default class ChangeFolderTask extends ChangeMailTask { static attributes = Object.assign({}, ChangeMailTask.attributes, { + previousFolder: Attributes.Object({ + modelKey: 'folder', + itemClass: Folder, + }), folder: Attributes.Object({ modelKey: 'folder', itemClass: Folder, @@ -23,7 +27,17 @@ export default class ChangeFolderTask extends ChangeMailTask { }); constructor(data = {}) { + if (data.threads) { + data.threads = data.threads.filter(t => t.folder.id !== data.folder.id); + } + if (data.messages) { + data.messages = data.messages.filter(m => m.folder.id !== data.folder.id); + } + super(data); + + // grab the folder we'll revert to in case of undo/redo + this.previousFolder = this.previousFolder || [].concat(data.threads, data.messages).pop().folder; } label() { @@ -65,4 +79,12 @@ export default class ChangeFolderTask extends ChangeMailTask { _isArchive() { return this.folder.name === "archive" || this.folder.name === "all" } + + createUndoTask() { + const task = super.createUndoTask(); + const {folder, previousFolder} = task; + task.folder = previousFolder; + task.previousFolder = folder; + return task; + } } diff --git a/packages/client-app/src/flux/tasks/change-labels-task.es6 b/packages/client-app/src/flux/tasks/change-labels-task.es6 index 4afe016bc..3685d0710 100644 --- a/packages/client-app/src/flux/tasks/change-labels-task.es6 +++ b/packages/client-app/src/flux/tasks/change-labels-task.es6 @@ -15,16 +15,19 @@ export default class ChangeLabelsTask extends ChangeMailTask { static attributes = Object.assign({}, ChangeMailTask.attributes, { labelsToAdd: Attributes.Collection({ modelKey: 'labelsToAdd', - ItemClass: Label, + itemClass: Label, }), labelsToRemove: Attributes.Collection({ modelKey: 'labelsToRemove', - ItemClass: Label, + itemClass: Label, }), }); - constructor(options = {}) { - super(options); + constructor(data = {}) { + if (data.messages) { + throw new Error("ChangeLabelsTask: Changing individual message labels is unsupported"); + } + super(data); } label() { @@ -83,4 +86,12 @@ export default class ChangeLabelsTask extends ChangeMailTask { } super.validate(); } + + createUndoTask() { + const task = super.createUndoTask(); + const {labelsToAdd, labelsToRemove} = task; + task.labelsToAdd = labelsToRemove; + task.labelsToRemove = labelsToAdd; + return task; + } } diff --git a/packages/client-app/src/flux/tasks/change-mail-task.es6 b/packages/client-app/src/flux/tasks/change-mail-task.es6 index 5dc491812..ca45659e2 100644 --- a/packages/client-app/src/flux/tasks/change-mail-task.es6 +++ b/packages/client-app/src/flux/tasks/change-mail-task.es6 @@ -9,19 +9,10 @@ Subclasses implement {ChangeMailTask::changesToModel} and {ChangeMailTask::requestBodyForModel} to define the specific transforms they provide, and override {ChangeMailTask::performLocal} to perform additional consistency checks. - -ChangeMailTask aims to be fast and efficient. It does not write changes to -the database or make API requests for models that are unmodified by -{ChangeMailTask::changesToModel} - -ChangeMailTask stores the previous values of all models it changes into -this._restoreValues and handles undo/redo. When undoing, it restores previous -values and calls {ChangeMailTask::requestBodyForModel} to make undo API -requests. It does not call {ChangeMailTask::changesToModel}. */ export default class ChangeMailTask extends Task { - static attributes = Object.assign({}, ChangeMailTask.attributes, { + static attributes = Object.assign({}, Task.attributes, { taskDescription: Attributes.String({ modelKey: 'taskDescription', }), @@ -31,47 +22,36 @@ export default class ChangeMailTask extends Task { messageIds: Attributes.Collection({ modelKey: 'messageIds', }), + canBeUndone: Attributes.Boolean({ + modelKey: 'canBeUndone', + }), + isUndo: Attributes.Boolean({ + modelKey: 'isUndo', + }), }); - constructor({threads, thread, messages, message, ...rest} = {}) { + constructor({threads = [], messages = [], ...rest} = {}) { super(rest); - const t = threads || []; - if (thread) { - t.push(thread); - } - const m = messages || []; - if (message) { - m.push(message); - } - // we actually only keep a small bit of data now - this.threadIds = t.map(i => i.id); - this.messageIds = m.map(i => i.id); - this.accountId = (t[0] || m[0] || {}).accountId; + this.threadIds = this.threadIds || threads.map(i => i.id); + this.messageIds = this.messageIds || messages.map(i => i.id); + this.accountId = this.accountId || (threads[0] || messages[0] || {}).accountId; + + if (this.canBeUndone === undefined) { + this.canBeUndone = true; + } } // Task lifecycle - canBeUndone() { - return true; - } - - isUndo() { - return this._isUndoTask === true; - } - createUndoTask() { - if (this._isUndoTask) { + if (this.isUndo) { throw new Error("ChangeMailTask::createUndoTask Cannot create an undo task from an undo task."); } - if (!this._restoreValues) { - throw new Error("ChangeMailTask::createUndoTask Cannot undo a task which has not finished performLocal yet."); - } const task = this.createIdenticalTask(); - task._restoreValues = this._restoreValues; - task._isUndoTask = true; + task.isUndo = true; return task; } diff --git a/packages/client-app/src/flux/tasks/change-starred-task.es6 b/packages/client-app/src/flux/tasks/change-starred-task.es6 index 64305c938..83d8fda91 100644 --- a/packages/client-app/src/flux/tasks/change-starred-task.es6 +++ b/packages/client-app/src/flux/tasks/change-starred-task.es6 @@ -14,9 +14,14 @@ export default class ChangeStarredTask extends ChangeMailTask { }), }); - constructor({starred, ...rest} = {}) { - super(rest); - this.starred = starred; + constructor(data = {}) { + if (data.threads) { + data.threads = data.threads.filter(t => t.starred !== data.starred); + } + if (data.messages) { + data.messages = data.messages.filter(m => m.starred !== data.starred); + } + super(data); } label() { @@ -27,7 +32,7 @@ export default class ChangeStarredTask extends ChangeMailTask { const count = this.threadIds.length; const type = count > 1 ? "threads" : "thread"; - if (this._isUndoTask) { + if (this.isUndo) { return `Undoing changes to ${count} ${type}` } @@ -45,6 +50,12 @@ export default class ChangeStarredTask extends ChangeMailTask { super.validate(); } + createUndoTask() { + const task = super.createUndoTask(); + task.starred = !this.starred; + return task; + } + recordUserEvent() { if (this.source === "Mail Rules") { return @@ -54,7 +65,7 @@ export default class ChangeStarredTask extends ChangeMailTask { source: this.source, numThreads: this.threadIds.length, description: this.description(), - isUndo: this._isUndoTask, + isUndo: this.isUndo, }) } } diff --git a/packages/client-app/src/flux/tasks/change-unread-task.es6 b/packages/client-app/src/flux/tasks/change-unread-task.es6 index ee9207bd5..c2cae5ace 100644 --- a/packages/client-app/src/flux/tasks/change-unread-task.es6 +++ b/packages/client-app/src/flux/tasks/change-unread-task.es6 @@ -9,18 +9,19 @@ import ChangeMailTask from './change-mail-task'; export default class ChangeUnreadTask extends ChangeMailTask { static attributes = Object.assign({}, ChangeMailTask.attributes, { - starred: Attributes.Boolean({ + unread: Attributes.Boolean({ modelKey: 'unread', }), - _canBeUndone: Attributes.Boolean({ - modelKey: '_canBeUndone', - }), }); - constructor({unread, canBeUndone, ...rest} = {}) { - super(rest); - this.unread = unread; - this._canBeUndone = canBeUndone; + constructor(data = {}) { + if (data.threads) { + data.threads = data.threads.filter(t => t.unread !== data.unread); + } + if (data.messages) { + data.messages = data.messages.filter(m => m.unread !== data.unread); + } + super(data); } label() { @@ -31,7 +32,7 @@ export default class ChangeUnreadTask extends ChangeMailTask { const count = this.threadIds.length; const type = count > 1 ? 'threads' : 'thread'; - if (this._isUndoTask) { + if (this.isUndo) { return `Undoing changes to ${count} ${type}`; } @@ -42,10 +43,9 @@ export default class ChangeUnreadTask extends ChangeMailTask { return `Marked as ${newState}`; } - canBeUndone() { - if (this._canBeUndone == null) { - return super.canBeUndone() - } - return this._canBeUndone + createUndoTask() { + const task = super.createUndoTask(); + task.unread = !this.unread; + return task; } } diff --git a/packages/client-app/src/flux/tasks/destroy-model-task.es6 b/packages/client-app/src/flux/tasks/destroy-model-task.es6 index d7bc5e1a3..e7449653c 100644 --- a/packages/client-app/src/flux/tasks/destroy-model-task.es6 +++ b/packages/client-app/src/flux/tasks/destroy-model-task.es6 @@ -25,7 +25,4 @@ export default class DestroyModelTask extends Task { validate() { } - - canBeUndone() { return false } - } diff --git a/packages/client-app/src/flux/tasks/syncback-model-task.es6 b/packages/client-app/src/flux/tasks/syncback-model-task.es6 index f4646406c..cc196f42e 100644 --- a/packages/client-app/src/flux/tasks/syncback-model-task.es6 +++ b/packages/client-app/src/flux/tasks/syncback-model-task.es6 @@ -21,8 +21,4 @@ export default class SyncbackModelTask extends Task { this.validateRequiredFields(["clientId"]) return Promise.resolve() } - - canBeUndone() { return false } - - isUndo() { return false } } diff --git a/packages/client-app/src/flux/tasks/task.es6 b/packages/client-app/src/flux/tasks/task.es6 index 647db478a..1790b3a7d 100644 --- a/packages/client-app/src/flux/tasks/task.es6 +++ b/packages/client-app/src/flux/tasks/task.es6 @@ -6,15 +6,14 @@ import {generateTempId} from '../models/utils'; import {PermanentErrorCodes} from '../nylas-api'; import {APIError} from '../errors'; -const TaskStatus = { - Retry: "RETRY", - Success: "SUCCESS", - Continue: "CONTINUE", - Failed: "FAILED", +const Status = { + Local: "local", + Remote: "remote", + Complete: "complete", }; export default class Task extends Model { - static Status = TaskStatus; + static Status = Status; static SubclassesUseModelTable = Task; static attributes = Object.assign({}, Model.attributes, { @@ -44,8 +43,8 @@ export default class Task extends Model { // On construction, all Tasks instances are given a unique `id`. constructor(data) { super(data); + this.status = this.status || Status.Local; this.id = this.id || generateTempId(); - this.accountId = null; } // Public: Override to raise exceptions if your task is missing required @@ -54,23 +53,6 @@ export default class Task extends Model { } - // Public: It's up to you to determine how you want to indicate whether - // or not you have an instance of an "Undo Task". We commonly use a - // simple instance variable boolean flag. - // - // Returns `true` (is an Undo Task) or `false` (is not an Undo Task) - isUndo() { - return false; - } - - // Public: Determines whether or not this task can be undone via the - // {UndoRedoStore} - // - // Returns `true` (can be undone) or `false` (can't be undone) - canBeUndone() { - return false; - } - // Public: Return from `createIdenticalTask` and set a flag so your // `performLocal` and `performRemote` methods know that this is an undo // task. @@ -82,7 +64,9 @@ export default class Task extends Model { createIdenticalTask() { const json = this.toJSON(); delete json.status; - return (new this.constructor()).fromJSON(json); + delete json.version; + delete json.id; + return new this.constructor(json); } // Public: code to run if (someone tries to dequeue your task while it is) @@ -107,19 +91,6 @@ export default class Task extends Model { return 1; } - // Private: Allows for serialization of tasks - toJSON() { - return Object.assign(super.toJSON(), this); - } - - // Private: Allows for deserialization of tasks - fromJSON(json) { - for (const key of Object.keys(json)) { - this[key] = json[key]; - } - return this; - } - onError(err) { // noop }