Commit 3d46d7ca authored by Phil Hughes's avatar Phil Hughes

Merge branch...

Merge branch '205938-line-numbering-in-diff-perhaps-related-to-show-unchanged-lines-seems-broken' into 'master'

Fix parallel diff expansion line numbers

See merge request gitlab-org/gitlab!25303
parents e2fe1d7e b8166a20
......@@ -3,7 +3,7 @@ import { mapState, mapActions } from 'vuex';
import createFlash from '~/flash';
import { s__ } from '~/locale';
import Icon from '~/vue_shared/components/icon.vue';
import { UNFOLD_COUNT } from '../constants';
import { UNFOLD_COUNT, INLINE_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE } from '../constants';
import * as utils from '../store/utils';
import tooltip from '../../vue_shared/directives/tooltip';
......@@ -11,6 +11,16 @@ const EXPAND_ALL = 0;
const EXPAND_UP = 1;
const EXPAND_DOWN = 2;
const lineNumberByViewType = (viewType, diffLine) => {
const numberGetters = {
[INLINE_DIFF_VIEW_TYPE]: line => line?.new_line,
[PARALLEL_DIFF_VIEW_TYPE]: line => (line?.right || line?.left)?.new_line,
};
const numberGetter = numberGetters[viewType];
return numberGetter && numberGetter(diffLine);
};
export default {
directives: {
tooltip,
......@@ -67,12 +77,16 @@ export default {
...mapActions('diffs', ['loadMoreLines']),
getPrevLineNumber(oldLineNumber, newLineNumber) {
const diffFile = utils.findDiffFile(this.diffFiles, this.fileHash);
const indexForInline = utils.findIndexInInlineLines(diffFile.highlighted_diff_lines, {
const lines = {
[INLINE_DIFF_VIEW_TYPE]: diffFile.highlighted_diff_lines,
[PARALLEL_DIFF_VIEW_TYPE]: diffFile.parallel_diff_lines,
};
const index = utils.getPreviousLineIndex(this.diffViewType, diffFile, {
oldLineNumber,
newLineNumber,
});
const prevLine = diffFile.highlighted_diff_lines[indexForInline - 2];
return (prevLine && prevLine.new_line) || 0;
return lineNumberByViewType(this.diffViewType, lines[this.diffViewType][index - 2]) || 0;
},
callLoadMoreLines(
endpoint,
......@@ -114,7 +128,7 @@ export default {
this.handleExpandAllLines(expandOptions);
}
},
handleExpandUpLines(expandOptions = EXPAND_ALL) {
handleExpandUpLines(expandOptions) {
const { endpoint, fileHash, view, oldLineNumber, newLineNumber, offset } = expandOptions;
const bottom = this.isBottom;
......
......@@ -140,6 +140,7 @@ export default {
addContextLines({
inlineLines: diffFile.highlighted_diff_lines,
parallelLines: diffFile.parallel_diff_lines,
diffViewType: state.diffViewType,
contextLines: lines,
bottom,
lineNumbers,
......
......@@ -13,6 +13,8 @@ import {
LINES_TO_BE_RENDERED_DIRECTLY,
MAX_LINES_TO_BE_RENDERED,
TREE_TYPE,
INLINE_DIFF_VIEW_TYPE,
PARALLEL_DIFF_VIEW_TYPE,
} from '../constants';
export function findDiffFile(files, match, matchKey = 'file_hash') {
......@@ -93,8 +95,7 @@ export function getNoteFormData(params) {
export const findIndexInInlineLines = (lines, lineNumbers) => {
const { oldLineNumber, newLineNumber } = lineNumbers;
return _.findIndex(
lines,
return lines.findIndex(
line => line.old_line === oldLineNumber && line.new_line === newLineNumber,
);
};
......@@ -102,8 +103,7 @@ export const findIndexInInlineLines = (lines, lineNumbers) => {
export const findIndexInParallelLines = (lines, lineNumbers) => {
const { oldLineNumber, newLineNumber } = lineNumbers;
return _.findIndex(
lines,
return lines.findIndex(
line =>
line.left &&
line.right &&
......@@ -112,13 +112,32 @@ export const findIndexInParallelLines = (lines, lineNumbers) => {
);
};
const indexGettersByViewType = {
[INLINE_DIFF_VIEW_TYPE]: findIndexInInlineLines,
[PARALLEL_DIFF_VIEW_TYPE]: findIndexInParallelLines,
};
export const getPreviousLineIndex = (diffViewType, file, lineNumbers) => {
const findIndex = indexGettersByViewType[diffViewType];
const lines = {
[INLINE_DIFF_VIEW_TYPE]: file.highlighted_diff_lines,
[PARALLEL_DIFF_VIEW_TYPE]: file.parallel_diff_lines,
};
return findIndex && findIndex(lines[diffViewType], lineNumbers);
};
export function removeMatchLine(diffFile, lineNumbers, bottom) {
const indexForInline = findIndexInInlineLines(diffFile.highlighted_diff_lines, lineNumbers);
const indexForParallel = findIndexInParallelLines(diffFile.parallel_diff_lines, lineNumbers);
const factor = bottom ? 1 : -1;
diffFile.highlighted_diff_lines.splice(indexForInline + factor, 1);
diffFile.parallel_diff_lines.splice(indexForParallel + factor, 1);
if (indexForInline > -1) {
diffFile.highlighted_diff_lines.splice(indexForInline + factor, 1);
}
if (indexForParallel > -1) {
diffFile.parallel_diff_lines.splice(indexForParallel + factor, 1);
}
}
export function addLineReferences(lines, lineNumbers, bottom, isExpandDown, nextLineNumbers) {
......@@ -160,8 +179,8 @@ export function addLineReferences(lines, lineNumbers, bottom, isExpandDown, next
return linesWithNumbers;
}
export function addContextLines(options) {
const { inlineLines, parallelLines, contextLines, lineNumbers, isExpandDown } = options;
function addParallelContextLines(options) {
const { parallelLines, contextLines, lineNumbers, isExpandDown } = options;
const normalizedParallelLines = contextLines.map(line => ({
left: line,
right: line,
......@@ -170,17 +189,40 @@ export function addContextLines(options) {
const factor = isExpandDown ? 1 : 0;
if (!isExpandDown && options.bottom) {
inlineLines.push(...contextLines);
parallelLines.push(...normalizedParallelLines);
} else {
const inlineIndex = findIndexInInlineLines(inlineLines, lineNumbers);
const parallelIndex = findIndexInParallelLines(parallelLines, lineNumbers);
inlineLines.splice(inlineIndex + factor, 0, ...contextLines);
parallelLines.splice(parallelIndex + factor, 0, ...normalizedParallelLines);
}
}
function addInlineContextLines(options) {
const { inlineLines, contextLines, lineNumbers, isExpandDown } = options;
const factor = isExpandDown ? 1 : 0;
if (!isExpandDown && options.bottom) {
inlineLines.push(...contextLines);
} else {
const inlineIndex = findIndexInInlineLines(inlineLines, lineNumbers);
inlineLines.splice(inlineIndex + factor, 0, ...contextLines);
}
}
export function addContextLines(options) {
const { diffViewType } = options;
const contextLineHandlers = {
[INLINE_DIFF_VIEW_TYPE]: addInlineContextLines,
[PARALLEL_DIFF_VIEW_TYPE]: addParallelContextLines,
};
const contextLineHandler = contextLineHandlers[diffViewType];
if (contextLineHandler) {
contextLineHandler(options);
}
}
/**
* Trims the first char of the `richText` property when it's either a space or a diff symbol.
* @param {Object} line
......
......@@ -7,10 +7,11 @@ describe 'User views diffs', :js do
create(:merge_request_with_diffs, source_project: project, target_project: project, source_branch: 'merge-test')
end
let(:project) { create(:project, :public, :repository) }
let(:view) { 'inline' }
before do
stub_feature_flags(diffs_batch_load: false)
visit(diffs_project_merge_request_path(project, merge_request))
visit(diffs_project_merge_request_path(project, merge_request, view: view))
wait_for_requests
......@@ -20,12 +21,20 @@ describe 'User views diffs', :js do
shared_examples 'unfold diffs' do
it 'unfolds diffs upwards' do
first('.js-unfold').click
expect(find('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"] .text-file')).to have_content('.bundle')
page.within('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"]') do
expect(find('.text-file')).to have_content('.bundle')
expect(page).to have_selector('.new_line [data-linenumber="1"]', count: 1)
end
end
it 'unfolds diffs to the start' do
first('.js-unfold-all').click
expect(find('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"] .text-file')).to have_content('.rbc')
it 'unfolds diffs in the middle' do
page.within('.file-holder[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd"]') do
all('.js-unfold-all')[1].click
expect(page).to have_selector('.new_line [data-linenumber="24"]', count: 1)
expect(page).not_to have_selector('.new_line [data-linenumber="1"]')
end
end
it 'unfolds diffs downwards' do
......@@ -66,13 +75,7 @@ describe 'User views diffs', :js do
end
context 'when in the side-by-side view' do
before do
find('.js-show-diff-settings').click
click_button 'Side-by-side'
wait_for_requests
end
let(:view) { 'parallel' }
it 'shows diffs in parallel' do
expect(page).to have_css('.parallel')
......
import Vue from 'vue';
import { cloneDeep } from 'lodash';
import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
import { createStore } from '~/mr_notes/stores';
import DiffExpansionCell from '~/diffs/components/diff_expansion_cell.vue';
import { getPreviousLineIndex } from '~/diffs/store/utils';
import { INLINE_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE } from '~/diffs/constants';
import diffFileMockData from '../mock_data/diff_file';
const EXPAND_UP_CLASS = '.js-unfold';
const EXPAND_DOWN_CLASS = '.js-unfold-down';
const EXPAND_ALL_CLASS = '.js-unfold-all';
const LINE_TO_USE = 5;
const lineSources = {
[INLINE_DIFF_VIEW_TYPE]: 'highlighted_diff_lines',
[PARALLEL_DIFF_VIEW_TYPE]: 'parallel_diff_lines',
};
const lineHandlers = {
[INLINE_DIFF_VIEW_TYPE]: line => line,
[PARALLEL_DIFF_VIEW_TYPE]: line => line.right || line.left,
};
function makeLoadMoreLinesPayload({
sinceLine,
toLine,
oldLineNumber,
diffViewType,
fileHash,
nextLineNumbers = {},
unfold = false,
bottom = false,
isExpandDown = false,
}) {
return {
endpoint: 'contextLinesPath',
params: {
since: sinceLine,
to: toLine,
offset: toLine + 1 - oldLineNumber,
view: diffViewType,
unfold,
bottom,
},
lineNumbers: {
oldLineNumber,
newLineNumber: toLine + 1,
},
nextLineNumbers,
fileHash,
isExpandDown,
};
}
function getLine(file, type, index) {
const source = lineSources[type];
const handler = lineHandlers[type];
return handler(file[source][index]);
}
describe('DiffExpansionCell', () => {
const matchLine = diffFileMockData.highlighted_diff_lines[5];
let mockFile;
let mockLine;
let store;
let vm;
beforeEach(() => {
mockFile = cloneDeep(diffFileMockData);
mockLine = getLine(mockFile, INLINE_DIFF_VIEW_TYPE, LINE_TO_USE);
store = createStore();
store.state.diffs.diffFiles = [mockFile];
spyOn(store, 'dispatch').and.returnValue(Promise.resolve());
});
const createComponent = (options = {}) => {
const cmp = Vue.extend(DiffExpansionCell);
const defaults = {
fileHash: diffFileMockData.file_hash,
fileHash: mockFile.file_hash,
contextLinesPath: 'contextLinesPath',
line: matchLine,
line: mockLine,
isTop: false,
isBottom: false,
};
const props = Object.assign({}, defaults, options);
return createComponentWithStore(cmp, createStore(), props).$mount();
vm = createComponentWithStore(cmp, store, props).$mount();
};
const findExpandUp = () => vm.$el.querySelector(EXPAND_UP_CLASS);
const findExpandDown = () => vm.$el.querySelector(EXPAND_DOWN_CLASS);
const findExpandAll = () => vm.$el.querySelector(EXPAND_ALL_CLASS);
describe('top row', () => {
it('should have "expand up" and "show all" option', () => {
const vm = createComponent({
createComponent({
isTop: true,
});
const el = vm.$el;
expect(el.querySelector(EXPAND_UP_CLASS)).not.toBe(null);
expect(el.querySelector(EXPAND_DOWN_CLASS)).toBe(null);
expect(el.querySelector(EXPAND_ALL_CLASS)).not.toBe(null);
expect(findExpandUp()).not.toBe(null);
expect(findExpandDown()).toBe(null);
expect(findExpandAll()).not.toBe(null);
});
});
describe('middle row', () => {
it('should have "expand down", "show all", "expand up" option', () => {
const vm = createComponent();
const el = vm.$el;
createComponent();
expect(el.querySelector(EXPAND_UP_CLASS)).not.toBe(null);
expect(el.querySelector(EXPAND_DOWN_CLASS)).not.toBe(null);
expect(el.querySelector(EXPAND_ALL_CLASS)).not.toBe(null);
expect(findExpandUp()).not.toBe(null);
expect(findExpandDown()).not.toBe(null);
expect(findExpandAll()).not.toBe(null);
});
});
describe('bottom row', () => {
it('should have "expand down" and "show all" option', () => {
const vm = createComponent({
createComponent({
isBottom: true,
});
const el = vm.$el;
expect(el.querySelector(EXPAND_UP_CLASS)).toBe(null);
expect(el.querySelector(EXPAND_DOWN_CLASS)).not.toBe(null);
expect(el.querySelector(EXPAND_ALL_CLASS)).not.toBe(null);
expect(findExpandUp()).toBe(null);
expect(findExpandDown()).not.toBe(null);
expect(findExpandAll()).not.toBe(null);
});
});
describe('any row', () => {
[
{ diffViewType: INLINE_DIFF_VIEW_TYPE, file: { parallel_diff_lines: [] } },
{ diffViewType: PARALLEL_DIFF_VIEW_TYPE, file: { highlighted_diff_lines: [] } },
].forEach(({ diffViewType, file }) => {
describe(`with diffViewType (${diffViewType})`, () => {
beforeEach(() => {
mockLine = getLine(mockFile, diffViewType, LINE_TO_USE);
store.state.diffs.diffFiles = [{ ...mockFile, ...file }];
store.state.diffs.diffViewType = diffViewType;
});
it('does not initially dispatch anything', () => {
expect(store.dispatch).not.toHaveBeenCalled();
});
it('on expand all clicked, dispatch loadMoreLines', () => {
const oldLineNumber = mockLine.meta_data.old_pos;
const newLineNumber = mockLine.meta_data.new_pos;
const previousIndex = getPreviousLineIndex(diffViewType, mockFile, {
oldLineNumber,
newLineNumber,
});
createComponent();
findExpandAll().click();
expect(store.dispatch).toHaveBeenCalledWith(
'diffs/loadMoreLines',
makeLoadMoreLinesPayload({
fileHash: mockFile.file_hash,
toLine: newLineNumber - 1,
sinceLine: previousIndex,
oldLineNumber,
diffViewType,
}),
);
});
it('on expand up clicked, dispatch loadMoreLines', () => {
mockLine.meta_data.old_pos = 200;
mockLine.meta_data.new_pos = 200;
const oldLineNumber = mockLine.meta_data.old_pos;
const newLineNumber = mockLine.meta_data.new_pos;
createComponent();
findExpandUp().click();
expect(store.dispatch).toHaveBeenCalledWith(
'diffs/loadMoreLines',
makeLoadMoreLinesPayload({
fileHash: mockFile.file_hash,
toLine: newLineNumber - 1,
sinceLine: 179,
oldLineNumber,
diffViewType,
unfold: true,
}),
);
});
it('on expand down clicked, dispatch loadMoreLines', () => {
mockFile[lineSources[diffViewType]][LINE_TO_USE + 1] = cloneDeep(
mockFile[lineSources[diffViewType]][LINE_TO_USE],
);
const nextLine = getLine(mockFile, diffViewType, LINE_TO_USE + 1);
nextLine.meta_data.old_pos = 300;
nextLine.meta_data.new_pos = 300;
mockLine.meta_data.old_pos = 200;
mockLine.meta_data.new_pos = 200;
createComponent();
findExpandDown().click();
expect(store.dispatch).toHaveBeenCalledWith('diffs/loadMoreLines', {
endpoint: 'contextLinesPath',
params: {
since: 1,
to: 21, // the load amount, plus 1 line
offset: 0,
view: diffViewType,
unfold: true,
bottom: true,
},
lineNumbers: {
// when expanding down, these are based on the previous line, 0, in this case
oldLineNumber: 0,
newLineNumber: 0,
},
nextLineNumbers: { old_line: 200, new_line: 200 },
fileHash: mockFile.file_hash,
isExpandDown: true,
});
});
});
});
});
});
......@@ -167,7 +167,7 @@ describe('DiffsStoreMutations', () => {
highlighted_diff_lines: [],
parallel_diff_lines: [],
};
const state = { diffFiles: [diffFile] };
const state = { diffFiles: [diffFile], diffViewType: 'viewType' };
const lines = [{ old_line: 1, new_line: 1 }];
const findDiffFileSpy = spyOnDependency(mutations, 'findDiffFile').and.returnValue(diffFile);
......@@ -195,6 +195,7 @@ describe('DiffsStoreMutations', () => {
expect(addContextLinesSpy).toHaveBeenCalledWith({
inlineLines: diffFile.highlighted_diff_lines,
parallelLines: diffFile.parallel_diff_lines,
diffViewType: 'viewType',
contextLines: options.contextLines,
bottom: options.params.bottom,
lineNumbers: options.lineNumbers,
......
import { clone } from 'lodash';
import * as utils from '~/diffs/store/utils';
import {
LINE_POSITION_LEFT,
......@@ -8,6 +9,7 @@ import {
NEW_LINE_TYPE,
OLD_LINE_TYPE,
MATCH_LINE_TYPE,
INLINE_DIFF_VIEW_TYPE,
PARALLEL_DIFF_VIEW_TYPE,
} from '~/diffs/constants';
import { MERGE_REQUEST_NOTEABLE_TYPE } from '~/notes/constants';
......@@ -47,7 +49,38 @@ describe('DiffsStoreUtils', () => {
describe('findIndexInParallelLines', () => {
it('should return correct index for given line numbers', () => {
expectSet(utils.findIndexInParallelLines, getDiffFileMock().parallel_diff_lines, {});
expectSet(utils.findIndexInParallelLines, getDiffFileMock().parallel_diff_lines, []);
});
});
});
describe('getPreviousLineIndex', () => {
[
{ diffViewType: INLINE_DIFF_VIEW_TYPE, file: { parallel_diff_lines: [] } },
{ diffViewType: PARALLEL_DIFF_VIEW_TYPE, file: { highlighted_diff_lines: [] } },
].forEach(({ diffViewType, file }) => {
describe(`with diffViewType (${diffViewType}) in split diffs`, () => {
let diffFile;
beforeEach(() => {
diffFile = { ...clone(diffFileMockData), ...file };
});
it('should return the correct previous line number', () => {
const emptyLines =
diffViewType === INLINE_DIFF_VIEW_TYPE
? diffFile.parallel_diff_lines
: diffFile.highlighted_diff_lines;
// This expectation asserts that we cannot possibly be using the opposite view type lines in the next expectation
expect(emptyLines.length).toBe(0);
expect(
utils.getPreviousLineIndex(diffViewType, diffFile, {
oldLineNumber: 3,
newLineNumber: 5,
}),
).toBe(4);
});
});
});
});
......@@ -80,44 +113,59 @@ describe('DiffsStoreUtils', () => {
});
describe('addContextLines', () => {
it('should add context lines', () => {
const diffFile = getDiffFileMock();
const inlineLines = diffFile.highlighted_diff_lines;
const parallelLines = diffFile.parallel_diff_lines;
const lineNumbers = { oldLineNumber: 3, newLineNumber: 5 };
const contextLines = [{ lineNumber: 42, line_code: '123' }];
const options = { inlineLines, parallelLines, contextLines, lineNumbers };
const inlineIndex = utils.findIndexInInlineLines(inlineLines, lineNumbers);
const parallelIndex = utils.findIndexInParallelLines(parallelLines, lineNumbers);
const normalizedParallelLine = {
left: options.contextLines[0],
right: options.contextLines[0],
line_code: '123',
};
utils.addContextLines(options);
expect(inlineLines[inlineIndex]).toEqual(contextLines[0]);
expect(parallelLines[parallelIndex]).toEqual(normalizedParallelLine);
});
it('should add context lines properly with bottom parameter', () => {
const diffFile = getDiffFileMock();
const inlineLines = diffFile.highlighted_diff_lines;
const parallelLines = diffFile.parallel_diff_lines;
const lineNumbers = { oldLineNumber: 3, newLineNumber: 5 };
const contextLines = [{ lineNumber: 42, line_code: '123' }];
const options = { inlineLines, parallelLines, contextLines, lineNumbers, bottom: true };
const normalizedParallelLine = {
left: options.contextLines[0],
right: options.contextLines[0],
line_code: '123',
};
utils.addContextLines(options);
[INLINE_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE].forEach(diffViewType => {
it(`should add context lines for ${diffViewType}`, () => {
const diffFile = getDiffFileMock();
const inlineLines = diffFile.highlighted_diff_lines;
const parallelLines = diffFile.parallel_diff_lines;
const lineNumbers = { oldLineNumber: 3, newLineNumber: 5 };
const contextLines = [{ lineNumber: 42, line_code: '123' }];
const options = { inlineLines, parallelLines, contextLines, lineNumbers, diffViewType };
const inlineIndex = utils.findIndexInInlineLines(inlineLines, lineNumbers);
const parallelIndex = utils.findIndexInParallelLines(parallelLines, lineNumbers);
const normalizedParallelLine = {
left: options.contextLines[0],
right: options.contextLines[0],
line_code: '123',
};
utils.addContextLines(options);
if (diffViewType === INLINE_DIFF_VIEW_TYPE) {
expect(inlineLines[inlineIndex]).toEqual(contextLines[0]);
} else {
expect(parallelLines[parallelIndex]).toEqual(normalizedParallelLine);
}
});
expect(inlineLines[inlineLines.length - 1]).toEqual(contextLines[0]);
expect(parallelLines[parallelLines.length - 1]).toEqual(normalizedParallelLine);
it(`should add context lines properly with bottom parameter for ${diffViewType}`, () => {
const diffFile = getDiffFileMock();
const inlineLines = diffFile.highlighted_diff_lines;
const parallelLines = diffFile.parallel_diff_lines;
const lineNumbers = { oldLineNumber: 3, newLineNumber: 5 };
const contextLines = [{ lineNumber: 42, line_code: '123' }];
const options = {
inlineLines,
parallelLines,
contextLines,
lineNumbers,
bottom: true,
diffViewType,
};
const normalizedParallelLine = {
left: options.contextLines[0],
right: options.contextLines[0],
line_code: '123',
};
utils.addContextLines(options);
if (diffViewType === INLINE_DIFF_VIEW_TYPE) {
expect(inlineLines[inlineLines.length - 1]).toEqual(contextLines[0]);
} else {
expect(parallelLines[parallelLines.length - 1]).toEqual(normalizedParallelLine);
}
});
});
});
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment