From 47a8a1905daac1777eda5d6fc6ed756c9719aed8 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 20 Apr 2018 09:16:05 -0400 Subject: [PATCH 1/5] Move selection models to lib/models/ --- lib/{views => models}/composite-list-selection.js | 0 lib/{views => models}/file-patch-selection.js | 0 lib/{views => models}/list-selection.js | 0 lib/views/file-patch-view.js | 2 +- lib/views/staging-view.js | 2 +- test/{views => models}/composite-list-selection.test.js | 2 +- test/{views => models}/file-patch-selection.test.js | 2 +- test/{views => models}/list-selection.test.js | 2 +- 8 files changed, 5 insertions(+), 5 deletions(-) rename lib/{views => models}/composite-list-selection.js (100%) rename lib/{views => models}/file-patch-selection.js (100%) rename lib/{views => models}/list-selection.js (100%) rename test/{views => models}/composite-list-selection.test.js (99%) rename test/{views => models}/file-patch-selection.test.js (99%) rename test/{views => models}/list-selection.test.js (95%) diff --git a/lib/views/composite-list-selection.js b/lib/models/composite-list-selection.js similarity index 100% rename from lib/views/composite-list-selection.js rename to lib/models/composite-list-selection.js diff --git a/lib/views/file-patch-selection.js b/lib/models/file-patch-selection.js similarity index 100% rename from lib/views/file-patch-selection.js rename to lib/models/file-patch-selection.js diff --git a/lib/views/list-selection.js b/lib/models/list-selection.js similarity index 100% rename from lib/views/list-selection.js rename to lib/models/list-selection.js diff --git a/lib/views/file-patch-view.js b/lib/views/file-patch-view.js index 8da207b76d..0f310faae9 100644 --- a/lib/views/file-patch-view.js +++ b/lib/views/file-patch-view.js @@ -8,7 +8,7 @@ import {autobind} from 'core-decorators'; import HunkView from './hunk-view'; import SimpleTooltip from './simple-tooltip'; import Commands, {Command} from './commands'; -import FilePatchSelection from './file-patch-selection'; +import FilePatchSelection from '../models/file-patch-selection'; import Switchboard from '../switchboard'; import RefHolder from '../models/ref-holder'; diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index df3b59a3f6..b0d09f1748 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -13,7 +13,7 @@ import isEqual from 'lodash.isequal'; import FilePatchListItemView from './file-patch-list-item-view'; import MergeConflictListItemView from './merge-conflict-list-item-view'; -import CompositeListSelection from './composite-list-selection'; +import CompositeListSelection from '../models/composite-list-selection'; import ResolutionProgress from '../models/conflicts/resolution-progress'; import ModelObserver from '../models/model-observer'; import FilePatchController from '../controllers/file-patch-controller'; diff --git a/test/views/composite-list-selection.test.js b/test/models/composite-list-selection.test.js similarity index 99% rename from test/views/composite-list-selection.test.js rename to test/models/composite-list-selection.test.js index 0eeac94c79..f1dd38907f 100644 --- a/test/views/composite-list-selection.test.js +++ b/test/models/composite-list-selection.test.js @@ -1,4 +1,4 @@ -import CompositeListSelection from '../../lib/views/composite-list-selection'; +import CompositeListSelection from '../../lib/models/composite-list-selection'; import {assertEqualSets} from '../helpers'; describe('CompositeListSelection', function() { diff --git a/test/views/file-patch-selection.test.js b/test/models/file-patch-selection.test.js similarity index 99% rename from test/views/file-patch-selection.test.js rename to test/models/file-patch-selection.test.js index ed715d648b..93a855606b 100644 --- a/test/views/file-patch-selection.test.js +++ b/test/models/file-patch-selection.test.js @@ -1,4 +1,4 @@ -import FilePatchSelection from '../../lib/views/file-patch-selection'; +import FilePatchSelection from '../../lib/models/file-patch-selection'; import Hunk from '../../lib/models/hunk'; import HunkLine from '../../lib/models/hunk-line'; import {assertEqualSets} from '../helpers'; diff --git a/test/views/list-selection.test.js b/test/models/list-selection.test.js similarity index 95% rename from test/views/list-selection.test.js rename to test/models/list-selection.test.js index 9713df983d..690b5eab25 100644 --- a/test/views/list-selection.test.js +++ b/test/models/list-selection.test.js @@ -1,4 +1,4 @@ -import ListSelection from '../../lib/views/list-selection'; +import ListSelection from '../../lib/models/list-selection'; import {assertEqualSets} from '../helpers'; // This class is mostly tested via CompositeListSelection and From 19e7f46f80cac44358134f91f5e221b7816028bf Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 25 Apr 2018 09:24:38 -0400 Subject: [PATCH 2/5] Make ListSelection immutable --- lib/models/list-selection.js | 78 ++++++++++++++---------------- test/models/list-selection.test.js | 34 ++++++------- 2 files changed, 53 insertions(+), 59 deletions(-) diff --git a/lib/models/list-selection.js b/lib/models/list-selection.js index ee0c513127..feba3e326f 100644 --- a/lib/models/list-selection.js +++ b/lib/models/list-selection.js @@ -1,6 +1,6 @@ import {autobind} from 'core-decorators'; -const COPY = {}; +const COPY = Symbol('copy'); export default class ListSelection { constructor(options = {}) { @@ -8,7 +8,9 @@ export default class ListSelection { this.options = { isItemSelectable: options.isItemSelectable || (item => !!item), }; - this.setItems(options.items || []); + + this.items = options.items || []; + this.selections = this.items.length > 0 ? [{head: 0, tail: 0}] : []; } else { this.options = { isItemSelectable: options.isItemSelectable, @@ -18,14 +20,12 @@ export default class ListSelection { } } - copy() { - // Deep-copy selections because it will be modified. - // (That's temporary, until ListSelection is changed to be immutable, too.) + copy(options = {}) { return new ListSelection({ _copy: COPY, - isItemSelectable: this.options.isItemSelectable, - items: this.items, - selections: this.selections.map(({head, tail, negate}) => ({head, tail, negate})), + isItemSelectable: options.isItemSelectable || this.options.isItemSelectable, + items: options.items || this.items, + selections: options.selections || this.selections, }); } @@ -36,19 +36,15 @@ export default class ListSelection { setItems(items) { let newSelectionIndex; - if (this.selections && this.selections.length > 0) { + if (this.selections.length > 0) { const [{head, tail}] = this.selections; newSelectionIndex = Math.min(head, tail, items.length - 1); } else { newSelectionIndex = 0; } - this.items = items; - if (items.length > 0) { - this.selections = [{head: newSelectionIndex, tail: newSelectionIndex}]; - } else { - this.selections = []; - } + const newSelections = items.length > 0 ? [{head: newSelectionIndex, tail: newSelectionIndex}] : []; + return this.copy({items, selections: newSelections}); } getItems() { @@ -63,31 +59,29 @@ export default class ListSelection { for (let i = 0; i < this.items.length; i++) { const item = this.items[i]; if (this.isItemSelectable(item)) { - this.selectItem(item, preserveTail); - break; + return this.selectItem(item, preserveTail); } } + return this; } selectLastItem(preserveTail) { for (let i = this.items.length - 1; i > 0; i--) { const item = this.items[i]; if (this.isItemSelectable(item)) { - this.selectItem(item, preserveTail); - break; + return this.selectItem(item, preserveTail); } } + return this; } selectAllItems() { - this.selectFirstItem(); - this.selectLastItem(true); + return this.selectFirstItem().selectLastItem(true); } selectNextItem(preserveTail) { if (this.selections.length === 0) { - this.selectFirstItem(); - return; + return this.selectFirstItem(); } let itemIndex = this.selections[0].head; @@ -100,13 +94,12 @@ export default class ListSelection { } } - this.selectItem(this.items[nextItemIndex], preserveTail); + return this.selectItem(this.items[nextItemIndex], preserveTail); } selectPreviousItem(preserveTail) { if (this.selections.length === 0) { - this.selectLastItem(); - return; + return this.selectLastItem(); } let itemIndex = this.selections[0].head; @@ -120,7 +113,7 @@ export default class ListSelection { } } - this.selectItem(this.items[previousItemIndex], preserveTail); + return this.selectItem(this.items[previousItemIndex], preserveTail); } selectItem(item, preserveTail, addOrSubtract) { @@ -130,24 +123,25 @@ export default class ListSelection { const itemIndex = this.items.indexOf(item); if (preserveTail && this.selections[0]) { - this.selections[0].head = itemIndex; + const newSelections = [{head: itemIndex, tail: this.selections[0].tail}, ...this.selections.slice(1)]; + return this.copy({selections: newSelections}); } else { const selection = {head: itemIndex, tail: itemIndex}; if (addOrSubtract) { if (this.getSelectedItems().has(item)) { selection.negate = true; } - this.selections.unshift(selection); + return this.copy({selections: [selection, ...this.selections]}); } else { - this.selections = [selection]; + return this.copy({selections: [selection]}); } } } addOrSubtractSelection(item) { - this.selectItem(item, false, true); + return this.selectItem(item, false, true); } coalesce() { - if (this.selections.length === 0) { return; } + if (this.selections.length === 0) { return this; } const mostRecent = this.selections[0]; let mostRecentStart = Math.min(mostRecent.head, mostRecent.tail); @@ -159,30 +153,29 @@ export default class ListSelection { mostRecentEnd++; } + const newSelections = [mostRecent]; for (let i = 1; i < this.selections.length;) { const current = this.selections[i]; const currentStart = Math.min(current.head, current.tail); const currentEnd = Math.max(current.head, current.tail); if (mostRecentStart <= currentEnd + 1 && currentStart - 1 <= mostRecentEnd) { if (mostRecent.negate) { - const truncatedSelections = []; if (current.head > current.tail) { if (currentEnd > mostRecentEnd) { // suffix - truncatedSelections.push({tail: mostRecentEnd + 1, head: currentEnd}); + newSelections.push({tail: mostRecentEnd + 1, head: currentEnd}); } if (currentStart < mostRecentStart) { // prefix - truncatedSelections.push({tail: currentStart, head: mostRecentStart - 1}); + newSelections.push({tail: currentStart, head: mostRecentStart - 1}); } } else { if (currentStart < mostRecentStart) { // prefix - truncatedSelections.push({head: currentStart, tail: mostRecentStart - 1}); + newSelections.push({head: currentStart, tail: mostRecentStart - 1}); } if (currentEnd > mostRecentEnd) { // suffix - truncatedSelections.push({head: mostRecentEnd + 1, tail: currentEnd}); + newSelections.push({head: mostRecentEnd + 1, tail: currentEnd}); } } - this.selections.splice(i, 1, ...truncatedSelections); - i += truncatedSelections.length; + i++; } else { mostRecentStart = Math.min(mostRecentStart, currentStart); mostRecentEnd = Math.max(mostRecentEnd, currentEnd); @@ -193,14 +186,17 @@ export default class ListSelection { mostRecent.head = mostRecentStart; mostRecent.tail = mostRecentEnd; } - this.selections.splice(i, 1); + i++; } } else { + newSelections.push(current); i++; } } - if (mostRecent.negate) { this.selections.shift(); } + if (mostRecent.negate) { newSelections.shift(); } + + return this.copy({selections: newSelections}); } getSelectedItems() { diff --git a/test/models/list-selection.test.js b/test/models/list-selection.test.js index 690b5eab25..0bc9dc956b 100644 --- a/test/models/list-selection.test.js +++ b/test/models/list-selection.test.js @@ -1,35 +1,33 @@ import ListSelection from '../../lib/models/list-selection'; import {assertEqualSets} from '../helpers'; -// This class is mostly tested via CompositeListSelection and -// FilePatchSelection. This file contains unit tests that are more convenient to -// write directly against this class. +// This class is mostly tested via CompositeListSelection and FilePatchSelection. This file contains unit tests that are +// more convenient to write directly against this class. describe('ListSelection', function() { describe('coalesce', function() { it('correctly handles adding and subtracting a single item (regression)', function() { - const selection = new ListSelection({items: ['a', 'b', 'c']}); - selection.selectLastItem(true); - selection.coalesce(); + let selection = new ListSelection({items: ['a', 'b', 'c']}); + selection = selection.selectLastItem(true); + selection = selection.coalesce(); assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b', 'c'])); - selection.addOrSubtractSelection('b'); - selection.coalesce(); + selection = selection.addOrSubtractSelection('b'); + selection = selection.coalesce(); assertEqualSets(selection.getSelectedItems(), new Set(['a', 'c'])); - selection.addOrSubtractSelection('b'); - global.debug = true; - selection.coalesce(); + selection = selection.addOrSubtractSelection('b'); + selection = selection.coalesce(); assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b', 'c'])); }); }); describe('selectItem', () => { // https://github.com/atom/github/issues/467 - it('selects an item when there are no selections', () => { - const selection = new ListSelection({items: ['a', 'b', 'c']}); - selection.addOrSubtractSelection('a'); - selection.coalesce(); - assert.equal(selection.getSelectedItems().size, 0); - selection.selectItem('a', true); - assert.equal(selection.getSelectedItems().size, 1); + it.only('selects an item when there are no selections', () => { + let selection = new ListSelection({items: ['a', 'b', 'c']}); + selection = selection.addOrSubtractSelection('a'); + selection = selection.coalesce(); + assert.strictEqual(selection.getSelectedItems().size, 0); + selection = selection.selectItem('a', true); + assert.strictEqual(selection.getSelectedItems().size, 1); }); }); }); From e8a67d95ce46584eb7eb73ea9ad8f0c69fc797db Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 25 Apr 2018 16:34:12 -0400 Subject: [PATCH 3/5] Make CompositeListSelection immutable --- lib/models/composite-list-selection.js | 233 ++++++++---- test/models/composite-list-selection.test.js | 362 +++++++++---------- test/models/list-selection.test.js | 2 +- 3 files changed, 339 insertions(+), 258 deletions(-) diff --git a/lib/models/composite-list-selection.js b/lib/models/composite-list-selection.js index 9e90c2ace8..cfdf122a7c 100644 --- a/lib/models/composite-list-selection.js +++ b/lib/models/composite-list-selection.js @@ -1,56 +1,136 @@ import ListSelection from './list-selection'; +const COPY = Symbol('COPY'); + export default class CompositeListSelection { - constructor({listsByKey, idForItem}) { - this.keysBySelection = new Map(); - this.selections = []; - this.idForItem = idForItem || (item => item); - this.resolveNextUpdatePromise = () => {}; + constructor(options) { + if (options._copy !== COPY) { + this.keysBySelection = new Map(); + this.selections = []; + this.idForItem = options.idForItem || (item => item); + this.resolveNextUpdatePromise = () => {}; + this.activeSelectionIndex = null; + + for (const [key, items] of options.listsByKey) { + const selection = new ListSelection({items}); + this.keysBySelection.set(selection, key); + this.selections.push(selection); + + if (this.activeSelectionIndex === null && selection.getItems().length) { + this.activeSelectionIndex = this.selections.length - 1; + } + } + + if (this.activeSelectionIndex === null) { + this.activeSelectionIndex = 0; + } + } else { + this.keysBySelection = options.keysBySelection; + this.selections = options.selections; + this.idForItem = options.idForItem; + this.activeSelectionIndex = options.activeSelectionIndex; + this.resolveNextUpdatePromise = options.resolveNextUpdatePromise; + } + } + + copy(options = {}) { + let selections = []; + let keysBySelection = new Map(); - for (const key in listsByKey) { - const selection = new ListSelection({items: listsByKey[key]}); - this.keysBySelection.set(selection, key); - this.selections.push(selection); + if (options.keysBySelection || options.selections) { + if (!options.keysBySelection || !options.selections) { + throw new Error('keysBySelection and selection must always be updated simultaneously'); + } + + selections = options.selections; + keysBySelection = options.keysBySelection; + } else { + selections = this.selections; + keysBySelection = this.keysBySelection; } - this.selectFirstNonEmptyList(); + return new CompositeListSelection({ + keysBySelection, + selections, + activeSelectionIndex: options.activeSelectionIndex !== undefined + ? options.activeSelectionIndex + : this.activeSelectionIndex, + idForItem: options.idForItem || this.idForItem, + resolveNextUpdatePromise: options.resolveNextUpdatePromise || this.resolveNextUpdatePromise, + _copy: COPY, + }); } updateLists(listsByKey) { - let wasChanged = false; + let isDifferent = false; - const keys = Object.keys(listsByKey); - for (let i = 0; i < keys.length; i++) { - const newItems = listsByKey[keys[i]]; - const selection = this.selections[i]; + if (listsByKey.length === 0) { + return this; + } + + const newKeysBySelection = new Map(); + const newSelections = []; + + for (let i = 0; i < listsByKey.length; i++) { + const [key, newItems] = listsByKey[i]; + let selection = this.selections[i]; const oldItems = selection.getItems(); - if (!wasChanged) { - if (newItems.length !== oldItems.length) { - wasChanged = true; - } else { - for (let j = 0; j < oldItems.length; j++) { - if (oldItems[j] !== newItems[j]) { - wasChanged = true; - break; - } - } - } + if (!isDifferent) { + isDifferent = oldItems.length !== newItems.length || oldItems.some((oldItem, j) => oldItem === newItems[j]); } const oldHeadItem = selection.getHeadItem(); - selection.setItems(newItems); + selection = selection.setItems(newItems); let newHeadItem = null; if (oldHeadItem) { newHeadItem = newItems.find(item => this.idForItem(item) === this.idForItem(oldHeadItem)); } - if (newHeadItem) { selection.selectItem(newHeadItem); } + if (newHeadItem) { + selection = selection.selectItem(newHeadItem); + } + + newKeysBySelection.set(selection, key); + newSelections.push(selection); } - if (this.getActiveSelection().getItems().length === 0) { - this.activateNextSelection() || this.activatePreviousSelection(); + + let updated = this.copy({ + keysBySelection: newKeysBySelection, + selections: newSelections, + }); + + if (updated.getActiveSelection().getItems().length === 0) { + const next = updated.activateNextSelection(); + updated = next !== updated ? next : updated.activatePreviousSelection(); } - if (wasChanged) { this.resolveNextUpdatePromise(); } + if (isDifferent) { + updated.resolveNextUpdatePromise(); + } + + return updated; + } + + updateActiveSelection(fn) { + const oldSelection = this.getActiveSelection(); + const newSelection = fn(oldSelection); + if (oldSelection === newSelection) { + return this; + } + + const key = this.keysBySelection.get(oldSelection); + + const newKeysBySelection = new Map(this.keysBySelection); + newKeysBySelection.delete(oldSelection); + newKeysBySelection.set(newSelection, key); + + const newSelections = this.selections.slice(); + newSelections[this.activeSelectionIndex] = newSelection; + + return this.copy({ + keysBySelection: newKeysBySelection, + selections: newSelections, + }); } getNextUpdatePromise() { @@ -60,13 +140,9 @@ export default class CompositeListSelection { } selectFirstNonEmptyList() { - this.activeSelectionIndex = 0; - for (let i = 0; i < this.selections.length; i++) { - if (this.selections[i].getItems().length) { - this.activeSelectionIndex = i; - break; - } - } + return this.copy({ + activeSelectionIndex: this.selections.findIndex(selection => selection.getItems().length > 0), + }); } getActiveListKey() { @@ -88,76 +164,83 @@ export default class CompositeListSelection { activateSelection(selection) { const index = this.selections.indexOf(selection); if (index === -1) { throw new Error('Selection not found'); } - this.activeSelectionIndex = index; + return this.copy({activeSelectionIndex: index}); } activateNextSelection() { for (let i = this.activeSelectionIndex + 1; i < this.selections.length; i++) { if (this.selections[i].getItems().length > 0) { - this.activeSelectionIndex = i; - return true; + return this.copy({activeSelectionIndex: i}); } } - return false; + return this; } activatePreviousSelection() { for (let i = this.activeSelectionIndex - 1; i >= 0; i--) { if (this.selections[i].getItems().length > 0) { - this.activeSelectionIndex = i; - return true; + return this.copy({activeSelectionIndex: i}); } } - return false; + return this; } activateLastSelection() { for (let i = this.selections.length - 1; i >= 0; i--) { if (this.selections[i].getItems().length > 0) { - this.activeSelectionIndex = i; - return true; + return this.copy({activeSelectionIndex: i}); } } - return false; + return this; } selectItem(item, preserveTail = false) { const selection = this.selectionForItem(item); - if (!selection) { throw new Error(`No item found: ${item}`); } - if (!preserveTail) { this.activateSelection(selection); } - if (selection === this.getActiveSelection()) { selection.selectItem(item, preserveTail); } + if (!selection) { + throw new Error(`No item found: ${item}`); + } + + let next = this; + if (!preserveTail) { + next = next.activateSelection(selection); + } + if (selection === next.getActiveSelection()) { + next = next.updateActiveSelection(s => s.selectItem(item, preserveTail)); + } + return next; } addOrSubtractSelection(item) { const selection = this.selectionForItem(item); - if (!selection) { throw new Error(`No item found: ${item}`); } + if (!selection) { + throw new Error(`No item found: ${item}`); + } if (selection === this.getActiveSelection()) { - selection.addOrSubtractSelection(item); + return this.updateActiveSelection(s => s.addOrSubtractSelection(item)); } else { - this.activateSelection(selection); - selection.selectItem(item); + return this.activateSelection(selection).updateActiveSelection(s => s.selectItem(item)); } } selectAllItems() { - this.getActiveSelection().selectAllItems(); + return this.updateActiveSelection(s => s.selectAllItems()); } selectFirstItem(preserveTail) { - this.getActiveSelection().selectFirstItem(preserveTail); + return this.updateActiveSelection(s => s.selectFirstItem(preserveTail)); } selectLastItem(preserveTail) { - this.getActiveSelection().selectLastItem(preserveTail); + return this.updateActiveSelection(s => s.selectLastItem(preserveTail)); } coalesce() { - this.getActiveSelection().coalesce(); + return this.updateActiveSelection(s => s.coalesce()); } selectionForItem(item) { - return this.selections.find(selection => selection.getItems().indexOf(item) > -1); + return this.selections.find(selection => selection.getItems().includes(item)); } listKeyForItem(item) { @@ -165,32 +248,30 @@ export default class CompositeListSelection { } selectNextItem(preserveTail = false) { - if (!preserveTail && this.getActiveSelection().getHeadItem() === this.getActiveSelection().getLastItem()) { - if (this.activateNextSelection()) { - this.getActiveSelection().selectFirstItem(); - return true; + let next = this; + if (!preserveTail && next.getActiveSelection().getHeadItem() === next.getActiveSelection().getLastItem()) { + next = next.activateNextSelection(); + if (next !== this) { + return next.updateActiveSelection(s => s.selectFirstItem()); } else { - this.getActiveSelection().selectLastItem(); - return false; + return next.updateActiveSelection(s => s.selectLastItem()); } } else { - this.getActiveSelection().selectNextItem(preserveTail); - return true; + return next.updateActiveSelection(s => s.selectNextItem(preserveTail)); } } selectPreviousItem(preserveTail = false) { - if (!preserveTail && this.getActiveSelection().getHeadItem() === this.getActiveSelection().getItems()[0]) { - if (this.activatePreviousSelection()) { - this.getActiveSelection().selectLastItem(); - return true; + let next = this; + if (!preserveTail && next.getActiveSelection().getHeadItem() === next.getActiveSelection().getItems()[0]) { + next = next.activatePreviousSelection(); + if (next !== this) { + return next.updateActiveSelection(s => s.selectLastItem()); } else { - this.getActiveSelection().selectFirstItem(); - return false; + return next.updateActiveSelection(s => s.selectFirstItem()); } } else { - this.getActiveSelection().selectPreviousItem(preserveTail); - return true; + return next.updateActiveSelection(s => s.selectPreviousItem(preserveTail)); } } diff --git a/test/models/composite-list-selection.test.js b/test/models/composite-list-selection.test.js index f1dd38907f..9c881de563 100644 --- a/test/models/composite-list-selection.test.js +++ b/test/models/composite-list-selection.test.js @@ -4,310 +4,310 @@ import {assertEqualSets} from '../helpers'; describe('CompositeListSelection', function() { describe('selection', function() { it('allows specific items to be selected, but does not select across lists', function() { - const selection = new CompositeListSelection({ - listsByKey: { - unstaged: ['a', 'b'], - conflicts: ['c'], - staged: ['d', 'e', 'f'], - }, + let selection = new CompositeListSelection({ + listsByKey: [ + ['unstaged', ['a', 'b']], + ['conflicts', ['c']], + ['staged', ['d', 'e', 'f']], + ], }); - selection.selectItem('e'); - assert.equal(selection.getActiveListKey(), 'staged'); + selection = selection.selectItem('e'); + assert.strictEqual(selection.getActiveListKey(), 'staged'); assertEqualSets(selection.getSelectedItems(), new Set(['e'])); - selection.selectItem('f', true); - assert.equal(selection.getActiveListKey(), 'staged'); + selection = selection.selectItem('f', true); + assert.strictEqual(selection.getActiveListKey(), 'staged'); assertEqualSets(selection.getSelectedItems(), new Set(['e', 'f'])); - selection.selectItem('d', true); + selection = selection.selectItem('d', true); - assert.equal(selection.getActiveListKey(), 'staged'); + assert.strictEqual(selection.getActiveListKey(), 'staged'); assertEqualSets(selection.getSelectedItems(), new Set(['d', 'e'])); - selection.selectItem('c', true); - assert.equal(selection.getActiveListKey(), 'staged'); + selection = selection.selectItem('c', true); + assert.strictEqual(selection.getActiveListKey(), 'staged'); assertEqualSets(selection.getSelectedItems(), new Set(['d', 'e'])); }); it('allows the next and previous item to be selected', function() { - const selection = new CompositeListSelection({ - listsByKey: { - unstaged: ['a', 'b'], - conflicts: ['c'], - staged: ['d', 'e'], - }, + let selection = new CompositeListSelection({ + listsByKey: [ + ['unstaged', ['a', 'b']], + ['conflicts', ['c']], + ['staged', ['d', 'e']], + ], }); - assert.equal(selection.getActiveListKey(), 'unstaged'); + assert.strictEqual(selection.getActiveListKey(), 'unstaged'); assertEqualSets(selection.getSelectedItems(), new Set(['a'])); - assert.isTrue(selection.selectNextItem()); - assert.equal(selection.getActiveListKey(), 'unstaged'); + selection = selection.selectNextItem(); + assert.strictEqual(selection.getActiveListKey(), 'unstaged'); assertEqualSets(selection.getSelectedItems(), new Set(['b'])); - assert.isTrue(selection.selectNextItem()); - assert.equal(selection.getActiveListKey(), 'conflicts'); + selection = selection.selectNextItem(); + assert.strictEqual(selection.getActiveListKey(), 'conflicts'); assertEqualSets(selection.getSelectedItems(), new Set(['c'])); - assert.isTrue(selection.selectNextItem()); - assert.equal(selection.getActiveListKey(), 'staged'); + selection = selection.selectNextItem(); + assert.strictEqual(selection.getActiveListKey(), 'staged'); assertEqualSets(selection.getSelectedItems(), new Set(['d'])); - assert.isTrue(selection.selectNextItem()); - assert.equal(selection.getActiveListKey(), 'staged'); + selection = selection.selectNextItem(); + assert.strictEqual(selection.getActiveListKey(), 'staged'); assertEqualSets(selection.getSelectedItems(), new Set(['e'])); - assert.isFalse(selection.selectNextItem()); - assert.equal(selection.getActiveListKey(), 'staged'); + selection = selection.selectNextItem(); + assert.strictEqual(selection.getActiveListKey(), 'staged'); assertEqualSets(selection.getSelectedItems(), new Set(['e'])); - assert.isTrue(selection.selectPreviousItem()); - assert.equal(selection.getActiveListKey(), 'staged'); + selection = selection.selectPreviousItem(); + assert.strictEqual(selection.getActiveListKey(), 'staged'); assertEqualSets(selection.getSelectedItems(), new Set(['d'])); - assert.isTrue(selection.selectPreviousItem()); - assert.equal(selection.getActiveListKey(), 'conflicts'); + selection = selection.selectPreviousItem(); + assert.strictEqual(selection.getActiveListKey(), 'conflicts'); assertEqualSets(selection.getSelectedItems(), new Set(['c'])); - assert.isTrue(selection.selectPreviousItem()); - assert.equal(selection.getActiveListKey(), 'unstaged'); + selection = selection.selectPreviousItem(); + assert.strictEqual(selection.getActiveListKey(), 'unstaged'); assertEqualSets(selection.getSelectedItems(), new Set(['b'])); - assert.isTrue(selection.selectPreviousItem()); - assert.equal(selection.getActiveListKey(), 'unstaged'); + selection = selection.selectPreviousItem(); + assert.strictEqual(selection.getActiveListKey(), 'unstaged'); assertEqualSets(selection.getSelectedItems(), new Set(['a'])); - assert.isFalse(selection.selectPreviousItem()); - assert.equal(selection.getActiveListKey(), 'unstaged'); + selection = selection.selectPreviousItem(); + assert.strictEqual(selection.getActiveListKey(), 'unstaged'); assertEqualSets(selection.getSelectedItems(), new Set(['a'])); }); it('allows the selection to be expanded to the next or previous item', function() { - const selection = new CompositeListSelection({ - listsByKey: { - unstaged: ['a', 'b'], - conflicts: ['c'], - staged: ['d', 'e'], - }, + let selection = new CompositeListSelection({ + listsByKey: [ + ['unstaged', ['a', 'b']], + ['conflicts', ['c']], + ['staged', ['d', 'e']], + ], }); - assert.equal(selection.getActiveListKey(), 'unstaged'); + assert.strictEqual(selection.getActiveListKey(), 'unstaged'); assertEqualSets(selection.getSelectedItems(), new Set(['a'])); - selection.selectNextItem(true); - assert.equal(selection.getActiveListKey(), 'unstaged'); + selection = selection.selectNextItem(true); + assert.strictEqual(selection.getActiveListKey(), 'unstaged'); assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b'])); // Does not expand selections across lists - selection.selectNextItem(true); - assert.equal(selection.getActiveListKey(), 'unstaged'); + selection = selection.selectNextItem(true); + assert.strictEqual(selection.getActiveListKey(), 'unstaged'); assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b'])); - selection.selectItem('e'); - selection.selectPreviousItem(true); - selection.selectPreviousItem(true); - assert.equal(selection.getActiveListKey(), 'staged'); + selection = selection.selectItem('e'); + selection = selection.selectPreviousItem(true); + selection = selection.selectPreviousItem(true); + assert.strictEqual(selection.getActiveListKey(), 'staged'); assertEqualSets(selection.getSelectedItems(), new Set(['d', 'e'])); }); it('skips empty lists when selecting the next or previous item', function() { - const selection = new CompositeListSelection({ - listsByKey: { - unstaged: ['a', 'b'], - conflicts: [], - staged: ['d', 'e'], - }, + let selection = new CompositeListSelection({ + listsByKey: [ + ['unstaged', ['a', 'b']], + ['conflicts', []], + ['staged', ['d', 'e']], + ], }); - selection.selectNextItem(); - selection.selectNextItem(); - assert.equal(selection.getActiveListKey(), 'staged'); + selection = selection.selectNextItem(); + selection = selection.selectNextItem(); + assert.strictEqual(selection.getActiveListKey(), 'staged'); assertEqualSets(selection.getSelectedItems(), new Set(['d'])); - selection.selectPreviousItem(); - assert.equal(selection.getActiveListKey(), 'unstaged'); + selection = selection.selectPreviousItem(); + assert.strictEqual(selection.getActiveListKey(), 'unstaged'); assertEqualSets(selection.getSelectedItems(), new Set(['b'])); }); it('collapses the selection when moving down with the next list empty or up with the previous list empty', function() { - const selection = new CompositeListSelection({ - listsByKey: { - unstaged: ['a', 'b'], - conflicts: [], - staged: [], - }, + let selection = new CompositeListSelection({ + listsByKey: [ + ['unstaged', ['a', 'b']], + ['conflicts', []], + ['staged', []], + ], }); - selection.selectNextItem(true); + selection = selection.selectNextItem(true); assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b'])); - selection.selectNextItem(); + selection = selection.selectNextItem(); assertEqualSets(selection.getSelectedItems(), new Set(['b'])); - selection.updateLists({ - unstaged: [], - conflicts: [], - staged: ['a', 'b'], - }); + selection.updateLists([ + ['unstaged', []], + ['conflicts', []], + ['staged', ['a', 'b']], + ]); - selection.selectNextItem(); - selection.selectPreviousItem(true); + selection = selection.selectNextItem(); + selection = selection.selectPreviousItem(true); assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b'])); - selection.selectPreviousItem(); + selection = selection.selectPreviousItem(); assertEqualSets(selection.getSelectedItems(), new Set(['a'])); }); it('allows selections to be added in the current active list, but updates the existing selection when activating a different list', function() { - const selection = new CompositeListSelection({ - listsByKey: { - unstaged: ['a', 'b', 'c'], - conflicts: [], - staged: ['e', 'f', 'g'], - }, + let selection = new CompositeListSelection({ + listsByKey: [ + ['unstaged', ['a', 'b', 'c']], + ['conflicts', []], + ['staged', ['e', 'f', 'g']], + ], }); - selection.addOrSubtractSelection('c'); + selection = selection.addOrSubtractSelection('c'); assertEqualSets(selection.getSelectedItems(), new Set(['a', 'c'])); - selection.addOrSubtractSelection('g'); + selection = selection.addOrSubtractSelection('g'); assertEqualSets(selection.getSelectedItems(), new Set(['g'])); }); it('allows all items in the active list to be selected', function() { - const selection = new CompositeListSelection({ - listsByKey: { - unstaged: ['a', 'b', 'c'], - conflicts: [], - staged: ['e', 'f', 'g'], - }, + let selection = new CompositeListSelection({ + listsByKey: [ + ['unstaged', ['a', 'b', 'c']], + ['conflicts', []], + ['staged', ['e', 'f', 'g']], + ], }); - selection.selectAllItems(); + selection = selection.selectAllItems(); assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b', 'c'])); - selection.activateNextSelection(); - selection.selectAllItems(); + selection = selection.activateNextSelection(); + selection = selection.selectAllItems(); assertEqualSets(selection.getSelectedItems(), new Set(['e', 'f', 'g'])); }); it('allows the first or last item in the active list to be selected', function() { - const selection = new CompositeListSelection({ - listsByKey: { - unstaged: ['a', 'b', 'c'], - conflicts: [], - staged: ['e', 'f', 'g'], - }, + let selection = new CompositeListSelection({ + listsByKey: [ + ['unstaged', ['a', 'b', 'c']], + ['conflicts', []], + ['staged', ['e', 'f', 'g']], + ], }); - selection.activateNextSelection(); - selection.selectLastItem(); + selection = selection.activateNextSelection(); + selection = selection.selectLastItem(); assertEqualSets(selection.getSelectedItems(), new Set(['g'])); - selection.selectFirstItem(); + selection = selection.selectFirstItem(); assertEqualSets(selection.getSelectedItems(), new Set(['e'])); - selection.selectLastItem(true); + selection = selection.selectLastItem(true); assertEqualSets(selection.getSelectedItems(), new Set(['e', 'f', 'g'])); - selection.selectNextItem(); + selection = selection.selectNextItem(); assertEqualSets(selection.getSelectedItems(), new Set(['g'])); - selection.selectFirstItem(true); + selection = selection.selectFirstItem(true); assertEqualSets(selection.getSelectedItems(), new Set(['e', 'f', 'g'])); }); it('allows the last non-empty selection to be chosen', function() { - const selection = new CompositeListSelection({ - listsByKey: { - unstaged: ['a', 'b', 'c'], - conflicts: ['e', 'f'], - staged: [], - }, + let selection = new CompositeListSelection({ + listsByKey: [ + ['unstaged', ['a', 'b', 'c']], + ['conflicts', ['e', 'f']], + ['staged', []], + ], }); - assert.isTrue(selection.activateLastSelection()); + selection = selection.activateLastSelection(); assertEqualSets(selection.getSelectedItems(), new Set(['e'])); }); }); describe('updateLists(listsByKey)', function() { it('keeps the selection head of each list pointed to an item with the same id', function() { - let listsByKey = { - unstaged: [{filePath: 'a'}, {filePath: 'b'}], - conflicts: [{filePath: 'c'}], - staged: [{filePath: 'd'}, {filePath: 'e'}, {filePath: 'f'}], - }; - const selection = new CompositeListSelection({ + let listsByKey = [ + ['unstaged', [{filePath: 'a'}, {filePath: 'b'}]], + ['conflicts', [{filePath: 'c'}]], + ['staged', [{filePath: 'd'}, {filePath: 'e'}, {filePath: 'f'}]], + ]; + let selection = new CompositeListSelection({ listsByKey, idForItem: item => item.filePath, }); - selection.selectItem(listsByKey.unstaged[1]); - selection.selectItem(listsByKey.staged[1]); - selection.selectItem(listsByKey.staged[2], true); + selection = selection.selectItem(listsByKey[0][1][1]); + selection = selection.selectItem(listsByKey[2][1][1]); + selection = selection.selectItem(listsByKey[2][1][2], true); - listsByKey = { - unstaged: [{filePath: 'a'}, {filePath: 'q'}, {filePath: 'b'}, {filePath: 'r'}], - conflicts: [{filePath: 's'}, {filePath: 'c'}], - staged: [{filePath: 'd'}, {filePath: 't'}, {filePath: 'e'}, {filePath: 'f'}], - }; + listsByKey = [ + ['unstaged', [{filePath: 'a'}, {filePath: 'q'}, {filePath: 'b'}, {filePath: 'r'}]], + ['conflicts', [{filePath: 's'}, {filePath: 'c'}]], + ['staged', [{filePath: 'd'}, {filePath: 't'}, {filePath: 'e'}, {filePath: 'f'}]], + ]; - selection.updateLists(listsByKey); + selection = selection.updateLists(listsByKey); - assert.equal(selection.getActiveListKey(), 'staged'); - assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.staged[3]])); + assert.strictEqual(selection.getActiveListKey(), 'staged'); + assertEqualSets(selection.getSelectedItems(), new Set([listsByKey[2][1][3]])); - selection.activatePreviousSelection(); - assert.equal(selection.getActiveListKey(), 'conflicts'); - assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.conflicts[1]])); + selection = selection.activatePreviousSelection(); + assert.strictEqual(selection.getActiveListKey(), 'conflicts'); + assertEqualSets(selection.getSelectedItems(), new Set([listsByKey[1][1][1]])); - selection.activatePreviousSelection(); - assert.equal(selection.getActiveListKey(), 'unstaged'); - assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.unstaged[2]])); + selection = selection.activatePreviousSelection(); + assert.strictEqual(selection.getActiveListKey(), 'unstaged'); + assertEqualSets(selection.getSelectedItems(), new Set([listsByKey[0][1][2]])); }); it('collapses to the start of the previous selection if the old head item is removed', function() { - let listsByKey = { - unstaged: [{filePath: 'a'}, {filePath: 'b'}, {filePath: 'c'}], - conflicts: [], - staged: [{filePath: 'd'}, {filePath: 'e'}, {filePath: 'f'}], - }; - const selection = new CompositeListSelection({ + let listsByKey = [ + ['unstaged', [{filePath: 'a'}, {filePath: 'b'}, {filePath: 'c'}]], + ['conflicts', []], + ['staged', [{filePath: 'd'}, {filePath: 'e'}, {filePath: 'f'}]], + ]; + let selection = new CompositeListSelection({ listsByKey, idForItem: item => item.filePath, }); - selection.selectItem(listsByKey.unstaged[1]); - selection.selectItem(listsByKey.unstaged[2], true); - selection.selectItem(listsByKey.staged[1]); + selection = selection.selectItem(listsByKey[0][1][1]); + selection = selection.selectItem(listsByKey[0][1][2], true); + selection = selection.selectItem(listsByKey[2][1][1]); - listsByKey = { - unstaged: [{filePath: 'a'}], - conflicts: [], - staged: [{filePath: 'd'}, {filePath: 'f'}], - }; - selection.updateLists(listsByKey); + listsByKey = [ + ['unstaged', [{filePath: 'a'}]], + ['conflicts', []], + ['staged', [{filePath: 'd'}, {filePath: 'f'}]], + ]; + selection = selection.updateLists(listsByKey); - assert.equal(selection.getActiveListKey(), 'staged'); - assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.staged[1]])); + assert.strictEqual(selection.getActiveListKey(), 'staged'); + assertEqualSets(selection.getSelectedItems(), new Set([listsByKey[2][1][1]])); - selection.activatePreviousSelection(); - assert.equal(selection.getActiveListKey(), 'unstaged'); - assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.unstaged[0]])); + selection = selection.activatePreviousSelection(); + assert.strictEqual(selection.getActiveListKey(), 'unstaged'); + assertEqualSets(selection.getSelectedItems(), new Set([listsByKey[0][1][0]])); }); it('activates the first non-empty list following or preceding the current active list if one exists', function() { - const selection = new CompositeListSelection({ - listsByKey: { - unstaged: ['a', 'b'], - conflicts: [], - staged: [], - }, - }); - - selection.updateLists({ - unstaged: [], - conflicts: [], - staged: ['a', 'b'], + let selection = new CompositeListSelection({ + listsByKey: [ + ['unstaged', ['a', 'b']], + ['conflicts', []], + ['staged', []], + ], }); - assert.equal(selection.getActiveListKey(), 'staged'); - selection.updateLists({ - unstaged: ['a', 'b'], - conflicts: [], - staged: [], - }); - assert.equal(selection.getActiveListKey(), 'unstaged'); + selection = selection.updateLists([ + ['unstaged', []], + ['conflicts', []], + ['staged', ['a', 'b']], + ]); + assert.strictEqual(selection.getActiveListKey(), 'staged'); + + selection = selection.updateLists([ + ['unstaged', ['a', 'b']], + ['conflicts', []], + ['staged', []], + ]); + assert.strictEqual(selection.getActiveListKey(), 'unstaged'); }); }); }); diff --git a/test/models/list-selection.test.js b/test/models/list-selection.test.js index 0bc9dc956b..9394883cb4 100644 --- a/test/models/list-selection.test.js +++ b/test/models/list-selection.test.js @@ -21,7 +21,7 @@ describe('ListSelection', function() { describe('selectItem', () => { // https://github.com/atom/github/issues/467 - it.only('selects an item when there are no selections', () => { + it('selects an item when there are no selections', () => { let selection = new ListSelection({items: ['a', 'b', 'c']}); selection = selection.addOrSubtractSelection('a'); selection = selection.coalesce(); From d40fcabf564f558ae9891bf67e0ab0015157b5d7 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 25 Apr 2018 22:00:39 -0400 Subject: [PATCH 4/5] Update FilePatchSelection now that ListSelection is immutable --- lib/models/file-patch-selection.js | 134 ++++++++++++----------- lib/models/list-selection.js | 5 +- lib/views/staging-view.js | 10 +- test/models/file-patch-selection.test.js | 1 - 4 files changed, 80 insertions(+), 70 deletions(-) diff --git a/lib/models/file-patch-selection.js b/lib/models/file-patch-selection.js index 2b0063005f..091bd27fd8 100644 --- a/lib/models/file-patch-selection.js +++ b/lib/models/file-patch-selection.js @@ -1,6 +1,6 @@ import ListSelection from './list-selection'; -const COPY = {}; +const COPY = Symbol('Copy'); export default class FilePatchSelection { constructor(hunks) { @@ -38,8 +38,8 @@ export default class FilePatchSelection { copy(options = {}) { const mode = options.mode || this.mode; - const hunksSelection = options.hunksSelection || this.hunksSelection.copy(); - const linesSelection = options.linesSelection || this.linesSelection.copy(); + let hunksSelection = options.hunksSelection || this.hunksSelection; + let linesSelection = options.linesSelection || this.linesSelection; let hunksByLine = null; if (options.hunks) { @@ -60,7 +60,7 @@ export default class FilePatchSelection { } // Update hunks, preserving selection index - hunksSelection.setItems(newHunks); + hunksSelection = hunksSelection.setItems(newHunks); const oldLines = this.linesSelection.getItems(); const newLines = []; @@ -92,9 +92,13 @@ export default class FilePatchSelection { } } - linesSelection.setItems(newLines); - if (newSelectedLine) { linesSelection.selectItem(newSelectedLine); } - if (wasChanged) { this.resolveNextUpdatePromise(); } + linesSelection = linesSelection.setItems(newLines); + if (newSelectedLine) { + linesSelection = linesSelection.selectItem(newSelectedLine); + } + if (wasChanged) { + this.resolveNextUpdatePromise(); + } } else { // Hunks are unchanged. Don't recompute hunksByLine. hunksByLine = this.hunksByLine; @@ -171,38 +175,38 @@ export default class FilePatchSelection { } selectHunk(hunk, preserveTail = false) { - const hunksSelection = this.hunksSelection.copy(); - hunksSelection.selectItem(hunk, preserveTail); - - return this.copy({mode: 'hunk', hunksSelection}); + return this.copy({ + mode: 'hunk', + hunksSelection: this.hunksSelection.selectItem(hunk, preserveTail), + }); } addOrSubtractHunkSelection(hunk) { - const hunksSelection = this.hunksSelection.copy(); - hunksSelection.addOrSubtractSelection(hunk); - - return this.copy({mode: 'hunk', hunksSelection}); + return this.copy({ + mode: 'hunk', + hunksSelection: this.hunksSelection.addOrSubtractSelection(hunk), + }); } selectAllHunks() { - const hunksSelection = this.hunksSelection.copy(); - hunksSelection.selectAllItems(); - - return this.copy({mode: 'hunk', hunksSelection}); + return this.copy({ + mode: 'hunk', + hunksSelection: this.hunksSelection.selectAllItems(), + }); } selectFirstHunk(preserveTail) { - const hunksSelection = this.hunksSelection.copy(); - hunksSelection.selectFirstItem(preserveTail); - - return this.copy({mode: 'hunk', hunksSelection}); + return this.copy({ + mode: 'hunk', + hunksSelection: this.hunksSelection.selectFirstItem(preserveTail), + }); } selectLastHunk(preserveTail) { - const hunksSelection = this.hunksSelection.copy(); - hunksSelection.selectLastItem(preserveTail); - - return this.copy({mode: 'hunk', hunksSelection}); + return this.copy({ + mode: 'hunk', + hunksSelection: this.hunksSelection.selectLastItem(preserveTail), + }); } jumpToNextHunk() { @@ -216,17 +220,17 @@ export default class FilePatchSelection { } selectNextHunk(preserveTail) { - const hunksSelection = this.hunksSelection.copy(); - hunksSelection.selectNextItem(preserveTail); - - return this.copy({mode: 'hunk', hunksSelection}); + return this.copy({ + mode: 'hunk', + hunksSelection: this.hunksSelection.selectNextItem(preserveTail), + }); } selectPreviousHunk(preserveTail) { - const hunksSelection = this.hunksSelection.copy(); - hunksSelection.selectPreviousItem(preserveTail); - - return this.copy({mode: 'hunk', hunksSelection}); + return this.copy({ + mode: 'hunk', + hunksSelection: this.hunksSelection.selectPreviousItem(preserveTail), + }); } getSelectedHunks() { @@ -249,45 +253,52 @@ export default class FilePatchSelection { } selectLine(line, preserveTail = false) { - const linesSelection = this.linesSelection.copy(); - linesSelection.selectItem(line, preserveTail); - return this.copy({mode: 'line', linesSelection}); + return this.copy({ + mode: 'line', + linesSelection: this.linesSelection.selectItem(line, preserveTail), + }); } addOrSubtractLineSelection(line) { - const linesSelection = this.linesSelection.copy(); - linesSelection.addOrSubtractSelection(line); - return this.copy({mode: 'line', linesSelection}); + return this.copy({ + mode: 'line', + linesSelection: this.linesSelection.addOrSubtractSelection(line), + }); } selectAllLines(preserveTail) { - const linesSelection = this.linesSelection.copy(); - linesSelection.selectAllItems(preserveTail); - return this.copy({mode: 'line', linesSelection}); + return this.copy({ + mode: 'line', + linesSelection: this.linesSelection.selectAllItems(preserveTail), + }); } selectFirstLine(preserveTail) { - const linesSelection = this.linesSelection.copy(); - linesSelection.selectFirstItem(preserveTail); - return this.copy({mode: 'line', linesSelection}); + return this.copy({ + mode: 'line', + linesSelection: this.linesSelection.selectFirstItem(preserveTail), + }); } selectLastLine(preserveTail) { - const linesSelection = this.linesSelection.copy(); - linesSelection.selectLastItem(preserveTail); - return this.copy({mode: 'line', linesSelection}); + return this.copy({ + mode: 'line', + linesSelection: this.linesSelection.selectLastItem(preserveTail), + }); } selectNextLine(preserveTail = false) { - const linesSelection = this.linesSelection.copy(); - linesSelection.selectNextItem(preserveTail); - return this.copy({mode: 'line', linesSelection}); + return this.copy({ + mode: 'line', + linesSelection: this.linesSelection.selectNextItem(preserveTail), + }); } selectPreviousLine(preserveTail = false) { - const linesSelection = this.linesSelection.copy(); - linesSelection.selectPreviousItem(preserveTail); - return this.copy({mode: 'line', linesSelection}); + return this.copy({ + mode: 'line', + linesSelection: this.linesSelection.selectPreviousItem(preserveTail), + }); } getSelectedLines() { @@ -313,13 +324,10 @@ export default class FilePatchSelection { } coalesce() { - const hunksSelection = this.hunksSelection.copy(); - const linesSelection = this.linesSelection.copy(); - - hunksSelection.coalesce(); - linesSelection.coalesce(); - - return this.copy({hunksSelection, linesSelection}); + return this.copy({ + hunksSelection: this.hunksSelection.coalesce(), + linesSelection: this.linesSelection.coalesce(), + }); } getNextUpdatePromise() { diff --git a/lib/models/list-selection.js b/lib/models/list-selection.js index feba3e326f..8792a06dc5 100644 --- a/lib/models/list-selection.js +++ b/lib/models/list-selection.js @@ -123,7 +123,10 @@ export default class ListSelection { const itemIndex = this.items.indexOf(item); if (preserveTail && this.selections[0]) { - const newSelections = [{head: itemIndex, tail: this.selections[0].tail}, ...this.selections.slice(1)]; + const newSelections = [ + {head: itemIndex, tail: this.selections[0].tail, negate: this.selections[0].negate}, + ...this.selections.slice(1), + ]; return this.copy({selections: newSelections}); } else { const selection = {head: itemIndex, tail: itemIndex}; diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index b0d09f1748..ecdfff3338 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -55,11 +55,11 @@ export default class StagingView { this.listElementsByItem = new WeakMap(); this.selection = new CompositeListSelection({ - listsByKey: { - unstaged: this.props.unstagedChanges, - conflicts: this.props.mergeConflicts || [], - staged: this.props.stagedChanges, - }, + listsByKey: [ + ['unstaged', this.props.unstagedChanges], + ['conflicts', this.props.mergeConflicts || []], + ['staged', this.props.stagedChanges], + ], idForItem: item => item.filePath, }); diff --git a/test/models/file-patch-selection.test.js b/test/models/file-patch-selection.test.js index 93a855606b..31ff38e420 100644 --- a/test/models/file-patch-selection.test.js +++ b/test/models/file-patch-selection.test.js @@ -114,7 +114,6 @@ describe('FilePatchSelection', function() { const selection0 = new FilePatchSelection(hunks) .selectLine(hunks[0].lines[2]) .selectLine(hunks[1].lines[2], true); - assertEqualSets(selection0.getSelectedLines(), new Set([ hunks[0].lines[2], hunks[1].lines[1], From 9ca23b00a07b3130c5d112efe774710050ce4080 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 25 Apr 2018 22:11:17 -0400 Subject: [PATCH 5/5] Update the StagingView's usage of its CompositeListSelection --- lib/views/staging-view.js | 54 +++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index ecdfff3338..da7effb588 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -135,11 +135,11 @@ export default class StagingView { }); const previouslySelectedItems = this.selection.getSelectedItems(); - this.selection.updateLists({ - unstaged: this.props.unstagedChanges, - conflicts: this.props.mergeConflicts || [], - staged: this.props.stagedChanges, - }); + this.selection = this.selection.updateLists([ + ['unstaged', this.props.unstagedChanges], + ['conflicts', this.props.mergeConflicts || []], + ['staged', this.props.stagedChanges], + ]); const currentlySelectedItems = this.selection.getSelectedItems(); if (this.props.resolutionProgress !== oldProps.resolutionProgress) { @@ -185,11 +185,12 @@ export default class StagingView { @autobind activateNextList() { - if (!this.selection.activateNextSelection()) { + const next = this.selection.activateNextSelection(); + if (this.selection === next) { return false; } + this.selection = next.coalesce(); - this.selection.coalesce(); this.debouncedDidChangeSelectedItem(); etch.update(this); return true; @@ -197,22 +198,24 @@ export default class StagingView { @autobind activatePreviousList() { - if (!this.selection.activatePreviousSelection()) { + const next = this.selection.activatePreviousSelection(); + if (this.selection === next) { return false; } - this.selection.coalesce(); + this.selection = next.coalesce(); this.debouncedDidChangeSelectedItem(); etch.update(this); return true; } activateLastList() { - if (!this.selection.activateLastSelection()) { + const next = this.selection.activateLastSelection(); + if (this.selection === next) { return false; } - this.selection.coalesce(); + this.selection = next.coalesce(); this.debouncedDidChangeSelectedItem(); etch.update(this); return true; @@ -247,7 +250,7 @@ export default class StagingView { async confirmSelectedItems() { const itemPaths = Array.from(this.selection.getSelectedItems()).map(item => item.filePath); await this.props.attemptFileStageOperation(itemPaths, this.selection.getActiveListKey()); - this.selection.coalesce(); + this.selection = this.selection.coalesce(); this.debouncedDidChangeSelectedItem(); return etch.update(this); } @@ -257,35 +260,30 @@ export default class StagingView { } selectPrevious(preserveTail = false) { - this.selection.selectPreviousItem(preserveTail); - this.selection.coalesce(); + this.selection = this.selection.selectPreviousItem(preserveTail).coalesce(); if (!preserveTail) { this.debouncedDidChangeSelectedItem(); } return etch.update(this); } selectNext(preserveTail = false) { - this.selection.selectNextItem(preserveTail); - this.selection.coalesce(); + this.selection = this.selection.selectNextItem(preserveTail).coalesce(); if (!preserveTail) { this.debouncedDidChangeSelectedItem(); } return etch.update(this); } selectAll() { - this.selection.selectAllItems(); - this.selection.coalesce(); + this.selection = this.selection.selectAllItems().coalesce(); return etch.update(this); } selectFirst(preserveTail = false) { - this.selection.selectFirstItem(preserveTail); - this.selection.coalesce(); + this.selection = this.selection.selectFirstItem(preserveTail).coalesce(); if (!preserveTail) { this.debouncedDidChangeSelectedItem(); } return etch.update(this); } selectLast(preserveTail = false) { - this.selection.selectLastItem(preserveTail); - this.selection.coalesce(); + this.selection = this.selection.selectLastItem(preserveTail).coalesce(); if (!preserveTail) { this.debouncedDidChangeSelectedItem(); } return etch.update(this); } @@ -373,7 +371,7 @@ export default class StagingView { return null; } - this.selection.selectItem(item); + this.selection = this.selection.selectItem(item); return etch.update(this); } @@ -494,7 +492,7 @@ export default class StagingView { async contextMenuOnItem(event, item) { if (!this.selection.getSelectedItems().has(item)) { event.stopPropagation(); - this.selection.selectItem(item, event.shiftKey); + this.selection = this.selection.selectItem(item, event.shiftKey); await etch.update(this); const newEvent = new MouseEvent(event.type, event); requestAnimationFrame(() => { @@ -511,9 +509,9 @@ export default class StagingView { this.mouseSelectionInProgress = true; this.selectionChanged = true; if (event.metaKey || (event.ctrlKey && windows)) { - this.selection.addOrSubtractSelection(item); + this.selection = this.selection.addOrSubtractSelection(item); } else { - this.selection.selectItem(item, event.shiftKey); + this.selection = this.selection.selectItem(item, event.shiftKey); } await etch.update(this); } @@ -523,14 +521,14 @@ export default class StagingView { async mousemoveOnItem(event, item) { if (this.mouseSelectionInProgress) { this.selectionChanged = true; - this.selection.selectItem(item, true); + this.selection = this.selection.selectItem(item, true); await etch.update(this); } } @autobind mouseup() { - this.selection.coalesce(); + this.selection = this.selection.coalesce(); if (this.selectionChanged) { this.didChangeSelectedItems(true); } this.mouseSelectionInProgress = false; this.selectionChanged = false;