diff --git a/app/assets/javascripts/diffs/components/diff_expansion_cell.vue b/app/assets/javascripts/diffs/components/diff_expansion_cell.vue new file mode 100644 index 0000000000000000000000000000000000000000..6c4096884684ff3d03bd028f1859e310f058a85b --- /dev/null +++ b/app/assets/javascripts/diffs/components/diff_expansion_cell.vue @@ -0,0 +1,246 @@ +<script> +import createFlash from '~/flash'; +import { s__ } from '~/locale'; +import { mapState, mapActions } from 'vuex'; +import Icon from '~/vue_shared/components/icon.vue'; +import { UNFOLD_COUNT } from '../constants'; +import * as utils from '../store/utils'; +import tooltip from '../../vue_shared/directives/tooltip'; + +const EXPAND_ALL = 0; +const EXPAND_UP = 1; +const EXPAND_DOWN = 2; + +export default { + directives: { + tooltip, + }, + components: { + Icon, + }, + props: { + fileHash: { + type: String, + required: true, + }, + contextLinesPath: { + type: String, + required: true, + }, + line: { + type: Object, + required: true, + }, + isTop: { + type: Boolean, + required: false, + default: false, + }, + isBottom: { + type: Boolean, + required: false, + default: false, + }, + colspan: { + type: Number, + required: false, + default: 3, + }, + }, + computed: { + ...mapState({ + diffViewType: state => state.diffs.diffViewType, + diffFiles: state => state.diffs.diffFiles, + }), + canExpandUp() { + return !this.isBottom; + }, + canExpandDown() { + return this.isBottom || !this.isTop; + }, + }, + created() { + this.EXPAND_DOWN = EXPAND_DOWN; + this.EXPAND_UP = EXPAND_UP; + }, + methods: { + ...mapActions('diffs', ['loadMoreLines']), + getPrevLineNumber(oldLineNumber, newLineNumber) { + const diffFile = utils.findDiffFile(this.diffFiles, this.fileHash); + const indexForInline = utils.findIndexInInlineLines(diffFile.highlighted_diff_lines, { + oldLineNumber, + newLineNumber, + }); + const prevLine = diffFile.highlighted_diff_lines[indexForInline - 2]; + return (prevLine && prevLine.new_line) || 0; + }, + callLoadMoreLines( + endpoint, + params, + lineNumbers, + fileHash, + isExpandDown = false, + nextLineNumbers = {}, + ) { + this.loadMoreLines({ endpoint, params, lineNumbers, fileHash, isExpandDown, nextLineNumbers }) + .then(() => { + this.isRequesting = false; + }) + .catch(() => { + createFlash(s__('Diffs|Something went wrong while fetching diff lines.')); + this.isRequesting = false; + }); + }, + handleExpandLines(type = EXPAND_ALL) { + if (this.isRequesting) { + return; + } + + this.isRequesting = true; + const endpoint = this.contextLinesPath; + const { fileHash } = this; + const view = this.diffViewType; + const oldLineNumber = this.line.meta_data.old_pos || 0; + const newLineNumber = this.line.meta_data.new_pos || 0; + const offset = newLineNumber - oldLineNumber; + + const expandOptions = { endpoint, fileHash, view, oldLineNumber, newLineNumber, offset }; + + if (type === EXPAND_UP) { + this.handleExpandUpLines(expandOptions); + } else if (type === EXPAND_DOWN) { + this.handleExpandDownLines(expandOptions); + } else { + this.handleExpandAllLines(expandOptions); + } + }, + handleExpandUpLines(expandOptions = EXPAND_ALL) { + const { endpoint, fileHash, view, oldLineNumber, newLineNumber, offset } = expandOptions; + + const bottom = this.isBottom; + const lineNumber = newLineNumber - 1; + const to = lineNumber; + let since = lineNumber - UNFOLD_COUNT; + let unfold = true; + + const prevLineNumber = this.getPrevLineNumber(oldLineNumber, newLineNumber); + if (since <= prevLineNumber + 1) { + since = prevLineNumber + 1; + unfold = false; + } + + const params = { since, to, bottom, offset, unfold, view }; + const lineNumbers = { oldLineNumber, newLineNumber }; + this.callLoadMoreLines(endpoint, params, lineNumbers, fileHash); + }, + handleExpandDownLines(expandOptions) { + const { + endpoint, + fileHash, + view, + oldLineNumber: metaOldPos, + newLineNumber: metaNewPos, + offset, + } = expandOptions; + + const bottom = true; + const nextLineNumbers = { + old_line: metaOldPos, + new_line: metaNewPos, + }; + + let unfold = true; + let isExpandDown = false; + let oldLineNumber = metaOldPos; + let newLineNumber = metaNewPos; + let lineNumber = metaNewPos + 1; + let since = lineNumber; + let to = lineNumber + UNFOLD_COUNT; + + if (!this.isBottom) { + const prevLineNumber = this.getPrevLineNumber(oldLineNumber, newLineNumber); + + isExpandDown = true; + oldLineNumber = prevLineNumber - offset; + newLineNumber = prevLineNumber; + lineNumber = prevLineNumber + 1; + since = lineNumber; + to = lineNumber + UNFOLD_COUNT; + + if (to >= metaNewPos) { + to = metaNewPos - 1; + unfold = false; + } + } + + const params = { since, to, bottom, offset, unfold, view }; + const lineNumbers = { oldLineNumber, newLineNumber }; + this.callLoadMoreLines( + endpoint, + params, + lineNumbers, + fileHash, + isExpandDown, + nextLineNumbers, + ); + }, + handleExpandAllLines(expandOptions) { + const { endpoint, fileHash, view, oldLineNumber, newLineNumber, offset } = expandOptions; + const bottom = this.isBottom; + const unfold = false; + let since; + let to; + + if (this.isTop) { + since = 1; + to = newLineNumber - 1; + } else if (bottom) { + since = newLineNumber + 1; + to = -1; + } else { + const prevLineNumber = this.getPrevLineNumber(oldLineNumber, newLineNumber); + since = prevLineNumber + 1; + to = newLineNumber - 1; + } + + const params = { since, to, bottom, offset, unfold, view }; + const lineNumbers = { oldLineNumber, newLineNumber }; + this.callLoadMoreLines(endpoint, params, lineNumbers, fileHash); + }, + }, +}; +</script> + +<template> + <td :colspan="colspan"> + <div class="content"> + <a + v-if="canExpandUp" + v-tooltip + class="cursor-pointer js-unfold unfold-icon" + data-placement="top" + data-container="body" + :title="__('Expand up')" + @click="handleExpandLines(EXPAND_UP)" + > + <!-- TODO: remove style & replace with correct icon, waiting for MR https://gitlab.com/gitlab-org/gitlab-design/issues/499 --> + <icon :size="12" name="expand-left" aria-hidden="true" style="transform: rotate(270deg);" /> + </a> + <a class="mx-2 cursor-pointer js-unfold-all" @click="handleExpandLines()"> + <span>{{ s__('Diffs|Show all lines') }}</span> + </a> + <a + v-if="canExpandDown" + v-tooltip + class="cursor-pointer js-unfold-down has-tooltip unfold-icon" + data-placement="top" + data-container="body" + :title="__('Expand down')" + @click="handleExpandLines(EXPAND_DOWN)" + > + <!-- TODO: remove style & replace with correct icon, waiting for MR https://gitlab.com/gitlab-org/gitlab-design/issues/499 --> + <icon :size="12" name="expand-left" aria-hidden="true" style="transform: rotate(90deg);" /> + </a> + </div> + </td> +</template> diff --git a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue index 351110f0a87d1d52c3a50022c6789c627f4d0f12..434d554d148eea04628e29fd0e1d3d9c3420a9ba 100644 --- a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue +++ b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue @@ -1,11 +1,8 @@ <script> -import createFlash from '~/flash'; -import { s__ } from '~/locale'; import { mapState, mapGetters, mapActions } from 'vuex'; import Icon from '~/vue_shared/components/icon.vue'; import DiffGutterAvatars from './diff_gutter_avatars.vue'; -import { LINE_POSITION_RIGHT, UNFOLD_COUNT } from '../constants'; -import * as utils from '../store/utils'; +import { LINE_POSITION_RIGHT } from '../constants'; export default { components: { @@ -115,89 +112,36 @@ export default { handleCommentButton() { this.showCommentForm({ lineCode: this.line.line_code, fileHash: this.fileHash }); }, - handleLoadMoreLines() { - if (this.isRequesting) { - return; - } - - this.isRequesting = true; - const endpoint = this.contextLinesPath; - const oldLineNumber = this.line.meta_data.old_pos || 0; - const newLineNumber = this.line.meta_data.new_pos || 0; - const offset = newLineNumber - oldLineNumber; - const bottom = this.isBottom; - const { fileHash } = this; - const view = this.diffViewType; - let unfold = true; - let lineNumber = newLineNumber - 1; - let since = lineNumber - UNFOLD_COUNT; - let to = lineNumber; - - if (bottom) { - lineNumber = newLineNumber + 1; - since = lineNumber; - to = lineNumber + UNFOLD_COUNT; - } else { - const diffFile = utils.findDiffFile(this.diffFiles, this.fileHash); - const indexForInline = utils.findIndexInInlineLines(diffFile.highlighted_diff_lines, { - oldLineNumber, - newLineNumber, - }); - const prevLine = diffFile.highlighted_diff_lines[indexForInline - 2]; - const prevLineNumber = (prevLine && prevLine.new_line) || 0; - - if (since <= prevLineNumber + 1) { - since = prevLineNumber + 1; - unfold = false; - } - } - - const params = { since, to, bottom, offset, unfold, view }; - const lineNumbers = { oldLineNumber, newLineNumber }; - this.loadMoreLines({ endpoint, params, lineNumbers, fileHash }) - .then(() => { - this.isRequesting = false; - }) - .catch(() => { - createFlash(s__('Diffs|Something went wrong while fetching diff lines.')); - this.isRequesting = false; - }); - }, }, }; </script> <template> <div> - <span v-if="isMatchLine" class="context-cell" role="button" @click="handleLoadMoreLines" - >...</span + <button + v-if="shouldRenderCommentButton" + v-show="shouldShowCommentButton" + type="button" + class="add-diff-note js-add-diff-note-button qa-diff-comment" + title="Add a comment to this line" + @click="handleCommentButton" + > + <icon :size="12" name="comment" /> + </button> + <a + v-if="lineNumber" + :data-linenumber="lineNumber" + :href="lineHref" + @click="setHighlightedRow(lineCode)" > - <template v-else> - <button - v-if="shouldRenderCommentButton" - v-show="shouldShowCommentButton" - type="button" - class="add-diff-note js-add-diff-note-button qa-diff-comment" - title="Add a comment to this line" - @click="handleCommentButton" - > - <icon :size="12" name="comment" /> - </button> - <a - v-if="lineNumber" - :data-linenumber="lineNumber" - :href="lineHref" - @click="setHighlightedRow(lineCode)" - > - </a> - <diff-gutter-avatars - v-if="shouldShowAvatarsOnGutter" - :discussions="line.discussions" - :discussions-expanded="line.discussionsExpanded" - @toggleLineDiscussions=" - toggleLineDiscussions({ lineCode, fileHash, expanded: !line.discussionsExpanded }) - " - /> - </template> + </a> + <diff-gutter-avatars + v-if="shouldShowAvatarsOnGutter" + :discussions="line.discussions" + :discussions-expanded="line.discussionsExpanded" + @toggleLineDiscussions=" + toggleLineDiscussions({ lineCode, fileHash, expanded: !line.discussionsExpanded }) + " + /> </div> </template> diff --git a/app/assets/javascripts/diffs/components/inline_diff_expansion_row.vue b/app/assets/javascripts/diffs/components/inline_diff_expansion_row.vue new file mode 100644 index 0000000000000000000000000000000000000000..6e732727f42e7d0820aa2cd7b31e58a2816e81df --- /dev/null +++ b/app/assets/javascripts/diffs/components/inline_diff_expansion_row.vue @@ -0,0 +1,53 @@ +<script> +import Icon from '~/vue_shared/components/icon.vue'; +import DiffExpansionCell from './diff_expansion_cell.vue'; +import { MATCH_LINE_TYPE } from '../constants'; + +export default { + components: { + Icon, + DiffExpansionCell, + }, + props: { + fileHash: { + type: String, + required: true, + }, + contextLinesPath: { + type: String, + required: true, + }, + line: { + type: Object, + required: true, + }, + isTop: { + type: Boolean, + required: false, + default: false, + }, + isBottom: { + type: Boolean, + required: false, + default: false, + }, + }, + computed: { + isMatchLine() { + return this.line.type === MATCH_LINE_TYPE; + }, + }, +}; +</script> + +<template> + <tr v-if="isMatchLine" class="line_expansion match"> + <diff-expansion-cell + :file-hash="fileHash" + :context-lines-path="contextLinesPath" + :line="line" + :is-top="isTop" + :is-bottom="isBottom" + /> + </tr> +</template> diff --git a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue index 2d5262baeecd648bd585c517d641efe4eeec4a54..55a8df43c62fef4fb5b297e0b796c4f872482349 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue @@ -2,6 +2,7 @@ import { mapActions, mapState } from 'vuex'; import DiffTableCell from './diff_table_cell.vue'; import { + MATCH_LINE_TYPE, NEW_LINE_TYPE, OLD_LINE_TYPE, CONTEXT_LINE_TYPE, @@ -58,6 +59,9 @@ export default { inlineRowId() { return this.line.line_code || `${this.fileHash}_${this.line.old_line}_${this.line.new_line}`; }, + isMatchLine() { + return this.line.type === MATCH_LINE_TYPE; + }, }, created() { this.newLineType = NEW_LINE_TYPE; @@ -81,6 +85,7 @@ export default { <template> <tr + v-if="!isMatchLine" :id="inlineRowId" :class="classNameMap" class="line_holder" diff --git a/app/assets/javascripts/diffs/components/inline_diff_view.vue b/app/assets/javascripts/diffs/components/inline_diff_view.vue index b2bc3d9914fe033e99925524d3d2982ee448136e..aee01409db7ef53fbd514cb5f83190b484f806b4 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_view.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue @@ -3,6 +3,7 @@ import { mapGetters } from 'vuex'; import draftCommentsMixin from 'ee_else_ce/diffs/mixins/draft_comments'; import inlineDiffTableRow from './inline_diff_table_row.vue'; import inlineDiffCommentRow from './inline_diff_comment_row.vue'; +import inlineDiffExpansionRow from './inline_diff_expansion_row.vue'; export default { components: { @@ -10,6 +11,7 @@ export default { inlineDiffTableRow, InlineDraftCommentRow: () => import('ee_component/batch_comments/components/inline_draft_comment_row.vue'), + inlineDiffExpansionRow, }, mixins: [draftCommentsMixin], props: { @@ -43,10 +45,24 @@ export default { :data-commit-id="commitId" class="code diff-wrap-lines js-syntax-highlight text-file js-diff-inline-view" > + <!-- Need to insert an empty row to solve "table-layout:fixed" equal width when expansion row is the first line --> + <tr> + <td style="width: 50px;"></td> + <td style="width: 50px;"></td> + <td></td> + </tr> <tbody> <template v-for="(line, index) in diffLines"> + <inline-diff-expansion-row + :key="`expand-${index}`" + :file-hash="diffFile.file_hash" + :context-lines-path="diffFile.context_lines_path" + :line="line" + :is-top="index === 0" + :is-bottom="index + 1 === diffLinesLength" + /> <inline-diff-table-row - :key="line.line_code" + :key="`${line.line_code || index}`" :file-hash="diffFile.file_hash" :context-lines-path="diffFile.context_lines_path" :line="line" diff --git a/app/assets/javascripts/diffs/components/parallel_diff_expansion_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_expansion_row.vue new file mode 100644 index 0000000000000000000000000000000000000000..c1b30eab199b1ef3ff08ab20b13eefc0677a12c8 --- /dev/null +++ b/app/assets/javascripts/diffs/components/parallel_diff_expansion_row.vue @@ -0,0 +1,56 @@ +<script> +import { MATCH_LINE_TYPE } from '../constants'; +import DiffExpansionCell from './diff_expansion_cell.vue'; + +export default { + components: { + DiffExpansionCell, + }, + props: { + fileHash: { + type: String, + required: true, + }, + contextLinesPath: { + type: String, + required: true, + }, + line: { + type: Object, + required: true, + }, + isTop: { + type: Boolean, + required: false, + default: false, + }, + isBottom: { + type: Boolean, + required: false, + default: false, + }, + }, + computed: { + isMatchLineLeft() { + return this.line.left && this.line.left.type === MATCH_LINE_TYPE; + }, + isMatchLineRight() { + return this.line.right && this.line.right.type === MATCH_LINE_TYPE; + }, + }, +}; +</script> +<template> + <tr class="line_expansion match"> + <template v-if="isMatchLineLeft || isMatchLineRight"> + <diff-expansion-cell + :file-hash="fileHash" + :context-lines-path="contextLinesPath" + :line="line.left" + :is-top="isTop" + :is-bottom="isBottom" + :colspan="4" + /> + </template> + </tr> +</template> diff --git a/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue index c60246bf8ef54b1f4f17075022d5f56bb1c7e0c7..4c95d618b0f4a39547ddac0e78b6aa3ad92e5bb6 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue @@ -3,6 +3,7 @@ import { mapActions, mapState } from 'vuex'; import $ from 'jquery'; import DiffTableCell from './diff_table_cell.vue'; import { + MATCH_LINE_TYPE, NEW_LINE_TYPE, OLD_LINE_TYPE, CONTEXT_LINE_TYPE, @@ -75,6 +76,12 @@ export default { }, ]; }, + isMatchLineLeft() { + return this.line.left && this.line.left.type === MATCH_LINE_TYPE; + }, + isMatchLineRight() { + return this.line.right && this.line.right.type === MATCH_LINE_TYPE; + }, }, created() { this.newLineType = NEW_LINE_TYPE; @@ -122,7 +129,7 @@ export default { @mouseover="handleMouseMove" @mouseout="handleMouseMove" > - <template v-if="line.left"> + <template v-if="line.left && !isMatchLineLeft"> <diff-table-cell :file-hash="fileHash" :context-lines-path="contextLinesPath" @@ -148,7 +155,7 @@ export default { <td class="diff-line-num old_line empty-cell"></td> <td class="line_content parallel left-side empty-cell"></td> </template> - <template v-if="line.right"> + <template v-if="line.right && !isMatchLineRight"> <diff-table-cell :file-hash="fileHash" :context-lines-path="contextLinesPath" diff --git a/app/assets/javascripts/diffs/components/parallel_diff_view.vue b/app/assets/javascripts/diffs/components/parallel_diff_view.vue index c477e68c33c4d5080093a736608c64a165050645..d400eb2c5864d93a4e83efd0ffa8b155f3d59a7c 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_view.vue @@ -3,9 +3,11 @@ import { mapGetters } from 'vuex'; import draftCommentsMixin from 'ee_else_ce/diffs/mixins/draft_comments'; import parallelDiffTableRow from './parallel_diff_table_row.vue'; import parallelDiffCommentRow from './parallel_diff_comment_row.vue'; +import parallelDiffExpansionRow from './parallel_diff_expansion_row.vue'; export default { components: { + parallelDiffExpansionRow, parallelDiffTableRow, parallelDiffCommentRow, ParallelDraftCommentRow: () => @@ -43,8 +45,23 @@ export default { :data-commit-id="commitId" class="code diff-wrap-lines js-syntax-highlight text-file" > + <!-- Need to insert an empty row to solve "table-layout:fixed" equal width when expansion row is the first line --> + <tr> + <td style="width: 50px;"></td> + <td></td> + <td style="width: 50px;"></td> + <td></td> + </tr> <tbody> <template v-for="(line, index) in diffLines"> + <parallel-diff-expansion-row + :key="`expand-${index}`" + :file-hash="diffFile.file_hash" + :context-lines-path="diffFile.context_lines_path" + :line="line" + :is-top="index === 0" + :is-bottom="index + 1 === diffLinesLength" + /> <parallel-diff-table-row :key="line.line_code" :file-hash="diffFile.file_hash" diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 32e0d8f42ee79f17f471bb15c9bf365ba2d63f17..8434ef5cc7a1d942df0c58c55258f38e9b6954c2 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -183,7 +183,7 @@ export const cancelCommentForm = ({ commit }, { lineCode, fileHash }) => { }; export const loadMoreLines = ({ commit }, options) => { - const { endpoint, params, lineNumbers, fileHash } = options; + const { endpoint, params, lineNumbers, fileHash, isExpandDown, nextLineNumbers } = options; params.from_merge_request = true; @@ -195,6 +195,8 @@ export const loadMoreLines = ({ commit }, options) => { contextLines, params, fileHash, + isExpandDown, + nextLineNumbers, }); }); }; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index a66f205bbbd4f7115bc12f4f34965288b98c5738..a6915a46c0023e7cb457bfa2d217aceedc0f9940 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -71,18 +71,30 @@ export default { }, [types.ADD_CONTEXT_LINES](state, options) { - const { lineNumbers, contextLines, fileHash } = options; + const { lineNumbers, contextLines, fileHash, isExpandDown, nextLineNumbers } = options; const { bottom } = options.params; const diffFile = findDiffFile(state.diffFiles, fileHash); removeMatchLine(diffFile, lineNumbers, bottom); - const lines = addLineReferences(contextLines, lineNumbers, bottom).map(line => ({ - ...line, - line_code: line.line_code || `${fileHash}_${line.old_line}_${line.new_line}`, - discussions: line.discussions || [], - hasForm: false, - })); + const lines = addLineReferences( + contextLines, + lineNumbers, + bottom, + isExpandDown, + nextLineNumbers, + ).map(line => { + const lineCode = + line.type === 'match' + ? `${fileHash}_${line.meta_data.old_pos}_${line.meta_data.new_pos}_match` + : line.line_code || `${fileHash}_${line.old_line}_${line.new_line}`; + return { + ...line, + line_code: lineCode, + discussions: line.discussions || [], + hasForm: false, + }; + }); addContextLines({ inlineLines: diffFile.highlighted_diff_lines, @@ -90,6 +102,7 @@ export default { contextLines: lines, bottom, lineNumbers, + isExpandDown, }); }, diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 1c3ed84001cf776c0f4f160d3475513f0679f58e..d46bdea9b5076b203be162606fafc5be6bf94519 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -121,7 +121,7 @@ export function removeMatchLine(diffFile, lineNumbers, bottom) { diffFile.parallel_diff_lines.splice(indexForParallel + factor, 1); } -export function addLineReferences(lines, lineNumbers, bottom) { +export function addLineReferences(lines, lineNumbers, bottom, isExpandDown, nextLineNumbers) { const { oldLineNumber, newLineNumber } = lineNumbers; const lineCount = lines.length; let matchLineIndex = -1; @@ -135,15 +135,20 @@ export function addLineReferences(lines, lineNumbers, bottom) { new_line: bottom ? newLineNumber + index + 1 : newLineNumber + index - lineCount, }); } - return l; }); if (matchLineIndex > -1) { const line = linesWithNumbers[matchLineIndex]; - const targetLine = bottom - ? linesWithNumbers[matchLineIndex - 1] - : linesWithNumbers[matchLineIndex + 1]; + let targetLine; + + if (isExpandDown) { + targetLine = nextLineNumbers; + } else if (bottom) { + targetLine = linesWithNumbers[matchLineIndex - 1]; + } else { + targetLine = linesWithNumbers[matchLineIndex + 1]; + } Object.assign(line, { meta_data: { @@ -152,26 +157,27 @@ export function addLineReferences(lines, lineNumbers, bottom) { }, }); } - return linesWithNumbers; } export function addContextLines(options) { - const { inlineLines, parallelLines, contextLines, lineNumbers } = options; + const { inlineLines, parallelLines, contextLines, lineNumbers, isExpandDown } = options; const normalizedParallelLines = contextLines.map(line => ({ left: line, right: line, line_code: line.line_code, })); + const factor = isExpandDown ? 1 : 0; - if (options.bottom) { + if (!isExpandDown && options.bottom) { inlineLines.push(...contextLines); parallelLines.push(...normalizedParallelLines); } else { const inlineIndex = findIndexInInlineLines(inlineLines, lineNumbers); const parallelIndex = findIndexInParallelLines(parallelLines, lineNumbers); - inlineLines.splice(inlineIndex, 0, ...contextLines); - parallelLines.splice(parallelIndex, 0, ...normalizedParallelLines); + + inlineLines.splice(inlineIndex + factor, 0, ...contextLines); + parallelLines.splice(parallelIndex + factor, 0, ...normalizedParallelLines); } } diff --git a/app/assets/stylesheets/highlight/white_base.scss b/app/assets/stylesheets/highlight/white_base.scss index ee0ec94c63601551073c19aa6dfb073e23441109..b3974df86397b5ac5b1a1917935a6f9ee3a90b53 100644 --- a/app/assets/stylesheets/highlight/white_base.scss +++ b/app/assets/stylesheets/highlight/white_base.scss @@ -101,6 +101,26 @@ pre.code, color: $white-code-color; } +// Expansion line +.line_expansion { + background-color: $gray-light; + + td { + border-top: 1px solid $border-color; + border-bottom: 1px solid $border-color; + text-align: center; + } + + a { + color: $blue-600; + } + + .unfold-icon { + display: inline-block; + padding: 8px 0; + } +} + // Diff line .line_holder { &.match .line_content, diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 95ea49ad465088bd75e8b01931f2f6b6e4f4047b..ffb27e54f348455a69f0fb49bd8123aa41d4d332 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -408,6 +408,14 @@ table.code { table-layout: fixed; border-radius: 0 0 $border-radius-default $border-radius-default; + tr:first-of-type.line_expansion > td { + border-top: 0; + } + + tr:nth-last-of-type(2).line_expansion > td { + border-bottom: 0; + } + tr.line_holder td { line-height: $code-line-height; font-size: $code-font-size; diff --git a/app/presenters/blobs/unfold_presenter.rb b/app/presenters/blobs/unfold_presenter.rb index 21a1e1309e03e076a54458f2083319190bc253ac..4c6dd6895cf5dc9f27190c06df8e68dad0d81128 100644 --- a/app/presenters/blobs/unfold_presenter.rb +++ b/app/presenters/blobs/unfold_presenter.rb @@ -9,7 +9,7 @@ module Blobs attribute :full, Boolean, default: false attribute :since, GtOneCoercion - attribute :to, GtOneCoercion + attribute :to, Integer attribute :bottom, Boolean attribute :unfold, Boolean, default: true attribute :offset, Integer @@ -24,9 +24,7 @@ module Blobs @all_lines = blob.data.lines super(params) - if full? - self.attributes = { since: 1, to: @all_lines.size, bottom: false, unfold: false, offset: 0, indent: 0 } - end + self.attributes = prepare_attributes end # Returns an array of Gitlab::Diff::Line with match line added @@ -43,7 +41,9 @@ module Blobs end def lines - @lines ||= limit(highlight.lines).map(&:html_safe) + strong_memoize(:lines) do + limit(highlight.lines).map(&:html_safe) + end end def match_line_text @@ -56,10 +56,34 @@ module Blobs private + def prepare_attributes + return attributes unless full? || to == -1 + + full_opts = { + since: 1, + to: all_lines_size, + bottom: false, + unfold: false, + offset: 0, + indent: 0 + } + + return full_opts if full? + + full_opts.merge(attributes.slice(:since)) + end + + def all_lines_size + strong_memoize(:all_lines_size) do + @all_lines.size + end + end + def add_match_line(diff_lines) return unless unfold? + return if bottom? && to >= all_lines_size - if bottom? && to < @all_lines.size + if bottom? && to < all_lines_size old_pos = to - offset new_pos = to elsif since != 1 @@ -75,7 +99,9 @@ module Blobs end def limited_blob_lines - @limited_blob_lines ||= limit(@all_lines) + strong_memoize(:limited_blob_lines) do + limit(@all_lines) + end end def limit(lines) diff --git a/changelogs/unreleased/58035-expand-mr-diff.yml b/changelogs/unreleased/58035-expand-mr-diff.yml new file mode 100644 index 0000000000000000000000000000000000000000..7163cce29f24cf21d5f47c779a0076377428446a --- /dev/null +++ b/changelogs/unreleased/58035-expand-mr-diff.yml @@ -0,0 +1,5 @@ +--- +title: Add new expansion options for merge request diffs +merge_request: 30927 +author: +type: added diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 4836c70d2e3aa78d7d872e774365566369fb9327..f22ded239bbf615f68f60323ad7e72b6fc302637 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4910,6 +4910,9 @@ msgstr "" msgid "Diffs|No file name available" msgstr "" +msgid "Diffs|Show all lines" +msgstr "" + msgid "Diffs|Something went wrong while fetching diff lines." msgstr "" @@ -5882,6 +5885,9 @@ msgstr "" msgid "Expand sidebar" msgstr "" +msgid "Expand up" +msgstr "" + msgid "Expiration date" msgstr "" diff --git a/spec/features/merge_request/user_posts_diff_notes_spec.rb b/spec/features/merge_request/user_posts_diff_notes_spec.rb index ead564aea2863485de7edb7690476f40c8a06e39..abae6ffbd7177b7e092e2d955612bab68a3c48de 100644 --- a/spec/features/merge_request/user_posts_diff_notes_spec.rb +++ b/spec/features/merge_request/user_posts_diff_notes_spec.rb @@ -68,12 +68,12 @@ describe 'Merge request > User posts diff notes', :js do context 'with a match line' do it 'does not allow commenting on the left side' do line_holder = find('.match', match: :first).find(:xpath, '..') - should_not_allow_commenting(line_holder, 'left') + match_should_not_allow_commenting(line_holder) end it 'does not allow commenting on the right side' do line_holder = find('.match', match: :first).find(:xpath, '..') - should_not_allow_commenting(line_holder, 'right') + match_should_not_allow_commenting(line_holder) end end @@ -136,7 +136,7 @@ describe 'Merge request > User posts diff notes', :js do context 'with a match line' do it 'does not allow commenting' do - should_not_allow_commenting(find('.match', match: :first)) + match_should_not_allow_commenting(find('.match', match: :first)) end end @@ -222,7 +222,7 @@ describe 'Merge request > User posts diff notes', :js do context 'with a match line' do it 'does not allow commenting' do - should_not_allow_commenting(find('.match', match: :first)) + match_should_not_allow_commenting(find('.match', match: :first)) end end end @@ -251,6 +251,10 @@ describe 'Merge request > User posts diff notes', :js do expect(line[:num]).not_to have_css comment_button_class end + def match_should_not_allow_commenting(line_holder) + expect(line_holder).not_to have_css comment_button_class + end + def write_comment_on_line(line_holder, diff_side) click_diff_line(line_holder, diff_side) diff --git a/spec/features/merge_request/user_views_diffs_spec.rb b/spec/features/merge_request/user_views_diffs_spec.rb index 6d58d43a29507f6851494129fc98fb734915806a..2d1eb260236521ec1f18686ca7c85b6440f9ed7a 100644 --- a/spec/features/merge_request/user_views_diffs_spec.rb +++ b/spec/features/merge_request/user_views_diffs_spec.rb @@ -17,11 +17,25 @@ describe 'User views diffs', :js do end shared_examples 'unfold diffs' do - it 'unfolds diffs' do + it 'unfolds diffs upwards' do first('.js-unfold').click - expect(find('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"] .text-file')).to have_content('.bundle') 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') + end + + it 'unfolds diffs downwards' do + first('.js-unfold-down').click + expect(find('.file-holder[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd"] .text-file')).to have_content('.popen3') + end + + it 'unfolds diffs to the end' do + page.all('.js-unfold-down').last + expect(find('.file-holder[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9"] .text-file')).to have_content('end') + end end it 'shows diffs' do diff --git a/spec/javascripts/diffs/components/diff_expansion_cell_spec.js b/spec/javascripts/diffs/components/diff_expansion_cell_spec.js new file mode 100644 index 0000000000000000000000000000000000000000..cb1966b212267260ae5055906fae55e32196af8d --- /dev/null +++ b/spec/javascripts/diffs/components/diff_expansion_cell_spec.js @@ -0,0 +1,64 @@ +import Vue from 'vue'; +import store from '~/mr_notes/stores'; +import DiffExpansionCell from '~/diffs/components/diff_expansion_cell.vue'; +import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; +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'; + +describe('DiffExpansionCell', () => { + const matchLine = diffFileMockData.highlighted_diff_lines[5]; + + const createComponent = (options = {}) => { + const cmp = Vue.extend(DiffExpansionCell); + const defaults = { + fileHash: diffFileMockData.file_hash, + contextLinesPath: 'contextLinesPath', + line: matchLine, + isTop: false, + isBottom: false, + }; + const props = Object.assign({}, defaults, options); + + return createComponentWithStore(cmp, store, props).$mount(); + }; + + describe('top row', () => { + it('should have "expand up" and "show all" option', () => { + const vm = 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); + }); + }); + + describe('middle row', () => { + it('should have "expand down", "show all", "expand up" option', () => { + const vm = createComponent(); + const el = vm.$el; + + 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); + }); + }); + + describe('bottom row', () => { + it('should have "expand down" and "show all" option', () => { + const vm = 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); + }); + }); +}); diff --git a/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js b/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js index 038db8eaa7c600276e70c3eed49b759ceef3936f..99b5496c24b58d5828170d6bf82fd34f2be57510 100644 --- a/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js +++ b/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js @@ -70,15 +70,6 @@ describe('DiffLineGutterContent', () => { }); describe('template', () => { - it('should render three dots for context lines', () => { - const component = createComponent({ - isMatchLine: true, - }); - - expect(component.$el.querySelector('span').classList.contains('context-cell')).toEqual(true); - expect(component.$el.innerText).toEqual('...'); - }); - it('should render comment button', () => { const component = createComponent({ showCommentButton: true, diff --git a/spec/javascripts/diffs/components/inline_diff_expansion_row_spec.js b/spec/javascripts/diffs/components/inline_diff_expansion_row_spec.js new file mode 100644 index 0000000000000000000000000000000000000000..bf50070a4f5d134778a5f9611d588d73a9b5ff72 --- /dev/null +++ b/spec/javascripts/diffs/components/inline_diff_expansion_row_spec.js @@ -0,0 +1,31 @@ +import Vue from 'vue'; +import store from '~/mr_notes/stores'; +import InlineDiffExpansionRow from '~/diffs/components/inline_diff_expansion_row.vue'; +import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; +import diffFileMockData from '../mock_data/diff_file'; + +describe('InlineDiffExpansionRow', () => { + const matchLine = diffFileMockData.highlighted_diff_lines[5]; + + const createComponent = (options = {}) => { + const cmp = Vue.extend(InlineDiffExpansionRow); + const defaults = { + fileHash: diffFileMockData.file_hash, + contextLinesPath: 'contextLinesPath', + line: matchLine, + isTop: false, + isBottom: false, + }; + const props = Object.assign({}, defaults, options); + + return createComponentWithStore(cmp, store, props).$mount(); + }; + + describe('template', () => { + it('should render expansion row for match lines', () => { + const vm = createComponent(); + + expect(vm.$el.classList.contains('line_expansion')).toBe(true); + }); + }); +}); diff --git a/spec/javascripts/diffs/components/inline_diff_view_spec.js b/spec/javascripts/diffs/components/inline_diff_view_spec.js index 9b61dbe7975b6413fa6f90663e06841ccef05ea9..3963e37fae22663f0cac5176da5df44a8fdc31b5 100644 --- a/spec/javascripts/diffs/components/inline_diff_view_spec.js +++ b/spec/javascripts/diffs/components/inline_diff_view_spec.js @@ -28,9 +28,9 @@ describe('InlineDiffView', () => { it('should have rendered diff lines', () => { const el = component.$el; - expect(el.querySelectorAll('tr.line_holder').length).toEqual(6); + expect(el.querySelectorAll('tr.line_holder').length).toEqual(5); expect(el.querySelectorAll('tr.line_holder.new').length).toEqual(2); - expect(el.querySelectorAll('tr.line_holder.match').length).toEqual(1); + expect(el.querySelectorAll('tr.line_expansion.match').length).toEqual(1); expect(el.textContent.indexOf('Bad dates')).toBeGreaterThan(-1); }); diff --git a/spec/javascripts/diffs/components/parallel_diff_expansion_row_spec.js b/spec/javascripts/diffs/components/parallel_diff_expansion_row_spec.js new file mode 100644 index 0000000000000000000000000000000000000000..134738932b3608a7fe08ed76ce6856b51c25cb30 --- /dev/null +++ b/spec/javascripts/diffs/components/parallel_diff_expansion_row_spec.js @@ -0,0 +1,31 @@ +import Vue from 'vue'; +import store from '~/mr_notes/stores'; +import ParallelDiffExpansionRow from '~/diffs/components/parallel_diff_expansion_row.vue'; +import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; +import diffFileMockData from '../mock_data/diff_file'; + +describe('ParallelDiffExpansionRow', () => { + const matchLine = diffFileMockData.highlighted_diff_lines[5]; + + const createComponent = (options = {}) => { + const cmp = Vue.extend(ParallelDiffExpansionRow); + const defaults = { + fileHash: diffFileMockData.file_hash, + contextLinesPath: 'contextLinesPath', + line: matchLine, + isTop: false, + isBottom: false, + }; + const props = Object.assign({}, defaults, options); + + return createComponentWithStore(cmp, store, props).$mount(); + }; + + describe('template', () => { + it('should render expansion row for match lines', () => { + const vm = createComponent(); + + expect(vm.$el.classList.contains('line_expansion')).toBe(true); + }); + }); +}); diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index f8872a3eb13c1dc6e80f70add6e2d718d2828205..5806cb470340c4e35d5d82d24f50641af66ca017 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -380,7 +380,9 @@ describe('DiffsStoreActions', () => { const params = { since: 6, to: 26 }; const lineNumbers = { oldLineNumber: 3, newLineNumber: 5 }; const fileHash = 'ff9200'; - const options = { endpoint, params, lineNumbers, fileHash }; + const isExpandDown = false; + const nextLineNumbers = {}; + const options = { endpoint, params, lineNumbers, fileHash, isExpandDown, nextLineNumbers }; const mock = new MockAdapter(axios); const contextLines = { contextLines: [{ lineCode: 6 }] }; mock.onGet(endpoint).reply(200, contextLines); @@ -392,7 +394,7 @@ describe('DiffsStoreActions', () => { [ { type: types.ADD_CONTEXT_LINES, - payload: { lineNumbers, contextLines, params, fileHash }, + payload: { lineNumbers, contextLines, params, fileHash, isExpandDown, nextLineNumbers }, }, ], [], diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js index 9c13c7ceb7a9c6a3db916700f690bc08d869a145..3e033b6c9dce56dfc8a315b04a93045a9f5ef495 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/spec/javascripts/diffs/store/mutations_spec.js @@ -81,6 +81,8 @@ describe('DiffsStoreMutations', () => { params: { bottom: true, }, + isExpandDown: false, + nextLineNumbers: {}, }; const diffFile = { file_hash: options.fileHash, @@ -108,6 +110,8 @@ describe('DiffsStoreMutations', () => { options.contextLines, options.lineNumbers, options.params.bottom, + options.isExpandDown, + options.nextLineNumbers, ); expect(addContextLinesSpy).toHaveBeenCalledWith({ @@ -116,6 +120,7 @@ describe('DiffsStoreMutations', () => { contextLines: options.contextLines, bottom: options.params.bottom, lineNumbers: options.lineNumbers, + isExpandDown: false, }); }); }); diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index 1f877910125c52cf24ff321f664aace23ad7b70e..65eb4c9d2a3ce4a7689c3ab0bf345e5b04c2ac5f 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -260,6 +260,17 @@ describe('DiffsStoreUtils', () => { expect(linesWithReferences[1].meta_data.old_pos).toEqual(2); expect(linesWithReferences[1].meta_data.new_pos).toEqual(3); }); + + it('should add correct line references when isExpandDown is true', () => { + const lines = [{ type: null }, { type: MATCH_LINE_TYPE }]; + const linesWithReferences = utils.addLineReferences(lines, lineNumbers, false, true, { + old_line: 10, + new_line: 11, + }); + + expect(linesWithReferences[1].meta_data.old_pos).toEqual(10); + expect(linesWithReferences[1].meta_data.new_pos).toEqual(11); + }); }); describe('trimFirstCharOfLineContent', () => { diff --git a/spec/presenters/blobs/unfold_presenter_spec.rb b/spec/presenters/blobs/unfold_presenter_spec.rb index 1534c572b30d0e65263c7d6dd4d8070f7393e7c7..ab3f80802576e0ac8dfa097ae98093134fd64fb5 100644 --- a/spec/presenters/blobs/unfold_presenter_spec.rb +++ b/spec/presenters/blobs/unfold_presenter_spec.rb @@ -39,6 +39,21 @@ describe Blobs::UnfoldPresenter do expect(result.indent).to eq(0) end end + + context 'when to is -1' do + let(:params) { { full: false, since: 2, to: -1, bottom: true, offset: 1, indent: 1 } } + + it 'sets other attributes' do + result = subject + + expect(result.full?).to eq(false) + expect(result.since).to eq(2) + expect(result.to).to eq(blob.lines.size) + expect(result.bottom).to eq(false) + expect(result.offset).to eq(0) + expect(result.indent).to eq(0) + end + end end describe '#diff_lines' do @@ -83,8 +98,9 @@ describe Blobs::UnfoldPresenter do end end - context 'when since is greater than 1' do - let(:params) { { since: 5, to: 10, offset: 10 } } + context 'when "since" is greater than 1' do + let(:default_params) { { since: 5, to: 10, offset: 10 } } + let(:params) { default_params } it 'adds top match line' do line = subject.diff_lines.first @@ -93,6 +109,38 @@ describe Blobs::UnfoldPresenter do expect(line.old_pos).to eq(5) expect(line.new_pos).to eq(5) end + + context '"to" is higher than blob size' do + let(:params) { default_params.merge(to: total_lines + 10, bottom: true) } + + it 'does not add bottom match line' do + line = subject.diff_lines.last + + expect(line.type).to be_nil + end + end + + context '"to" is equal to blob size' do + let(:params) { default_params.merge(to: total_lines, bottom: true) } + + it 'does not add bottom match line' do + line = subject.diff_lines.last + + expect(line.type).to be_nil + end + end + + context '"to" is less than blob size' do + let(:params) { default_params.merge(to: total_lines - 3, bottom: true) } + + it 'adds bottom match line' do + line = subject.diff_lines.last + + expect(line.type).to eq('match') + expect(line.old_pos).to eq(total_lines - 3 - params[:offset]) + expect(line.new_pos).to eq(total_lines - 3) + end + end end context 'when "to" is less than blob size' do @@ -116,6 +164,22 @@ describe Blobs::UnfoldPresenter do expect(line.type).to be_nil end end + + context 'when "to" is "-1"' do + let(:params) { { since: 10, to: -1, offset: 10, bottom: true } } + + it 'does not add bottom match line' do + line = subject.diff_lines.last + + expect(line.type).to be_nil + end + + it 'last line is the latest blob line' do + line = subject.diff_lines.last + + expect(line.text).to eq(total_lines.to_s) + end + end end describe '#lines' do