Replace the component bounded `cachedValue` by a global `UnsavedEdits`

This new draft saving system is currently only implemented for the
card description and comment. We need better a component
inheritance/composition model to support this for all editable fields.

Fixes #186

.jshintrc

@@ -81,6 +81,7 @@
     "Popup": true,
     "Sidebar": true,
     "Utils": true,
+    "InlinedForm": true,
 
     // XXX Temp, we should remove these
     "allowIsBoardAdmin": true,

client/components/activities/comments.jade

@@ -4,5 +4,6 @@ template(name="commentForm")
     +userAvatar(userId=currentUser._id)
     form.js-new-comment-form
       +editor(class="js-new-comment-input")
+        | {{getUnsavedValue 'cardComment' currentCard._id}}
       .add-controls
         button.primary.confirm.clear.js-add-comment(type="submit") {{_ 'comment'}}

client/components/activities/comments.js

@@ -1,47 +1,83 @@
-var commentFormIsOpen = new ReactiveVar(false);
+let commentFormIsOpen = new ReactiveVar(false);
 
-Template.commentForm.helpers({
-  commentFormIsOpen: function() {
-    return commentFormIsOpen.get();
-  }
-});
+BlazeComponent.extendComponent({
+  template() {
+    return 'commentForm';
+  },
 
-Template.commentForm.events({
-  'click .js-new-comment:not(.focus)': function() {
-    commentFormIsOpen.set(true);
+  onDestroyed() {
+    commentFormIsOpen.set(false);
   },
-  'submit .js-new-comment-form': function(evt, tpl) {
-    var input = tpl.$('.js-new-comment-input');
-    if ($.trim(input.val())) {
-      CardComments.insert({
-        boardId: this.boardId,
-        cardId: this._id,
-        text: input.val()
-      });
-      input.val('');
-      input.blur();
-      commentFormIsOpen.set(false);
-      Tracker.flush();
-      autosize.update(input);
-    }
-    evt.preventDefault();
+
+  commentFormIsOpen() {
+    return commentFormIsOpen.get();
   },
-  // Pressing Ctrl+Enter should submit the form
-  'keydown form textarea': function(evt, tpl) {
-    if (evt.keyCode === 13 && (evt.metaKey || evt.ctrlKey)) {
-      tpl.find('button[type=submit]').click();
-    }
+
+  getInput() {
+    return this.$('.js-new-comment-input');
+  },
+
+  events() {
+    return [{
+      'click .js-new-comment:not(.focus)': function() {
+        commentFormIsOpen.set(true);
+      },
+      'submit .js-new-comment-form': function(evt) {
+        let input = this.getInput();
+        if ($.trim(input.val())) {
+          CardComments.insert({
+            boardId: this.boardId,
+            cardId: this._id,
+            text: input.val()
+          });
+          resetCommentInput(input);
+          Tracker.flush();
+          autosize.update(input);
+        }
+        evt.preventDefault();
+      },
+      // Pressing Ctrl+Enter should submit the form
+      'keydown form textarea': function(evt) {
+        if (evt.keyCode === 13 && (evt.metaKey || evt.ctrlKey)) {
+          this.find('button[type=submit]').click();
+        }
+      }
+    }];
   }
-});
+}).register('commentForm');
 
-Template.commentForm.onDestroyed(function() {
+// XXX This should be a static method of the `commentForm` component
+function resetCommentInput(input) {
+  input.val('');
+  input.blur();
   commentFormIsOpen.set(false);
-});
+}
+
+// XXX This should handled a `onUpdated` callback of the `commentForm` component
+// but since this callback doesn't exists, and `onRendered` is not called if the
+// data is not destroyed and recreated, we simulate the desired callback using
+// Tracker.autorun to register the component dependencies, and re-run when these
+// dependencies are invalidated. A better component API would remove this hack.
+Tracker.autorun(() => {
+  Session.get('currentCard');
+  Tracker.afterFlush(() => {
+    autosize.update($('.js-new-comment-input'));
+  });
+})
 
 EscapeActions.register('inlinedForm',
   function() {
-    commentFormIsOpen.set(false);
-    $('.js-new-comment-input').blur();
+    const draftKey = {
+      fieldName: 'cardComment',
+      docId: Session.get('currentCard')
+    };
+    let commentInput = $('.js-new-comment-input');
+    if ($.trim(commentInput.val())) {
+      UnsavedEdits.set(draftKey, commentInput.val());
+    } else {
+      UnsavedEdits.reset(draftKey);
+    }
+    resetCommentInput(commentInput);
   },
   function() { return commentFormIsOpen.get(); }, {
     noClickEscapeOn: '.js-new-comment'

client/components/cards/cardDetails.jade

@@ -34,11 +34,11 @@ template(name="cardDetails")
     //- XXX We should use "editable" to avoid repetiting ourselves
     if currentUser.isBoardMember
       h3.card-details-item-title Description
-      +inlinedForm(classNames="card-description js-card-description")
+      +inlinedCardDescription(classNames="card-description js-card-description")
         +editor(autofocus=true)
-          = description
+          | {{getUnsavedValue 'cardDescription' _id description}}
         .edit-controls.clearfix
-          button.primary(type="submit") {{_ 'edit'}}
+          button.primary(type="submit") {{_ 'save'}}
           a.fa.fa-times-thin.js-close-inlined-form
       else
         a.js-open-inlined-form
@@ -47,6 +47,12 @@ template(name="cardDetails")
               = description
           else
             | {{_ 'edit'}}
+        if (hasUnsavedValue 'cardDescription' _id)
+          p.quiet
+            | You have an unsaved description.
+            a.js-open-inlined-form View it
+            = ' - '
+            a.js-close-inlined-form Discard
     else if description
       h3.card-details-item-title Description
       +viewer

client/components/cards/cardDetails.js

@@ -1,7 +1,3 @@
-// XXX Obviously this shouldn't be a global, but this is currently the only way
-// to share a variable between two files.
-
-
 BlazeComponent.extendComponent({
   template: function() {
     return 'cardDetails';
@@ -80,6 +76,37 @@ BlazeComponent.extendComponent({
   }
 }).register('cardDetails');
 
+// We extends the normal InlinedForm component to support UnsavedEdits draft
+// feature.
+(class extends InlinedForm {
+  _getUnsavedEditKey() {
+    return {
+      fieldName: 'cardDescription',
+      docId: Session.get('currentCard'),
+    }
+  }
+
+  close(isReset = false) {
+    if (this.isOpen.get() && ! isReset) {
+      UnsavedEdits.set(this._getUnsavedEditKey(), this.getValue());
+    }
+    super();
+  }
+
+  reset() {
+    UnsavedEdits.reset(this._getUnsavedEditKey());
+    this.close(true);
+  }
+
+  events() {
+    const parentEvents = InlinedForm.prototype.events()[0];
+    return [{
+      ...parentEvents,
+      'click .js-close-inlined-form': this.reset,
+    }];
+  }
+}).register('inlinedCardDescription');
+
 Template.cardDetailsActionsPopup.events({
   'click .js-members': Popup.open('cardMembers'),
   'click .js-labels': Popup.open('cardLabels'),

client/components/forms/cachedValue.js

@@ -1,22 +0,0 @@
-var emptyValue = '';
-
-Mixins.CachedValue = BlazeComponent.extendComponent({
-  onCreated: function() {
-    this._cachedValue = emptyValue;
-  },
-
-  setCache: function(value) {
-    this._cachedValue = value;
-  },
-
-  getCache: function(defaultValue) {
-    if (this._cachedValue === emptyValue)
-      return defaultValue || '';
-    else
-      return this._cachedValue;
-  },
-
-  resetCache: function() {
-    this.setCache('');
-  }
-});

client/components/lists/listHeader.jade

@@ -11,7 +11,7 @@ template(name="listHeader")
 
 template(name="editListTitleForm")
   .list-composer
-    input.full-line(type="text" value=title autofocus)
+    input.full-line(type="text" value="{{../trySomething}}" autofocus)
     .edit-controls.clearfix
       button.primary.confirm(type="submit") {{_ 'save'}}
       a.fa.fa-times-thin.js-close-inlined-form

client/config/router.js

@@ -37,7 +37,7 @@ FlowRouter.route('/b/:id/:slug', {
 FlowRouter.route('/b/:boardId/:slug/:cardId', {
   name: 'card',
   action: function(params) {
-    EscapeActions.executeUpTo('popup-close');
+    EscapeActions.executeUpTo('inlinedForm');
     Session.set('currentBoard', params.boardId);
     Session.set('currentCard', params.cardId);
 

client/lib/escapeActions.js

@@ -17,10 +17,10 @@ EscapeActions = {
     'inlinedForm',
     'detailsPane',
     'multiselection',
-    'sidebarView'
+    'sidebarView',
   ],
 
-  register: function(label, action, condition = () => true, options = {}) {
+  register(label, action, condition = () => true, options = {}) {
     const priority = this.hierarchy.indexOf(label);
     if (priority === -1) {
       throw Error('You must define the label in the EscapeActions hierarchy');
@@ -33,35 +33,35 @@ EscapeActions = {
 
     let noClickEscapeOn = options.noClickEscapeOn;
 
-    this._actions[priority] = {
+    this._actions = _.sortBy([...this._actions, {
       priority,
       condition,
       action,
       noClickEscapeOn,
-      enabledOnClick
-    };
+      enabledOnClick,
+    }], (action) => action.priority);
   },
 
-  executeLowest: function() {
+  executeLowest() {
     return this._execute({
       multipleAction: false
     });
   },
 
-  executeAll: function() {
+  executeAll() {
     return this._execute({
       multipleActions: true
     });
   },
 
-  executeUpTo: function(maxLabel) {
+  executeUpTo(maxLabel) {
     return this._execute({
       maxLabel: maxLabel,
       multipleActions: true
     });
   },
 
-  clickExecute: function(target, maxLabel) {
+  clickExecute(target, maxLabel) {
     if (this._nextclickPrevented) {
       this._nextclickPrevented = false;
     } else {
@@ -74,18 +74,18 @@ EscapeActions = {
     }
   },
 
-  preventNextClick: function() {
+  preventNextClick() {
     this._nextclickPrevented = true;
   },
 
-  _stopClick: function(action, clickTarget) {
+  _stopClick(action, clickTarget) {
     if (! _.isString(action.noClickEscapeOn))
       return false;
     else
       return $(clickTarget).closest(action.noClickEscapeOn).length > 0;
   },
 
-  _execute: function(options) {
+  _execute(options) {
     const maxLabel = options.maxLabel;
     const multipleActions = options.multipleActions;
     const isClick = !! options.isClick;
@@ -99,8 +99,7 @@ EscapeActions = {
     else
       maxPriority = this.hierarchy.indexOf(maxLabel);
 
-    for (let i = 0; i < this._actions.length; i++) {
-      let currentAction = this._actions[i];
+    for (let currentAction of this._actions) {
       if (currentAction.priority > maxPriority)
         return executedAtLeastOne;
 

client/components/forms/inlinedform.js → client/lib/inlinedform.js

@@ -13,19 +13,13 @@
 //     // the content when the form is close (optional)
 
 // We can only have one inlined form element opened at a time
-// XXX Could we avoid using a global here ? This is used in Mousetrap
-// keyboard.js
-var currentlyOpenedForm = new ReactiveVar(null);
+currentlyOpenedForm = new ReactiveVar(null);
 
-BlazeComponent.extendComponent({
+InlinedForm = BlazeComponent.extendComponent({
   template: function() {
     return 'inlinedForm';
   },
 
-  mixins: function() {
-    return [Mixins.CachedValue];
-  },
-
   onCreated: function() {
     this.isOpen = new ReactiveVar(false);
   },
@@ -42,7 +36,6 @@ BlazeComponent.extendComponent({
   },
 
   close: function() {
-    this.saveValue();
     this.isOpen.set(false);
     currentlyOpenedForm.set(null);
   },
@@ -52,10 +45,6 @@ BlazeComponent.extendComponent({
     return this.isOpen.get() && input && input.value;
   },
 
-  saveValue: function() {
-    this.callFirstWith(this, 'setCache', this.getValue());
-  },
-
   events: function() {
     return [{
       'click .js-close-inlined-form': this.close,
@@ -73,7 +62,6 @@ BlazeComponent.extendComponent({
         if (this.currentData().autoclose !== false) {
           Tracker.afterFlush(() => {
             this.close();
-            this.callFirstWith(this, 'resetCache');
           });
         }
       }

client/lib/unsavedEdits.js

@@ -0,0 +1,82 @@
+Meteor.subscribe('unsaved-edits');
+
+// `UnsavedEdits` is a global key-value store used to save drafts of user
+// inputs. We used to have the notion of a `cachedValue` that was local to a
+// component but the global store has multiple advantages:
+// 1. When the component is unmounted (ie, destroyed) the draft isn't lost
+// 2. The drafts are synced across multiple computers
+// 3. The drafts are synced across multiple browser tabs
+//    XXX This currently doesn't work in purely offline mode since the sync is
+//    handled with the DDP connection to the server. To solve this, we could use
+//    something like GroundDB that syncs using localstorage.
+//
+// The key is a dictionary composed of two fields:
+// * a `fieldName` which identifies the particular field. Since this is a global
+//   identifier a good practice would be to compose it with the collection name
+//   and the document field, eg. `boardTitle`, `cardDescription`.
+// * a `docId` which identifies the appropriate document. In general we use
+//   MongoDB `_id` field.
+//
+// The value is a string containing the draft.
+
+UnsavedEdits = {
+  // XXX Wanted to have the collection has an instance variable, but
+  // unfortunately the collection isn't defined yet at this point. We need ES6
+  // modules to solve the file order issue!
+  //
+  // _collection: UnsavedEditCollection,
+
+  get({ fieldName, docId }, defaultTo = '') {
+    let unsavedValue = this._getCollectionDocument(fieldName, docId);
+    if (unsavedValue) {
+      return unsavedValue.value
+    } else {
+      return defaultTo;
+    }
+  },
+
+  has({ fieldName, docId }) {
+    return Boolean(this.get({fieldName, docId}));
+  },
+
+  set({ fieldName, docId }, value) {
+    let currentDoc = this._getCollectionDocument(fieldName, docId);
+    if (currentDoc) {
+      UnsavedEditCollection.update(currentDoc._id, {
+        $set: {
+          value: value
+        }
+      });
+    } else {
+      UnsavedEditCollection.insert({
+        fieldName,
+        docId,
+        value,
+      });
+    }
+  },
+
+  reset({ fieldName, docId }) {
+    let currentDoc = this._getCollectionDocument(fieldName, docId);
+    if (currentDoc) {
+      UnsavedEditCollection.remove(currentDoc._id);
+    }
+  },
+
+  _getCollectionDocument(fieldName, docId) {
+    return UnsavedEditCollection.findOne({fieldName, docId});
+  }
+}
+
+Blaze.registerHelper('getUnsavedValue', (fieldName, docId, defaultTo) => {
+  // Workaround some blaze feature that ass a list of keywords arguments as the
+  // last parameter (even if the caller didn't specify any).
+  if (! _.isString(defaultTo)) {
+    defaultTo = '';
+  }
+  return UnsavedEdits.get({ fieldName, docId }, defaultTo);
+});
+
+Blaze.registerHelper('hasUnsavedValue', (fieldName, docId) => {
+  return UnsavedEdits.has({ fieldName, docId });
+});

collections/unsavedEdits.js

@@ -0,0 +1,34 @@
+// This collection shouldn't be manipulated directly by instead throw the
+// `UnsavedEdits` API on the client.
+UnsavedEditCollection = new Mongo.Collection('unsaved-edits');
+
+UnsavedEditCollection.attachSchema(new SimpleSchema({
+  fieldName: {
+    type: String
+  },
+  docId: {
+    type: String
+  },
+  value: {
+    type: String
+  },
+  userId: {
+    type: String
+  },
+}));
+
+if (Meteor.isServer) {
+  function isAuthor(userId, doc, fieldNames = []) {
+    return userId === doc.userId && fieldNames.indexOf('userId') === -1;
+  }
+  UnsavedEditCollection.allow({
+    insert: isAuthor,
+    update: isAuthor,
+    remove: isAuthor,
+    fetch: ['userId']
+  });
+}
+
+UnsavedEditCollection.before.insert(function(userId, doc) {
+  doc.userId = userId;
+});

server/publications/unsavedEdits.js

@@ -0,0 +1,5 @@
+Meteor.publish('unsaved-edits', function() {
+  return UnsavedEditCollection.find({
+    userId: this.userId
+  });
+});