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

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: ->

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", ->

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 ->

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);
   }
 }
 

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) {

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) {