From 46a906ebf3f0923599fa7d11bec29573a721783e Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Wed, 19 Oct 2016 09:49:13 -0600 Subject: [PATCH 01/25] Extract SelectionList from FilePatchSelection so we can reuse the logic --- lib/views/file-patch-selection.js | 312 +++++++----------------- lib/views/list-selection.js | 196 +++++++++++++++ test/views/file-patch-selection.test.js | 4 +- 3 files changed, 292 insertions(+), 220 deletions(-) create mode 100644 lib/views/list-selection.js diff --git a/lib/views/file-patch-selection.js b/lib/views/file-patch-selection.js index c9ff427117..7b018eba1c 100644 --- a/lib/views/file-patch-selection.js +++ b/lib/views/file-patch-selection.js @@ -1,21 +1,22 @@ /** @babel */ +import ListSelection from './list-selection' + export default class FilePatchSelection { constructor (hunks) { this.mode = 'hunk' - this.selections = [{head: 0, tail: 0}] + this.hunksSelection = new ListSelection() + this.linesSelection = new ListSelection({isItemSelectable: (line) => line.isChanged()}) this.updateHunks(hunks) } toggleMode () { if (this.mode === 'hunk') { - this.mode = 'line' - const firstLineOfSelectedHunk = this.hunks[this.selections[0].head].lines[0] + const firstLineOfSelectedHunk = this.getHeadHunk().lines[0] this.selectLine(firstLineOfSelectedHunk) if (!firstLineOfSelectedHunk.isChanged()) this.selectNextLine() } else { - this.mode = 'hunk' - const selectedLine = this.lines[this.selections[0].head] + const selectedLine = this.getHeadLine() const hunkContainingSelectedLine = this.hunksByLine.get(selectedLine) this.selectHunk(hunkContainingSelectedLine) } @@ -65,199 +66,91 @@ export default class FilePatchSelection { } } - selectNextHunk (preserveTail) { - let nextHunkIndex = this.selections[0].head - if (nextHunkIndex < this.hunks.length - 1) nextHunkIndex++ - this.selectHunk(this.hunks[nextHunkIndex], preserveTail) - } - - selectPreviousHunk (preserveTail) { - let previousHunkIndex = this.selections[0].head - if (previousHunkIndex > 0) previousHunkIndex-- - this.selectHunk(this.hunks[previousHunkIndex], preserveTail) - } - - selectNextLine (preserveTail = false) { - if (this.selections.length === 0) { - this.selectFirstLine() - return - } - - let lineIndex = this.selections[0].head - let nextLineIndex = lineIndex - - while (lineIndex < this.lines.length - 1) { - lineIndex++ - if (this.lines[lineIndex].isChanged()) { - nextLineIndex = lineIndex - break - } - } - - this.selectLine(this.lines[nextLineIndex], preserveTail) + selectHunk (hunk, preserveTail = false) { + this.mode = 'hunk' + this.hunksSelection.selectItem(hunk, preserveTail) } - selectPreviousLine (preserveTail = false) { - if (this.selections.length === 0) { - this.selectLastLine() - return - } - - let lineIndex = this.selections[0].head - let previousLineIndex = lineIndex - - while (lineIndex > 0) { - lineIndex-- - if (this.lines[lineIndex].isChanged()) { - previousLineIndex = lineIndex - break - } - } - - this.selectLine(this.lines[previousLineIndex], preserveTail) + addOrSubtractHunkSelection (hunk) { + this.mode = 'hunk' + this.hunksSelection.addOrSubtractSelection(hunk) } selectAllHunks () { - this.selectFirstHunk() - this.selectLastHunk(true) + this.mode = 'hunk' + this.hunksSelection.selectAllItems() } selectFirstHunk (preserveTail) { - this.selectHunk(this.hunks[0], preserveTail) + this.mode = 'hunk' + this.hunksSelection.selectFirstItem(preserveTail) } selectLastHunk (preserveTail) { - this.selectHunk(this.hunks[this.hunks.length - 1], preserveTail) + this.mode = 'hunk' + this.hunksSelection.selectLastItem(preserveTail) } - selectAllLines () { - this.selectFirstLine() - this.selectLastLine(true) + selectNextHunk (preserveTail) { + this.mode = 'hunk' + this.hunksSelection.selectNextItem(preserveTail) } - selectFirstLine (preserveTail) { - for (let i = 0; i < this.lines.length; i++) { - const line = this.lines[i] - if (line.isChanged()) { - this.selectLine(line, preserveTail) - break - } - } + selectPreviousHunk (preserveTail) { + this.mode = 'hunk' + this.hunksSelection.selectPreviousItem(preserveTail) } - selectLastLine (preserveTail) { - for (let i = this.lines.length - 1; i > 0; i--) { - const line = this.lines[i] - if (line.isChanged()) { - this.selectLine(line, preserveTail) - break - } + getSelectedHunks (hunk) { + if (this.mode === 'line') { + const selectedHunks = new Set() + const selectedLines = this.getSelectedLines() + selectedLines.forEach(line => selectedHunks.add(this.hunksByLine.get(line))) + return selectedHunks + } else { + return this.hunksSelection.getSelectedItems() } } - selectHunk (hunk, preserveTail = false) { - this.mode = 'hunk' - this.selectItem(this.hunks, hunk, preserveTail, false) + getHeadHunk () { + if (this.mode === 'hunk') { + return this.hunksSelection.getHeadItem() + } } selectLine (line, preserveTail = false) { this.mode = 'line' - this.selectItem(this.lines, line, preserveTail, false) + this.linesSelection.selectItem(line, preserveTail) } - coalesce () { - if (this.selections.length === 0) return - - const mostRecent = this.selections[0] - let mostRecentStart = Math.min(mostRecent.head, mostRecent.tail) - let mostRecentEnd = Math.max(mostRecent.head, mostRecent.tail) - while (mostRecentStart > 0 && !this.lines[mostRecentStart - 1].isChanged()) { - mostRecentStart-- - } - while (mostRecentEnd < (this.lines.length - 1) && !this.lines[mostRecentEnd + 1].isChanged()) { - mostRecentEnd++ - } - - 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}) - } - if (currentStart < mostRecentStart) { // prefix - truncatedSelections.push({tail: currentStart, head: mostRecentStart - 1}) - } - } else { - if (currentStart < mostRecentStart) { // prefix - truncatedSelections.push({head: currentStart, tail: mostRecentStart - 1}) - } - if (currentEnd > mostRecentEnd) { // suffix - truncatedSelections.push({head: mostRecentEnd + 1, tail: currentEnd}) - } - } - this.selections.splice(i, 1, ...truncatedSelections) - i += truncatedSelections.length - } else { - if (mostRecent.head > mostRecent.tail) { - mostRecent.head = Math.max(mostRecentEnd, currentEnd) - mostRecent.tail = Math.min(mostRecentStart, currentStart) - } else { - mostRecent.head = Math.min(mostRecentStart, currentStart) - mostRecent.tail = Math.max(mostRecentEnd, currentEnd) - } - this.selections.splice(i, 1) - } - } else { - i++ - } - } - - if (mostRecent.negate) this.selections.shift() + addOrSubtractLineSelection (line) { + this.mode = 'line' + this.linesSelection.addOrSubtractSelection(line) } - addHunkSelection (hunk) { - this.mode = 'hunk' - this.selectItem(this.hunks, hunk, false, true) + selectAllLines (preserveTail) { + this.mode = 'line' + this.linesSelection.selectAllItems(preserveTail) } - addOrSubtractLineSelection (line) { + selectFirstLine (preserveTail) { this.mode = 'line' - this.selectItem(this.lines, line, false, true) + this.linesSelection.selectFirstItem(preserveTail) } - selectItem (items, item, preserveTail, addOrSubtract) { - if (addOrSubtract && preserveTail) { - throw new Error('addOrSubtract and preserveTail cannot both be true at the same time') - } + selectLastLine (preserveTail) { + this.mode = 'line' + this.linesSelection.selectLastItem(preserveTail) + } - const itemIndex = items.indexOf(item) - if (preserveTail) { - this.selections[0].head = itemIndex - } else { - const selection = {head: itemIndex, tail: itemIndex} - if (addOrSubtract) { - if (this.getSelectedItems().has(item)) selection.negate = true - this.selections.unshift(selection) - } else { - this.selections = [selection] - } - } + selectNextLine (preserveTail = false) { + this.mode = 'line' + this.linesSelection.selectNextItem(preserveTail) } - getSelectedHunks (hunk) { - if (this.mode === 'line') { - const selectedHunks = new Set() - const selectedLines = this.getSelectedLines() - selectedLines.forEach(line => selectedHunks.add(this.hunksByLine.get(line))) - return selectedHunks - } else { - return this.getSelectedItems() - } + selectPreviousLine (preserveTail = false) { + this.mode = 'line' + this.linesSelection.selectPreviousItem(preserveTail) } getSelectedLines () { @@ -270,78 +163,61 @@ export default class FilePatchSelection { }) return selectedLines } else { - return this.getSelectedItems() - } - } - - getHeadHunk () { - if (this.mode === 'hunk' && this.selections.length > 0) { - return this.hunks[this.selections[0].head] + return this.linesSelection.getSelectedItems() } } getHeadLine () { - if (this.mode === 'line' && this.selections.length > 0) { - return this.lines[this.selections[0].head] - } - } - - getSelectedItems () { - const selectedItems = new Set() - const items = (this.mode === 'hunk') ? this.hunks : this.lines - for (let {head, tail, negate} of this.selections.slice().reverse()) { - let start = Math.min(head, tail) - let end = Math.max(head, tail) - for (let i = start; i <= end; i++) { - const item = items[i] - if (this.mode === 'hunk' || item.isChanged()) { - if (negate) { - selectedItems.delete(item) - } else { - selectedItems.add(item) - } - } - } + if (this.mode === 'line') { + return this.linesSelection.getHeadItem() } - return selectedItems } - updateHunks (hunks) { - const oldLines = this.lines - this.hunks = hunks - this.lines = [] + updateHunks (newHunks) { this.hunksByLine = new Map() - for (let hunk of hunks) { + const newLines = [] + for (let hunk of newHunks) { for (let line of hunk.lines) { - this.lines.push(line) + newLines.push(line) this.hunksByLine.set(line, hunk) } } - if (this.lines.length > 0) { - const oldSelectionStart = Math.min(this.selections[0].head, this.selections[0].tail) - let newSelectionHeadAndTail - - if (this.mode === 'hunk') { - newSelectionHeadAndTail = Math.min(this.hunks.length - 1, oldSelectionStart) - } else { - let changedLineCount = 0 - for (let i = 0; i < oldSelectionStart; i++) { - if (oldLines[i].isChanged()) changedLineCount++ - } + // Update hunks, preserving selection index + const oldHunks = this.hunksSelection.getItems() + let newSelectedHunk + if (oldHunks.length > 0 && newHunks.length > 0) { + const oldSelectionStartIndex = this.hunksSelection.getMostRecentSelectionStartIndex() + newSelectedHunk = newHunks[Math.min(newHunks.length - 1, oldSelectionStartIndex)] + } + this.hunksSelection.setItems(newHunks) + if (newSelectedHunk) this.hunksSelection.selectItem(newSelectedHunk) + + // Update lines, preserving selection index in *changed* lines + const oldLines = this.linesSelection.getItems() + let newSelectedLine + if (oldLines.length > 0 && newLines.length > 0) { + const oldSelectionStartIndex = this.linesSelection.getMostRecentSelectionStartIndex() + let changedLineCount = 0 + for (let i = 0; i < oldSelectionStartIndex; i++) { + if (oldLines[i].isChanged()) changedLineCount++ + } - for (let i = 0; i < this.lines.length; i++) { - if (this.lines[i].isChanged()) { - newSelectionHeadAndTail = i - if (changedLineCount === 0) break - changedLineCount-- - } + for (let i = 0; i < newLines.length; i++) { + const line = newLines[i] + if (line.isChanged()) { + newSelectedLine = line + if (changedLineCount === 0) break + changedLineCount-- } } - - this.selections = [{head: newSelectionHeadAndTail, tail: newSelectionHeadAndTail}] - } else { - this.selections = [] } + this.linesSelection.setItems(newLines) + if (newSelectedLine) this.linesSelection.selectItem(newSelectedLine) + } + + coalesce () { + this.hunksSelection.coalesce() + this.linesSelection.coalesce() } } diff --git a/lib/views/list-selection.js b/lib/views/list-selection.js new file mode 100644 index 0000000000..501b3dbc69 --- /dev/null +++ b/lib/views/list-selection.js @@ -0,0 +1,196 @@ +/** @babel */ + +export default class ListSelection { + constructor (options = {}) { + if (options.isItemSelectable) this.isItemSelectable = options.isItemSelectable + this.items = [] + this.selections = [] + } + + isItemSelectable (item) { + return true + } + + setItems (items) { + this.items = items + if (items.length > 0) { + this.selections = [{head: 0, tail: 0}] + } else { + this.selections = [] + } + } + + getItems () { + return this.items + } + + selectFirstItem (preserveTail) { + for (let i = 0; i < this.items.length; i++) { + const item = this.items[i] + if (this.isItemSelectable(item)) { + this.selectItem(item, preserveTail) + break + } + } + } + + 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 + } + } + } + + selectAllItems () { + this.selectFirstItem() + this.selectLastItem(true) + } + + selectNextItem (preserveTail) { + if (this.selections.length === 0) { + this.selectFirstItem() + return + } + + let itemIndex = this.selections[0].head + let nextItemIndex = itemIndex + while (itemIndex < this.items.length - 1) { + itemIndex++ + if (this.isItemSelectable(this.items[itemIndex])) { + nextItemIndex = itemIndex + break + } + } + + this.selectItem(this.items[nextItemIndex], preserveTail) + } + + selectPreviousItem (preserveTail) { + if (this.selections.length === 0) { + this.selectLastItem() + return + } + + let itemIndex = this.selections[0].head + let previousItemIndex = itemIndex + + while (itemIndex > 0) { + itemIndex-- + if (this.isItemSelectable(this.items[itemIndex])) { + previousItemIndex = itemIndex + break + } + } + + this.selectItem(this.items[previousItemIndex], preserveTail) + } + + selectItem (item, preserveTail, addOrSubtract) { + if (addOrSubtract && preserveTail) { + throw new Error('addOrSubtract and preserveTail cannot both be true at the same time') + } + + const itemIndex = this.items.indexOf(item) + if (preserveTail) { + this.selections[0].head = itemIndex + } else { + const selection = {head: itemIndex, tail: itemIndex} + if (addOrSubtract) { + if (this.getSelectedItems().has(item)) selection.negate = true + this.selections.unshift(selection) + } else { + this.selections = [selection] + } + } + } + + addOrSubtractSelection (item) { + this.selectItem(item, false, true) + } + + coalesce () { + if (this.selections.length === 0) return + + const mostRecent = this.selections[0] + let mostRecentStart = Math.min(mostRecent.head, mostRecent.tail) + let mostRecentEnd = Math.max(mostRecent.head, mostRecent.tail) + while (mostRecentStart > 0 && !this.isItemSelectable(this.items[mostRecentStart - 1])) { + mostRecentStart-- + } + while (mostRecentEnd < (this.items.length - 1) && !this.isItemSelectable(this.items[mostRecentEnd + 1])) { + mostRecentEnd++ + } + + 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}) + } + if (currentStart < mostRecentStart) { // prefix + truncatedSelections.push({tail: currentStart, head: mostRecentStart - 1}) + } + } else { + if (currentStart < mostRecentStart) { // prefix + truncatedSelections.push({head: currentStart, tail: mostRecentStart - 1}) + } + if (currentEnd > mostRecentEnd) { // suffix + truncatedSelections.push({head: mostRecentEnd + 1, tail: currentEnd}) + } + } + this.selections.splice(i, 1, ...truncatedSelections) + i += truncatedSelections.length + } else { + if (mostRecent.head > mostRecent.tail) { + mostRecent.head = Math.max(mostRecentEnd, currentEnd) + mostRecent.tail = Math.min(mostRecentStart, currentStart) + } else { + mostRecent.head = Math.min(mostRecentStart, currentStart) + mostRecent.tail = Math.max(mostRecentEnd, currentEnd) + } + this.selections.splice(i, 1) + } + } else { + i++ + } + } + + if (mostRecent.negate) this.selections.shift() + } + + getSelectedItems () { + const selectedItems = new Set() + for (let {head, tail, negate} of this.selections.slice().reverse()) { + let start = Math.min(head, tail) + let end = Math.max(head, tail) + for (let i = start; i <= end; i++) { + const item = this.items[i] + if (this.isItemSelectable(item)) { + if (negate) { + selectedItems.delete(item) + } else { + selectedItems.add(item) + } + } + } + } + return selectedItems + } + + getHeadItem () { + return this.items[this.selections[0].head] + } + + getMostRecentSelectionStartIndex () { + const selection = this.selections[0] + return Math.min(selection.head, selection.tail) + } +} diff --git a/test/views/file-patch-selection.test.js b/test/views/file-patch-selection.test.js index f2e1d16b60..e787855cc0 100644 --- a/test/views/file-patch-selection.test.js +++ b/test/views/file-patch-selection.test.js @@ -563,7 +563,7 @@ describe('FilePatchSelection', () => { assertEqualSets(selection.getSelectedHunks(), new Set([hunks[0], hunks[1]])) }) - it('adds a new hunk selection with addHunkSelection and always updates the head of the most recent hunk selection', function () { + it('adds a new hunk selection with addOrSubtractHunkSelection and always updates the head of the most recent hunk selection', function () { const hunks = [ new Hunk(1, 1, 0, 1, [ new HunkLine('line-1', 'added', -1, 1), @@ -580,7 +580,7 @@ describe('FilePatchSelection', () => { ] const selection = new FilePatchSelection(hunks) - selection.addHunkSelection(hunks[2]) + selection.addOrSubtractHunkSelection(hunks[2]) assertEqualSets(selection.getSelectedHunks(), new Set([hunks[0], hunks[2]])) selection.selectHunk(hunks[3], true) From d741449035a085af8a30186221270ccfb31a49ab Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Wed, 19 Oct 2016 14:07:01 -0600 Subject: [PATCH 02/25] Start on CompositeListSelection This will be used to model the selection state of the staging view. Signed-off-by: Katrina Uychaco --- lib/views/composite-list-selection.js | 64 +++++++++++++++++++++ lib/views/list-selection.js | 7 ++- test/helpers.js | 5 ++ test/views/composite-list-selection.test.js | 57 ++++++++++++++++++ test/views/file-patch-selection.test.js | 6 +- 5 files changed, 132 insertions(+), 7 deletions(-) create mode 100644 lib/views/composite-list-selection.js create mode 100644 test/views/composite-list-selection.test.js diff --git a/lib/views/composite-list-selection.js b/lib/views/composite-list-selection.js new file mode 100644 index 0000000000..61630e6578 --- /dev/null +++ b/lib/views/composite-list-selection.js @@ -0,0 +1,64 @@ +/** @babel */ + +import ListSelection from './list-selection' + +export default class CompositeListSelection { + constructor (listsByKey) { + this.keysBySelection = new Map() + this.selections = [] + + for (const key in listsByKey) { + const selection = new ListSelection({items: listsByKey[key]}) + this.keysBySelection.set(selection, key) + this.selections.push(selection) + } + + this.activeSelectionIndex = 0 + } + + getActiveListKey () { + return this.keysBySelection.get(this.getActiveSelection()) + } + + getSelectedItems () { + return this.getActiveSelection().getSelectedItems() + } + + getActiveSelection () { + return this.selections[this.activeSelectionIndex] + } + + activateNextSelection () { + if (this.activeSelectionIndex < this.selections.length - 1) { + this.activeSelectionIndex++ + return true + } else { + return false + } + } + + activatePreviousSelection () { + if (this.activeSelectionIndex > 0) { + this.activeSelectionIndex-- + return true + } else { + return false + } + } + + selectNextItem () { + if (this.getActiveSelection().getHeadItem() === this.getActiveSelection().getLastItem()) { + if (this.activateNextSelection()) this.getActiveSelection().selectFirstItem() + } else { + this.getActiveSelection().selectNextItem() + } + } + + selectPreviousItem () { + if (this.getActiveSelection().getHeadItem() === this.getActiveSelection().getItems()[0]) { + if (this.activatePreviousSelection()) this.getActiveSelection().selectLastItem() + } else { + this.getActiveSelection().selectPreviousItem() + } + } +} diff --git a/lib/views/list-selection.js b/lib/views/list-selection.js index 501b3dbc69..afc1502565 100644 --- a/lib/views/list-selection.js +++ b/lib/views/list-selection.js @@ -3,8 +3,7 @@ export default class ListSelection { constructor (options = {}) { if (options.isItemSelectable) this.isItemSelectable = options.isItemSelectable - this.items = [] - this.selections = [] + this.setItems(options.items || []) } isItemSelectable (item) { @@ -24,6 +23,10 @@ export default class ListSelection { return this.items } + getLastItem () { + return this.items[this.items.length - 1] + } + selectFirstItem (preserveTail) { for (let i = 0; i < this.items.length; i++) { const item = this.items[i] diff --git a/test/helpers.js b/test/helpers.js index 3c3e1af273..c77dbe8858 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -64,3 +64,8 @@ export function assertDeepPropertyVals (actual, expected) { assert.deepEqual(extractObjectSubset(actual, expected), expected) } + +export function assertEqualSets (a, b) { + assert.equal(a.size, b.size, 'Sets are a different size') + a.forEach(item => assert(b.has(item), 'Sets have different elements')) +} diff --git a/test/views/composite-list-selection.test.js b/test/views/composite-list-selection.test.js new file mode 100644 index 0000000000..4652076ea3 --- /dev/null +++ b/test/views/composite-list-selection.test.js @@ -0,0 +1,57 @@ +/** @babel */ + +import CompositeListSelection from '../../lib/views/composite-list-selection' +import {assertEqualSets} from '../helpers' + +describe('CompositeListSelection', () => { + it('allows the next and previous item to be selected', function () { + const selection = new CompositeListSelection({ + unstagedChanges: ['a', 'b'], + mergeConflicts: ['c'], + stagedChanges: ['d', 'e'], + }) + + assert.equal(selection.getActiveListKey(), 'unstagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['a'])) + + selection.selectNextItem() + assert.equal(selection.getActiveListKey(), 'unstagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['b'])) + + selection.selectNextItem() + assert.equal(selection.getActiveListKey(), 'mergeConflicts') + assertEqualSets(selection.getSelectedItems(), new Set(['c'])) + + selection.selectNextItem() + assert.equal(selection.getActiveListKey(), 'stagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['d'])) + + selection.selectNextItem() + assert.equal(selection.getActiveListKey(), 'stagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['e'])) + + selection.selectNextItem() + assert.equal(selection.getActiveListKey(), 'stagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['e'])) + + selection.selectPreviousItem() + assert.equal(selection.getActiveListKey(), 'stagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['d'])) + + selection.selectPreviousItem() + assert.equal(selection.getActiveListKey(), 'mergeConflicts') + assertEqualSets(selection.getSelectedItems(), new Set(['c'])) + + selection.selectPreviousItem() + assert.equal(selection.getActiveListKey(), 'unstagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['b'])) + + selection.selectPreviousItem() + assert.equal(selection.getActiveListKey(), 'unstagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['a'])) + + selection.selectPreviousItem() + assert.equal(selection.getActiveListKey(), 'unstagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['a'])) + }) +}) diff --git a/test/views/file-patch-selection.test.js b/test/views/file-patch-selection.test.js index e787855cc0..c462264dfa 100644 --- a/test/views/file-patch-selection.test.js +++ b/test/views/file-patch-selection.test.js @@ -3,11 +3,7 @@ import FilePatchSelection from '../../lib/views/file-patch-selection' import Hunk from '../../lib/models/hunk' import HunkLine from '../../lib/models/hunk-line' - -function assertEqualSets (a, b) { - assert.equal(a.size, b.size, 'Sets are a different size') - a.forEach(item => assert(b.has(item), 'Sets have different elements')) -} +import {assertEqualSets} from '../helpers' describe('FilePatchSelection', () => { describe('line selection', () => { From 218040b4e264060c44562812ddb386f653d4e5d1 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Mon, 24 Oct 2016 14:49:39 -0600 Subject: [PATCH 03/25] Implement selectItem and preserveTail option on selectNext/PreviousItem Signed-off-by: Katrina Uychaco --- lib/views/composite-list-selection.js | 29 ++++++++++--- test/views/composite-list-selection.test.js | 47 +++++++++++++++++++++ 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/lib/views/composite-list-selection.js b/lib/views/composite-list-selection.js index 61630e6578..725ad3d3a7 100644 --- a/lib/views/composite-list-selection.js +++ b/lib/views/composite-list-selection.js @@ -28,6 +28,12 @@ export default class CompositeListSelection { return this.selections[this.activeSelectionIndex] } + activateSelection (selection) { + const index = this.selections.indexOf(selection) + if (index === -1) throw new Error('Selection not found') + this.activeSelectionIndex = index + } + activateNextSelection () { if (this.activeSelectionIndex < this.selections.length - 1) { this.activeSelectionIndex++ @@ -46,19 +52,30 @@ export default class CompositeListSelection { } } - selectNextItem () { - if (this.getActiveSelection().getHeadItem() === this.getActiveSelection().getLastItem()) { + 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) + } + + selectionForItem (item) { + return this.selections.find(selection => selection.getItems().indexOf(item) > -1) + } + + selectNextItem (preserveTail = false) { + if (!preserveTail && this.getActiveSelection().getHeadItem() === this.getActiveSelection().getLastItem()) { if (this.activateNextSelection()) this.getActiveSelection().selectFirstItem() } else { - this.getActiveSelection().selectNextItem() + this.getActiveSelection().selectNextItem(preserveTail) } } - selectPreviousItem () { - if (this.getActiveSelection().getHeadItem() === this.getActiveSelection().getItems()[0]) { + selectPreviousItem (preserveTail = false) { + if (!preserveTail && this.getActiveSelection().getHeadItem() === this.getActiveSelection().getItems()[0]) { if (this.activatePreviousSelection()) this.getActiveSelection().selectLastItem() } else { - this.getActiveSelection().selectPreviousItem() + this.getActiveSelection().selectPreviousItem(preserveTail) } } } diff --git a/test/views/composite-list-selection.test.js b/test/views/composite-list-selection.test.js index 4652076ea3..d5f87f00cf 100644 --- a/test/views/composite-list-selection.test.js +++ b/test/views/composite-list-selection.test.js @@ -4,6 +4,27 @@ import CompositeListSelection from '../../lib/views/composite-list-selection' import {assertEqualSets} from '../helpers' describe('CompositeListSelection', () => { + it('allows specific items to be selected, but does not select across lists', function () { + const selection = new CompositeListSelection({ + unstagedChanges: ['a', 'b'], + mergeConflicts: ['c'], + stagedChanges: ['d', 'e', 'f'], + }) + + selection.selectItem('e') + assert.equal(selection.getActiveListKey(), 'stagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['e'])) + selection.selectItem('f', true) + assert.equal(selection.getActiveListKey(), 'stagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['e', 'f'])) + selection.selectItem('d', true) + assert.equal(selection.getActiveListKey(), 'stagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['d', 'e'])) + selection.selectItem('c', true) + assert.equal(selection.getActiveListKey(), 'stagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['d', 'e'])) + }) + it('allows the next and previous item to be selected', function () { const selection = new CompositeListSelection({ unstagedChanges: ['a', 'b'], @@ -54,4 +75,30 @@ describe('CompositeListSelection', () => { assert.equal(selection.getActiveListKey(), 'unstagedChanges') assertEqualSets(selection.getSelectedItems(), new Set(['a'])) }) + + it('allows the selection to be expanded to the next or previous item', function () { + const selection = new CompositeListSelection({ + unstagedChanges: ['a', 'b'], + mergeConflicts: ['c'], + stagedChanges: ['d', 'e'], + }) + + assert.equal(selection.getActiveListKey(), 'unstagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['a'])) + + selection.selectNextItem(true) + assert.equal(selection.getActiveListKey(), 'unstagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b'])) + + // Does not expand selections across lists + selection.selectNextItem(true) + assert.equal(selection.getActiveListKey(), 'unstagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b'])) + + selection.selectItem('e') + selection.selectPreviousItem(true) + selection.selectPreviousItem(true) + assert.equal(selection.getActiveListKey(), 'stagedChanges') + assertEqualSets(selection.getSelectedItems(), new Set(['d', 'e'])) + }) }) From 7c7bd5c7e8f170c49c23da243a8178895173f21d Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Tue, 25 Oct 2016 10:57:30 -0600 Subject: [PATCH 04/25] Start on CompositeListSelection.updateLists --- lib/views/composite-list-selection.js | 19 +- test/views/composite-list-selection.test.js | 222 ++++++++++++-------- 2 files changed, 150 insertions(+), 91 deletions(-) diff --git a/lib/views/composite-list-selection.js b/lib/views/composite-list-selection.js index 725ad3d3a7..f6ac51c1cf 100644 --- a/lib/views/composite-list-selection.js +++ b/lib/views/composite-list-selection.js @@ -3,9 +3,10 @@ import ListSelection from './list-selection' export default class CompositeListSelection { - constructor (listsByKey) { + constructor ({listsByKey, idForItem}) { this.keysBySelection = new Map() this.selections = [] + this.idForItem = idForItem || (item => item) for (const key in listsByKey) { const selection = new ListSelection({items: listsByKey[key]}) @@ -16,6 +17,18 @@ export default class CompositeListSelection { this.activeSelectionIndex = 0 } + updateLists (listsByKey) { + const keys = Object.keys(listsByKey) + for (let i = 0; i < keys.length; i++) { + const newItems = listsByKey[keys[i]] + const selection = this.selections[i] + const oldHeadItemId = this.idForItem(selection.getHeadItem()) + selection.setItems(newItems) + const newHeadItem = newItems.find(item => this.idForItem(item) === oldHeadItemId) + selection.selectItem(newHeadItem) + } + } + getActiveListKey () { return this.keysBySelection.get(this.getActiveSelection()) } @@ -43,7 +56,7 @@ export default class CompositeListSelection { } } - activatePreviousSelection () { + activatePreviousList () { if (this.activeSelectionIndex > 0) { this.activeSelectionIndex-- return true @@ -73,7 +86,7 @@ export default class CompositeListSelection { selectPreviousItem (preserveTail = false) { if (!preserveTail && this.getActiveSelection().getHeadItem() === this.getActiveSelection().getItems()[0]) { - if (this.activatePreviousSelection()) this.getActiveSelection().selectLastItem() + if (this.activatePreviousList()) this.getActiveSelection().selectLastItem() } else { this.getActiveSelection().selectPreviousItem(preserveTail) } diff --git a/test/views/composite-list-selection.test.js b/test/views/composite-list-selection.test.js index d5f87f00cf..bb74f63975 100644 --- a/test/views/composite-list-selection.test.js +++ b/test/views/composite-list-selection.test.js @@ -4,101 +4,147 @@ import CompositeListSelection from '../../lib/views/composite-list-selection' import {assertEqualSets} from '../helpers' describe('CompositeListSelection', () => { - it('allows specific items to be selected, but does not select across lists', function () { - const selection = new CompositeListSelection({ - unstagedChanges: ['a', 'b'], - mergeConflicts: ['c'], - stagedChanges: ['d', 'e', 'f'], + 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'], + } + }) + + selection.selectItem('e') + assert.equal(selection.getActiveListKey(), 'staged') + assertEqualSets(selection.getSelectedItems(), new Set(['e'])) + selection.selectItem('f', true) + assert.equal(selection.getActiveListKey(), 'staged') + assertEqualSets(selection.getSelectedItems(), new Set(['e', 'f'])) + selection.selectItem('d', true) + + assert.equal(selection.getActiveListKey(), 'staged') + assertEqualSets(selection.getSelectedItems(), new Set(['d', 'e'])) + selection.selectItem('c', true) + assert.equal(selection.getActiveListKey(), 'staged') + assertEqualSets(selection.getSelectedItems(), new Set(['d', 'e'])) }) - selection.selectItem('e') - assert.equal(selection.getActiveListKey(), 'stagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['e'])) - selection.selectItem('f', true) - assert.equal(selection.getActiveListKey(), 'stagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['e', 'f'])) - selection.selectItem('d', true) - assert.equal(selection.getActiveListKey(), 'stagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['d', 'e'])) - selection.selectItem('c', true) - assert.equal(selection.getActiveListKey(), 'stagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['d', 'e'])) - }) - - it('allows the next and previous item to be selected', function () { - const selection = new CompositeListSelection({ - unstagedChanges: ['a', 'b'], - mergeConflicts: ['c'], - stagedChanges: ['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'], + } + }) + + assert.equal(selection.getActiveListKey(), 'unstaged') + assertEqualSets(selection.getSelectedItems(), new Set(['a'])) + + selection.selectNextItem() + assert.equal(selection.getActiveListKey(), 'unstaged') + assertEqualSets(selection.getSelectedItems(), new Set(['b'])) + + selection.selectNextItem() + assert.equal(selection.getActiveListKey(), 'conflicts') + assertEqualSets(selection.getSelectedItems(), new Set(['c'])) + + selection.selectNextItem() + assert.equal(selection.getActiveListKey(), 'staged') + assertEqualSets(selection.getSelectedItems(), new Set(['d'])) + + selection.selectNextItem() + assert.equal(selection.getActiveListKey(), 'staged') + assertEqualSets(selection.getSelectedItems(), new Set(['e'])) + + selection.selectNextItem() + assert.equal(selection.getActiveListKey(), 'staged') + assertEqualSets(selection.getSelectedItems(), new Set(['e'])) + + selection.selectPreviousItem() + assert.equal(selection.getActiveListKey(), 'staged') + assertEqualSets(selection.getSelectedItems(), new Set(['d'])) + + selection.selectPreviousItem() + assert.equal(selection.getActiveListKey(), 'conflicts') + assertEqualSets(selection.getSelectedItems(), new Set(['c'])) + + selection.selectPreviousItem() + assert.equal(selection.getActiveListKey(), 'unstaged') + assertEqualSets(selection.getSelectedItems(), new Set(['b'])) + + selection.selectPreviousItem() + assert.equal(selection.getActiveListKey(), 'unstaged') + assertEqualSets(selection.getSelectedItems(), new Set(['a'])) + + selection.selectPreviousItem() + assert.equal(selection.getActiveListKey(), 'unstaged') + assertEqualSets(selection.getSelectedItems(), new Set(['a'])) }) - assert.equal(selection.getActiveListKey(), 'unstagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['a'])) - - selection.selectNextItem() - assert.equal(selection.getActiveListKey(), 'unstagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['b'])) - - selection.selectNextItem() - assert.equal(selection.getActiveListKey(), 'mergeConflicts') - assertEqualSets(selection.getSelectedItems(), new Set(['c'])) - - selection.selectNextItem() - assert.equal(selection.getActiveListKey(), 'stagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['d'])) - - selection.selectNextItem() - assert.equal(selection.getActiveListKey(), 'stagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['e'])) - - selection.selectNextItem() - assert.equal(selection.getActiveListKey(), 'stagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['e'])) - - selection.selectPreviousItem() - assert.equal(selection.getActiveListKey(), 'stagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['d'])) - - selection.selectPreviousItem() - assert.equal(selection.getActiveListKey(), 'mergeConflicts') - assertEqualSets(selection.getSelectedItems(), new Set(['c'])) - - selection.selectPreviousItem() - assert.equal(selection.getActiveListKey(), 'unstagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['b'])) - - selection.selectPreviousItem() - assert.equal(selection.getActiveListKey(), 'unstagedChanges') - 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'], + } + }) + + assert.equal(selection.getActiveListKey(), 'unstaged') + assertEqualSets(selection.getSelectedItems(), new Set(['a'])) + + selection.selectNextItem(true) + assert.equal(selection.getActiveListKey(), 'unstaged') + assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b'])) + + // Does not expand selections across lists + selection.selectNextItem(true) + assert.equal(selection.getActiveListKey(), 'unstaged') + assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b'])) + + selection.selectItem('e') + selection.selectPreviousItem(true) + selection.selectPreviousItem(true) + assert.equal(selection.getActiveListKey(), 'staged') + assertEqualSets(selection.getSelectedItems(), new Set(['d', 'e'])) + }) - selection.selectPreviousItem() - assert.equal(selection.getActiveListKey(), 'unstagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['a'])) }) - it('allows the selection to be expanded to the next or previous item', function () { - const selection = new CompositeListSelection({ - unstagedChanges: ['a', 'b'], - mergeConflicts: ['c'], - stagedChanges: ['d', '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({ + listsByKey, idForItem: (item) => item.filePath + }) + + selection.selectItem(listsByKey.unstaged[1]) + selection.selectItem(listsByKey.staged[1]) + selection.selectItem(listsByKey.staged[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'}], + } + + selection.updateLists(listsByKey) + + assert.equal(selection.getActiveListKey(), 'staged') + assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.staged[3]])) + + selection.activatePreviousList() + assert.equal(selection.getActiveListKey(), 'conflicts') + assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.conflicts[1]])) + + selection.activatePreviousList() + assert.equal(selection.getActiveListKey(), 'unstaged') + assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.unstaged[2]])) }) - - assert.equal(selection.getActiveListKey(), 'unstagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['a'])) - - selection.selectNextItem(true) - assert.equal(selection.getActiveListKey(), 'unstagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b'])) - - // Does not expand selections across lists - selection.selectNextItem(true) - assert.equal(selection.getActiveListKey(), 'unstagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b'])) - - selection.selectItem('e') - selection.selectPreviousItem(true) - selection.selectPreviousItem(true) - assert.equal(selection.getActiveListKey(), 'stagedChanges') - assertEqualSets(selection.getSelectedItems(), new Set(['d', 'e'])) }) }) From 78f0436c88db55a79e33f26be8e73ae6ed0af4d6 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Tue, 25 Oct 2016 12:14:10 -0600 Subject: [PATCH 05/25] Preserve selection start in CompositeListSelection ...if the old head item is removed from the list. --- lib/views/composite-list-selection.js | 28 +++++++++++--------- lib/views/list-selection.js | 14 ++++++++-- test/views/composite-list-selection.test.js | 29 +++++++++++++++++++++ 3 files changed, 56 insertions(+), 15 deletions(-) diff --git a/lib/views/composite-list-selection.js b/lib/views/composite-list-selection.js index f6ac51c1cf..e504a240b8 100644 --- a/lib/views/composite-list-selection.js +++ b/lib/views/composite-list-selection.js @@ -22,10 +22,10 @@ export default class CompositeListSelection { for (let i = 0; i < keys.length; i++) { const newItems = listsByKey[keys[i]] const selection = this.selections[i] - const oldHeadItemId = this.idForItem(selection.getHeadItem()) + const oldHeadItem = selection.getHeadItem() selection.setItems(newItems) - const newHeadItem = newItems.find(item => this.idForItem(item) === oldHeadItemId) - selection.selectItem(newHeadItem) + const newHeadItem = oldHeadItem ? newItems.find(item => this.idForItem(item) === this.idForItem(oldHeadItem)) : null + if (newHeadItem) selection.selectItem(newHeadItem) } } @@ -48,21 +48,23 @@ export default class CompositeListSelection { } activateNextSelection () { - if (this.activeSelectionIndex < this.selections.length - 1) { - this.activeSelectionIndex++ - return true - } else { - return false + for (let i = this.activeSelectionIndex + 1; i < this.selections.length; i++) { + if (this.selections[i].getItems().length > 0) { + this.activeSelectionIndex = i + return true + } } + return false } activatePreviousList () { - if (this.activeSelectionIndex > 0) { - this.activeSelectionIndex-- - return true - } else { - return false + for (let i = this.activeSelectionIndex - 1; i >= 0; i--) { + if (this.selections[i].getItems().length > 0) { + this.activeSelectionIndex = i + return true + } } + return false } selectItem (item, preserveTail = false) { diff --git a/lib/views/list-selection.js b/lib/views/list-selection.js index afc1502565..f34820e02c 100644 --- a/lib/views/list-selection.js +++ b/lib/views/list-selection.js @@ -11,9 +11,17 @@ export default class ListSelection { } setItems (items) { + let newSelectionIndex + if (this.selections && 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: 0, tail: 0}] + this.selections = [{head: newSelectionIndex, tail: newSelectionIndex}] } else { this.selections = [] } @@ -189,7 +197,9 @@ export default class ListSelection { } getHeadItem () { - return this.items[this.selections[0].head] + if (this.selections.length > 0) { + return this.items[this.selections[0].head] + } } getMostRecentSelectionStartIndex () { diff --git a/test/views/composite-list-selection.test.js b/test/views/composite-list-selection.test.js index bb74f63975..06bb7bad72 100644 --- a/test/views/composite-list-selection.test.js +++ b/test/views/composite-list-selection.test.js @@ -146,5 +146,34 @@ describe('CompositeListSelection', () => { assert.equal(selection.getActiveListKey(), 'unstaged') assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.unstaged[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({ + listsByKey, idForItem: (item) => item.filePath + }) + + selection.selectItem(listsByKey.unstaged[1]) + selection.selectItem(listsByKey.unstaged[2], true) + selection.selectItem(listsByKey.staged[1]) + + listsByKey = { + unstaged: [{filePath: 'a'}], + conflicts: [], + staged: [{filePath: 'd'}, {filePath: 'f'}], + } + selection.updateLists(listsByKey) + + assert.equal(selection.getActiveListKey(), 'staged') + assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.staged[1]])) + + selection.activatePreviousList() + assert.equal(selection.getActiveListKey(), 'unstaged') + assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.unstaged[0]])) + }) }) }) From 24ef6dbe1ff9ed825a20b4061c2a1dc0dd0d9a95 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Tue, 25 Oct 2016 12:58:40 -0600 Subject: [PATCH 06/25] Remove code that is now redundant --- lib/views/file-patch-selection.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lib/views/file-patch-selection.js b/lib/views/file-patch-selection.js index 7b018eba1c..46b098469f 100644 --- a/lib/views/file-patch-selection.js +++ b/lib/views/file-patch-selection.js @@ -184,14 +184,7 @@ export default class FilePatchSelection { } // Update hunks, preserving selection index - const oldHunks = this.hunksSelection.getItems() - let newSelectedHunk - if (oldHunks.length > 0 && newHunks.length > 0) { - const oldSelectionStartIndex = this.hunksSelection.getMostRecentSelectionStartIndex() - newSelectedHunk = newHunks[Math.min(newHunks.length - 1, oldSelectionStartIndex)] - } this.hunksSelection.setItems(newHunks) - if (newSelectedHunk) this.hunksSelection.selectItem(newSelectedHunk) // Update lines, preserving selection index in *changed* lines const oldLines = this.linesSelection.getItems() From 24443b6c4d86d518e6221066ac981d27d790aa13 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Fri, 28 Oct 2016 15:24:51 -0600 Subject: [PATCH 07/25] Don't select empty lists in CompositeListSelection --- lib/views/composite-list-selection.js | 17 +++- test/views/composite-list-selection.test.js | 88 ++++++++++++++++++--- 2 files changed, 92 insertions(+), 13 deletions(-) diff --git a/lib/views/composite-list-selection.js b/lib/views/composite-list-selection.js index e504a240b8..16603c8b86 100644 --- a/lib/views/composite-list-selection.js +++ b/lib/views/composite-list-selection.js @@ -27,6 +27,9 @@ export default class CompositeListSelection { const newHeadItem = oldHeadItem ? newItems.find(item => this.idForItem(item) === this.idForItem(oldHeadItem)) : null if (newHeadItem) selection.selectItem(newHeadItem) } + if (this.getActiveSelection().getItems().length === 0) { + this.activateNextSelection() || this.activatePreviousSelection() + } } getActiveListKey () { @@ -57,7 +60,7 @@ export default class CompositeListSelection { return false } - activatePreviousList () { + activatePreviousSelection () { for (let i = this.activeSelectionIndex - 1; i >= 0; i--) { if (this.selections[i].getItems().length > 0) { this.activeSelectionIndex = i @@ -80,7 +83,11 @@ export default class CompositeListSelection { selectNextItem (preserveTail = false) { if (!preserveTail && this.getActiveSelection().getHeadItem() === this.getActiveSelection().getLastItem()) { - if (this.activateNextSelection()) this.getActiveSelection().selectFirstItem() + if (this.activateNextSelection()) { + this.getActiveSelection().selectFirstItem() + } else { + this.getActiveSelection().selectLastItem() + } } else { this.getActiveSelection().selectNextItem(preserveTail) } @@ -88,7 +95,11 @@ export default class CompositeListSelection { selectPreviousItem (preserveTail = false) { if (!preserveTail && this.getActiveSelection().getHeadItem() === this.getActiveSelection().getItems()[0]) { - if (this.activatePreviousList()) this.getActiveSelection().selectLastItem() + if (this.activatePreviousSelection()) { + this.getActiveSelection().selectLastItem() + } else { + this.getActiveSelection().selectFirstItem() + } } else { this.getActiveSelection().selectPreviousItem(preserveTail) } diff --git a/test/views/composite-list-selection.test.js b/test/views/composite-list-selection.test.js index 06bb7bad72..cbde301abb 100644 --- a/test/views/composite-list-selection.test.js +++ b/test/views/composite-list-selection.test.js @@ -10,7 +10,7 @@ describe('CompositeListSelection', () => { listsByKey: { unstaged: ['a', 'b'], conflicts: ['c'], - staged: ['d', 'e', 'f'], + staged: ['d', 'e', 'f'] } }) @@ -34,7 +34,7 @@ describe('CompositeListSelection', () => { listsByKey: { unstaged: ['a', 'b'], conflicts: ['c'], - staged: ['d', 'e'], + staged: ['d', 'e'] } }) @@ -87,7 +87,7 @@ describe('CompositeListSelection', () => { listsByKey: { unstaged: ['a', 'b'], conflicts: ['c'], - staged: ['d', 'e'], + staged: ['d', 'e'] } }) @@ -110,6 +110,50 @@ describe('CompositeListSelection', () => { 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'] + } + }) + + selection.selectNextItem() + selection.selectNextItem() + assert.equal(selection.getActiveListKey(), 'staged') + assertEqualSets(selection.getSelectedItems(), new Set(['d'])) + selection.selectPreviousItem() + assert.equal(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: [] + } + }) + + selection.selectNextItem(true) + assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b'])) + selection.selectNextItem() + assertEqualSets(selection.getSelectedItems(), new Set(['b'])) + + selection.updateLists({ + unstaged: [], + conflicts: [], + staged: ['a', 'b'] + }) + + selection.selectNextItem() + selection.selectPreviousItem(true) + assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b'])) + selection.selectPreviousItem() + assertEqualSets(selection.getSelectedItems(), new Set(['a'])) + }) }) describe('updateLists(listsByKey)', function () { @@ -117,7 +161,7 @@ describe('CompositeListSelection', () => { let listsByKey = { unstaged: [{filePath: 'a'}, {filePath: 'b'}], conflicts: [{filePath: 'c'}], - staged: [{filePath: 'd'}, {filePath: 'e'}, {filePath: 'f'}], + staged: [{filePath: 'd'}, {filePath: 'e'}, {filePath: 'f'}] } const selection = new CompositeListSelection({ listsByKey, idForItem: (item) => item.filePath @@ -130,7 +174,7 @@ describe('CompositeListSelection', () => { listsByKey = { unstaged: [{filePath: 'a'}, {filePath: 'q'}, {filePath: 'b'}, {filePath: 'r'}], conflicts: [{filePath: 's'}, {filePath: 'c'}], - staged: [{filePath: 'd'}, {filePath: 't'}, {filePath: 'e'}, {filePath: 'f'}], + staged: [{filePath: 'd'}, {filePath: 't'}, {filePath: 'e'}, {filePath: 'f'}] } selection.updateLists(listsByKey) @@ -138,11 +182,11 @@ describe('CompositeListSelection', () => { assert.equal(selection.getActiveListKey(), 'staged') assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.staged[3]])) - selection.activatePreviousList() + selection.activatePreviousSelection() assert.equal(selection.getActiveListKey(), 'conflicts') assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.conflicts[1]])) - selection.activatePreviousList() + selection.activatePreviousSelection() assert.equal(selection.getActiveListKey(), 'unstaged') assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.unstaged[2]])) }) @@ -151,7 +195,7 @@ describe('CompositeListSelection', () => { let listsByKey = { unstaged: [{filePath: 'a'}, {filePath: 'b'}, {filePath: 'c'}], conflicts: [], - staged: [{filePath: 'd'}, {filePath: 'e'}, {filePath: 'f'}], + staged: [{filePath: 'd'}, {filePath: 'e'}, {filePath: 'f'}] } const selection = new CompositeListSelection({ listsByKey, idForItem: (item) => item.filePath @@ -164,16 +208,40 @@ describe('CompositeListSelection', () => { listsByKey = { unstaged: [{filePath: 'a'}], conflicts: [], - staged: [{filePath: 'd'}, {filePath: 'f'}], + staged: [{filePath: 'd'}, {filePath: 'f'}] } selection.updateLists(listsByKey) assert.equal(selection.getActiveListKey(), 'staged') assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.staged[1]])) - selection.activatePreviousList() + selection.activatePreviousSelection() assert.equal(selection.getActiveListKey(), 'unstaged') assertEqualSets(selection.getSelectedItems(), new Set([listsByKey.unstaged[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'] + }) + assert.equal(selection.getActiveListKey(), 'staged') + + selection.updateLists({ + unstaged: ['a', 'b'], + conflicts: [], + staged: [] + }) + assert.equal(selection.getActiveListKey(), 'unstaged') + }) }) }) From e6a8ed3ac2c382bb1dedcaa449ae19910b0a5d17 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Fri, 28 Oct 2016 15:25:30 -0600 Subject: [PATCH 08/25] Start using CompositeListSelection in StagingView --- lib/views/composite-list-selection.js | 4 + lib/views/file-patch-list-item-view.js | 14 +- lib/views/merge-conflict-list-item-view.js | 12 +- lib/views/staging-view.js | 293 ++++++++------------- 4 files changed, 117 insertions(+), 206 deletions(-) diff --git a/lib/views/composite-list-selection.js b/lib/views/composite-list-selection.js index 16603c8b86..b22c815a3e 100644 --- a/lib/views/composite-list-selection.js +++ b/lib/views/composite-list-selection.js @@ -81,6 +81,10 @@ export default class CompositeListSelection { return this.selections.find(selection => selection.getItems().indexOf(item) > -1) } + listKeyForItem (item) { + return this.keysBySelection.get(this.selectionForItem(item)) + } + selectNextItem (preserveTail = false) { if (!preserveTail && this.getActiveSelection().getHeadItem() === this.getActiveSelection().getLastItem()) { if (this.activateNextSelection()) { diff --git a/lib/views/file-patch-list-item-view.js b/lib/views/file-patch-list-item-view.js index 884773e056..b803858738 100644 --- a/lib/views/file-patch-list-item-view.js +++ b/lib/views/file-patch-list-item-view.js @@ -10,20 +10,8 @@ export default stateless(etch, ({filePatch, selected, selectItem, selectionEnabl const status = classNameForStatus[filePatch.getStatus()] const className = selected ? 'is-selected' : '' - let itemAlreadySelected = false - const selectItemIfSelectionEnabled = () => { - if (!itemAlreadySelected) { - if (selectionEnabled) { - selectItem(filePatch) - itemAlreadySelected = true - } - } - } - return ( -
selectItemIfSelectionEnabled()} - onmouseup={() => selectItemIfSelectionEnabled()}> +
{filePatch.getPath()}
diff --git a/lib/views/merge-conflict-list-item-view.js b/lib/views/merge-conflict-list-item-view.js index 8af74ddb69..05ef5b861c 100644 --- a/lib/views/merge-conflict-list-item-view.js +++ b/lib/views/merge-conflict-list-item-view.js @@ -16,18 +16,8 @@ export default stateless(etch, ({mergeConflict, selected, selectItem, selectionE let status = classNameForStatus[mergeConflict.getFileStatus()] const className = selected ? 'is-selected' : '' - let mousedOverItem = false - const selectItemIfSelectionEnabled = () => { - if (!mousedOverItem) { - if (selectionEnabled) selectItem(mergeConflict) - mousedOverItem = true - } - } - return ( -
selectItemIfSelectionEnabled()} - onmouseup={() => selectItemIfSelectionEnabled()}> +
{mergeConflict.getPath()} diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index df2368f2d8..0018a64ec6 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -2,182 +2,126 @@ /** @jsx etch.dom */ import etch from 'etch' -import {Disposable} from 'atom' +import {Disposable, CompositeDisposable} from 'atom' -import ListView from './list-view' -import FilePatch from '../models/file-patch' -import MergeConflict from '../models/merge-conflict' import FilePatchListItemView from './file-patch-list-item-view' import MergeConflictListItemView from './merge-conflict-list-item-view' -import MultiListCollection from '../multi-list-collection' +import CompositeListSelection from './composite-list-selection' import {shortenSha} from '../helpers' -export const ListTypes = { - STAGED: Symbol('LIST_STAGED'), - UNSTAGED: Symbol('LIST_UNSTAGED'), - CONFLICTS: Symbol('LIST_CONFLICTS') -} - -const ListNames = { - [ListTypes.STAGED]: 'staged', - [ListTypes.UNSTAGED]: 'unstaged', - [ListTypes.CONFLICTS]: 'conflicts' -} - export default class StagingView { constructor (props) { this.props = props - this.didSelectItem = this.didSelectItem.bind(this) - this.selectItem = this.selectItem.bind(this) - this.stageFilePatch = this.stageFilePatch.bind(this) - this.unstageFilePatch = this.unstageFilePatch.bind(this) - this.enableSelections = this.enableSelections.bind(this) - this.disableSelections = this.disableSelections.bind(this) - this.renderFilePatchListItem = this.renderFilePatchListItem.bind(this) - this.renderMergeConflictListItem = this.renderMergeConflictListItem.bind(this) - this.multiListCollection = new MultiListCollection([ - { key: ListTypes.UNSTAGED, items: this.props.unstagedChanges }, - { key: ListTypes.CONFLICTS, items: this.props.mergeConflicts || [] }, - { key: ListTypes.STAGED, items: this.props.stagedChanges } - ], this.didSelectItem) + this.mousedownOnItem = this.mousedownOnItem.bind(this) + this.mousemoveOnItem = this.mousemoveOnItem.bind(this) + this.mouseup = this.mouseup.bind(this) + this.mouseSelectionInProgress = false + this.selection = new CompositeListSelection({ + listsByKey: { + unstaged: this.props.unstagedChanges, + conflicts: this.props.mergeConflicts || [], + staged: this.props.stagedChanges + }, + idForItem: item => item.getPath() + }) etch.initialize(this) - this.subscriptions = atom.commands.add(this.element, { - 'core:move-up': this.selectPreviousFilePatch.bind(this), - 'core:move-down': this.selectNextFilePatch.bind(this), - 'core:select-up': this.selectPreviousFilePatch.bind(this, {addToExisting: true, stopAtBounds: true}), - 'core:select-down': this.selectNextFilePatch.bind(this, {addToExisting: true, stopAtBounds: true}), - 'core:confirm': this.confirmSelectedItems.bind(this), - 'git:focus-next-list': this.focusNextList.bind(this), - 'git:focus-previous-list': this.focusPreviousList.bind(this), - 'git:focus-diff-view': this.focusFilePatchView.bind(this) - }) - window.addEventListener('mouseup', this.disableSelections) - this.subscriptions.add(new Disposable(() => window.removeEventListener('mouseup', this.disableSelections))) + this.subscriptions = new CompositeDisposable() + this.subscriptions.add(atom.commands.add(this.element, { + 'core:move-up': () => this.selectPrevious(), + 'core:move-down': () => this.selectNext(), + 'core:select-up': () => this.selectPrevious(true), + 'core:select-down': () => this.selectNext(true), + 'core:confirm': () => this.confirmSelectedItems(), + 'git:activate-next-list': () => this.activateNextList(), + 'git:activate-previous-list': () => this.activatePreviousList(), + 'git:focus-diff-view': () => this.focusFilePatchView() + })) + window.addEventListener('mouseup', this.mouseup) + this.subscriptions.add(new Disposable(() => window.removeEventListener('mouseup', this.mouseup))) } update (props) { this.props = props - this.multiListCollection.updateLists([ - { key: ListTypes.UNSTAGED, items: this.props.unstagedChanges }, - { key: ListTypes.CONFLICTS, items: this.props.mergeConflicts || [] }, - { key: ListTypes.STAGED, items: this.props.stagedChanges } - ]) - return etch.update(this) - } - - enableSelections () { - this.validItems = new Set(this.multiListCollection.getItemsForKey(this.getSelectedListKey())) - this.selectionEnabled = true - return etch.update(this) - } - - disableSelections () { - this.tail = null - this.validItems = null - this.selectionEnabled = false - return etch.update(this) - } - - selectList (listKey, {suppressCallback} = {}) { - if (this.multiListCollection.getItemsForKey(listKey).length) { - this.multiListCollection.selectKeys([listKey], {suppressCallback}) - } - this.enableSelections() + this.selection.updateLists({ + unstaged: this.props.unstagedChanges, + conflicts: this.props.mergeConflicts || [], + staged: this.props.stagedChanges + }) return etch.update(this) } - focusNextList () { - this.multiListCollection.selectNextList({wrap: true}) + activateNextList () { + this.selection.activateNextSelection() return etch.update(this) } - focusPreviousList () { - this.multiListCollection.selectPreviousList({wrap: true}) + activatePreviousList () { + this.selection.activatePreviousSelection() return etch.update(this) } confirmSelectedItems () { - this.disableSelections() - const itemPaths = Array.from(this.getSelectedItems()).map(item => item.getPath()) - const listKey = this.getSelectedListKey() - if (listKey === ListTypes.STAGED) { + const itemPaths = Array.from(this.selection.getSelectedItems()).map(item => item.getPath()) + if (this.selection.getActiveListKey() === 'staged') { return this.props.unstageFiles(itemPaths) } else { return this.props.stageFiles(itemPaths) } } - getSelectedListKey () { - return this.multiListCollection.getActiveListKey() - } - - getSelectedItems () { - return this.multiListCollection.getSelectedItems() - } - - stageFilePatch (filePatch) { - return this.props.stageFiles([filePatch.getPath()]) - } - - unstageFilePatch (filePatch) { - return this.props.unstageFiles([filePatch.getPath()]) - } - - selectPreviousFilePatch (options = {}) { - this.multiListCollection.selectPreviousItem(options) + selectPrevious (preserveTail = false) { + this.selection.selectPreviousItem(preserveTail) return etch.update(this) } - selectNextFilePatch (options = {}) { - this.multiListCollection.selectNextItem(options) + selectNext (preserveTail = false) { + this.selection.selectNextItem(preserveTail) return etch.update(this) } - selectItem (item) { - if (!this.validItems.has(item)) return - const selectedList = this.getSelectedListKey() - if (!this.tail) this.tail = {key: selectedList, item} - this.head = {key: selectedList, item} - this.multiListCollection.selectItemsAndKeysInRange(this.tail, this.head) - return etch.update(this) + focusFilePatchView () { } - didSelectItem (item, listKey, {focus} = {}) { - if (!item || !this.isFocused()) return - if (item.constructor === FilePatch && this.props.didSelectFilePatch) { - const listName = ListNames[listKey] - this.props.didSelectFilePatch(item, listName, {focus}) - } else if (item.constructor === MergeConflict && this.props.didSelectMergeConflictFile) { - this.props.didSelectMergeConflictFile(item.getPath(), {focus}) + mousedownOnItem (event, item) { + if (event.detail >= 2) { + if (this.selection.listKeyForItem(item) === 'staged') { + this.props.unstageFiles([item.getPath()]) + } else { + this.props.stageFiles([item.getPath()]) + } + } else { + this.selection.selectItem(item, event.shiftKey) + this.mouseSelectionInProgress = true } + return etch.update(this) } - focusFilePatchView () { - const item = this.multiListCollection.getActiveItem() - const listKey = this.multiListCollection.getActiveListKey() - this.didSelectItem(item, listKey, {focus: true}) + mousemoveOnItem (event, item) { + if (this.mouseSelectionInProgress) { + this.selection.selectItem(item, true) + return etch.update(this) + } else { + return Promise.resolve() + } } - buildDebugData () { - const getPath = (fp) => fp ? fp.getNewPath() : '' - const multiListData = this.multiListCollection.toObject() - return { - ...multiListData, - lists: multiListData.lists.map(list => list.map(getPath)) - } + mouseup () { + this.mouseSelectionInProgress = false } render () { + const selectedItems = this.selection.getSelectedItems() + let stagedClassName = '' let unstagedClassName = '' let conflictsClassName = '' - const selectedList = this.getSelectedListKey() - if (selectedList === ListTypes.STAGED) { + const selectedList = this.selection.getActiveListKey() + if (selectedList === 'staged') { stagedClassName = 'is-focused' - } else if (selectedList === ListTypes.UNSTAGED) { + } else if (selectedList === 'unstaged') { unstagedClassName = 'is-focused' - } else if (selectedList === ListTypes.CONFLICTS) { + } else if (selectedList === 'conflicts') { conflictsClassName = 'is-focused' } @@ -187,87 +131,72 @@ export default class StagingView { Merge Conflicts - this.selectList(ListTypes.CONFLICTS, {suppressCallback: true}) } - className='git-StagingView-list git-FilePatchListView' - ref='mergeConflictListView' - didConfirmItem={this.stageFilePatch} - items={this.multiListCollection.getItemsForKey(ListTypes.CONFLICTS)} - selectedItems={this.getSelectedItems()} - renderItem={this.renderMergeConflictListItem} - /> +
+ { + this.props.mergeConflicts.map((mergeConflict) => ( + this.mousedownOnItem(event, mergeConflict)} + onmousemove={(event) => this.mousemoveOnItem(event, mergeConflict)} + selected={selectedItems.has(mergeConflict)} + /> + )) + } +
) return ( -
this.disableSelections()}> +
Unstaged Changes
- this.selectList(ListTypes.UNSTAGED, {suppressCallback: true})} - className='git-StagingView-list git-FilePatchListView' - ref='unstagedChangesView' - didConfirmItem={this.stageFilePatch} - items={this.multiListCollection.getItemsForKey(ListTypes.UNSTAGED)} - selectedItems={this.getSelectedItems()} - renderItem={this.renderFilePatchListItem} - /> + +
+ { + this.props.unstagedChanges.map((filePatch) => ( + this.mousedownOnItem(event, filePatch)} + onmousemove={(event) => this.mousemoveOnItem(event, filePatch)} + selected={selectedItems.has(filePatch)} + /> + )) + } +
- { this.multiListCollection.getItemsForKey(ListTypes.CONFLICTS).length ? mergeConflictsView :
) } - renderFilePatchListItem (filePatch, selected, handleItemClickEvent) { - return ( - - ) - } - - renderMergeConflictListItem (mergeConflict, selected, handleItemClickEvent) { - return ( - - ) - } - destroy () { this.subscriptions.dispose() etch.destroy(this) From 05f64a72d2fd04490d70418a73b87bc24fddb866 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Fri, 28 Oct 2016 15:44:27 -0600 Subject: [PATCH 09/25] Allow selections to be added/subtracted --- lib/views/composite-list-selection.js | 12 ++++++++++++ lib/views/staging-view.js | 6 +++++- test/views/composite-list-selection.test.js | 16 ++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/lib/views/composite-list-selection.js b/lib/views/composite-list-selection.js index b22c815a3e..6c12665574 100644 --- a/lib/views/composite-list-selection.js +++ b/lib/views/composite-list-selection.js @@ -77,6 +77,18 @@ export default class CompositeListSelection { if (selection === this.getActiveSelection()) selection.selectItem(item, preserveTail) } + addOrSubtractSelection (item) { + const selection = this.selectionForItem(item) + if (!selection) throw new Error(`No item found: ${item}`) + + if (selection === this.getActiveSelection()) { + selection.addOrSubtractSelection(item) + } else { + this.activateSelection(selection) + selection.selectItem(item) + } + } + selectionForItem (item) { return this.selections.find(selection => selection.getItems().indexOf(item) > -1) } diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index 0018a64ec6..47142f84ab 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -91,7 +91,11 @@ export default class StagingView { this.props.stageFiles([item.getPath()]) } } else { - this.selection.selectItem(item, event.shiftKey) + if (event.ctrlKey || event.metaKey) { + this.selection.addOrSubtractSelection(item) + } else { + this.selection.selectItem(item, event.shiftKey) + } this.mouseSelectionInProgress = true } return etch.update(this) diff --git a/test/views/composite-list-selection.test.js b/test/views/composite-list-selection.test.js index cbde301abb..2ec68677f4 100644 --- a/test/views/composite-list-selection.test.js +++ b/test/views/composite-list-selection.test.js @@ -154,6 +154,22 @@ describe('CompositeListSelection', () => { selection.selectPreviousItem() assertEqualSets(selection.getSelectedItems(), new Set(['a'])) }) + + it('allows selections to be added in the current active list, but updates the existing seleciton when activating a different list', function () { + const selection = new CompositeListSelection({ + listsByKey: { + unstaged: ['a', 'b', 'c'], + conflicts: [], + staged: ['e', 'f', 'g'] + } + }) + + selection.addOrSubtractSelection('c') + assertEqualSets(selection.getSelectedItems(), new Set(['a', 'c'])) + + selection.addOrSubtractSelection('g') + assertEqualSets(selection.getSelectedItems(), new Set(['g'])) + }) }) describe('updateLists(listsByKey)', function () { From ac2fac8116b8a6295c4f0ed3645c688b347f319e Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Fri, 28 Oct 2016 15:55:17 -0600 Subject: [PATCH 10/25] Support core:move-to-top/bottom & core:select-all commands when staging --- lib/views/composite-list-selection.js | 12 +++++++ lib/views/staging-view.js | 20 +++++++++++ test/views/composite-list-selection.test.js | 39 +++++++++++++++++++++ 3 files changed, 71 insertions(+) diff --git a/lib/views/composite-list-selection.js b/lib/views/composite-list-selection.js index 6c12665574..8980d49c1b 100644 --- a/lib/views/composite-list-selection.js +++ b/lib/views/composite-list-selection.js @@ -89,6 +89,18 @@ export default class CompositeListSelection { } } + selectAllItems () { + this.getActiveSelection().selectAllItems() + } + + selectFirstItem (preserveTail) { + this.getActiveSelection().selectFirstItem(preserveTail) + } + + selectLastItem (preserveTail) { + this.getActiveSelection().selectLastItem(preserveTail) + } + selectionForItem (item) { return this.selections.find(selection => selection.getItems().indexOf(item) > -1) } diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index 47142f84ab..397b1e4a67 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -32,6 +32,11 @@ export default class StagingView { 'core:move-down': () => this.selectNext(), 'core:select-up': () => this.selectPrevious(true), 'core:select-down': () => this.selectNext(true), + 'core:select-all': () => this.selectAll(), + 'core:move-to-top': () => this.selectFirst(), + 'core:move-to-bottom': () => this.selectLast(), + 'core:select-to-top': () => this.selectFirst(true), + 'core:select-to-bottom': () => this.selectLast(true), 'core:confirm': () => this.confirmSelectedItems(), 'git:activate-next-list': () => this.activateNextList(), 'git:activate-previous-list': () => this.activatePreviousList(), @@ -80,6 +85,21 @@ export default class StagingView { return etch.update(this) } + selectAll () { + this.selection.selectAllItems() + return etch.update(this) + } + + selectFirst (preserveTail = false) { + this.selection.selectFirstItem(preserveTail) + return etch.update(this) + } + + selectLast (preserveTail = false) { + this.selection.selectLastItem(preserveTail) + return etch.update(this) + } + focusFilePatchView () { } diff --git a/test/views/composite-list-selection.test.js b/test/views/composite-list-selection.test.js index 2ec68677f4..ddb0249bb4 100644 --- a/test/views/composite-list-selection.test.js +++ b/test/views/composite-list-selection.test.js @@ -170,6 +170,45 @@ describe('CompositeListSelection', () => { 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'] + } + }) + + selection.selectAllItems() + assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b', 'c'])) + + selection.activateNextSelection() + 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'] + } + }) + + selection.activateNextSelection() + selection.selectLastItem() + assertEqualSets(selection.getSelectedItems(), new Set(['g'])) + selection.selectFirstItem() + assertEqualSets(selection.getSelectedItems(), new Set(['e'])) + selection.selectLastItem(true) + assertEqualSets(selection.getSelectedItems(), new Set(['e', 'f', 'g'])) + selection.selectNextItem() + assertEqualSets(selection.getSelectedItems(), new Set(['g'])) + selection.selectFirstItem(true) + assertEqualSets(selection.getSelectedItems(), new Set(['e', 'f', 'g'])) + }) }) describe('updateLists(listsByKey)', function () { From d0269ee67f135ce925ea795d1f4913fe803064ee Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Fri, 28 Oct 2016 15:59:45 -0600 Subject: [PATCH 11/25] Coalesce staging view selections at appropriate times --- lib/views/composite-list-selection.js | 4 ++++ lib/views/staging-view.js | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/lib/views/composite-list-selection.js b/lib/views/composite-list-selection.js index 8980d49c1b..8bb5d88fcd 100644 --- a/lib/views/composite-list-selection.js +++ b/lib/views/composite-list-selection.js @@ -101,6 +101,10 @@ export default class CompositeListSelection { this.getActiveSelection().selectLastItem(preserveTail) } + coalesce () { + this.getActiveSelection().coalesce() + } + selectionForItem (item) { return this.selections.find(selection => selection.getItems().indexOf(item) > -1) } diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index 397b1e4a67..25d9574a72 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -77,26 +77,31 @@ export default class StagingView { selectPrevious (preserveTail = false) { this.selection.selectPreviousItem(preserveTail) + this.selection.coalesce() return etch.update(this) } selectNext (preserveTail = false) { this.selection.selectNextItem(preserveTail) + this.selection.coalesce() return etch.update(this) } selectAll () { this.selection.selectAllItems() + this.selection.coalesce() return etch.update(this) } selectFirst (preserveTail = false) { this.selection.selectFirstItem(preserveTail) + this.selection.coalesce() return etch.update(this) } selectLast (preserveTail = false) { this.selection.selectLastItem(preserveTail) + this.selection.coalesce() return etch.update(this) } @@ -132,6 +137,7 @@ export default class StagingView { mouseup () { this.mouseSelectionInProgress = false + this.selection.coalesce() } render () { From 42d659710a9422404af81b3c059af393e4a82dd0 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Fri, 28 Oct 2016 16:16:35 -0600 Subject: [PATCH 12/25] Fix bug adding/subtracting a selection for a single item in the list --- lib/views/list-selection.js | 12 +++++++----- test/views/list-selection.test.js | 25 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 5 deletions(-) create mode 100644 test/views/list-selection.test.js diff --git a/lib/views/list-selection.js b/lib/views/list-selection.js index f34820e02c..1af2f30c30 100644 --- a/lib/views/list-selection.js +++ b/lib/views/list-selection.js @@ -160,12 +160,14 @@ export default class ListSelection { this.selections.splice(i, 1, ...truncatedSelections) i += truncatedSelections.length } else { - if (mostRecent.head > mostRecent.tail) { - mostRecent.head = Math.max(mostRecentEnd, currentEnd) - mostRecent.tail = Math.min(mostRecentStart, currentStart) + mostRecentStart = Math.min(mostRecentStart, currentStart) + mostRecentEnd = Math.max(mostRecentEnd, currentEnd) + if (mostRecent.head >= mostRecent.tail) { + mostRecent.head = mostRecentEnd + mostRecent.tail = mostRecentStart } else { - mostRecent.head = Math.min(mostRecentStart, currentStart) - mostRecent.tail = Math.max(mostRecentEnd, currentEnd) + mostRecent.head = mostRecentStart + mostRecent.tail = mostRecentEnd } this.selections.splice(i, 1) } diff --git a/test/views/list-selection.test.js b/test/views/list-selection.test.js new file mode 100644 index 0000000000..56a05e33bf --- /dev/null +++ b/test/views/list-selection.test.js @@ -0,0 +1,25 @@ +/** @babel */ + +import ListSelection from '../../lib/views/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. +describe('ListSelection', () => { + describe('coalesce', () => { + it('correctly handles adding and subtracting a single item (regression)', function () { + const selection = new ListSelection({items: ['a', 'b', 'c']}) + selection.selectLastItem(true) + selection.coalesce() + assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b', 'c'])) + selection.addOrSubtractSelection('b') + selection.coalesce() + assertEqualSets(selection.getSelectedItems(), new Set(['a', 'c'])) + selection.addOrSubtractSelection('b') + global.debug = true + selection.coalesce() + assertEqualSets(selection.getSelectedItems(), new Set(['a', 'b', 'c'])) + }) + }) +}) From 9db850ae8f9db9fc05603f8ba7d5c9363cea0637 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Fri, 28 Oct 2016 16:21:41 -0600 Subject: [PATCH 13/25] Fix lint errors --- lib/views/file-patch-view.js | 2 -- test/views/file-patch-selection.test.js | 26 ++++++++++++------------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/lib/views/file-patch-view.js b/lib/views/file-patch-view.js index b041c912b5..f3be001ad0 100644 --- a/lib/views/file-patch-view.js +++ b/lib/views/file-patch-view.js @@ -7,8 +7,6 @@ import etch from 'etch' import HunkView from './hunk-view' import FilePatchSelection from './file-patch-selection' -const EMPTY_SET = new Set() - export default class FilePatchView { constructor (props) { this.props = props diff --git a/test/views/file-patch-selection.test.js b/test/views/file-patch-selection.test.js index c462264dfa..2e398d7a9c 100644 --- a/test/views/file-patch-selection.test.js +++ b/test/views/file-patch-selection.test.js @@ -7,7 +7,7 @@ import {assertEqualSets} from '../helpers' describe('FilePatchSelection', () => { describe('line selection', () => { - it('starts a new line selection with selectLine and updates an existing selection when preserveTail is true', () => { + it('starts a new line selection with selectLine and updates an existing selection when preserveTail is true', () => { const hunks = [ new Hunk(1, 1, 1, 3, [ new HunkLine('line-1', 'added', -1, 1), @@ -505,7 +505,7 @@ describe('FilePatchSelection', () => { it('does not blow up when coalescing with no selections', function () { const hunks = [ new Hunk(1, 1, 0, 1, [ - new HunkLine('line-1', 'added', -1, 1), + new HunkLine('line-1', 'added', -1, 1) ]) ] const selection = new FilePatchSelection(hunks) @@ -522,11 +522,11 @@ describe('FilePatchSelection', () => { it('selects the first hunk by default', function () { const hunks = [ new Hunk(1, 1, 0, 1, [ - new HunkLine('line-1', 'added', -1, 1), + new HunkLine('line-1', 'added', -1, 1) ]), new Hunk(5, 6, 0, 1, [ - new HunkLine('line-2', 'added', -1, 6), - ]), + new HunkLine('line-2', 'added', -1, 6) + ]) ] const selection = new FilePatchSelection(hunks) assertEqualSets(selection.getSelectedHunks(), new Set([hunks[0]])) @@ -535,16 +535,16 @@ describe('FilePatchSelection', () => { it('starts a new hunk selection with selectHunk and updates an existing selection when preserveTail is true', function () { const hunks = [ new Hunk(1, 1, 0, 1, [ - new HunkLine('line-1', 'added', -1, 1), + new HunkLine('line-1', 'added', -1, 1) ]), new Hunk(5, 6, 0, 1, [ - new HunkLine('line-2', 'added', -1, 6), + new HunkLine('line-2', 'added', -1, 6) ]), new Hunk(10, 12, 0, 1, [ - new HunkLine('line-3', 'added', -1, 12), + new HunkLine('line-3', 'added', -1, 12) ]), new Hunk(15, 18, 0, 1, [ - new HunkLine('line-4', 'added', -1, 18), + new HunkLine('line-4', 'added', -1, 18) ]) ] const selection = new FilePatchSelection(hunks) @@ -562,16 +562,16 @@ describe('FilePatchSelection', () => { it('adds a new hunk selection with addOrSubtractHunkSelection and always updates the head of the most recent hunk selection', function () { const hunks = [ new Hunk(1, 1, 0, 1, [ - new HunkLine('line-1', 'added', -1, 1), + new HunkLine('line-1', 'added', -1, 1) ]), new Hunk(5, 6, 0, 1, [ - new HunkLine('line-2', 'added', -1, 6), + new HunkLine('line-2', 'added', -1, 6) ]), new Hunk(10, 12, 0, 1, [ - new HunkLine('line-3', 'added', -1, 12), + new HunkLine('line-3', 'added', -1, 12) ]), new Hunk(15, 18, 0, 1, [ - new HunkLine('line-4', 'added', -1, 18), + new HunkLine('line-4', 'added', -1, 18) ]) ] const selection = new FilePatchSelection(hunks) From b7fe2550c5b8ad6979eb0e2afe55d21fc1833471 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Fri, 28 Oct 2016 16:26:45 -0600 Subject: [PATCH 14/25] Fix integration tests on GitPanelController --- lib/views/staging-view.js | 8 ++++---- test/controllers/git-panel-controller.test.js | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index 25d9574a72..1721e4c237 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -108,12 +108,12 @@ export default class StagingView { focusFilePatchView () { } - mousedownOnItem (event, item) { + async mousedownOnItem (event, item) { if (event.detail >= 2) { if (this.selection.listKeyForItem(item) === 'staged') { - this.props.unstageFiles([item.getPath()]) + await this.props.unstageFiles([item.getPath()]) } else { - this.props.stageFiles([item.getPath()]) + await this.props.stageFiles([item.getPath()]) } } else { if (event.ctrlKey || event.metaKey) { @@ -123,7 +123,7 @@ export default class StagingView { } this.mouseSelectionInProgress = true } - return etch.update(this) + await etch.update(this) } mousemoveOnItem (event, item) { diff --git a/test/controllers/git-panel-controller.test.js b/test/controllers/git-panel-controller.test.js index a63166e472..3e4f64bef2 100644 --- a/test/controllers/git-panel-controller.test.js +++ b/test/controllers/git-panel-controller.test.js @@ -90,13 +90,13 @@ describe('GitPanelController', () => { assert.equal(stagingView.props.unstagedChanges.length, 2) assert.equal(stagingView.props.stagedChanges.length, 0) - await stagingView.stageFilePatch(stagingView.props.unstagedChanges[0]) + await stagingView.mousedownOnItem({detail: 2}, stagingView.props.unstagedChanges[0]) await controller.getLastModelDataRefreshPromise() - await stagingView.stageFilePatch(stagingView.props.unstagedChanges[0]) + await stagingView.mousedownOnItem({detail: 2}, stagingView.props.unstagedChanges[0]) await controller.getLastModelDataRefreshPromise() assert.equal(stagingView.props.unstagedChanges.length, 0) assert.equal(stagingView.props.stagedChanges.length, 2) - await stagingView.unstageFilePatch(stagingView.props.stagedChanges[1]) + await stagingView.mousedownOnItem({detail: 2}, stagingView.props.stagedChanges[1]) await controller.getLastModelDataRefreshPromise() assert.equal(stagingView.props.unstagedChanges.length, 1) assert.equal(stagingView.props.stagedChanges.length, 1) @@ -137,7 +137,7 @@ describe('GitPanelController', () => { // click Cancel choice = 1 - await stagingView.stageFilePatch(conflict1) + await stagingView.mousedownOnItem({detail: 2}, conflict1) await controller.getLastModelDataRefreshPromise() assert.equal(atom.confirm.calledOnce, true) assert.equal(stagingView.props.mergeConflicts.length, 5) @@ -146,7 +146,7 @@ describe('GitPanelController', () => { // click Stage choice = 0 atom.confirm.reset() - await stagingView.stageFilePatch(conflict1) + await stagingView.mousedownOnItem({detail: 2}, conflict1) await controller.getLastModelDataRefreshPromise() assert.equal(atom.confirm.calledOnce, true) assert.equal(stagingView.props.mergeConflicts.length, 4) @@ -156,7 +156,7 @@ describe('GitPanelController', () => { const conflict2 = stagingView.props.mergeConflicts.filter((c) => c.getPath() === 'modified-on-both-theirs.txt')[0] atom.confirm.reset() fs.writeFileSync(path.join(workdirPath, conflict2.getPath()), 'text with no merge markers') - await stagingView.stageFilePatch(conflict2) + await stagingView.mousedownOnItem({detail: 2}, conflict2) await controller.getLastModelDataRefreshPromise() assert.equal(atom.confirm.called, false) assert.equal(stagingView.props.mergeConflicts.length, 3) From 57d0d1bfc6597b8cba0f7725c25528807334ca20 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Fri, 28 Oct 2016 20:19:51 -0600 Subject: [PATCH 15/25] Get StagingView tests passing Delete some that have been subsumed by unit tests on the selection. --- lib/views/staging-view.js | 10 +- test/views/staging-view.test.js | 319 ++------------------------------ 2 files changed, 18 insertions(+), 311 deletions(-) diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index 1721e4c237..358e1224bc 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -161,9 +161,9 @@ export default class StagingView { Merge Conflicts -
+
{ - this.props.mergeConflicts.map((mergeConflict) => ( + (this.props.mergeConflicts || []).map((mergeConflict) => ( this.mousedownOnItem(event, mergeConflict)} @@ -184,7 +184,7 @@ export default class StagingView { Unstaged Changes -
+
{ this.props.unstagedChanges.map((filePatch) => (
- { this.mergeConflicts ? mergeConflictsView :