Fix for selection update issue, delete items + scroll up issue

spec/list-selection-spec.coffee

@@ -54,35 +54,44 @@ describe "ListSelection", ->
 
   describe "remove", ->
     beforeEach ->
-      @selection.set([@items[2], @items[4]])
+      @selection.set([@items[2], @items[4], @items[7]])
 
     it "should do nothing if called without a valid item", ->
       @selection.remove(null)
       @selection.remove(undefined)
       @selection.remove(false)
-      expect(@selection.ids()).toEqual(['2', '4'])
+      expect(@selection.ids()).toEqual(['2', '4', '7'])
 
     it "should remove the item from the set", ->
       @selection.remove(@items[2])
-      expect(@selection.ids()).toEqual(['4'])
+      expect(@selection.ids()).toEqual(['4', '7'])
 
-    it "should throw an exception if the item passed is not a model", ->
+    it "should throw an exception if any item passed is not a model", ->
       expect( => @selection.remove('hi')).toThrow()
 
+    it "should accept an array of models as well as a single item", ->
+      @selection.remove([@items[2], @items[4]])
+      expect(@selection.ids()).toEqual(['7'])
+
     it "should trigger", ->
       @selection.remove()
       expect(@trigger).toHaveBeenCalled()
 
-  describe "updateModelReferences", ->
+  describe "_applyChangeRecord", ->
     it "should replace items in the selection with the matching provided items, if present", ->
       @selection.set([@items[2], @items[4], @items[7]])
       expect(@selection.items()[0]).toBe(@items[2])
-
       expect(@selection.items()[0].subject).toBe(undefined)
       newItem2 = new Thread(id: '2', subject:'Hello world!')
-      @selection.updateModelReferences([newItem2])
+      @selection._applyChangeRecord({objectClass: 'Thread', objects: [newItem2], type: 'persist'})
       expect(@selection.items()[0].subject).toBe('Hello world!')
 
+    it "should rremove items in the selection if type is unpersist", ->
+      @selection.set([@items[2], @items[4], @items[7]])
+      newItem2 = new Thread(id: '2', subject:'Hello world!')
+      @selection._applyChangeRecord({objectClass: 'Thread', objects: [newItem2], type: 'unpersist'})
+      expect(@selection.ids()).toEqual(['4', '7'])
+
   describe "toggle", ->
     beforeEach ->
       @selection.set([@items[2]])

spec/models/query-subscription-spec.coffee

