Foundry376/Mailspring
1
1
Fork 0
mirror of https://github.com/Foundry376/Mailspring.git synced 2025-03-09 06:23:30 +08:00
Code Issues Projects Releases Packages Wiki Activity

fix(thread-list): Archive performance improvements, white rows fix

Browse source 
Summary:
Debounce changes out of the DatabaseStore to prevent lots of calls to persistModel from flooding the app

Tasks must always call super so they get IDs

The task queue shouldn't save every time it adds/removes a task - there could be hundreds

ActivityBar package is actually surprisingly slow, re-rendering needlessly

setState in MultiselectList sometimes renders immediately. Don't do this, because sometimes we're rendering twice back to back

Remove dead references

Never allow duplicate tags in the tags array

Don't archive threads that already have the archive tag (it doesn't do anything bad, but why bother creating tasks?)

Update DB specs

Test Plan: Run tests

Reviewers: evan

Reviewed By: evan

Differential Revision: https://review.inboxapp.com/D1506
This commit is contained in:
Ben Gotow 2015-05-14 14:12:53 -07:00
parent 1cf43ab1df
commit 9a0c3a245e
18 changed files with 134 additions and 47 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
internal_packages/inbox-activity-bar/lib/activity-bar-curl-item.cjsx
View file

@ -11,6 +11,9 @@ class ActivityBarCurlItem extends React.Component
{@props.item.command}
</div>
shouldComponentUpdate: (nextProps) =>
return @props.item isnt nextProps.item
_onCopyCommand: =>
clipboard = require('clipboard')
clipboard.writeText(@props.item.command)

5
internal_packages/inbox-activity-bar/lib/activity-bar-long-poll-item.cjsx
View file

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

17
internal_packages/inbox-activity-bar/lib/activity-bar-store.coffee
View file

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

5
internal_packages/inbox-activity-bar/lib/activity-bar-task.cjsx
View file

@ -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
</div>
</div>
shouldComponentUpdate: (nextProps, nextState) =>
return not Utils.isEqualReact(nextProps, @props) or not Utils.isEqualReact(nextState, @state)
_taskSummary: =>
qs = @props.task.queueState
errType = ""

10
internal_packages/inbox-activity-bar/lib/activity-bar.cjsx
View file

@ -90,7 +90,7 @@ class ActivityBar extends React.Component
if @state.section == 'curl'
itemDivs = @state.curlHistory.filter(matchingFilter).map (item) ->
<ActivityBarCurlItem item={item}/>
<ActivityBarCurlItem item={item} key={item.id}/>
expandedDiv = <div className="expanded-section curl-history">{itemDivs}</div>
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]
<ActivityBarTask task=task
key=task.id
<ActivityBarTask task={task}
key={task.id}
type="queued" />
queueCompleted = @state.completed.filter(matchingFilter)
queueCompletedDivs = for i in [@state.completed.length - 1..0] by -1
task = @state.completed[i]
<ActivityBarTask task=task
key=task.id
<ActivityBarTask task={task}
key={task.id}
type="completed" />
expandedDiv =

2
internal_packages/thread-list/lib/thread-buttons.cjsx
View file

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

4
internal_packages/thread-list/lib/thread-list-store.coffee
View file

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

24
spec-inbox/stores/database-store-spec.coffee
View file

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

15
spec-inbox/tasks/add-remove-tags-spec.coffee
View file

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

1
src/components/multiselect-action-bar.cjsx
View file

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

16
src/components/multiselect-list.cjsx
View file

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

48
src/flux/stores/database-store.coffee
View file

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

14
src/flux/stores/task-queue.coffee
View file

@ -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()

9
src/flux/tasks/add-remove-tags.coffee
View file

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

1
src/flux/tasks/create-metadata-task.coffee
View file

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

1
src/flux/tasks/destroy-metadata-task.coffee
View file

@ -9,6 +9,7 @@ NamespaceStore = require '../stores/namespace-store'
module.exports =
class DestroyMetadataTask extends Task
constructor: ({@type, @publicId, @key}) ->
super
@name = "DestroyMetadataTask"
shouldDequeueOtherTask: (other) ->

3
src/flux/tasks/mark-message-read.coffee
View file

@ -6,7 +6,8 @@ _ = require 'underscore-plus'
module.exports =
class MarkMessageReadTask extends Task
constructor: (@message) -> super
constructor: (@message) ->
super
performLocal: ->
new Promise (resolve, reject) =>

3
src/flux/tasks/send-draft.coffee
View file

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