diff options
38 files changed, 875 insertions, 421 deletions
diff --git a/app/assets/javascripts/diff_notes/diff_notes_bundle.js b/app/assets/javascripts/diff_notes/diff_notes_bundle.js index e17daec6a92..406091820e9 100644 --- a/app/assets/javascripts/diff_notes/diff_notes_bundle.js +++ b/app/assets/javascripts/diff_notes/diff_notes_bundle.js @@ -15,12 +15,14 @@ import './components/resolve_count'; import './components/resolve_discussion_btn'; import './components/diff_note_avatars'; import './components/new_issue_for_discussion'; -import { hasVueMRDiscussionsCookie } from '../lib/utils/common_utils'; export default () => { - const projectPathHolder = document.querySelector('.merge-request') || document.querySelector('.commit-box'); + const projectPathHolder = + document.querySelector('.merge-request') || + document.querySelector('.commit-box'); const projectPath = projectPathHolder.dataset.projectPath; - const COMPONENT_SELECTOR = 'resolve-btn, resolve-discussion-btn, jump-to-discussion, comment-and-resolve-btn, new-issue-for-discussion-btn'; + const COMPONENT_SELECTOR = + 'resolve-btn, resolve-discussion-btn, jump-to-discussion, comment-and-resolve-btn, new-issue-for-discussion-btn'; window.gl = window.gl || {}; window.gl.diffNoteApps = {}; @@ -28,9 +30,9 @@ export default () => { window.ResolveService = new gl.DiffNotesResolveServiceClass(projectPath); gl.diffNotesCompileComponents = () => { - $('diff-note-avatars').each(function () { + $('diff-note-avatars').each(function() { const tmp = Vue.extend({ - template: $(this).get(0).outerHTML + template: $(this).get(0).outerHTML, }); const tmpApp = new tmp().$mount(); @@ -41,12 +43,12 @@ export default () => { }); }); - const $components = $(COMPONENT_SELECTOR).filter(function () { + const $components = $(COMPONENT_SELECTOR).filter(function() { return $(this).closest('resolve-count').length !== 1; }); if ($components) { - $components.each(function () { + $components.each(function() { const $this = $(this); const noteId = $this.attr(':note-id'); const discussionId = $this.attr(':discussion-id'); @@ -54,7 +56,7 @@ export default () => { if ($this.is('comment-and-resolve-btn') && !discussionId) return; const tmp = Vue.extend({ - template: $this.get(0).outerHTML + template: $this.get(0).outerHTML, }); const tmpApp = new tmp().$mount(); @@ -69,14 +71,5 @@ export default () => { gl.diffNotesCompileComponents(); - if (!hasVueMRDiscussionsCookie()) { - new Vue({ - el: '#resolve-count-app', - components: { - 'resolve-count': ResolveCount - }, - }); - } - $(window).trigger('resize.nav'); }; diff --git a/app/assets/javascripts/diffs/components/changed_files.vue b/app/assets/javascripts/diffs/components/changed_files.vue index 4bab49bfca5..dee3dbf95d0 100644 --- a/app/assets/javascripts/diffs/components/changed_files.vue +++ b/app/assets/javascripts/diffs/components/changed_files.vue @@ -1,10 +1,14 @@ <script> +import _ from 'underscore'; import { mapGetters, mapActions } from 'vuex'; +import ClipboardButton from '~/vue_shared/components/clipboard_button.vue'; import Icon from '~/vue_shared/components/icon.vue'; +import { pluralize } from '~/lib/utils/text_utility'; export default { components: { Icon, + ClipboardButton, }, props: { diffFiles: { @@ -42,18 +46,15 @@ export default { }, }, mounted() { - if ( - typeof CSS === 'undefined' || - !CSS.supports('(position: -webkit-sticky) or (position: sticky)') - ) - return; - - document.addEventListener('scroll', this.handleScroll.bind(this), { - passive: true, - }); + this.throttledHandleScroll = _.throttle(this.handleScroll, 100); + document.addEventListener('scroll', this.throttledHandleScroll); + }, + beforeDestroy() { + document.removeEventListener('scroll', this.throttledHandleScroll); }, methods: { - ...mapActions(['setDiffViewType']), + ...mapActions(['setInlineDiffViewType', 'setParallelDiffViewType']), + pluralize, handleScroll() { if (!this.$refs.stickyBar) return; @@ -88,7 +89,13 @@ export default { truncatedDiffPath(path) { const maxLength = 60; - return path.length > maxLength ? `...${path.slice(0, maxLength)}` : path; + if (path.length > maxLength) { + const start = path.length - maxLength; + const end = start + maxLength; + return `...${path.slice(start, end)}`; + } + + return path; }, }, }; @@ -114,26 +121,26 @@ export default { {{ __('Hide whitespace changes') }} </a> <div class="btn-group"> - <a - @click.prevent="setDiffViewType()" + <button + type="button" + @click="setInlineDiffViewType" :class="{ active: isInlineView }" id="inline-diff-btn" class="btn" data-view-type="inline" - href="#" > {{ __('Inline') }} - </a> - <a - @click.prevent="setDiffViewType(true)" + </button> + <button + type="button" + @click="setParallelDiffViewType" :class="{ active: isParallelView }" id="parallel-diff-btn" class="btn" data-view-type="parallel" - href="#" > {{ __('Side-by-side') }} - </a> + </button> </div> </div> @@ -146,7 +153,7 @@ export default { aria-expanded="false" > <span> - {{ n__('%d changed file', '%d changed files', diffFiles.length) }} + {{ pluralize(`${diffFiles.length} changed file`, diffFiles.length) }} </span> <icon name="chevron-down" @@ -239,11 +246,11 @@ export default { > with <strong class="cgreen"> - {{ n__('%d addition', '%d additions', sumAddedLines) }} + {{ pluralize(`${sumAddedLines} addition`, sumAddedLines) }} </strong> and <strong class="cred"> - {{ n__('%d deletion', '%d deletions', sumRemovedLines) }} + {{ pluralize(`${sumRemovedLines} deletion`, sumRemovedLines) }} </strong> </span> @@ -251,7 +258,16 @@ export default { v-show="activeFile" class="prepend-left-5" > - {{ truncatedDiffPath(activeFile) }} + <strong class="prepend-right-5"> + {{ truncatedDiffPath(activeFile) }} + </strong> + <clipboard-button + :text="activeFile" + :title="s__('Copy file name to clipboard')" + tooltip-placement="bottom" + tooltip-container="body" + class="btn btn-default btn-transparent btn-clipboard" + /> </span> </div> </div> diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index a884594c202..c15941282ee 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -16,28 +16,52 @@ export default { data() { return { isExpanded: true, + isActive: false, }; }, mounted() { - document.addEventListener('scroll', () => { + document.addEventListener('scroll', this.handleScroll); + }, + beforeDestroy() { + document.removeEventListener('scroll', this.handleScroll); + }, + methods: { + handleToggle() { + this.isExpanded = !this.isExpanded; + }, + handleScroll() { + if (!this.updating) { + requestAnimationFrame(this.scrollUpdate); + this.updating = true; + } + }, + scrollUpdate() { + const header = document.querySelector('.js-diff-files-changed'); + if (!header) { + this.updating = false; + return; + } + const { top, bottom } = this.$el.getBoundingClientRect(); + const { + top: topOfFixedHeader, + bottom: bottomOfFixedHeader, + } = header.getBoundingClientRect(); - const topOfFixedHeader = 100; - const bottomOfFixedHeader = 120; + const headerOverlapsContent = top < topOfFixedHeader && bottom > bottomOfFixedHeader; + const fullyAboveHeader = bottom < bottomOfFixedHeader; + const fullyBelowHeader = top > topOfFixedHeader; - if (top < topOfFixedHeader && bottom > bottomOfFixedHeader) { + if (headerOverlapsContent && !this.isActive) { this.$emit('setActive'); - } - - if (top > bottomOfFixedHeader || bottom < bottomOfFixedHeader) { + this.isActive = true; + } else if (this.isActive && (fullyAboveHeader || fullyBelowHeader)) { this.$emit('unsetActive'); + this.isActive = false; } - }); - }, - methods: { - handleToggle() { - this.isExpanded = !this.isExpanded; - }, + + this.updating = false + } }, }; </script> 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 d8c57b8a629..ddec37a97b0 100644 --- a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue +++ b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue @@ -1,8 +1,15 @@ <script> -import { MATCH_LINE_TYPE } from '../constants'; +import { mapState, mapGetters, mapActions } from 'vuex'; +import Icon from '~/vue_shared/components/icon.vue'; +import { MATCH_LINE_TYPE, UNFOLD_COUNT, CONTEXT_LINE_TYPE } from '../constants'; +import * as utils from '../store/utils'; export default { props: { + fileHash: { + type: String, + required: true, + }, lineType: { type: String, required: false, @@ -23,49 +30,122 @@ export default { required: false, default: '', }, + metaData: { + type: Object, + required: false, + default: () => ({}), + }, showCommentButton: { type: Boolean, required: false, default: false, }, + contextLinesPath: { + type: String, + required: true, + }, + isBottom: { + type: Boolean, + required: false, + default: false, + }, + }, + components: { + Icon, }, computed: { + ...mapState({ + diffViewType: state => state.diffs.diffViewType, + diffFiles: state => state.diffs.diffFiles, + }), + ...mapGetters(['isLoggedIn']), isMatchLine() { return this.lineType === MATCH_LINE_TYPE; }, + isContextLine() { + return this.lineType === CONTEXT_LINE_TYPE; + }, getLineHref() { - return `#${this.lineCode}`; + return this.lineCode ? `#${this.lineCode}` : '#'; + }, + shouldShowCommentButton() { + return ( + this.isLoggedIn && + this.showCommentButton && + !this.isMatchLine && + !this.isContextLine + ); }, }, methods: { + ...mapActions(['loadMoreLines']), handleCommentButton() { this.$emit('showCommentForm', { lineCode: this.lineCode, linePosition: this.linePosition, }); }, + handleLoadMoreLines() { + const endpoint = this.contextLinesPath; + const oldLineNumber = this.metaData.oldPos || 0; + const newLineNumber = this.metaData.newPos || 0; + const offset = newLineNumber - oldLineNumber; + const bottom = this.isBottom; + const fileHash = this.fileHash; + 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.highlightedDiffLines, + { oldLineNumber, newLineNumber }, + ); + const prevLine = diffFile.highlightedDiffLines[indexForInline - 2]; + const prevLineNumber = (prevLine && prevLine.newLine) || 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 }); + }, }, }; </script> <template> <div> - <span v-if="isMatchLine">...</span> + <span + v-if="isMatchLine" + @click="handleLoadMoreLines" + class="context-cell" + >...</span> <template v-else > <button - v-if="showCommentButton" + v-if="shouldShowCommentButton" @click="handleCommentButton" type="button" class="add-diff-note js-add-diff-note-button" title="Add a comment to this line" > - <i - aria-hidden="true" - class="fa fa-comment-o" - > - </i> + <icon + name="comment" + :size="12" + /> </button> <a v-if="lineNumber" diff --git a/app/assets/javascripts/diffs/components/inline_diff_view.vue b/app/assets/javascripts/diffs/components/inline_diff_view.vue index 32de9554ce5..82e4497cd15 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_view.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue @@ -1,6 +1,11 @@ <script> import diffContentMixin from '../mixins/diff_content'; -import { MATCH_LINE_TYPE, LINE_HOVER_CLASS_NAME } from '../constants'; +import { + MATCH_LINE_TYPE, + CONTEXT_LINE_TYPE, + LINE_HOVER_CLASS_NAME, + LINE_UNFOLD_CLASS_NAME, +} from '../constants'; export default { mixins: [diffContentMixin], @@ -11,10 +16,13 @@ export default { getLineClass(line) { const isSameLine = this.hoveredLineCode === line.lineCode; const isMatchLine = line.type === MATCH_LINE_TYPE; + const isContextLine = line.type === CONTEXT_LINE_TYPE; return { - [line.type]: true, - [LINE_HOVER_CLASS_NAME]: isSameLine && !isMatchLine, + [line.type]: line.type, + [LINE_HOVER_CLASS_NAME]: + this.isLoggedIn && isSameLine && !isMatchLine && !isContextLine, + [LINE_UNFOLD_CLASS_NAME]: this.isLoggedIn && isMatchLine, }; }, }, @@ -32,7 +40,7 @@ export default { <tr :id="line.lineCode" :key="line.lineCode" - :class="line.type" + :class="getRowClass(line)" class="line_holder" @mouseover="handleMouse(line.lineCode, true)" @mouseout="handleMouse(line.lineCode, false)" @@ -42,10 +50,14 @@ export default { class="diff-line-num old_line" > <diff-line-gutter-content + :file-hash="fileHash" :line-type="line.type" :line-code="line.lineCode" :line-number="line.oldLine" + :meta-data="line.metaData" :show-comment-button="true" + :context-lines-path="diffFile.contextLinesPath" + :is-bottom="index + 1 === diffLinesLength" @showCommentForm="handleShowCommentForm" /> </td> @@ -54,9 +66,13 @@ export default { class="diff-line-num new_line" > <diff-line-gutter-content + :file-hash="fileHash" :line-type="line.type" :line-code="line.lineCode" :line-number="line.newLine" + :meta-data="line.metaData" + :is-bottom="index + 1 === diffLinesLength" + :context-lines-path="diffFile.contextLinesPath" /> </td> <td diff --git a/app/assets/javascripts/diffs/components/parallel_diff_view.vue b/app/assets/javascripts/diffs/components/parallel_diff_view.vue index 922f8e58dec..8ab3d7cdf6f 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_view.vue @@ -3,7 +3,9 @@ import diffContentMixin from '../mixins/diff_content'; import { EMPTY_CELL_TYPE, MATCH_LINE_TYPE, + CONTEXT_LINE_TYPE, LINE_HOVER_CLASS_NAME, + LINE_UNFOLD_CLASS_NAME, } from '../constants'; export default { @@ -31,14 +33,17 @@ export default { }, getClassName(line, position) { const { type, lineCode } = line[position]; + const isMatchLine = type === MATCH_LINE_TYPE; const isContextLine = - type !== MATCH_LINE_TYPE && type !== EMPTY_CELL_TYPE; + !isMatchLine && type !== EMPTY_CELL_TYPE && type !== CONTEXT_LINE_TYPE; const isSameLine = this.hoveredLineCode === lineCode; const isSameSection = position === this.hoveredSection; return { [type]: type, - [LINE_HOVER_CLASS_NAME]: isContextLine && isSameLine && isSameSection, + [LINE_HOVER_CLASS_NAME]: + this.isLoggedIn && isContextLine && isSameLine && isSameSection, + [LINE_UNFOLD_CLASS_NAME]: this.isLoggedIn && isMatchLine, }; }, handleMouse(e, line, isHover) { @@ -72,6 +77,7 @@ export default { > <tr :key="index" + :class="getRowClass(line)" class="line_holder parallel" @mouseover="handleMouse($event, line, true)" @mouseout="handleMouse($event, line, false)" @@ -82,10 +88,14 @@ export default { class="diff-line-num old_line" > <diff-line-gutter-content + :file-hash="fileHash" :line-type="line.left.type" :line-code="line.left.lineCode" :line-number="line.left.oldLine" + :meta-data="line.left.metaData" :show-comment-button="true" + :context-lines-path="diffFile.contextLinesPath" + :is-bottom="index + 1 === diffLinesLength" line-position="left" @showCommentForm="handleShowCommentForm" /> @@ -103,10 +113,14 @@ export default { class="diff-line-num new_line" > <diff-line-gutter-content + :file-hash="fileHash" :line-type="line.right.type" :line-code="line.right.lineCode" :line-number="line.right.newLine" + :meta-data="line.right.metaData" :show-comment-button="true" + :context-lines-path="diffFile.contextLinesPath" + :is-bottom="index + 1 === diffLinesLength" line-position="right" @showCommentForm="handleShowCommentForm" /> diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index fa8cccbc46c..f93e270575f 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -1,9 +1,12 @@ export const INLINE_DIFF_VIEW_TYPE = 'inline'; export const PARALLEL_DIFF_VIEW_TYPE = 'parallel'; export const MATCH_LINE_TYPE = 'match'; +export const CONTEXT_LINE_TYPE = 'context'; export const DIFF_VIEW_COOKIE_NAME = 'diff_view'; export const EMPTY_CELL_TYPE = 'empty-cell'; export const LINE_HOVER_CLASS_NAME = 'is-over'; +export const LINE_UNFOLD_CLASS_NAME = 'unfold js-unfold'; +export const CONTEXT_LINE_CLASS_NAME = 'diff-expanded'; export const COMMENT_FORM_TYPE = 'commentForm'; export const LINE_POSITION_LEFT = 'left'; export const LINE_POSITION_RIGHT = 'right'; @@ -11,3 +14,4 @@ export const TEXT_DIFF_POSITION_TYPE = 'text'; export const DIFF_NOTE_TYPE = 'DiffNote'; export const NEW_LINE_TYPE = 'new'; export const OLD_LINE_TYPE = 'old'; +export const UNFOLD_COUNT = 20; diff --git a/app/assets/javascripts/diffs/mixins/diff_content.js b/app/assets/javascripts/diffs/mixins/diff_content.js index 11db6a10a3e..bb2ece68bc0 100644 --- a/app/assets/javascripts/diffs/mixins/diff_content.js +++ b/app/assets/javascripts/diffs/mixins/diff_content.js @@ -2,6 +2,7 @@ import { mapGetters, mapActions } from 'vuex'; import diffDiscussions from '../components/diff_discussions.vue'; import diffLineGutterContent from '../components/diff_line_gutter_content.vue'; import diffLineNoteForm from '../components/diff_line_note_form.vue'; +import { CONTEXT_LINE_TYPE, CONTEXT_LINE_CLASS_NAME } from '../constants'; export default { props: { @@ -26,7 +27,7 @@ export default { diffLineGutterContent, }, computed: { - ...mapGetters(['discussionsByLineCode']), + ...mapGetters(['discussionsByLineCode', 'isLoggedIn']), userColorScheme() { return window.gon.user_color_scheme; }, @@ -47,9 +48,25 @@ export default { return line; }); }, + diffLinesLength() { + return this.normalizedDiffLines.length; + }, + fileHash() { + return this.diffFile.fileHash; + }, }, methods: { ...mapActions(['showCommentForm', 'cancelCommentForm']), + getRowClass(line) { + const isContextLine = line.left + ? line.left.type === CONTEXT_LINE_TYPE + : line.type === CONTEXT_LINE_TYPE; + + return { + [line.type]: line.type, + [CONTEXT_LINE_CLASS_NAME]: isContextLine, + }; + }, trimFirstChar(line) { if (!line.richText) { return line; diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 321967a5479..3185c90cecb 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -1,5 +1,5 @@ import Vue from 'vue'; -import VueResource from 'vue-resource'; +import axios from '~/lib/utils/axios_utils'; import Cookies from 'js-cookie'; import { handleLocationHash } from '~/lib/utils/common_utils'; import * as types from './mutation_types'; @@ -9,8 +9,6 @@ import { DIFF_VIEW_COOKIE_NAME, } from '../constants'; -Vue.use(VueResource); - export const setEndpoint = ({ commit }, endpoint) => { commit(types.SET_ENDPOINT, endpoint); }; @@ -22,22 +20,24 @@ export const setLoadingState = ({ commit }, state) => { export const fetchDiffFiles = ({ state, commit }) => { commit(types.SET_LOADING, true); - return Vue.http + return axios .get(state.endpoint) - .then(res => res.json()) .then(res => { commit(types.SET_LOADING, false); - commit(types.SET_DIFF_FILES, res.diff_files); + commit(types.SET_DIFF_FILES, res.data.diff_files); return Vue.nextTick(); }) .then(handleLocationHash); }; -export const setDiffViewType = ({ commit }, isParallel) => { - const type = isParallel ? PARALLEL_DIFF_VIEW_TYPE : INLINE_DIFF_VIEW_TYPE; +export const setInlineDiffViewType = ({ commit }) => { + commit(types.SET_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE); + Cookies.set(DIFF_VIEW_COOKIE_NAME, INLINE_DIFF_VIEW_TYPE); +}; - commit(types.SET_DIFF_VIEW_TYPE, type); - Cookies.set(DIFF_VIEW_COOKIE_NAME, type); +export const setParallelDiffViewType = ({ commit }) => { + commit(types.SET_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE); + Cookies.set(DIFF_VIEW_COOKIE_NAME, PARALLEL_DIFF_VIEW_TYPE); }; export const showCommentForm = ({ commit }, params) => { @@ -47,3 +47,18 @@ export const showCommentForm = ({ commit }, params) => { export const cancelCommentForm = ({ commit }, params) => { commit(types.REMOVE_COMMENT_FORM_LINE, params); }; + +export const loadMoreLines = ({ commit }, options) => { + const { endpoint, params, lineNumbers, fileHash } = options; + + return axios.get(endpoint, { params }).then(res => { + const contextLines = res.data || []; + + commit(types.ADD_CONTEXT_LINES, { + lineNumbers, + contextLines, + params, + fileHash, + }); + }); +}; diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js index cec771fc04e..805fa510a56 100644 --- a/app/assets/javascripts/diffs/store/mutation_types.js +++ b/app/assets/javascripts/diffs/store/mutation_types.js @@ -4,3 +4,4 @@ export const SET_DIFF_FILES = 'SET_DIFF_FILES'; export const SET_DIFF_VIEW_TYPE = 'SET_DIFF_VIEW_TYPE'; export const ADD_COMMENT_FORM_LINE = 'ADD_COMMENT_FORM_LINE'; export const REMOVE_COMMENT_FORM_LINE = 'REMOVE_COMMENT_FORM_LINE'; +export const ADD_CONTEXT_LINES = 'ADD_CONTEXT_LINES'; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index d83a966fed9..8d7c26ed212 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -8,8 +8,8 @@ export default { Object.assign(state, { endpoint }); }, - [types.SET_LOADING](state, loadingState) { - Object.assign(state, { isLoading: loadingState }); + [types.SET_LOADING](state, isLoading) { + Object.assign(state, { isLoading }); }, [types.SET_DIFF_FILES](state, diffFiles) { @@ -20,8 +20,8 @@ export default { }); }, - [types.SET_DIFF_VIEW_TYPE](state, type) { - Object.assign(state, { diffViewType: type }); + [types.SET_DIFF_VIEW_TYPE](state, diffViewType) { + Object.assign(state, { diffViewType }); }, [types.ADD_COMMENT_FORM_LINE](state, { diffLines, lineCode, linePosition }) { @@ -109,4 +109,21 @@ export default { } } }, + + [types.ADD_CONTEXT_LINES](state, options) { + const { lineNumbers, contextLines, fileHash } = options; + const { bottom } = options.params; + const diffFile = utils.findDiffFile(state.diffFiles, fileHash); + const { highlightedDiffLines, parallelDiffLines } = diffFile; + + utils.removeMatchLine(diffFile, lineNumbers, bottom); + const lines = utils.addLineReferences(contextLines, lineNumbers, bottom); + utils.addContextLines({ + inlineLines: highlightedDiffLines, + parallelLines: parallelDiffLines, + contextLines: lines, + bottom, + lineNumbers, + }); + }, }; diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 9aa44cd4fdb..f7a4eddf60d 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -1,4 +1,5 @@ import _ from 'underscore'; +import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; import { LINE_POSITION_LEFT, LINE_POSITION_RIGHT, @@ -6,6 +7,7 @@ import { DIFF_NOTE_TYPE, NEW_LINE_TYPE, OLD_LINE_TYPE, + MATCH_LINE_TYPE, } from '../constants'; export const findDiffLineIndex = options => { @@ -26,6 +28,9 @@ export const findDiffLineIndex = options => { }); }; +export const findDiffFile = (files, hash) => + files.filter(file => file.fileHash === hash)[0]; + export const getReversePosition = linePosition => { if (linePosition === LINE_POSITION_RIGHT) { return LINE_POSITION_LEFT; @@ -81,3 +86,101 @@ export const getNoteFormData = params => { data: postData, }; }; + +export const findIndexInInlineLines = (lines, lineNumbers) => { + const { oldLineNumber, newLineNumber } = lineNumbers; + + return _.findIndex( + lines, + line => line.oldLine === oldLineNumber && line.newLine === newLineNumber, + ); +}; + +export const findIndexInParallelLines = (lines, lineNumbers) => { + const { oldLineNumber, newLineNumber } = lineNumbers; + + return _.findIndex( + lines, + line => + line.left && + line.right && + line.left.oldLine === oldLineNumber && + line.right.newLine === newLineNumber, + ); +}; + +export const removeMatchLine = (diffFile, lineNumbers, bottom) => { + const indexForInline = findIndexInInlineLines( + diffFile.highlightedDiffLines, + lineNumbers, + ); + + const indexForParallel = findIndexInParallelLines( + diffFile.parallelDiffLines, + lineNumbers, + ); + + const factor = bottom ? 1 : -1; + + diffFile.highlightedDiffLines.splice(indexForInline + factor, 1); + diffFile.parallelDiffLines.splice(indexForParallel + factor, 1); +}; + +export const addLineReferences = (lines, lineNumbers, bottom) => { + const { oldLineNumber, newLineNumber } = lineNumbers; + const lineCount = lines.length; + let matchLineIndex = -1; + + const linesWithNumbers = lines.map((l, index) => { + const line = convertObjectPropsToCamelCase(l); + + if (line.type === MATCH_LINE_TYPE) { + matchLineIndex = index; + } else { + Object.assign(line, { + oldLine: bottom + ? oldLineNumber + index + 1 + : oldLineNumber + index - lineCount, + newLine: bottom + ? newLineNumber + index + 1 + : newLineNumber + index - lineCount, + }); + } + + return line; + }); + + if (matchLineIndex > -1) { + const line = linesWithNumbers[matchLineIndex]; + const targetLine = bottom + ? linesWithNumbers[matchLineIndex - 1] + : linesWithNumbers[matchLineIndex + 1]; + + Object.assign(line, { + metaData: { + oldPos: targetLine.oldLine, + newPos: targetLine.newLine, + }, + }); + } + + return linesWithNumbers; +}; + +export const addContextLines = options => { + const { inlineLines, parallelLines, contextLines, lineNumbers } = options; + const inlineIndex = findIndexInInlineLines(inlineLines, lineNumbers); + const parallelIndex = findIndexInParallelLines(parallelLines, lineNumbers); + const normalizedParallelLines = contextLines.map(line => ({ + left: line, + right: line, + })); + + if (options.bottom) { + inlineLines.push(...contextLines); + parallelLines.push(...normalizedParallelLines); + } else { + inlineLines.splice(inlineIndex, 0, ...contextLines); + parallelLines.splice(parallelIndex, 0, ...normalizedParallelLines); + } +}; diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index 081c8c48365..606c836acee 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/app/assets/javascripts/lib/utils/common_utils.js @@ -5,7 +5,10 @@ import { getLocationHash } from './url_utility'; import { convertToCamelCase } from './text_utility'; import { isObject } from './type_utility'; -export const getPagePath = (index = 0) => $('body').attr('data-page').split(':')[index]; +export const getPagePath = (index = 0) => + $('body') + .attr('data-page') + .split(':')[index]; export const isInGroupsPage = () => getPagePath() === 'groups'; @@ -35,32 +38,39 @@ export const checkPageAndAction = (page, action) => { export const isInIssuePage = () => checkPageAndAction('issues', 'show'); export const isInMRPage = () => checkPageAndAction('merge_requests', 'show'); export const isInNoteablePage = () => isInIssuePage() || isInMRPage(); -export const hasVueMRDiscussionsCookie = () => Cookies.get('vue_mr_discussions'); -export const ajaxGet = url => axios.get(url, { - params: { format: 'js' }, - responseType: 'text', -}).then(({ data }) => { - $.globalEval(data); -}); +export const ajaxGet = url => + axios + .get(url, { + params: { format: 'js' }, + responseType: 'text', + }) + .then(({ data }) => { + $.globalEval(data); + }); -export const rstrip = (val) => { +export const rstrip = val => { if (val) { return val.replace(/\s+$/, ''); } return val; }; -export const updateTooltipTitle = ($tooltipEl, newTitle) => $tooltipEl.attr('title', newTitle).tooltip('fixTitle'); +export const updateTooltipTitle = ($tooltipEl, newTitle) => + $tooltipEl.attr('title', newTitle).tooltip('fixTitle'); -export const disableButtonIfEmptyField = (fieldSelector, buttonSelector, eventName = 'input') => { +export const disableButtonIfEmptyField = ( + fieldSelector, + buttonSelector, + eventName = 'input', +) => { const field = $(fieldSelector); const closestSubmit = field.closest('form').find(buttonSelector); if (rstrip(field.val()) === '') { closestSubmit.disable(); } // eslint-disable-next-line func-names - return field.on(eventName, function () { + return field.on(eventName, function() { if (rstrip($(this).val()) === '') { return closestSubmit.disable(); } @@ -77,7 +87,9 @@ export const handleLocationHash = () => { // This is required to handle non-unicode characters in hash hash = decodeURIComponent(hash); - const target = document.getElementById(hash) || document.getElementById(`user-content-${hash}`); + const target = + document.getElementById(hash) || + document.getElementById(`user-content-${hash}`); const fixedTabs = document.querySelector('.js-tabs-affix'); const fixedDiffStats = document.querySelector('.js-diff-files-changed'); const fixedNav = document.querySelector('.navbar-gitlab'); @@ -102,7 +114,7 @@ export const handleLocationHash = () => { // Check if element scrolled into viewport from above or below // Courtesy http://stackoverflow.com/a/7557433/414749 -export const isInViewport = (el) => { +export const isInViewport = el => { const rect = el.getBoundingClientRect(); return ( @@ -113,25 +125,31 @@ export const isInViewport = (el) => { ); }; -export const parseUrl = (url) => { +export const parseUrl = url => { const parser = document.createElement('a'); parser.href = url; return parser; }; -export const parseUrlPathname = (url) => { +export const parseUrlPathname = url => { const parsedUrl = parseUrl(url); // parsedUrl.pathname will return an absolute path for Firefox and a relative path for IE11 // We have to make sure we always have an absolute path. - return parsedUrl.pathname.charAt(0) === '/' ? parsedUrl.pathname : `/${parsedUrl.pathname}`; + return parsedUrl.pathname.charAt(0) === '/' + ? parsedUrl.pathname + : `/${parsedUrl.pathname}`; }; // We can trust that each param has one & since values containing & will be encoded // Remove the first character of search as it is always ? -export const getUrlParamsArray = () => window.location.search.slice(1).split('&').map((param) => { - const split = param.split('='); - return [decodeURI(split[0]), split[1]].join('='); -}); +export const getUrlParamsArray = () => + window.location.search + .slice(1) + .split('&') + .map(param => { + const split = param.split('='); + return [decodeURI(split[0]), split[1]].join('='); + }); export const isMetaKey = e => e.metaKey || e.ctrlKey || e.altKey || e.shiftKey; @@ -141,7 +159,7 @@ export const isMetaKey = e => e.metaKey || e.ctrlKey || e.altKey || e.shiftKey; // 3) Middle-click or Mouse Wheel Click (e.which is 2) export const isMetaClick = e => e.metaKey || e.ctrlKey || e.which === 2; -export const scrollToElement = (element) => { +export const scrollToElement = element => { let $el = element; if (!(element instanceof $)) { $el = $(element); @@ -150,9 +168,12 @@ export const scrollToElement = (element) => { const mrTabsHeight = $('.merge-request-tabs').height() || 0; const headerHeight = $('.navbar-gitlab').height() || 0; - return $('body, html').animate({ - scrollTop: top - mrTabsHeight - headerHeight, - }, 200); + return $('body, html').animate( + { + scrollTop: top - mrTabsHeight - headerHeight, + }, + 200, + ); }; /** @@ -191,13 +212,15 @@ export const insertText = (target, text) => { const textBefore = value.substring(0, selectionStart); const textAfter = value.substring(selectionEnd, value.length); - const insertedText = text instanceof Function ? text(textBefore, textAfter) : text; + const insertedText = + text instanceof Function ? text(textBefore, textAfter) : text; const newText = textBefore + insertedText + textAfter; // eslint-disable-next-line no-param-reassign target.value = newText; // eslint-disable-next-line no-param-reassign - target.selectionStart = target.selectionEnd = selectionStart + insertedText.length; + target.selectionStart = target.selectionEnd = + selectionStart + insertedText.length; // Trigger autosave target.dispatchEvent(new Event('input')); @@ -209,7 +232,8 @@ export const insertText = (target, text) => { }; export const nodeMatchesSelector = (node, selector) => { - const matches = Element.prototype.matches || + const matches = + Element.prototype.matches || Element.prototype.matchesSelector || Element.prototype.mozMatchesSelector || Element.prototype.msMatchesSelector || @@ -238,10 +262,10 @@ export const nodeMatchesSelector = (node, selector) => { this will take in the headers from an API response and normalize them this way we don't run into production issues when nginx gives us lowercased header keys */ -export const normalizeHeaders = (headers) => { +export const normalizeHeaders = headers => { const upperCaseHeaders = {}; - Object.keys(headers || {}).forEach((e) => { + Object.keys(headers || {}).forEach(e => { upperCaseHeaders[e.toUpperCase()] = headers[e]; }); @@ -252,11 +276,11 @@ export const normalizeHeaders = (headers) => { this will take in the getAllResponseHeaders result and normalize them this way we don't run into production issues when nginx gives us lowercased header keys */ -export const normalizeCRLFHeaders = (headers) => { +export const normalizeCRLFHeaders = headers => { const headersObject = {}; const headersArray = headers.split('\n'); - headersArray.forEach((header) => { + headersArray.forEach(header => { const keyValue = header.split(': '); headersObject[keyValue[0]] = keyValue[1]; }); @@ -292,15 +316,13 @@ export const parseIntPagination = paginationInformation => ({ export const parseQueryStringIntoObject = (query = '') => { if (query === '') return {}; - return query - .split('&') - .reduce((acc, element) => { - const val = element.split('='); - Object.assign(acc, { - [val[0]]: decodeURIComponent(val[1]), - }); - return acc; - }, {}); + return query.split('&').reduce((acc, element) => { + const val = element.split('='); + Object.assign(acc, { + [val[0]]: decodeURIComponent(val[1]), + }); + return acc; + }, {}); }; /** @@ -309,9 +331,13 @@ export const parseQueryStringIntoObject = (query = '') => { * * @param {Object} params */ -export const objectToQueryString = (params = {}) => Object.keys(params).map(param => `${param}=${params[param]}`).join('&'); +export const objectToQueryString = (params = {}) => + Object.keys(params) + .map(param => `${param}=${params[param]}`) + .join('&'); -export const buildUrlWithCurrentLocation = param => (param ? `${window.location.pathname}${param}` : window.location.pathname); +export const buildUrlWithCurrentLocation = param => + param ? `${window.location.pathname}${param}` : window.location.pathname; /** * Based on the current location and the string parameters provided @@ -319,7 +345,7 @@ export const buildUrlWithCurrentLocation = param => (param ? `${window.location. * * @param {String} param */ -export const historyPushState = (newUrl) => { +export const historyPushState = newUrl => { window.history.pushState({}, document.title, newUrl); }; @@ -368,7 +394,7 @@ export const backOff = (fn, timeout = 60000) => { let timeElapsed = 0; return new Promise((resolve, reject) => { - const stop = arg => ((arg instanceof Error) ? reject(arg) : resolve(arg)); + const stop = arg => (arg instanceof Error ? reject(arg) : resolve(arg)); const next = () => { if (timeElapsed < timeout) { @@ -384,7 +410,7 @@ export const backOff = (fn, timeout = 60000) => { }); }; -export const setFavicon = (faviconPath) => { +export const setFavicon = faviconPath => { const faviconEl = document.getElementById('favicon'); if (faviconEl && faviconPath) { faviconEl.setAttribute('href', faviconPath); @@ -400,7 +426,8 @@ export const resetFavicon = () => { }; export const setCiStatusFavicon = pageUrl => - axios.get(pageUrl) + axios + .get(pageUrl) .then(({ data }) => { if (data && data.favicon) { setFavicon(data.favicon); @@ -413,7 +440,9 @@ export const setCiStatusFavicon = pageUrl => export const spriteIcon = (icon, className = '') => { const classAttribute = className.length > 0 ? `class="${className}"` : ''; - return `<svg ${classAttribute}><use xlink:href="${gon.sprite_icons}#${icon}" /></svg>`; + return `<svg ${classAttribute}><use xlink:href="${ + gon.sprite_icons + }#${icon}" /></svg>`; }; /** @@ -435,7 +464,10 @@ export const convertObjectPropsToCamelCase = (obj = {}, options = {}) => { const val = obj[prop]; if (options.deep && (isObject(val) || Array.isArray(val))) { - result[convertToCamelCase(prop)] = convertObjectPropsToCamelCase(val, options); + result[convertToCamelCase(prop)] = convertObjectPropsToCamelCase( + val, + options, + ); } else { result[convertToCamelCase(prop)] = obj[prop]; } @@ -443,15 +475,18 @@ export const convertObjectPropsToCamelCase = (obj = {}, options = {}) => { }, initial); }; -export const imagePath = imgUrl => `${gon.asset_host || ''}${gon.relative_url_root || ''}/assets/${imgUrl}`; +export const imagePath = imgUrl => + `${gon.asset_host || ''}${gon.relative_url_root || ''}/assets/${imgUrl}`; export const addSelectOnFocusBehaviour = (selector = '.js-select-on-focus') => { // Click a .js-select-on-focus field, select the contents // Prevent a mouseup event from deselecting the input $(selector).on('focusin', function selectOnFocusCallback() { - $(this).select().one('mouseup', (e) => { - e.preventDefault(); - }); + $(this) + .select() + .one('mouseup', e => { + e.preventDefault(); + }); }); }; diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index a98b0b1c0b1..9f6af8c7a0d 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -12,7 +12,6 @@ import { parseUrlPathname, handleLocationHash, isMetaClick, - hasVueMRDiscussionsCookie, } from './lib/utils/common_utils'; import { getLocationHash } from './lib/utils/url_utility'; import initDiscussionTab from './image_diff/init_discussion_tab'; @@ -71,12 +70,12 @@ import Notes from './notes'; let location = window.location; export default class MergeRequestTabs { - constructor({ action, setUrl, stubLocation } = {}) { const mergeRequestTabs = document.querySelector('.js-tabs-affix'); const navbar = document.querySelector('.navbar-gitlab'); const peek = document.getElementById('peek'); const paddingTop = 16; + this.commitsTab = document.querySelector('.tab-content .commits.tab-pane'); this.diffsLoaded = false; this.pipelinesLoaded = false; @@ -109,21 +108,27 @@ export default class MergeRequestTabs { bindEvents() { $(document) - .on('shown.bs.tab', '.merge-request-tabs a[data-toggle="tab"]', this.tabShown) + .on( + 'shown.bs.tab', + '.merge-request-tabs a[data-toggle="tab"]', + this.tabShown, + ) .on('click', '.js-show-tab', this.showTab); - $('.merge-request-tabs a[data-toggle="tab"]') - .on('click', this.clickTab); + $('.merge-request-tabs a[data-toggle="tab"]').on('click', this.clickTab); } // Used in tests unbindEvents() { $(document) - .off('shown.bs.tab', '.merge-request-tabs a[data-toggle="tab"]', this.tabShown) + .off( + 'shown.bs.tab', + '.merge-request-tabs a[data-toggle="tab"]', + this.tabShown, + ) .off('click', '.js-show-tab', this.showTab); - $('.merge-request-tabs a[data-toggle="tab"]') - .off('click', this.clickTab); + $('.merge-request-tabs a[data-toggle="tab"]').off('click', this.clickTab); } destroyPipelinesView() { @@ -159,10 +164,6 @@ export default class MergeRequestTabs { this.resetViewContainer(); this.destroyPipelinesView(); } else if (this.isDiffAction(action)) { - if (!hasVueMRDiscussionsCookie()) { - this.loadDiff($target.attr('href')); - } - if (bp.getBreakpointSize() !== 'lg') { this.shrinkView(); } @@ -170,7 +171,7 @@ export default class MergeRequestTabs { this.expandViewContainer(); } this.destroyPipelinesView(); - $('.tab-content .commits.tab-pane').removeClass('active'); + this.commitsTab.classList.remove('active'); } else if (action === 'pipelines') { this.resetViewContainer(); this.mountPipelinesView(); @@ -192,10 +193,9 @@ export default class MergeRequestTabs { scrollToElement(container) { if (location.hash) { - const offset = 0 - ( - $('.navbar-gitlab').outerHeight() + - $('.js-tabs-affix').outerHeight() - ); + const offset = + 0 - + ($('.navbar-gitlab').outerHeight() + $('.js-tabs-affix').outerHeight()); const $el = $(`${container} ${location.hash}:not(.match)`); if ($el.length) { $.scrollTo($el[0], { offset }); @@ -233,7 +233,10 @@ export default class MergeRequestTabs { this.currentAction = action; // Remove a trailing '/commits' '/diffs' '/pipelines' - let newState = location.pathname.replace(/\/(commits|diffs|pipelines)(\.html)?\/?$/, ''); + let newState = location.pathname.replace( + /\/(commits|diffs|pipelines)(\.html)?\/?$/, + '', + ); // Append the new action if we're on a tab other than 'notes' if (this.currentAction !== 'show' && this.currentAction !== 'new') { @@ -249,9 +252,13 @@ export default class MergeRequestTabs { // Turbolinks' history. // // See https://github.com/rails/turbolinks/issues/363 - window.history.replaceState({ - url: newState, - }, document.title, newState); + window.history.replaceState( + { + url: newState, + }, + document.title, + newState, + ); return newState; } @@ -267,7 +274,8 @@ export default class MergeRequestTabs { this.toggleLoading(true); - axios.get(`${source}.json`) + axios + .get(`${source}.json`) .then(({ data }) => { document.querySelector('div#commits').innerHTML = data.html; localTimeAgo($('.js-timeago', 'div#commits')); @@ -283,7 +291,9 @@ export default class MergeRequestTabs { } mountPipelinesView() { - const pipelineTableViewEl = document.querySelector('#commit-pipeline-table-view'); + const pipelineTableViewEl = document.querySelector( + '#commit-pipeline-table-view', + ); const CommitPipelinesTable = gl.CommitPipelinesTable; this.commitPipelinesTable = new CommitPipelinesTable({ propsData: { @@ -314,7 +324,8 @@ export default class MergeRequestTabs { this.toggleLoading(true); - axios.get(`${urlPathname}.json${location.search}`) + axios + .get(`${urlPathname}.json${location.search}`) .then(({ data }) => { const $container = $('#diffs'); $container.html(data.html); @@ -328,7 +339,10 @@ export default class MergeRequestTabs { localTimeAgo($('.js-timeago', 'div#diffs')); syntaxHighlight($('#diffs .js-syntax-highlight')); - if (this.diffViewType() === 'parallel' && this.isDiffAction(this.currentAction)) { + if ( + this.diffViewType() === 'parallel' && + this.isDiffAction(this.currentAction) + ) { this.expandViewContainer(); } this.diffsLoaded = true; @@ -342,9 +356,10 @@ export default class MergeRequestTabs { forkButtons: $(el).find('.js-fork-suggestion-button'), cancelButtons: $(el).find('.js-cancel-fork-suggestion-button'), suggestionSections: $(el).find('.js-file-fork-suggestion-section'), - actionTextPieces: $(el).find('.js-file-fork-suggestion-section-action'), - }) - .init(); + actionTextPieces: $(el).find( + '.js-file-fork-suggestion-section-action', + ), + }).init(); }); // Scroll any linked note into view @@ -399,8 +414,10 @@ export default class MergeRequestTabs { resetViewContainer() { if (this.fixedLayoutPref !== null) { - $('.content-wrapper .container-fluid') - .toggleClass('container-limited', this.fixedLayoutPref); + $('.content-wrapper .container-fluid').toggleClass( + 'container-limited', + this.fixedLayoutPref, + ); } } @@ -449,12 +466,12 @@ export default class MergeRequestTabs { const $diffTabs = $('#diff-notes-app'); - $tabs.off('affix.bs.affix affix-top.bs.affix') + $tabs + .off('affix.bs.affix affix-top.bs.affix') .affix({ offset: { - top: () => ( - $diffTabs.offset().top - $tabs.height() - $fixedNav.height() - ), + top: () => + $diffTabs.offset().top - $tabs.height() - $fixedNav.height(), }, }) .on('affix.bs.affix', () => $diffTabs.css({ marginTop: $tabs.height() })) diff --git a/app/assets/javascripts/mr_notes/index.js b/app/assets/javascripts/mr_notes/index.js index 33492f7be65..c09952364f4 100644 --- a/app/assets/javascripts/mr_notes/index.js +++ b/app/assets/javascripts/mr_notes/index.js @@ -1,5 +1,5 @@ import Vue from 'vue'; -import Vuex, { mapActions, mapGetters } from 'vuex'; +import Vuex, { mapActions, mapState } from 'vuex'; import notesApp from '../notes/components/notes_app.vue'; import diffsApp from '../diffs/components/app.vue'; import discussionCounter from '../notes/components/discussion_counter.vue'; @@ -14,12 +14,14 @@ const store = new Vuex.Store({ modules: { page: mrPageStoreConfig, notes: notesStoreConfig, + diffs: diffsStoreConfig, }, }); export default function initMrNotes() { const mrShowNode = document.querySelector('.merge-request'); - window.mergeRequest = new MergeRequest({ + // eslint-disable-next-line no-new + new MergeRequest({ action: mrShowNode.dataset.mrAction, }); @@ -42,7 +44,9 @@ export default function initMrNotes() { }; }, computed: { - ...mapGetters(['activeTab']), + ...mapState({ + activeTab: state => state.page.activeTab, + }), }, mounted() { this.setActiveTab(window.mrTabs.getCurrentAction()); @@ -95,7 +99,9 @@ export default function initMrNotes() { }; }, computed: { - ...mapGetters(['activeTab']), + ...mapState({ + activeTab: state => state.page.activeTab, + }), }, render(createElement) { return createElement('diffs-app', { diff --git a/app/assets/javascripts/mr_notes/stores/getters.js b/app/assets/javascripts/mr_notes/stores/getters.js index c68a84ce7af..b10e9f9f9f1 100644 --- a/app/assets/javascripts/mr_notes/stores/getters.js +++ b/app/assets/javascripts/mr_notes/stores/getters.js @@ -1,5 +1,5 @@ export default { - activeTab(state) { - return state.activeTab; + isLoggedIn(state, getters) { + return !!getters.getUserData.id; }, }; diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index b37b30efdc0..e2b3f6b1806 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -33,7 +33,7 @@ import { getPagePath, scrollToElement, isMetaKey, - hasVueMRDiscussionsCookie, + isInMRPage, } from './lib/utils/common_utils'; import imageDiffHelper from './image_diff/helpers/index'; import { localTimeAgo } from './lib/utils/datetime_utility'; @@ -48,21 +48,9 @@ const MAX_VISIBLE_COMMIT_LIST_COUNT = 3; const REGEX_QUICK_ACTIONS = /^\/\w+.*$/gm; export default class Notes { - static initialize( - notes_url, - note_ids, - last_fetched_at, - view, - enableGFM = true, - ) { + static initialize(notes_url, note_ids, last_fetched_at, view, enableGFM = true) { if (!this.instance) { - this.instance = new Notes( - notes_url, - note_ids, - last_fetched_at, - view, - enableGFM, - ); + this.instance = new Notes(notes_url, note_ids, last_fetched_at, view, enableGFM); } } @@ -105,9 +93,7 @@ export default class Notes { this.basePollingInterval = 15000; this.maxPollingSteps = 4; - this.$wrapperEl = hasVueMRDiscussionsCookie() - ? $(document).find('.diffs') - : $(document); + this.$wrapperEl = isInMRPage() ? $(document).find('.diffs') : $(document); this.cleanBinding(); this.addBinding(); this.setPollingInterval(); @@ -147,55 +133,27 @@ export default class Notes { // Reopen and close actions for Issue/MR combined with note form submit this.$wrapperEl.on('click', '.js-comment-submit-button', this.postComment); this.$wrapperEl.on('click', '.js-comment-save-button', this.updateComment); - this.$wrapperEl.on( - 'keyup input', - '.js-note-text', - this.updateTargetButtons, - ); + this.$wrapperEl.on('keyup input', '.js-note-text', this.updateTargetButtons); // resolve a discussion this.$wrapperEl.on('click', '.js-comment-resolve-button', this.postComment); // remove a note (in general) this.$wrapperEl.on('click', '.js-note-delete', this.removeNote); // delete note attachment - this.$wrapperEl.on( - 'click', - '.js-note-attachment-delete', - this.removeAttachment, - ); + this.$wrapperEl.on('click', '.js-note-attachment-delete', this.removeAttachment); // reset main target form when clicking discard this.$wrapperEl.on('click', '.js-note-discard', this.resetMainTargetForm); // update the file name when an attachment is selected - this.$wrapperEl.on( - 'change', - '.js-note-attachment-input', - this.updateFormAttachment, - ); + this.$wrapperEl.on('change', '.js-note-attachment-input', this.updateFormAttachment); // reply to diff/discussion notes - this.$wrapperEl.on( - 'click', - '.js-discussion-reply-button', - this.onReplyToDiscussionNote, - ); + this.$wrapperEl.on('click', '.js-discussion-reply-button', this.onReplyToDiscussionNote); // add diff note this.$wrapperEl.on('click', '.js-add-diff-note-button', this.onAddDiffNote); // add diff note for images - this.$wrapperEl.on( - 'click', - '.js-add-image-diff-note-button', - this.onAddImageDiffNote, - ); + this.$wrapperEl.on('click', '.js-add-image-diff-note-button', this.onAddImageDiffNote); // hide diff note form - this.$wrapperEl.on( - 'click', - '.js-close-discussion-note-form', - this.cancelDiscussionForm, - ); + this.$wrapperEl.on('click', '.js-close-discussion-note-form', this.cancelDiscussionForm); // toggle commit list - this.$wrapperEl.on( - 'click', - '.system-note-commit-list-toggler', - this.toggleCommitList, - ); + this.$wrapperEl.on('click', '.system-note-commit-list-toggler', this.toggleCommitList); this.$wrapperEl.on('click', '.js-toggle-lazy-diff', this.loadLazyDiff); // fetch notes when tab becomes visible @@ -204,16 +162,8 @@ export default class Notes { this.$wrapperEl.on('issuable:change', this.refresh); // ajax:events that happen on Form when actions like Reopen, Close are performed on Issues and MRs. this.$wrapperEl.on('ajax:success', '.js-main-target-form', this.addNote); - this.$wrapperEl.on( - 'ajax:success', - '.js-discussion-note-form', - this.addDiscussionNote, - ); - this.$wrapperEl.on( - 'ajax:success', - '.js-main-target-form', - this.resetMainTargetForm, - ); + this.$wrapperEl.on('ajax:success', '.js-discussion-note-form', this.addDiscussionNote); + this.$wrapperEl.on('ajax:success', '.js-main-target-form', this.resetMainTargetForm); this.$wrapperEl.on( 'ajax:complete', '.js-main-target-form', @@ -223,8 +173,6 @@ export default class Notes { this.$wrapperEl.on('keydown', '.js-note-text', this.keydownNoteText); // When the URL fragment/hash has changed, `#note_xxx` $(window).on('hashchange', this.onHashChange); - this.boundGetContent = this.getContent.bind(this); - this.eventsBound = true; } cleanBinding() { @@ -251,16 +199,10 @@ export default class Notes { } static initCommentTypeToggle(form) { - const dropdownTrigger = form.querySelector( - '.js-comment-type-dropdown .dropdown-toggle', - ); - const dropdownList = form.querySelector( - '.js-comment-type-dropdown .dropdown-menu', - ); + const dropdownTrigger = form.querySelector('.js-comment-type-dropdown .dropdown-toggle'); + const dropdownList = form.querySelector('.js-comment-type-dropdown .dropdown-menu'); const noteTypeInput = form.querySelector('#note_type'); - const submitButton = form.querySelector( - '.js-comment-type-dropdown .js-comment-submit-button', - ); + const submitButton = form.querySelector('.js-comment-type-dropdown .js-comment-submit-button'); const closeButton = form.querySelector('.js-note-target-close'); const reopenButton = form.querySelector('.js-note-target-reopen'); @@ -296,9 +238,7 @@ export default class Notes { return; } myLastNote = $( - `li.note[data-author-id='${ - gon.current_user_id - }'][data-editable]:last`, + `li.note[data-author-id='${gon.current_user_id}'][data-editable]:last`, $textarea.closest('.note, .notes_holder, #notes'), ); if (myLastNote.length) { @@ -311,9 +251,7 @@ export default class Notes { discussionNoteForm = $textarea.closest('.js-discussion-note-form'); if (discussionNoteForm.length) { if ($textarea.val() !== '') { - if ( - !confirm('Are you sure you want to cancel creating this comment?') - ) { + if (!confirm('Are you sure you want to cancel creating this comment?')) { return; } } @@ -325,9 +263,7 @@ export default class Notes { originalText = $textarea.closest('form').data('originalNote'); newText = $textarea.val(); if (originalText !== newText) { - if ( - !confirm('Are you sure you want to cancel editing this comment?') - ) { + if (!confirm('Are you sure you want to cancel editing this comment?')) { return; } } @@ -395,8 +331,7 @@ export default class Notes { if (shouldReset == null) { shouldReset = true; } - nthInterval = - this.basePollingInterval * Math.pow(2, this.maxPollingSteps - 1); + nthInterval = this.basePollingInterval * Math.pow(2, this.maxPollingSteps - 1); if (shouldReset) { this.pollingInterval = this.basePollingInterval; } else if (this.pollingInterval < nthInterval) { @@ -417,10 +352,7 @@ export default class Notes { loadAwardsHandler() .then(awardsHandler => { - awardsHandler.addAwardToEmojiBar( - votesBlock, - noteEntity.commands_changes.emoji_award, - ); + awardsHandler.addAwardToEmojiBar(votesBlock, noteEntity.commands_changes.emoji_award); awardsHandler.scrollToAwards(); }) .catch(() => { @@ -470,17 +402,10 @@ export default class Notes { if (!noteEntity.valid) { if (noteEntity.errors && noteEntity.errors.commands_only) { - if ( - noteEntity.commands_changes && - Object.keys(noteEntity.commands_changes).length > 0 - ) { + if (noteEntity.commands_changes && Object.keys(noteEntity.commands_changes).length > 0) { $notesList.find('.system-note.being-posted').remove(); } - this.addFlash( - noteEntity.errors.commands_only, - 'notice', - this.parentTimeline.get(0), - ); + this.addFlash(noteEntity.errors.commands_only, 'notice', this.parentTimeline.get(0)); this.refresh(); } return; @@ -488,7 +413,7 @@ export default class Notes { const $note = $notesList.find(`#note_${noteEntity.id}`); if (Notes.isNewNote(noteEntity, this.note_ids)) { - if (hasVueMRDiscussionsCookie()) { + if (isInMRPage()) { return; } @@ -516,8 +441,7 @@ export default class Notes { // There can be CRLF vs LF mismatches if we don't sanitize and compare the same way const sanitizedNoteNote = normalizeNewlines(noteEntity.note); const isTextareaUntouched = - currentContent === initialContent || - currentContent === sanitizedNoteNote; + currentContent === initialContent || currentContent === sanitizedNoteNote; if (isEditing && isTextareaUntouched) { $textarea.val(noteEntity.note); @@ -547,13 +471,7 @@ export default class Notes { } this.note_ids.push(noteEntity.id); - form = - $form || - $( - `.js-discussion-note-form[data-discussion-id="${ - noteEntity.discussion_id - }"]`, - ); + form = $form || $(`.js-discussion-note-form[data-discussion-id="${noteEntity.discussion_id}"]`); row = form.length || !noteEntity.discussion_line_code ? form.closest('tr') @@ -569,9 +487,7 @@ export default class Notes { .first() .find('.js-avatar-container.' + lineType + '_line'); // is this the first note of discussion? - discussionContainer = $( - `.notes[data-discussion-id="${noteEntity.discussion_id}"]`, - ); + discussionContainer = $(`.notes[data-discussion-id="${noteEntity.discussion_id}"]`); if (!discussionContainer.length) { discussionContainer = form.closest('.discussion').find('.notes'); } @@ -579,18 +495,12 @@ export default class Notes { if (noteEntity.diff_discussion_html) { var $discussion = $(noteEntity.diff_discussion_html).renderGFM(); - if ( - !this.isParallelView() || - row.hasClass('js-temp-notes-holder') || - noteEntity.on_image - ) { + if (!this.isParallelView() || row.hasClass('js-temp-notes-holder') || noteEntity.on_image) { // insert the note and the reply button after the temp row row.after($discussion); } else { // Merge new discussion HTML in - var $notes = $discussion.find( - `.notes[data-discussion-id="${noteEntity.discussion_id}"]`, - ); + var $notes = $discussion.find(`.notes[data-discussion-id="${noteEntity.discussion_id}"]`); var contentContainerClass = '.' + $notes @@ -604,28 +514,12 @@ export default class Notes { .append($notes.closest('.content').children()); } } - // Init discussion on 'Discussion' page if it is merge request page - const page = $('body').attr('data-page'); - if ( - (page && page.indexOf('projects:merge_request') !== -1) || - !noteEntity.diff_discussion_html - ) { - if (!hasVueMRDiscussionsCookie()) { - Notes.animateAppendNote( - noteEntity.discussion_html, - $('.main-notes-list'), - ); - } - } } else { // append new note to all matching discussions Notes.animateAppendNote(noteEntity.html, discussionContainer); } - if ( - typeof gl.diffNotesCompileComponents !== 'undefined' && - noteEntity.discussion_resolvable - ) { + if (typeof gl.diffNotesCompileComponents !== 'undefined' && noteEntity.discussion_resolvable) { gl.diffNotesCompileComponents(); this.renderDiscussionAvatar(diffAvatarContainer, noteEntity); @@ -934,9 +828,7 @@ export default class Notes { form.removeClass('current-note-edit-form'); form.find('.js-finish-edit-warning').hide(); // Replace markdown textarea text with original note text. - return form - .find('.js-note-text') - .val(form.find('form.edit-note').data('originalNote')); + return form.find('.js-note-text').val(form.find('form.edit-note').data('originalNote')); } /** @@ -984,21 +876,15 @@ export default class Notes { // The notes tr can contain multiple lists of notes, like on the parallel diff // notesTr does not exist for image diffs - if ( - notesTr.find('.discussion-notes').length > 1 || - notesTr.length === 0 - ) { + if (notesTr.find('.discussion-notes').length > 1 || notesTr.length === 0) { const $diffFile = $notes.closest('.diff-file'); if ($diffFile.length > 0) { - const removeBadgeEvent = new CustomEvent( - 'removeBadge.imageDiff', - { - detail: { - // badgeNumber's start with 1 and index starts with 0 - badgeNumber: $notes.index() + 1, - }, + const removeBadgeEvent = new CustomEvent('removeBadge.imageDiff', { + detail: { + // badgeNumber's start with 1 and index starts with 0 + badgeNumber: $notes.index() + 1, }, - ); + }); $diffFile[0].dispatchEvent(removeBadgeEvent); } @@ -1101,9 +987,7 @@ export default class Notes { form.find('.js-note-new-discussion').remove(); this.setupNoteForm(form); - form - .removeClass('js-main-target-form') - .addClass('discussion-form js-discussion-note-form'); + form.removeClass('js-main-target-form').addClass('discussion-form js-discussion-note-form'); if (typeof gl.diffNotesCompileComponents !== 'undefined') { var $commentBtn = form.find('comment-and-resolve-btn'); @@ -1113,9 +997,7 @@ export default class Notes { } form.find('.js-note-text').focus(); - form - .find('.js-comment-resolve-button') - .attr('data-discussion-id', discussionID); + form.find('.js-comment-resolve-button').attr('data-discussion-id', discussionID); } /** @@ -1148,9 +1030,7 @@ export default class Notes { // Setup comment form let newForm; - const $noteContainer = $link - .closest('.diff-viewer') - .find('.note-container'); + const $noteContainer = $link.closest('.diff-viewer').find('.note-container'); const $form = $noteContainer.find('> .discussion-form'); if ($form.length === 0) { @@ -1219,9 +1099,7 @@ export default class Notes { notesContent = targetRow.find(notesContentSelector); addForm = true; } else { - const isCurrentlyShown = targetRow - .find('.content:not(:empty)') - .is(':visible'); + const isCurrentlyShown = targetRow.find('.content:not(:empty)').is(':visible'); const isForced = forceShow === true || forceShow === false; const showNow = forceShow === true || (!isCurrentlyShown && !isForced); @@ -1386,9 +1264,7 @@ export default class Notes { if ($note.find('.js-conflict-edit-warning').length === 0) { const $alert = $(`<div class="js-conflict-edit-warning alert alert-danger"> This comment has changed since you started editing, please review the - <a href="#note_${ - noteEntity.id - }" target="_blank" rel="noopener noreferrer"> + <a href="#note_${noteEntity.id}" target="_blank" rel="noopener noreferrer"> updated comment </a> to ensure information is not lost @@ -1398,9 +1274,7 @@ export default class Notes { } updateNotesCount(updateCount) { - return this.notesCountBadge.text( - parseInt(this.notesCountBadge.text(), 10) + updateCount, - ); + return this.notesCountBadge.text(parseInt(this.notesCountBadge.text(), 10) + updateCount); } static renderPlaceholderComponent($container) { @@ -1463,9 +1337,7 @@ export default class Notes { toggleCommitList(e) { const $element = $(e.currentTarget); - const $closestSystemCommitList = $element.siblings( - '.system-note-commit-list', - ); + const $closestSystemCommitList = $element.siblings('.system-note-commit-list'); $element .find('.fa') @@ -1498,9 +1370,7 @@ export default class Notes { $systemNote.find('.note-text').addClass('system-note-commit-list'); $systemNote.find('.system-note-commit-list-toggler').show(); } else { - $systemNote - .find('.note-text') - .addClass('system-note-commit-list hide-shade'); + $systemNote.find('.note-text').addClass('system-note-commit-list hide-shade'); } }); } @@ -1651,12 +1521,8 @@ export default class Notes { <div class="note-header"> <div class="note-header-info"> <a href="/${_.escape(currentUsername)}"> - <span class="hidden-xs">${_.escape( - currentUsername, - )}</span> - <span class="note-headline-light">${_.escape( - currentUsername, - )}</span> + <span class="hidden-xs">${_.escape(currentUsername)}</span> + <span class="note-headline-light">${_.escape(currentUsername)}</span> </a> </div> </div> @@ -1671,9 +1537,7 @@ export default class Notes { ); $tempNote.find('.hidden-xs').text(_.escape(currentUserFullname)); - $tempNote - .find('.note-headline-light') - .text(`@${_.escape(currentUsername)}`); + $tempNote.find('.note-headline-light').text(`@${_.escape(currentUsername)}`); return $tempNote; } @@ -1729,15 +1593,8 @@ export default class Notes { .attr('id') === 'discussion'; const isMainForm = $form.hasClass('js-main-target-form'); const isDiscussionForm = $form.hasClass('js-discussion-note-form'); - const isDiscussionResolve = $submitBtn.hasClass( - 'js-comment-resolve-button', - ); - const { - formData, - formContent, - formAction, - formContentOriginal, - } = this.getFormData($form); + const isDiscussionResolve = $submitBtn.hasClass('js-comment-resolve-button'); + const { formData, formContent, formAction, formContentOriginal } = this.getFormData($form); let noteUniqueId; let systemNoteUniqueId; let hasQuickActions = false; @@ -1825,9 +1682,7 @@ export default class Notes { // Reset cached commands list when command is applied if (hasQuickActions) { - $form - .find('textarea.js-note-text') - .trigger('clear-commands-cache.atwho'); + $form.find('textarea.js-note-text').trigger('clear-commands-cache.atwho'); } // Clear previous form errors @@ -1872,9 +1727,7 @@ export default class Notes { // append flash-container to the Notes list if ($notesContainer.length) { - $notesContainer.append( - '<div class="flash-container" style="display: none;"></div>', - ); + $notesContainer.append('<div class="flash-container" style="display: none;"></div>'); } } else if (isMainForm) { // Check if this was main thread comment @@ -1909,9 +1762,7 @@ export default class Notes { // Show form again on UI on failure if (isDiscussionForm && $notesContainer.length) { - const replyButton = $notesContainer - .parent() - .find('.js-discussion-reply-button'); + const replyButton = $notesContainer.parent().find('.js-discussion-reply-button'); this.replyToDiscussionNote(replyButton[0]); $form = $notesContainer.parent().find('form'); } @@ -1954,9 +1805,7 @@ export default class Notes { // Show updated comment content temporarily $noteBodyText.html(formContent); - $editingNote - .removeClass('is-editing fade-in-full') - .addClass('being-posted fade-in-half'); + $editingNote.removeClass('is-editing fade-in-full').addClass('being-posted fade-in-half'); $editingNote .find('.note-headline-meta a') .html( diff --git a/app/assets/javascripts/notes/components/diff_file_header.vue b/app/assets/javascripts/notes/components/diff_file_header.vue index 3e09391a9ac..5cc1367600e 100644 --- a/app/assets/javascripts/notes/components/diff_file_header.vue +++ b/app/assets/javascripts/notes/components/diff_file_header.vue @@ -27,6 +27,24 @@ export default { titleTag() { return this.diffFile.fileHash ? 'a' : 'span'; }, + baseSha() { + return this.diffFile.diffRefs.baseSha; + }, + contentSha() { + return this.diffFile.contentSha; + }, + truncatedBaseSha() { + return this.truncate(this.baseSha); + }, + truncatedContentSha() { + return this.truncate(this.contentSha); + }, + imageDiff() { + return !this.diffFile.text; + }, + replacedFile() { + return !(this.diffFile.newFile || this.diffFile.deletedFile); + }, }, methods: { handleToggle(e, checkTarget) { @@ -38,7 +56,9 @@ export default { this.$emit('toggleFile'); } }, - noop() {}, + truncate(sha) { + return sha.slice(0, 8); + }, }, }; </script> @@ -49,11 +69,14 @@ export default { ref="header" class="file-header-content" > - <i + <icon v-if="collapsible" @click.stop="handleToggle" - class="fa diff-toggle-caret fa-fw fa-caret-down" - ></i> + name="chevron-down" + aria-hidden="true" + :size="16" + class="diff-toggle-caret" + /> <div v-if="diffFile.submodule" > @@ -76,7 +99,7 @@ export default { :is="titleTag" :href="`#${diffFile.fileHash}`" > - <span v-html="diffFile.blobIcon"></span> + <i class="fa fa-fw" :class="`fa-${diffFile.blob.icon}`"></i> <span v-if="diffFile.renamedFile"> <strong class="file-title-name has-tooltip" @@ -121,5 +144,54 @@ export default { {{ diffFile.aMode }} → {{ diffFile.bMode }} </small> </template> + + + <div + v-if="!diffFile.submodule" + class="file-actions hidden-xs" + > + <template + v-if="diffFile.blob && diffFile.blob.readableText" + > + <button + class="js-toggle-diff-comments btn" + title="Toggle comments for this file" + type="button" + :class="{ + active: 'todo' + }" + > + <icon name="comment" /> + </button> + + <a + :href="diffFile.editPath" + class="btn btn-default js-edit-blob" + > + Edit + </a> + </template> + + <a + v-if="imageDiff && replacedFile" + class="btn view-file js-view-file" + :href="baseSha" + > + View replaced file @ <span class="commit-sha">{{ truncatedBaseSha }}</span> + </a> + <a + class="btn view-file js-view-file" + :href="contentSha" + > + View file @ <span class="commit-sha">{{ truncatedContentSha }}</span> + </a> + + <button + v-if="diffFile.environment" + > + View on environment + <!-- = view_on_environment_button(diff_file.content_sha, diff_file.file_path, environment) if environment --> + </button> + </div> </div> </template> diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index c2ce453d34a..b2e27f6dd3f 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -140,6 +140,7 @@ export const toggleIssueLocalState = ({ commit }, newState) => { }; export const saveNote = ({ commit, dispatch }, noteData) => { + // For MR discussuions we need to post as `note[note]` and issue we use `note.note`. const note = noteData.data['note[note]'] || noteData.data.note.note; let placeholderText = note; const hasQuickActions = utils.hasQuickActions(placeholderText); diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 718634595e3..82f31bee174 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -28,8 +28,8 @@ export const notesById = state => export const discussionsByLineCode = state => state.notes.reduce((acc, note) => { - if (note.diff_discussion) { - // For context line notes, there might be multiple notes with the same line code + if (note.diff_discussion && note.line_code) { + // For context about line notes: there might be multiple notes with the same line code const items = acc[note.line_code] || []; items.push(note); diff --git a/app/assets/javascripts/pages/projects/merge_requests/show/index.js b/app/assets/javascripts/pages/projects/merge_requests/show/index.js index e5b2827b50c..f61f4db78d5 100644 --- a/app/assets/javascripts/pages/projects/merge_requests/show/index.js +++ b/app/assets/javascripts/pages/projects/merge_requests/show/index.js @@ -1,4 +1,3 @@ -import { hasVueMRDiscussionsCookie } from '~/lib/utils/common_utils'; import initMrNotes from '~/mr_notes'; import initSidebarBundle from '~/sidebar/sidebar_bundle'; import initShow from '../init_merge_request_show'; @@ -6,8 +5,5 @@ import initShow from '../init_merge_request_show'; document.addEventListener('DOMContentLoaded', () => { initShow(); initSidebarBundle(); - - if (hasVueMRDiscussionsCookie()) { - initMrNotes(); - } + initMrNotes(); }); diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 7f037582ca0..7a94e5d27bc 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -71,6 +71,12 @@ span { white-space: pre-wrap; + + &.context-cell { + display: inline-block; + width: 100%; + height: 100%; + } } .line { diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index a698a11fb9b..35ecd05e34e 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -3,9 +3,15 @@ */ @-webkit-keyframes targe3-note { - from { background: $note-targe3-outside; } - 50% { background: $note-targe3-inside; } - to { background: $note-targe3-outside; } + from { + background: $note-targe3-outside; + } + 50% { + background: $note-targe3-inside; + } + to { + background: $note-targe3-outside; + } } ul.notes { @@ -36,7 +42,8 @@ ul.notes { } } - > li { // .timeline-entry + > li { + // .timeline-entry padding: 0; display: block; position: relative; @@ -153,7 +160,6 @@ ul.notes { } .note-header { - @include notes-media('max', $screen-xs-min) { .inline { display: block; @@ -282,7 +288,10 @@ ul.notes { position: absolute; left: 0; bottom: 0; - background: linear-gradient(rgba($white-light, 0.1) -100px, $white-light 100%); + background: linear-gradient( + rgba($white-light, 0.1) -100px, + $white-light 100% + ); } } } @@ -666,7 +675,6 @@ ul.notes { background-color: $white-light; } - a { color: $gl-link-color; } @@ -745,7 +753,8 @@ ul.notes { border: 0; outline: 0; color: $gray-darkest; - transition: color $general-hover-transition-duration $general-hover-transition-curve; + transition: color $general-hover-transition-duration + $general-hover-transition-curve; &.is-disabled { cursor: default; @@ -807,4 +816,8 @@ ul.notes { .diff-comment-form { display: block; } + + .add-diff-note svg { + margin-top: 4px; + } } diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 0c1c286a0a4..a366559ae1d 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -3,8 +3,8 @@ class Projects::BlobController < Projects::ApplicationController include ExtractsPath include CreatesCommit include RendersBlob + include NotesHelper include ActionView::Helpers::SanitizeHelper - prepend_before_action :authenticate_user!, only: [:edit] before_action :require_non_empty_project, except: [:new, :create] @@ -92,6 +92,7 @@ class Projects::BlobController < Projects::ApplicationController @lines = Gitlab::Highlight.highlight(@blob.path, @blob.data, repository: @repository).lines @form = UnfoldForm.new(params) + @lines = @lines[@form.since - 1..@form.to - 1].map(&:html_safe) if @form.bottom? @@ -102,11 +103,45 @@ class Projects::BlobController < Projects::ApplicationController @match_line = "@@ -#{line}+#{line} @@" end - render layout: false + if has_vue_discussions_cookie? + render_diff_lines + else + render layout: false + end end private + # Converts a String array to Gitlab::Diff::Line array + def render_diff_lines + @lines.map! do |line| + diff_line = Gitlab::Diff::Line.new(line ,'context', nil, nil, nil) + diff_line.rich_text = line + diff_line + end + + add_match_line + + render json: @lines + end + + def add_match_line + return unless @form.unfold? + + if @form.bottom? && @form.to < @blob.lines.size + old_pos = @form.to - @form.offset + new_pos = @form.to + elsif @form.since != 1 + old_pos = new_pos = @form.since + end + + return unless new_pos + + @match_line = Gitlab::Diff::Line.new(@match_line ,'match', nil, old_pos, new_pos) + + @form.bottom? ? @lines.push(@match_line) : @lines.unshift(@match_line) + end + def blob @blob ||= @repository.blob_at(@commit.id, @path) diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index fe8525a488c..242bcfb574d 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/app/controllers/projects/merge_requests/diffs_controller.rb @@ -1,6 +1,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::ApplicationController include DiffForPath include DiffHelper + include NotesHelper include RendersNotes before_action :apply_diff_view_cookie! @@ -11,7 +12,11 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic def show @environment = @merge_request.environments_for(current_user).last - render json: { html: view_to_html_string("projects/merge_requests/diffs/_diffs") } + if has_vue_discussions_cookie? + render json: DiffsSerializer.new.represent(@diffs, merge_request: @merge_request) + else + render json: { html: view_to_html_string("projects/merge_requests/diffs/_diffs") } + end end def diff_for_path diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 20aed60cb7a..a30ba37c568 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -179,7 +179,7 @@ module NotesHelper end def has_vue_discussions_cookie? - cookies[:vue_mr_discussions] == 'true' + true end def serialize_notes? diff --git a/app/serializers/blob_entity.rb b/app/serializers/blob_entity.rb index ad039a2623d..b501fd5e964 100644 --- a/app/serializers/blob_entity.rb +++ b/app/serializers/blob_entity.rb @@ -3,11 +3,13 @@ class BlobEntity < Grape::Entity expose :id, :path, :name, :mode + expose :readable_text?, as: :readable_text + expose :icon do |blob| IconsHelper.file_type_icon_class('file', blob.mode, blob.name) end - expose :url do |blob| + expose :url, if: -> (*) { request.respond_to?(:ref) } do |blob| project_blob_path(request.project, File.join(request.ref, blob.path)) end end diff --git a/app/serializers/diff_file_entity.rb b/app/serializers/diff_file_entity.rb index 6e68d275047..793a28a7beb 100644 --- a/app/serializers/diff_file_entity.rb +++ b/app/serializers/diff_file_entity.rb @@ -1,7 +1,9 @@ class DiffFileEntity < Grape::Entity + include RequestAwareEntity include DiffHelper include SubmoduleHelper include BlobHelper + include TreeHelper include IconsHelper include ActionView::Helpers::TagHelper @@ -11,15 +13,26 @@ class DiffFileEntity < Grape::Entity submodule_links(diff_file.blob, diff_file.content_sha, diff_file.repository).first end + expose :blob, using: BlobEntity + expose :blob_path do |diff_file| diff_file.blob.path end + expose :blob_name do |diff_file| + diff_file.blob.name + end + expose :blob_icon do |diff_file| blob_icon(diff_file.b_mode, diff_file.file_path) end + expose :file_hash do |diff_file| + Digest::SHA1.hexdigest(diff_file.file_path) + end + expose :file_path + expose :new_file?, as: :new_file expose :deleted_file?, as: :deleted_file expose :renamed_file?, as: :renamed_file expose :old_path @@ -28,6 +41,10 @@ class DiffFileEntity < Grape::Entity expose :a_mode expose :b_mode expose :text?, as: :text + expose :added_lines + expose :removed_lines + expose :diff_refs + expose :content_sha expose :old_path_html do |diff_file| old_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path) @@ -38,4 +55,57 @@ class DiffFileEntity < Grape::Entity _, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path) new_path end + + # TODO check if these are not creating a n+1 call + # we should probably also pass project as parameter + expose :edit_path, if: -> (_, options) { options[:merge_request] } do |diff_file| + merge_request = options[:merge_request] + + edit_blob_path(merge_request.source_project, merge_request.source_branch, diff_file.new_path) + end + + expose :view_path, if: -> (_, options) { options[:merge_request] } do |diff_file| + merge_request = options[:merge_request] + + project_blob_path(merge_request.source_project, tree_join(merge_request.source_branch, diff_file.new_path)) + end + + expose :context_lines_path, if: -> (diff_file, _) { diff_file.text? } do |diff_file| + project_blob_diff_path(diff_file.repository.project, tree_join(diff_file.content_sha, diff_file.file_path)) + end + + # Used for inline diffs + expose :highlighted_diff_lines, if: -> (diff_file, _) { diff_file.text? } do |diff_file| + lines = diff_file.highlighted_diff_lines + + add_bottom_match_line(lines, diff_file) + + lines + end + + # Used for parallel diffs + expose :parallel_diff_lines, if: -> (diff_file, _) { diff_file.text? } + + private + + # This adds the bottom line to the array which contains + # the data to load more context lines. + def add_bottom_match_line(lines, diff_file) + return if lines.empty? + + last_line = lines.last + + return unless last_line.new_pos < total_lines(diff_file.blob) + + match_line = Gitlab::Diff::Line.new("" ,'match', nil, last_line.old_pos, last_line.new_pos) + lines.push(match_line) + end + + def total_lines(blob) + @total_lines ||= begin + line_count = blob.lines.size + line_count -= 1 if line_count > 0 && blob.lines.last.blank? + line_count + end + end end diff --git a/app/serializers/diff_file_serializer.rb b/app/serializers/diff_file_serializer.rb new file mode 100644 index 00000000000..8fcb7d67e34 --- /dev/null +++ b/app/serializers/diff_file_serializer.rb @@ -0,0 +1,3 @@ +class DiffFileSerializer < BaseSerializer + entity DiffFileEntity +end diff --git a/app/serializers/diffs_entity.rb b/app/serializers/diffs_entity.rb new file mode 100644 index 00000000000..fb8027059cb --- /dev/null +++ b/app/serializers/diffs_entity.rb @@ -0,0 +1,13 @@ +class DiffsEntity < Grape::Entity + expose :real_size + + expose :added_lines do |diffs| + diffs.diff_files.sum(&:added_lines) + end + + expose :removed_lines do |diffs| + diffs.diff_files.sum(&:removed_lines) + end + + expose :diff_files, using: DiffFileEntity +end diff --git a/app/serializers/diffs_serializer.rb b/app/serializers/diffs_serializer.rb new file mode 100644 index 00000000000..6771e10c5ac --- /dev/null +++ b/app/serializers/diffs_serializer.rb @@ -0,0 +1,3 @@ +class DiffsSerializer < BaseSerializer + entity DiffsEntity +end diff --git a/app/serializers/discussion_entity.rb b/app/serializers/discussion_entity.rb index bbbcf6a97c1..6af0973c1dd 100644 --- a/app/serializers/discussion_entity.rb +++ b/app/serializers/discussion_entity.rb @@ -2,6 +2,8 @@ class DiscussionEntity < Grape::Entity include RequestAwareEntity expose :id, :reply_id + expose :position, if: -> (d, _) { defined? d.diff_file } + expose :line_code, if: -> (d, _) { defined? d.diff_file } expose :expanded?, as: :expanded expose :notes, using: NoteEntity diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml index 112f27ef513..6029738f437 100644 --- a/app/views/projects/merge_requests/show.html.haml +++ b/app/views/projects/merge_requests/show.html.haml @@ -76,11 +76,12 @@ %section.col-md-12 %script.js-notes-data{ type: "application/json" }= initial_notes_data(true).to_json.html_safe .issuable-discussion.js-vue-notes-event - = render "projects/merge_requests/discussion" - if has_vue_discussions_cookie? #js-vue-mr-discussions{ data: { notes_data: notes_data(@merge_request), noteable_data: serialize_issuable(@merge_request), current_user_data: UserSerializer.new.represent(current_user).to_json} } + - else + = render "projects/merge_requests/discussion" #commits.commits.tab-pane -# This tab is always loaded via AJAX diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index 014854da55c..30eab47575c 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -120,11 +120,13 @@ module Gitlab # Array of Gitlab::Diff::Line objects def diff_lines - @diff_lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a + @diff_lines ||= + Gitlab::Diff::Parser.new.parse(raw_diff.each_line, diff_file: self).to_a end def highlighted_diff_lines - @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight + @highlighted_diff_lines ||= + Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight end # Array[<Hash>] with right/left keys that contains Gitlab::Diff::Line objects which text is hightlighted diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index 269016daac2..16da773bb4a 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -5,7 +5,7 @@ module Gitlab delegate :old_path, :new_path, :old_sha, :new_sha, to: :diff_file, prefix: :diff - def initialize(diff_lines, repository: nil) + def initialize(diff_lines, since: nil, from: nil, repository: nil) @repository = repository if diff_lines.is_a?(Gitlab::Diff::File) diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb index 0603141e441..b8966110f5a 100644 --- a/lib/gitlab/diff/line.rb +++ b/lib/gitlab/diff/line.rb @@ -1,22 +1,26 @@ module Gitlab module Diff class Line - attr_reader :type, :index, :old_pos, :new_pos + attr_reader :line_code, :type, :index, :old_pos, :new_pos, :meta_data attr_writer :rich_text attr_accessor :text - def initialize(text, type, index, old_pos, new_pos, parent_file: nil) + def initialize(text, type, index, old_pos, new_pos, parent_file: nil, line_code: nil) @text, @type, @index = text, type, index @old_pos, @new_pos = old_pos, new_pos @parent_file = parent_file + + # When line code is not provided from cache store we build it + # using the parent_file(Diff::File or Conflict::File). + @line_code = line_code || set_line_code end def self.init_from_hash(hash) - new(hash[:text], hash[:type], hash[:index], hash[:old_pos], hash[:new_pos]) + new(hash[:text], hash[:type], hash[:index], hash[:old_pos], hash[:new_pos], line_code: hash[:line_code]) end def serialize_keys - @serialize_keys ||= %i(text type index old_pos new_pos) + @serialize_keys ||= %i(line_code text type index old_pos new_pos) end def to_hash @@ -58,20 +62,39 @@ module Gitlab end def rich_text - @parent_file.highlight_lines! if @parent_file && !@rich_text + @parent_file.try(:highlight_lines!) if @parent_file && !@rich_text @rich_text end + def meta_data + return unless meta? + + { + old_pos: old_pos, + new_pos: new_pos + } + end + def as_json(opts = nil) { + line_code: line_code, type: type, old_line: old_line, new_line: new_line, text: text, - rich_text: rich_text || text + rich_text: rich_text || text, + meta_data: meta_data } end + + private + + def set_line_code + return nil unless @parent_file + + @parent_file.line_code(self) + end end end end diff --git a/lib/gitlab/diff/parser.rb b/lib/gitlab/diff/parser.rb index 8302f30a0a2..7ae7ed286ed 100644 --- a/lib/gitlab/diff/parser.rb +++ b/lib/gitlab/diff/parser.rb @@ -3,7 +3,7 @@ module Gitlab class Parser include Enumerable - def parse(lines) + def parse(lines, diff_file: nil) return [] if lines.blank? @lines = lines @@ -31,17 +31,17 @@ module Gitlab next if line_old <= 1 && line_new <= 1 # top of file - yielder << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new) + yielder << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new, parent_file: diff_file) line_obj_index += 1 next elsif line[0] == '\\' type = "#{context}-nonewline" - yielder << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new) + yielder << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new, parent_file: diff_file) line_obj_index += 1 else type = identification_type(line) - yielder << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new) + yielder << Gitlab::Diff::Line.new(full_line, type, line_obj_index, line_old, line_new, parent_file: diff_file) line_obj_index += 1 end diff --git a/spec/serializers/discussion_entity_spec.rb b/spec/serializers/discussion_entity_spec.rb index 7ee8e38af1c..c0db71c693f 100644 --- a/spec/serializers/discussion_entity_spec.rb +++ b/spec/serializers/discussion_entity_spec.rb @@ -30,7 +30,7 @@ describe DiscussionEntity do let(:note) { create(:diff_note_on_merge_request) } it 'exposes diff file attributes' do - expect(subject).to include(:diff_file, :truncated_diff_lines, :image_diff_html) + expect(subject).to include(:diff_file, :truncated_diff_lines, :image_diff_html, :diff_lines, :highlighted_diff_lines) end end end |