From dd432e338b7bff7f30ff5e3d8141624d597c7576 Mon Sep 17 00:00:00 2001 From: Ben Gotow Date: Wed, 1 Jun 2016 14:36:57 -0700 Subject: [PATCH] fix(undo): Improve messaging around label changes, modernize undo-redo-store Summary: Just a small patch to address bad messaging Test Plan: Run one new test! Reviewers: jackie, juan Reviewed By: jackie, juan Differential Revision: https://phab.nylas.com/D3000 --- .../undo-redo/lib/undo-redo-component.cjsx | 2 +- spec/tasks/change-folder-task-spec.coffee | 10 ++--- spec/tasks/change-labels-task-spec.coffee | 32 +++++++++++--- src/flux/stores/undo-redo-store.es6 | 28 ++++++------ src/flux/tasks/change-folder-task.es6 | 19 +++----- src/flux/tasks/change-labels-task.es6 | 43 +++++++++++++++---- 6 files changed, 84 insertions(+), 50 deletions(-) diff --git a/internal_packages/undo-redo/lib/undo-redo-component.cjsx b/internal_packages/undo-redo/lib/undo-redo-component.cjsx index b7529000c..b110a4541 100644 --- a/internal_packages/undo-redo/lib/undo-redo-component.cjsx +++ b/internal_packages/undo-redo/lib/undo-redo-component.cjsx @@ -35,7 +35,7 @@ class UndoRedoComponent extends React.Component _getStateFromStores: -> tasks = UndoRedoStore.getMostRecent() - show = tasks? + show = tasks and tasks.length > 0 return {show, tasks} componentWillMount: -> diff --git a/spec/tasks/change-folder-task-spec.coffee b/spec/tasks/change-folder-task-spec.coffee index 5b0aca68c..9ec9e8527 100644 --- a/spec/tasks/change-folder-task-spec.coffee +++ b/spec/tasks/change-folder-task-spec.coffee @@ -50,21 +50,21 @@ describe "ChangeFolderTask", -> taskWithFolderId = new ChangeFolderTask folder: 'f2' messages: ['m1'] - expect(taskWithFolderId.description()).toEqual("Moved 1 message") + expect(taskWithFolderId.description()).toEqual("Moved to folder") taskWithFolder = new ChangeFolderTask folder: @testFolders['f2'] messages: ['m1'] - expect(taskWithFolder.description()).toEqual("Moved 1 message to MyDrafts") + expect(taskWithFolder.description()).toEqual("Moved to MyDrafts") it "should correctly mention threads and messages", -> taskWithFolderId = new ChangeFolderTask folder: 'f2' threads: ['t1'] - expect(taskWithFolderId.description()).toEqual("Moved 1 thread") + expect(taskWithFolderId.description()).toEqual("Moved to folder") taskWithFolder = new ChangeFolderTask folder: @testFolders['f2'] - messages: ['m1'] - expect(taskWithFolder.description()).toEqual("Moved 1 message to MyDrafts") + messages: ['m1', 'm2'] + expect(taskWithFolder.description()).toEqual("Moved 2 messages to MyDrafts") describe "performLocal", -> it "should check that a single folder is provided, and that we have threads or messages", -> diff --git a/spec/tasks/change-labels-task-spec.coffee b/spec/tasks/change-labels-task-spec.coffee index c30bf2e6e..f5624c13f 100644 --- a/spec/tasks/change-labels-task-spec.coffee +++ b/spec/tasks/change-labels-task-spec.coffee @@ -52,23 +52,41 @@ describe "ChangeLabelsTask", -> describe "description", -> it "should include the name of the added label if it's the only mutation and it was provided as an object", -> task = new ChangeLabelsTask(labelsToAdd: ["l1"], labelsToRemove: [], threads: ['t1']) - expect(task.description()).toEqual("Changed labels on 1 thread") + expect(task.description()).toEqual("Changed labels") task = new ChangeLabelsTask(labelsToAdd: [new Label(id: 'l1', displayName: 'LABEL')], labelsToRemove: [], threads: ['t1']) - expect(task.description()).toEqual("Added LABEL to 1 thread") - task = new ChangeLabelsTask(labelsToAdd: [new Label(id: 'l1', displayName: 'LABEL')], labelsToRemove: ['l2'], threads: ['t1']) - expect(task.description()).toEqual("Changed labels on 1 thread") + expect(task.description()).toEqual("Added LABEL") + task = new ChangeLabelsTask(labelsToAdd: [new Label(id: 'l1', displayName: 'LABEL')], labelsToRemove: ['l2'], threads: ['t1', 't2']) + expect(task.description()).toEqual("Moved 2 threads to LABEL") it "should include the name of the removed label if it's the only mutation and it was provided as an object", -> task = new ChangeLabelsTask(labelsToAdd: [], labelsToRemove: ["l1"], threads: ['t1']) - expect(task.description()).toEqual("Changed labels on 1 thread") + expect(task.description()).toEqual("Changed labels") task = new ChangeLabelsTask(labelsToAdd: [], labelsToRemove: [new Label(id: 'l1', displayName: 'LABEL')], threads: ['t1']) - expect(task.description()).toEqual("Removed LABEL from 1 thread") + expect(task.description()).toEqual("Removed LABEL") task = new ChangeLabelsTask(labelsToAdd: ['l2'], labelsToRemove: [new Label(id: 'l1', displayName: 'LABEL')], threads: ['t1']) - expect(task.description()).toEqual("Changed labels on 1 thread") + expect(task.description()).toEqual("Changed labels") it "should pluralize properly", -> task = new ChangeLabelsTask(labelsToAdd: ["l2"], labelsToRemove: ["l1"], threads: ['t1', 't2', 't3']) expect(task.description()).toEqual("Changed labels on 3 threads") + task = new ChangeLabelsTask(labelsToAdd: [new Label(id: 'l1', displayName: 'LABEL')], labelsToRemove: [], threads: ['t1', 't2']) + expect(task.description()).toEqual("Added LABEL to 2 threads") + + it "should include special cases for some common cases", -> + task = new ChangeLabelsTask(labelsToAdd: [new Label(name: "all")], labelsToRemove: [new Label(name: 'inbox')], threads: ['t1', 't2', 't3']) + expect(task.description()).toEqual("Archived 3 threads") + task = new ChangeLabelsTask(labelsToAdd: [new Label(name: "trash")], labelsToRemove: [new Label(name: 'inbox')], threads: ['t1', 't2', 't3']) + expect(task.description()).toEqual("Trashed 3 threads") + task = new ChangeLabelsTask(labelsToAdd: [new Label(name: "spam")], labelsToRemove: [new Label(name: 'inbox')], threads: ['t1', 't2', 't3']) + expect(task.description()).toEqual("Marked 3 threads as Spam") + task = new ChangeLabelsTask(labelsToAdd: [new Label(name: "inbox")], labelsToRemove: [new Label(name: 'spam')], threads: ['t1', 't2', 't3']) + expect(task.description()).toEqual("Unmarked 3 threads as Spam") + task = new ChangeLabelsTask(labelsToAdd: [new Label(name: "inbox")], labelsToRemove: [new Label(name: 'all')], threads: ['t1', 't2', 't3']) + expect(task.description()).toEqual("Unarchived 3 threads") + task = new ChangeLabelsTask(labelsToAdd: [new Label(name: "inbox")], labelsToRemove: [new Label(name: 'trash')], threads: ['t1', 't2', 't3']) + expect(task.description()).toEqual("Removed 3 threads from Trash") + task = new ChangeLabelsTask(labelsToAdd: [new Label(name: "inbox")], labelsToRemove: [new Label(name: 'trash')], threads: ['t1']) + expect(task.description()).toEqual("Removed from Trash") describe "_ensureAndUpdateLabels", -> beforeEach -> diff --git a/src/flux/stores/undo-redo-store.es6 b/src/flux/stores/undo-redo-store.es6 index b77159c39..a4c80919c 100644 --- a/src/flux/stores/undo-redo-store.es6 +++ b/src/flux/stores/undo-redo-store.es6 @@ -6,13 +6,11 @@ class UndoRedoStore extends NylasStore { constructor() { super() - this._onQueue = this._onQueue.bind(this); - this.undo = this.undo.bind(this); - this.redo = this.redo.bind(this); - this.getMostRecent = this.getMostRecent.bind(this); this._undo = []; this._redo = []; + this._mostRecentTasks = []; + this.listenTo(Actions.queueTask, this._onQueue); this.listenTo(Actions.queueTasks, this._onQueue); @@ -20,7 +18,7 @@ class UndoRedoStore extends NylasStore { NylasEnv.commands.add(document.body, {'core:redo': this.redo }); } - _onQueue(taskArg) { + _onQueue = (taskArg) => { if (!taskArg) { return; } let tasks = taskArg; if (!(tasks instanceof Array)) { tasks = [tasks]; } @@ -31,17 +29,19 @@ class UndoRedoStore extends NylasStore { if (undoable) { if (!isRedoTask) { this._redo = []; } this._undo.push(tasks); + this._mostRecentTasks = tasks; this.trigger(); } } - undo() { + undo = () => { const topTasks = this._undo.pop(); if (!topTasks) { return; } + + this._mostRecentTasks = []; this.trigger(); - for (let i = 0; i < topTasks.length; i++) { - const task = topTasks[i]; + for (const task of topTasks) { Actions.undoTaskId(task.id); } @@ -53,25 +53,21 @@ class UndoRedoStore extends NylasStore { this._redo.push(redoTasks); } - redo() { + redo = () => { const redoTasks = this._redo.pop(); if (!redoTasks) { return; } Actions.queueTasks(redoTasks); } - getMostRecent() { - for (let i = this._undo.length - 1; i >= 0; i--) { - const allReverting = _.every(this._undo[i], t => t._isReverting); - if (!allReverting) { return this._undo[i]; } - } - return [] + getMostRecent = () => { + return this._mostRecentTasks; } print() { console.log("Undo Stack"); console.log(this._undo); console.log("Redo Stack"); - return console.log(this._redo); + console.log(this._redo); } } diff --git a/src/flux/tasks/change-folder-task.es6 b/src/flux/tasks/change-folder-task.es6 index 0d3fb83a4..aa982547c 100644 --- a/src/flux/tasks/change-folder-task.es6 +++ b/src/flux/tasks/change-folder-task.es6 @@ -41,24 +41,17 @@ export default class ChangeFolderTask extends ChangeMailTask { return this.taskDescription; } - let folderText = ""; + let folderText = " to folder"; if (this.folder instanceof Category) { folderText = ` to ${this.folder.displayName}`; } - if (this.threads.length > 0) { - if (this.threads.length > 1) { - return `Moved ${this.threads.length} threads${folderText}`; - } - return `Moved 1 thread${folderText}`; + if (this.threads.length > 1) { + return `Moved ${this.threads.length} threads${folderText}`; + } else if (this.messages.length > 1) { + return `Moved ${this.messages.length} messages${folderText}`; } - if (this.messages.length > 0) { - if (this.messages.length > 1) { - return `Moved ${this.messages.length} messages${folderText}`; - } - return `Moved 1 message${folderText}`; - } - return `Moved objects${folderText}`; + return `Moved${folderText}`; } isDependentOnTask(other) { diff --git a/src/flux/tasks/change-labels-task.es6 b/src/flux/tasks/change-labels-task.es6 index 2eef153cb..f21838b5a 100644 --- a/src/flux/tasks/change-labels-task.es6 +++ b/src/flux/tasks/change-labels-task.es6 @@ -40,17 +40,44 @@ export default class ChangeLabelsTask extends ChangeMailTask { if (this.taskDescription) { return this.taskDescription; } - let type = "thread"; + + let countString = ""; if (this.threads.length > 1) { - type = "threads"; + countString = ` ${this.threads.length} threads`; } - if (this.labelsToAdd.length === 1 && this.labelsToRemove.length === 0 && this.labelsToAdd[0] instanceof Category) { - return `Added ${this.labelsToAdd[0].displayName} to ${this.threads.length} ${type}`; + + const removed = this.labelsToRemove[0]; + const added = this.labelsToAdd[0]; + const objectsAvailable = (added || removed) instanceof Category; + + // Note: In the future, we could move this logic to the task + // factory and pass the string in as this.taskDescription (ala Snooze), but + // it's nice to have them declaratively based on the actual labels. + if (objectsAvailable) { + if (this.labelsToAdd.length === 1 && this.labelsToRemove.length > 0) { + if (added.name === 'all') { + return `Archived${countString}`; + } else if (removed.name === 'all') { + return `Unarchived${countString}`; + } else if (added.name === 'spam') { + return `Marked${countString} as Spam`; + } else if (removed.name === 'spam') { + return `Unmarked${countString} as Spam`; + } else if (added.name === 'trash') { + return `Trashed${countString}`; + } else if (removed.name === 'trash') { + return `Removed${countString} from Trash`; + } + return `Moved${countString} to ${added.displayName}`; + } + if (this.labelsToAdd.length === 1 && this.labelsToRemove.length === 0) { + return `Added ${added.displayName}${countString ? ' to' : ''}${countString}`; + } + if (this.labelsToAdd.length === 0 && this.labelsToRemove.length === 1) { + return `Removed ${removed.displayName}${countString ? ' from' : ''}${countString}`; + } } - if (this.labelsToAdd.length === 0 && this.labelsToRemove.length === 1 && this.labelsToRemove[0] instanceof Category) { - return `Removed ${this.labelsToRemove[0].displayName} from ${this.threads.length} ${type}`; - } - return `Changed labels on ${this.threads.length} ${type}`; + return `Changed labels${countString ? ' on' : ''}${countString}`; } isDependentOnTask(other) {