diff --git a/internal_packages/inbox-activity-bar/lib/activity-bar-curl-item.cjsx b/internal_packages/inbox-activity-bar/lib/activity-bar-curl-item.cjsx index 696bb0359..c91f8fa64 100644 --- a/internal_packages/inbox-activity-bar/lib/activity-bar-curl-item.cjsx +++ b/internal_packages/inbox-activity-bar/lib/activity-bar-curl-item.cjsx @@ -11,6 +11,9 @@ class ActivityBarCurlItem extends React.Component {@props.item.command} + shouldComponentUpdate: (nextProps) => + return @props.item isnt nextProps.item + _onCopyCommand: => clipboard = require('clipboard') clipboard.writeText(@props.item.command) diff --git a/internal_packages/inbox-activity-bar/lib/activity-bar-long-poll-item.cjsx b/internal_packages/inbox-activity-bar/lib/activity-bar-long-poll-item.cjsx index a7bd33fc3..c2c26bdfd 100644 --- a/internal_packages/inbox-activity-bar/lib/activity-bar-long-poll-item.cjsx +++ b/internal_packages/inbox-activity-bar/lib/activity-bar-long-poll-item.cjsx @@ -1,6 +1,6 @@ React = require 'react/addons' moment = require 'moment' - +Utils = require 'inbox-exports' class ActivityBarLongPollItem extends React.Component @displayName: 'ActivityBarLongPollItem' @@ -8,6 +8,9 @@ class ActivityBarLongPollItem extends React.Component constructor: (@props) -> @state = expanded: false + shouldComponentUpdate: (nextProps, nextState) => + return not Utils.isEqualReact(nextProps, @props) or not Utils.isEqualReact(nextState, @state) + render: => if @state.expanded payload = JSON.stringify(@props.item) diff --git a/internal_packages/inbox-activity-bar/lib/activity-bar-store.coffee b/internal_packages/inbox-activity-bar/lib/activity-bar-store.coffee index d907bcbe8..806fd0958 100644 --- a/internal_packages/inbox-activity-bar/lib/activity-bar-store.coffee +++ b/internal_packages/inbox-activity-bar/lib/activity-bar-store.coffee @@ -1,6 +1,9 @@ Reflux = require 'reflux' {Actions} = require 'inbox-exports' qs = require 'querystring' +_ = require 'underscore-plus' + +curlItemId = 0 ActivityBarStore = Reflux.createStore init: -> @@ -20,6 +23,10 @@ ActivityBarStore = Reflux.createStore ########### PRIVATE #################################################### + triggerThrottled: -> + @_triggerThrottled ?= _.throttle(@trigger, 100) + @_triggerThrottled() + _setStoreDefaults: -> @_curlHistory = [] @_longPollHistory = [] @@ -53,11 +60,11 @@ ActivityBarStore = Reflux.createStore @_longPollHistory.unshift(deltas.reverse()...) if @_longPollHistory.length > 1000 @_longPollHistory.splice(1000, @_longPollHistory.length - 1000) - @trigger(@) + @triggerThrottled(@) _onLongPollStateChange: (state) -> @_longPollState = state - @trigger(@) + @triggerThrottled(@) _onAPIRequest: ({request, response}) -> url = request.url @@ -71,9 +78,13 @@ ActivityBarStore = Reflux.createStore data = "-d '#{postBody}'" unless request.method == 'GET' item = + id: curlItemId command: "curl -X #{request.method} #{data} #{url}" statusCode: response?.statusCode || 0 + @_curlHistory.unshift(item) - @trigger(@) + curlItemId += 1 + + @triggerThrottled(@) module.exports = ActivityBarStore diff --git a/internal_packages/inbox-activity-bar/lib/activity-bar-task.cjsx b/internal_packages/inbox-activity-bar/lib/activity-bar-task.cjsx index 949cf3f89..ebcda4110 100644 --- a/internal_packages/inbox-activity-bar/lib/activity-bar-task.cjsx +++ b/internal_packages/inbox-activity-bar/lib/activity-bar-task.cjsx @@ -1,5 +1,7 @@ React = require 'react/addons' classNames = require 'classnames' +_ = require 'underscore-plus' +{Utils} = require 'inbox-exports' class ActivityBarTask extends React.Component @displayName: 'ActivityBarTask' @@ -17,6 +19,9 @@ class ActivityBarTask extends React.Component + shouldComponentUpdate: (nextProps, nextState) => + return not Utils.isEqualReact(nextProps, @props) or not Utils.isEqualReact(nextState, @state) + _taskSummary: => qs = @props.task.queueState errType = "" diff --git a/internal_packages/inbox-activity-bar/lib/activity-bar.cjsx b/internal_packages/inbox-activity-bar/lib/activity-bar.cjsx index ea6666801..c1d4d0dab 100644 --- a/internal_packages/inbox-activity-bar/lib/activity-bar.cjsx +++ b/internal_packages/inbox-activity-bar/lib/activity-bar.cjsx @@ -90,7 +90,7 @@ class ActivityBar extends React.Component if @state.section == 'curl' itemDivs = @state.curlHistory.filter(matchingFilter).map (item) -> - + expandedDiv =
{itemDivs}
else if @state.section == 'long-polling' @@ -102,15 +102,15 @@ class ActivityBar extends React.Component queue = @state.queue.filter(matchingFilter) queueDivs = for i in [@state.queue.length - 1..0] by -1 task = @state.queue[i] - queueCompleted = @state.completed.filter(matchingFilter) queueCompletedDivs = for i in [@state.completed.length - 1..0] by -1 task = @state.completed[i] - expandedDiv = diff --git a/internal_packages/thread-list/lib/thread-buttons.cjsx b/internal_packages/thread-list/lib/thread-buttons.cjsx index b9eb2f5a0..af4243050 100644 --- a/internal_packages/thread-list/lib/thread-buttons.cjsx +++ b/internal_packages/thread-list/lib/thread-buttons.cjsx @@ -2,7 +2,7 @@ React = require "react/addons" classNames = require 'classnames' ThreadListStore = require './thread-list-store' {RetinaImg} = require 'ui-components' -{Actions, AddRemoveTagsTask, FocusedContentStore} = require "inbox-exports" +{Actions, FocusedContentStore} = require "inbox-exports" class ThreadBulkArchiveButton extends React.Component @displayName: 'ThreadBulkArchiveButton' diff --git a/internal_packages/thread-list/lib/thread-list-store.coffee b/internal_packages/thread-list/lib/thread-list-store.coffee index 18d239e63..e3e5167ce 100644 --- a/internal_packages/thread-list/lib/thread-list-store.coffee +++ b/internal_packages/thread-list/lib/thread-list-store.coffee @@ -98,6 +98,7 @@ ThreadListStore = Reflux.createStore keyboardId = FocusedContentStore.keyboardCursorId('thread') for thread in selected + continue if thread.hasTagId('archive') task = new AddRemoveTagsTask(thread, ['archive'], ['inbox']) Actions.queueTask(task) if thread.id is focusedId @@ -118,7 +119,8 @@ ThreadListStore = Reflux.createStore focused = FocusedContentStore.focused('thread') explicitOffset = if offset is "auto" then false else true - return unless focused + return unless focused and not focused.hasTagId('archive') + # Determine the current index index = @_view.indexOfId(focused.id) diff --git a/spec-inbox/stores/database-store-spec.coffee b/spec-inbox/stores/database-store-spec.coffee index 9da4286cd..e45a44a45 100644 --- a/spec-inbox/stores/database-store-spec.coffee +++ b/spec-inbox/stores/database-store-spec.coffee @@ -58,7 +58,7 @@ testModelInstanceB = new TestModel(id: 'BBB') describe "DatabaseStore", -> beforeEach -> spyOn(ModelQuery.prototype, 'where').andCallThrough() - spyOn(DatabaseStore, 'trigger') + spyOn(DatabaseStore, 'triggerSoon') @performed = [] @transactionCount = 0 @@ -113,9 +113,9 @@ describe "DatabaseStore", -> describe "persistModel", -> it "should cause the DatabaseStore to trigger with a change that contains the model", -> DatabaseStore.persistModel(testModelInstance) - expect(DatabaseStore.trigger).toHaveBeenCalled() + expect(DatabaseStore.triggerSoon).toHaveBeenCalled() - change = DatabaseStore.trigger.mostRecentCall.args[0] + change = DatabaseStore.triggerSoon.mostRecentCall.args[0] expect(change).toEqual({objectClass: TestModel.name, objects: [testModelInstance]}) it "should call through to writeModels", -> @@ -126,9 +126,9 @@ describe "DatabaseStore", -> describe "persistModels", -> it "should cause the DatabaseStore to trigger with a change that contains the models", -> DatabaseStore.persistModels([testModelInstanceA, testModelInstanceB]) - expect(DatabaseStore.trigger).toHaveBeenCalled() + expect(DatabaseStore.triggerSoon).toHaveBeenCalled() - change = DatabaseStore.trigger.mostRecentCall.args[0] + change = DatabaseStore.triggerSoon.mostRecentCall.args[0] expect(change).toEqual objectClass: TestModel.name, objects: [testModelInstanceA, testModelInstanceB] @@ -155,9 +155,9 @@ describe "DatabaseStore", -> it "should cause the DatabaseStore to trigger() with a change that contains the model", -> DatabaseStore.unpersistModel(testModelInstance) - expect(DatabaseStore.trigger).toHaveBeenCalled() + expect(DatabaseStore.triggerSoon).toHaveBeenCalled() - change = DatabaseStore.trigger.mostRecentCall.args[0] + change = DatabaseStore.triggerSoon.mostRecentCall.args[0] expect(change).toEqual({objectClass: TestModel.name, objects: [testModelInstance]}) describe "when the model has collection attributes", -> @@ -190,8 +190,8 @@ describe "DatabaseStore", -> queries = DatabaseStore.queriesForTableSetup(TestModel) expected = [ 'CREATE TABLE IF NOT EXISTS `TestModel` (id TEXT PRIMARY KEY,data BLOB,attr_queryable INTEGER)', - 'CREATE INDEX IF NOT EXISTS `TestModel-attr_queryable` ON `TestModel` (`attr_queryable`)', - 'CREATE INDEX IF NOT EXISTS `TestModel-id` ON `TestModel` (`id`)' + 'CREATE INDEX IF NOT EXISTS `TestModel_attr_queryable` ON `TestModel` (`attr_queryable`)', + 'CREATE UNIQUE INDEX IF NOT EXISTS `TestModel_id` ON `TestModel` (`id`)' ] for query,i in queries expect(query).toBe(expected[i]) @@ -201,9 +201,9 @@ describe "DatabaseStore", -> queries = DatabaseStore.queriesForTableSetup(TestModel) expected = [ 'CREATE TABLE IF NOT EXISTS `TestModel` (id TEXT PRIMARY KEY,data BLOB)', - 'CREATE INDEX IF NOT EXISTS `TestModel-id` ON `TestModel` (`id`)', + 'CREATE UNIQUE INDEX IF NOT EXISTS `TestModel_id` ON `TestModel` (`id`)', 'CREATE TABLE IF NOT EXISTS `TestModel-Tag` (id TEXT KEY, `value` TEXT)' - 'CREATE INDEX IF NOT EXISTS `TestModel-Tag-id-val` ON `TestModel-Tag` (`id`,`value`)', + 'CREATE UNIQUE INDEX IF NOT EXISTS `TestModel_Tag_id_val` ON `TestModel-Tag` (`id`,`value`)', ] for query,i in queries expect(query).toBe(expected[i]) @@ -258,7 +258,7 @@ describe "DatabaseStore", -> expect(@performed[1].query).toBe('DELETE FROM `TestModel-Tag` WHERE `id` IN (\'local-6806434c-b0cd\')') it "should insert new association records into join tables in a single query", -> - expect(@performed[2].query).toBe('INSERT INTO `TestModel-Tag` (`id`, `value`) VALUES (?,?),(?,?)') + expect(@performed[2].query).toBe('INSERT OR IGNORE INTO `TestModel-Tag` (`id`, `value`) VALUES (?,?),(?,?)') expect(@performed[2].values).toEqual(['local-6806434c-b0cd', 'a','local-6806434c-b0cd', 'b']) describe "when the model has joined data attributes", -> diff --git a/spec-inbox/tasks/add-remove-tags-spec.coffee b/spec-inbox/tasks/add-remove-tags-spec.coffee index b9273b5de..5fabb1437 100644 --- a/spec-inbox/tasks/add-remove-tags-spec.coffee +++ b/spec-inbox/tasks/add-remove-tags-spec.coffee @@ -92,6 +92,21 @@ describe "AddRemoveTagsTask", -> expect(testThread.tagIds().length).toBe(1) expect(testThread.tagIds()[0]).toBe('archive') + it "should never result in a tag ID being added twice", -> + testThread = new Thread + id: 'thread-id' + tags: [ + new Tag({name: 'archive', id: 'archive'}) + ] + task = new AddRemoveTagsTask(testThread, ['archive'], ['inbox']) + task.performLocal() + waitsFor -> + DatabaseStore.persistModel.callCount > 0 + runs -> + testThread = DatabaseStore.persistModel.mostRecentCall.args[0] + expect(testThread.tagIds().length).toBe(1) + expect(testThread.tagIds()[0]).toBe('archive') + describe "performRemote", -> beforeEach -> diff --git a/src/components/multiselect-action-bar.cjsx b/src/components/multiselect-action-bar.cjsx index 70a7e55c5..cce82901d 100644 --- a/src/components/multiselect-action-bar.cjsx +++ b/src/components/multiselect-action-bar.cjsx @@ -3,7 +3,6 @@ classNames = require 'classnames' _ = require 'underscore-plus' {Actions, - AddRemoveTagsTask, WorkspaceStore} = require "inbox-exports" InjectedComponentSet = require './injected-component-set' RetinaImg = require './retina-img' diff --git a/src/components/multiselect-list.cjsx b/src/components/multiselect-list.cjsx index 7533797b5..4a235542e 100644 --- a/src/components/multiselect-list.cjsx +++ b/src/components/multiselect-list.cjsx @@ -187,13 +187,15 @@ class MultiselectList extends React.Component else true - # Message list rendering is more important than thread list rendering. - # Since they're on the same event listner, and the event listeners are - # unordered, we need a way to push thread list updates later back in the - # queue. - _onChange: => _.delay => - @setState(@_getStateFromStores()) - , 1 + # This onChange handler can be called many times back to back and setState + # sometimes triggers an immediate render. Ensure that we never render back-to-back, + # because rendering this view (even just to determine that there are no changes) + # is expensive. + _onChange: => + @_onChangeDebounced ?= _.debounce => + @setState(@_getStateFromStores()) + , 1 + @_onChangeDebounced() _getStateFromStores: (props) => props ?= @props diff --git a/src/flux/stores/database-store.coffee b/src/flux/stores/database-store.coffee index e350c3afc..afb559568 100644 --- a/src/flux/stores/database-store.coffee +++ b/src/flux/stores/database-store.coffee @@ -207,6 +207,35 @@ class DatabaseStore @trigger({}) callback() + # TriggerSoon is a guarded version of trigger that can accumulate changes. + # This means that even if you're a bad person and call `persistModel` 100 times + # from 100 task objects queued at the same time, it will only create one + # `trigger` event. This is important since the database triggering impacts + # the entire application. + triggerSoon: (change) => + flush = => + return unless @_changeAccumulated + clearTimeout(@_changeFireTimer) if @_changeFireTimer + @trigger(@_changeAccumulated) + @_changeAccumulated = null + @_changeFireTimer = null + + set = (change) => + clearTimeout(@_changeFireTimer) if @_changeFireTimer + @_changeAccumulated = change + @_changeFireTimer = setTimeout(flush, 50) + + concat = (change) => + @_changeAccumulated.objects.push(change.objects...) + + if not @_changeAccumulated + set(change) + else if @_changeAccumulated.objectClass is change.objectClass + concat(change) + else + flush() + set(change) + writeModels: (tx, models) => # IMPORTANT: This method assumes that all the models you # provide are of the same class, and have different ids! @@ -274,7 +303,7 @@ class DatabaseStore for slice in [0..Math.floor(joinedValues.length / 400)] by 1 [ms, me] = [slice*200, slice*200 + 199] [vs, ve] = [slice*400, slice*400 + 399] - tx.execute("INSERT INTO `#{joinTable}` (`id`, `value`) VALUES #{joinMarks[ms..me].join(',')}", joinedValues[vs..ve]) + tx.execute("INSERT OR IGNORE INTO `#{joinTable}` (`id`, `value`) VALUES #{joinMarks[ms..me].join(',')}", joinedValues[vs..ve]) # For each joined data property stored in another table... values = [] @@ -328,7 +357,7 @@ class DatabaseStore tx.execute('BEGIN TRANSACTION') @writeModels(tx, [model]) tx.execute('COMMIT') - @trigger({objectClass: model.constructor.name, objects: [model]}) + @triggerSoon({objectClass: model.constructor.name, objects: [model]}) # Public: Asynchronously writes `models` to the cache and triggers a single change # event. Note: Models must be of the same class to be persisted in a batch operation. @@ -349,7 +378,7 @@ class DatabaseStore @writeModels(tx, models) tx.execute('COMMIT') - @trigger({objectClass: models[0].constructor.name, objects: models}) + @triggerSoon({objectClass: models[0].constructor.name, objects: models}) # Public: Asynchronously removes `model` from the cache and triggers a change event. # @@ -360,7 +389,7 @@ class DatabaseStore tx.execute('BEGIN TRANSACTION') @deleteModel(tx, model) tx.execute('COMMIT') - @trigger({objectClass: model.constructor.name, objects: [model]}) + @triggerSoon({objectClass: model.constructor.name, objects: [model]}) swapModel: ({oldModel, newModel, localId}) => @inTransaction {}, (tx) => @@ -369,7 +398,7 @@ class DatabaseStore @writeModels(tx, [newModel]) @writeModels(tx, [new LocalLink(id: localId, objectId: newModel.id)]) if localId tx.execute('COMMIT') - @trigger({objectClass: newModel.constructor.name, objects: [oldModel, newModel]}) + @triggerSoon({objectClass: newModel.constructor.name, objects: [oldModel, newModel]}) Actions.didSwapModel({oldModel, newModel, localId}) ### @@ -530,11 +559,13 @@ class DatabaseStore columns = ['id TEXT PRIMARY KEY', 'data BLOB'] columnAttributes.forEach (attr) -> columns.push(attr.columnSQL()) - queries.push("CREATE INDEX IF NOT EXISTS `#{klass.name}-#{attr.jsonKey}` ON `#{klass.name}` (`#{attr.jsonKey}`)") + # TODO: These indexes are not effective because SQLite only uses one index-per-table + # and there will almost always be an additional `where namespaceId =` clause. + queries.push("CREATE INDEX IF NOT EXISTS `#{klass.name}_#{attr.jsonKey}` ON `#{klass.name}` (`#{attr.jsonKey}`)") columnsSQL = columns.join(',') queries.unshift("CREATE TABLE IF NOT EXISTS `#{klass.name}` (#{columnsSQL})") - queries.push("CREATE INDEX IF NOT EXISTS `#{klass.name}-id` ON `#{klass.name}` (`id`)") + queries.push("CREATE UNIQUE INDEX IF NOT EXISTS `#{klass.name}_id` ON `#{klass.name}` (`id`)") # Identify collection attributes that can be matched against. These require # JOIN tables. (Right now the only one of these is Thread.tags) @@ -542,8 +573,9 @@ class DatabaseStore attr.queryable && attr instanceof AttributeCollection collectionAttributes.forEach (attribute) -> joinTable = tableNameForJoin(klass, attribute.itemClass) + joinIndexName = "#{joinTable.replace('-', '_')}_id_val" queries.push("CREATE TABLE IF NOT EXISTS `#{joinTable}` (id TEXT KEY, `value` TEXT)") - queries.push("CREATE INDEX IF NOT EXISTS `#{joinTable}-id-val` ON `#{joinTable}` (`id`,`value`)") + queries.push("CREATE UNIQUE INDEX IF NOT EXISTS `#{joinIndexName}` ON `#{joinTable}` (`id`,`value`)") joinedDataAttributes = _.filter attributes, (attr) -> attr instanceof AttributeJoinedData diff --git a/src/flux/stores/task-queue.coffee b/src/flux/stores/task-queue.coffee index 7b535c92e..05388840f 100644 --- a/src/flux/stores/task-queue.coffee +++ b/src/flux/stores/task-queue.coffee @@ -170,7 +170,7 @@ class TaskQueue _update: => @trigger() - @_saveQueueToDisk() + @_saveQueueToDiskDebounced() @_processQueue() _dequeueObsoleteTasks: (task) => @@ -238,10 +238,18 @@ class TaskQueue if not atom.inSpecMode() console.log("Queue deserialization failed with error: #{e.toString()}") - _saveQueueToDisk: (callback) => + + # It's very important that we debounce saving here. When the user bulk-archives + # items, they can easily process 1000 tasks at the same moment. We can't try to + # save 1000 times! (Do not remove debounce without a plan!) + + _saveQueueToDisk: => queueFile = path.join(atom.getConfigDirPath(), 'task-queue.json') queueJSON = JSON.stringify((@_queue ? [])) - fs.writeFile(queueFile, queueJSON, callback) + fs.writeFile(queueFile, queueJSON) + _saveQueueToDiskDebounced: => + @__saveQueueToDiskDebounced ?= _.debounce(@_saveQueueToDisk, 150) + @__saveQueueToDiskDebounced() module.exports = new TaskQueue() diff --git a/src/flux/tasks/add-remove-tags.coffee b/src/flux/tasks/add-remove-tags.coffee index 5d152f803..9b6dcf769 100644 --- a/src/flux/tasks/add-remove-tags.coffee +++ b/src/flux/tasks/add-remove-tags.coffee @@ -9,7 +9,8 @@ async = require 'async' module.exports = class AddRemoveTagsTask extends Task - constructor: (@thread, @tagIdsToAdd = [], @tagIdsToRemove = []) -> super + constructor: (@thread, @tagIdsToAdd = [], @tagIdsToRemove = []) -> + super tagForId: (id) -> @@ -36,9 +37,11 @@ class AddRemoveTagsTask extends Task # increment the thread version number thread.version += versionIncrement - # remove tags in the remove list + # filter the tags array to exclude tags we're removing and tags we're adding. + # Removing before adding is a quick way to make sure they're only in the set + # once. (super important) thread.tags = _.filter thread.tags, (tag) => - @tagIdsToRemove.indexOf(tag.id) is -1 + @tagIdsToRemove.indexOf(tag.id) is -1 and @tagIdsToAdd.indexOf(tag.id) is -1 # add tags in the add list for id in @tagIdsToAdd diff --git a/src/flux/tasks/create-metadata-task.coffee b/src/flux/tasks/create-metadata-task.coffee index ebe2c3839..645fde0cc 100644 --- a/src/flux/tasks/create-metadata-task.coffee +++ b/src/flux/tasks/create-metadata-task.coffee @@ -9,6 +9,7 @@ NamespaceStore = require '../stores/namespace-store' module.exports = class CreateMetadataTask extends Task constructor: ({@type, @publicId, @key, @value}) -> + super @name = "CreateMetadataTask" shouldDequeueOtherTask: (other) -> diff --git a/src/flux/tasks/destroy-metadata-task.coffee b/src/flux/tasks/destroy-metadata-task.coffee index a63a515a3..bfd324d7f 100644 --- a/src/flux/tasks/destroy-metadata-task.coffee +++ b/src/flux/tasks/destroy-metadata-task.coffee @@ -9,6 +9,7 @@ NamespaceStore = require '../stores/namespace-store' module.exports = class DestroyMetadataTask extends Task constructor: ({@type, @publicId, @key}) -> + super @name = "DestroyMetadataTask" shouldDequeueOtherTask: (other) -> diff --git a/src/flux/tasks/mark-message-read.coffee b/src/flux/tasks/mark-message-read.coffee index 49870bc95..dd67a5c48 100644 --- a/src/flux/tasks/mark-message-read.coffee +++ b/src/flux/tasks/mark-message-read.coffee @@ -6,7 +6,8 @@ _ = require 'underscore-plus' module.exports = class MarkMessageReadTask extends Task - constructor: (@message) -> super + constructor: (@message) -> + super performLocal: -> new Promise (resolve, reject) => diff --git a/src/flux/tasks/send-draft.coffee b/src/flux/tasks/send-draft.coffee index bf9ad9da5..8b2df4086 100644 --- a/src/flux/tasks/send-draft.coffee +++ b/src/flux/tasks/send-draft.coffee @@ -10,7 +10,8 @@ SyncbackDraftTask = require './syncback-draft' module.exports = class SendDraftTask extends Task - constructor: (@draftLocalId, {@fromPopout}={}) -> super + constructor: (@draftLocalId, {@fromPopout}={}) -> + super shouldDequeueOtherTask: (other) -> other instanceof SendDraftTask and other.draftLocalId is @draftLocalId