mirror of
https://github.com/Foundry376/Mailspring.git
synced 2024-09-21 15:56:10 +08:00
fix(selection): Fix issue where selecting offscreen thread (null) could break selection
Summary: - ModelViewSelection shouldn't break if you accidentally call it with null - ModelViewSelection methods should throw if you pass non-items, need to do this to narrow down other Sentry errors Test Plan: Run new specs Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1589
This commit is contained in:
parent
72fdfcd4a1
commit
eac4029ded
|
@ -63,11 +63,11 @@ describe "DatabaseView", ->
|
||||||
@view = new DatabaseView(Message, config)
|
@view = new DatabaseView(Message, config)
|
||||||
@view._pages =
|
@view._pages =
|
||||||
0:
|
0:
|
||||||
items: [{id: 'a'}, {id: 'b'}, {id: 'c'}]
|
items: [new Thread(id: 'a'), new Thread(id: 'b'), new Thread(id: 'c')]
|
||||||
metadata: {'a': 'a-metadata', 'b': 'b-metadata', 'c': 'c-metadata'}
|
metadata: {'a': 'a-metadata', 'b': 'b-metadata', 'c': 'c-metadata'}
|
||||||
loaded: true
|
loaded: true
|
||||||
1:
|
1:
|
||||||
items: [{id: 'd'}, {id: 'e'}, {id: 'f'}]
|
items: [new Thread(id: 'd'), new Thread(id: 'e'), new Thread(id: 'f')]
|
||||||
metadata: {'d': 'd-metadata', 'e': 'e-metadata', 'f': 'f-metadata'}
|
metadata: {'d': 'd-metadata', 'e': 'e-metadata', 'f': 'f-metadata'}
|
||||||
loaded: true
|
loaded: true
|
||||||
@view._count = 1
|
@view._count = 1
|
||||||
|
@ -229,7 +229,7 @@ describe "DatabaseView", ->
|
||||||
@view._pages = {}
|
@view._pages = {}
|
||||||
for i in [0..9]
|
for i in [0..9]
|
||||||
@view._pages[i] =
|
@view._pages[i] =
|
||||||
items: [{id: 'a'}, {id: 'b'}, {id: 'c'}]
|
items: [new Thread(id: 'a'), new Thread(id: 'b'), new Thread(id: 'c')]
|
||||||
metadata: {'a': 'a-metadata', 'b': 'b-metadata', 'c': 'c-metadata'}
|
metadata: {'a': 'a-metadata', 'b': 'b-metadata', 'c': 'c-metadata'}
|
||||||
loaded: true
|
loaded: true
|
||||||
|
|
||||||
|
@ -274,7 +274,7 @@ describe "DatabaseView", ->
|
||||||
beforeEach ->
|
beforeEach ->
|
||||||
@view.retrievePage(0)
|
@view.retrievePage(0)
|
||||||
@completeQuery = =>
|
@completeQuery = =>
|
||||||
@items = [{id: 'model-a'}, {id: 'model-b'}, {id: 'model-c'}]
|
@items = [new Thread(id: 'model-a'), new Thread(id: 'model-b'), new Thread(id: 'model-c')]
|
||||||
@queries[0].resolve(@items)
|
@queries[0].resolve(@items)
|
||||||
@queries = []
|
@queries = []
|
||||||
spyOn(@view, 'loaded').andCallFake -> true
|
spyOn(@view, 'loaded').andCallFake -> true
|
||||||
|
|
|
@ -50,6 +50,27 @@ describe "ModelViewSelection", ->
|
||||||
@selection.clear()
|
@selection.clear()
|
||||||
expect(@trigger).toHaveBeenCalled()
|
expect(@trigger).toHaveBeenCalled()
|
||||||
|
|
||||||
|
describe "remove", ->
|
||||||
|
beforeEach ->
|
||||||
|
@selection.set([@items[2], @items[4]])
|
||||||
|
|
||||||
|
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'])
|
||||||
|
|
||||||
|
it "should remove the item from the set", ->
|
||||||
|
@selection.remove(@items[2])
|
||||||
|
expect(@selection.ids()).toEqual(['4'])
|
||||||
|
|
||||||
|
it "should throw an exception if the item passed is not a model", ->
|
||||||
|
expect( => @selection.remove('hi')).toThrow()
|
||||||
|
|
||||||
|
it "should trigger", ->
|
||||||
|
@selection.remove()
|
||||||
|
expect(@trigger).toHaveBeenCalled()
|
||||||
|
|
||||||
describe "updateModelReferences", ->
|
describe "updateModelReferences", ->
|
||||||
it "should replace items in the selection with the matching provided items, if present", ->
|
it "should replace items in the selection with the matching provided items, if present", ->
|
||||||
@selection.set([@items[2], @items[4], @items[7]])
|
@selection.set([@items[2], @items[4], @items[7]])
|
||||||
|
@ -64,6 +85,15 @@ describe "ModelViewSelection", ->
|
||||||
beforeEach ->
|
beforeEach ->
|
||||||
@selection.set([@items[2]])
|
@selection.set([@items[2]])
|
||||||
|
|
||||||
|
it "should do nothing if called without a valid item", ->
|
||||||
|
@selection.toggle(null)
|
||||||
|
@selection.toggle(undefined)
|
||||||
|
@selection.toggle(false)
|
||||||
|
expect(@selection.ids()).toEqual(['2'])
|
||||||
|
|
||||||
|
it "should throw an exception if the item passed is not a model", ->
|
||||||
|
expect( => @selection.toggle('hi')).toThrow()
|
||||||
|
|
||||||
it "should select the item if it is not selected", ->
|
it "should select the item if it is not selected", ->
|
||||||
@selection.toggle(@items[3])
|
@selection.toggle(@items[3])
|
||||||
expect(@selection.ids()).toEqual(['2', '3'])
|
expect(@selection.ids()).toEqual(['2', '3'])
|
||||||
|
@ -82,6 +112,15 @@ describe "ModelViewSelection", ->
|
||||||
@selection.expandTo(@items[2])
|
@selection.expandTo(@items[2])
|
||||||
expect(@selection.ids()).toEqual(['2'])
|
expect(@selection.ids()).toEqual(['2'])
|
||||||
|
|
||||||
|
it "should do nothing if called without a valid item", ->
|
||||||
|
@selection.expandTo(null)
|
||||||
|
@selection.expandTo(undefined)
|
||||||
|
@selection.expandTo(false)
|
||||||
|
expect(@selection.ids()).toEqual([])
|
||||||
|
|
||||||
|
it "should throw an exception if the item passed is not a model", ->
|
||||||
|
expect( => @selection.expandTo('hi')).toThrow()
|
||||||
|
|
||||||
it "should select all items from the last selected item to the provided item", ->
|
it "should select all items from the last selected item to the provided item", ->
|
||||||
@selection.set([@items[2], @items[5]])
|
@selection.set([@items[2], @items[5]])
|
||||||
@selection.expandTo(@items[8])
|
@selection.expandTo(@items[8])
|
||||||
|
|
|
@ -88,11 +88,13 @@ class InjectedComponent extends React.Component
|
||||||
|
|
||||||
focus: =>
|
focus: =>
|
||||||
# Not forwarding event - just a method call
|
# Not forwarding event - just a method call
|
||||||
@refs.inner.focus() if @refs.inner.focus?
|
# Note that our inner may not be populated, and it may not have a focus method
|
||||||
|
@refs.inner.focus() if @refs.inner?.focus?
|
||||||
|
|
||||||
blur: =>
|
blur: =>
|
||||||
# Not forwarding an event - just a method call
|
# Not forwarding an event - just a method call
|
||||||
@refs.inner.blur() if @refs.inner.blur?
|
# Note that our inner may not be populated, and it may not have a blur method
|
||||||
|
@refs.inner.blur() if @refs.inner?.blur?
|
||||||
|
|
||||||
_getStateFromStores: (props) =>
|
_getStateFromStores: (props) =>
|
||||||
props ?= @props
|
props ?= @props
|
||||||
|
|
|
@ -23,18 +23,25 @@ class ModelViewSelection
|
||||||
set: (items) ->
|
set: (items) ->
|
||||||
@_items = []
|
@_items = []
|
||||||
for item in items
|
for item in items
|
||||||
throw new Error("selectItems must be called with Models") unless item instanceof Model
|
throw new Error("set must be called with Models") unless item instanceof Model
|
||||||
@_items.push(item)
|
@_items.push(item)
|
||||||
@trigger(@)
|
@trigger(@)
|
||||||
|
|
||||||
updateModelReferences: (items = []) ->
|
updateModelReferences: (items = []) ->
|
||||||
for newer in 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
|
for existing, idx in @_items
|
||||||
if existing.id is newer.id
|
if existing.id is newer.id
|
||||||
@_items[idx] = newer
|
@_items[idx] = newer
|
||||||
break
|
break
|
||||||
|
|
||||||
toggle: (item) ->
|
toggle: (item) ->
|
||||||
|
return unless item
|
||||||
|
throw new Error("toggle must be called with a Model") unless item instanceof Model
|
||||||
|
|
||||||
without = _.reject @_items, (t) -> t.id is item.id
|
without = _.reject @_items, (t) -> t.id is item.id
|
||||||
if without.length < @_items.length
|
if without.length < @_items.length
|
||||||
@_items = without
|
@_items = without
|
||||||
|
@ -43,12 +50,18 @@ class ModelViewSelection
|
||||||
@trigger(@)
|
@trigger(@)
|
||||||
|
|
||||||
remove: (item) ->
|
remove: (item) ->
|
||||||
|
return unless item
|
||||||
|
throw new Error("remove must be called with a Model") unless item instanceof Model
|
||||||
|
|
||||||
without = _.reject @_items, (t) -> t.id is item.id
|
without = _.reject @_items, (t) -> t.id is item.id
|
||||||
if without.length < @_items.length
|
if without.length < @_items.length
|
||||||
@_items = without
|
@_items = without
|
||||||
@trigger(@)
|
@trigger(@)
|
||||||
|
|
||||||
expandTo: (item) ->
|
expandTo: (item) ->
|
||||||
|
return unless item
|
||||||
|
throw new Error("expandTo must be called with a Model") unless item instanceof Model
|
||||||
|
|
||||||
if @_items.length is 0
|
if @_items.length is 0
|
||||||
@_items.push(item)
|
@_items.push(item)
|
||||||
else
|
else
|
||||||
|
|
Loading…
Reference in a new issue