@@ -96,14 +96,14 @@ describe "QuerySubscription", ->
         mustUpdate: true
         mustRefetchAllIds: true
       },{
-        name: 'Item saved - does not match query clauses'
+        name: 'Item saved - does not match query clauses, offset > 0'
         change:
           objectClass: Thread.name
           objects: [new Thread(accountId: 'b', clientId: '5', lastMessageReceivedTimestamp: 5)]
           type: 'persist'
         nextModels: 'unchanged'
-        mustUpdate: false
-        mustRefetchAllIds: false
+        mustUpdate: true
+        mustRefetchAllIds: true
       },{
         name: 'Item saved - matches query clauses'
         change:

src/flux/attributes/matcher.coffee

@@ -46,28 +46,33 @@ class Matcher
     @val
 
   evaluate: (model) ->
-    value = model[@attr.modelKey]
-    value = value() if value instanceof Function
+    modelValue = model[@attr.modelKey]
+    modelValue = modelValue() if modelValue instanceof Function
+    matcherValue = @val
+
+    # Given an array of strings or models, and a string or model search value,
+    # will find if a match exists.
+    modelArrayContainsValue = (array, searchItem) ->
+      asId = (v) -> if v and v.id then v.id else v
+      search = asId(searchItem)
+      for item in array
+        return true if asId(item) == search
+      return false
 
     switch @comparator
-      when '=' then return value == @val
-      when '<' then return value < @val
-      when '>' then return value > @val
-      when 'in' then return value in @val
+      when '=' then return modelValue == matcherValue
+      when '<' then return modelValue < matcherValue
+      when '>' then return modelValue > matcherValue
+      when 'in' then return modelValue in matcherValue
       when 'contains'
-        # You can provide an ID or an object, and an array of IDs or an array of objects
-        # Assumes that `value` is an array of items
-        !!_.find value, (x) =>
-          @val == x?.id || @val == x || @val?.id == x || @val?.id == x?.id
-      when 'containsAny'
-        # You can provide an ID or an object, and an array of IDs or an array of objects
-        # Assumes that `value` is an array of items
-        _.some @val, (subvalue) =>
-          !!_.find value, (x) =>
-            subvalue == x?.id || subvalue == x || subvalue?.id == x || subvalue?.id == x?.id
+        !!modelArrayContainsValue(modelValue, matcherValue)
 
-      when 'startsWith' then return value.startsWith(@val)
-      when 'like' then value.search(new RegExp(".*#{@val}.*", "gi")) >= 0
+      when 'containsAny'
+        _.any matcherValue, (submatcherValue) ->
+          !!modelArrayContainsValue(modelValue, submatcherValue)
+
+      when 'startsWith' then return modelValue.startsWith(matcherValue)
+      when 'like' then modelValue.search(new RegExp(".*#{matcherValue}.*", "gi")) >= 0
       else
         throw new Error("Matcher.evaulate() not sure how to evaluate @{@attr.modelKey} with comparator #{@comparator}")
 

src/flux/models/mutable-query-result-set.coffee

@@ -9,6 +9,7 @@ class MutableQueryResultSet extends QueryResultSet
     set = new QueryResultSet({
       _ids: [].concat(@_ids)
       _modelsHash: _.extend({}, @_modelsHash)
+      _query: @_query
       _offset: @_offset
     })
     Object.freeze(set)
@@ -70,4 +71,8 @@ class MutableQueryResultSet extends QueryResultSet
     delete @_modelsHash[item.id]
     @_ids.splice(idx, 1)
 
+  setQuery: (query) ->
+    @_query = query.clone()
+    @_query.finalize()
+
 module.exports = MutableQueryResultSet

src/flux/models/query-result-set.coffee

@@ -36,12 +36,14 @@ class QueryResultSet
   constructor: (other = {}) ->
     @_modelsHash = other._modelsHash ? {}
     @_offset = other._offset ? null
+    @_query = other._query ? null
     @_ids = other._ids ? []
 
   clone: ->
     new @constructor({
       _ids: [].concat(@_ids)
       _modelsHash: _.extend({}, @_modelsHash)
+      _query: @_query
       _offset: @_offset
     })
 
@@ -51,6 +53,9 @@ class QueryResultSet
   range: ->
     new QueryRange(offset: @_offset, limit: @_ids.length)
 
+  query: ->
+    @_query
+
   count: ->
     @_ids.length
 

src/flux/models/query-subscription.coffee

@@ -89,6 +89,13 @@ class QuerySubscription
           impactCount += 1
           mustRefetchAllIds = true if @_itemSortOrderHasChanged(oldItem, item)
 
+      # If we're not at the top of the result set, we can't be sure whether an
+      # item previously matched the set and doesn't anymore, impacting the items
+      # in the query range. We need to refetch IDs to be sure our set is correct.
+      if @_query.range().offset > 0 and impactCount < record.objects.length
+        impactCount += 1
+        mustRefetchAllIds = true
+
     if impactCount > 0
       if mustRefetchAllIds
         @log("Clearing result set - mustRefetchAllIds")
@@ -179,6 +186,7 @@ class QuerySubscription
       throw new Error("QuerySubscription: result set contains duplicate ids.")
 
     if @_options.asResultSet
+      @_set.setQuery(@_query)
       @_lastResult = @_set.immutableClone()
     else
       @_lastResult = @_query.formatResult(@_set.models())

src/flux/models/query.coffee

@@ -294,11 +294,15 @@ class ModelQuery
   # Private: Marks the object as final, preventing any changes to the where
   # clauses, orders, etc.
   finalize: ->
+    return if @_finalized
+
     if @_orders.length is 0
       natural = @_klass.naturalSortOrder()
       @_orders.push(natural) if natural
+
     if @_returnOne and not @_range.limit
       @limit(1)
+
     @_finalized = true
     @
 

src/flux/modules/reflux-coffee.coffee

@@ -151,7 +151,7 @@ module.exports =
     setupEmitter: ->
       return if @_emitter
       @_emitter ?= new EventEmitter()
-      @_emitter.setMaxListeners(50)
+      @_emitter.setMaxListeners(20)
 
     listen: (callback, bindContext) ->
       if not callback

src/flux/stores/database-store.coffee

@@ -89,6 +89,9 @@ class DatabaseStore extends NylasStore
     @_open = false
     @_waiting = []
 
+    @setupEmitter()
+    @_emitter.setMaxListeners(100)
+
     if NylasEnv.inSpecMode()
       @_databasePath = path.join(NylasEnv.getConfigDirPath(),'edgehill.test.db')
     else

src/flux/stores/list-data-source.coffee

@@ -7,6 +7,7 @@ class ListDataSource
 
   constructor: ->
     @_emitter = new EventEmitter()
+    @_cleanedup = false
     @selection = new ListSelection(@, @trigger)
     @
 
@@ -16,10 +17,18 @@ class ListDataSource
     @_emitter.emit('trigger', arg)
 
   listen: (callback, bindContext) ->
+    if @_cleanedup is true
+      throw new Error("ListDataSource: You cannot listen again after removing the last listener. This is an implementation detail.")
+
     eventHandler = ->
       callback.apply(bindContext, arguments)
     @_emitter.addListener('trigger', eventHandler)
-    return => @_emitter.removeListener('trigger', eventHandler)
+
+    return =>
+      @_emitter.removeListener('trigger', eventHandler)
+      if @_emitter.listenerCount('trigger') is 0
+        @_cleanedup = true
+        @cleanup()
 
   loaded: ->
     throw new Error("ListDataSource base class does not implement loaded()")
@@ -44,3 +53,6 @@ class ListDataSource
 
   setRetainedRange: ({start, end}) ->
     throw new Error("ListDataSource base class does not implement setRetainedRange()")
+
+  cleanup: ->
+    @selection.cleanup()

src/flux/stores/list-selection.coffee

@@ -1,4 +1,5 @@
 Model = require '../models/model'
+DatabaseStore = require './database-store'
 _ = require 'underscore'
 
 module.exports =
@@ -6,8 +7,12 @@ class ListSelection
 
   constructor: (@_view, @trigger) ->
     throw new Error("new ListSelection(): You must provide a view.") unless @_view
+    @_unlisten = DatabaseStore.listen(@_applyChangeRecord, @)
     @_items = []
 
+  cleanup: ->
+    @_unlisten()
+
   count: ->
     @_items.length
 
@@ -29,17 +34,6 @@ class ListSelection
       @_items.push(item)
     @trigger(@)
 
-  updateModelReferences: (items = []) ->
-    for newer in items
-      unless newer instanceof Model
-        console.error(JSON.stringify(newer))
-        throw new Error("updateModelReferences must be called with Models")
-
-      for existing, idx in @_items
-        if existing.id is newer.id
-          @_items[idx] = newer
-          break
-
   toggle: (item) ->
     return unless item
     throw new Error("toggle must be called with a Model") unless item instanceof Model
@@ -61,11 +55,18 @@ class ListSelection
       @_items = updated
       @trigger(@)
 
-  remove: (item) ->
-    return unless item
-    throw new Error("remove must be called with a Model") unless item instanceof Model
+  remove: (items) ->
+    return unless items
 
-    without = _.reject @_items, (t) -> t.id is item.id
+    items = [items] unless items instanceof Array
+
+    for item in items
+      unless item instanceof Model
+        throw new Error("remove: Must be passed a model or array of models")
+
+    itemIds = _.pluck(items, 'id')
+
+    without = _.reject @_items, (t) -> t.id in itemIds
     if without.length < @_items.length
       @_items = without
       @trigger(@)
@@ -128,3 +129,20 @@ class ListSelection
         @_items.push(next)
 
     @trigger()
+
+  _applyChangeRecord: (change) ->
+    return if @_items.length is 0
+    return if change.objectClass isnt @_items[0].constructor.name
+
+    if change.type is 'unpersist'
+      @remove(change.objects)
+    else if change.type is 'persist'
+      touched = 0
+      for newer in change.objects
+        for existing, idx in @_items
+          if existing.id is newer.id
+            @_items[idx] = newer
+            touched += 1
+            break
+      if touched > 0
+        @trigger(@)

src/flux/stores/observable-list-data-source.coffee

@@ -22,7 +22,7 @@ class ObservableListDataSource extends ListDataSource
     @_resultSet = null
     @_resultDesiredLast = null
 
-    $resultSetObservable.subscribe (nextResultSet) =>
+    @_subscription = $resultSetObservable.subscribe (nextResultSet) =>
       if nextResultSet.range().end is @_resultDesiredLast
         @_countEstimate = Math.max(@_countEstimate, nextResultSet.range().end + 1)
       else
@@ -31,7 +31,12 @@ class ObservableListDataSource extends ListDataSource
       previousResultSet = @_resultSet
       @_resultSet = nextResultSet
 
-      @selection.updateModelReferences(@_resultSet.models())
+      # If the result set is derived from a query, remove any items in the selection
+      # that do not match the query. This ensures that items "removed from the view"
+      # are removed from the selection.
+      query = nextResultSet.query()
+      @selection.removeItemsNotMatching(query.matchers()) if query
+
       @trigger({previous: previousResultSet, next: nextResultSet})
 
   setRetainedRange: ({start, end}) ->
@@ -64,5 +69,9 @@ class ObservableListDataSource extends ListDataSource
     return [] unless @_resultSet
     @_resultSet.models().filter(matchFn)
 
+  cleanup: ->
+    @_subscription?.dispose()
+    super
+
 
 module.exports = ObservableListDataSource