Ranged comments integration - gr-file-list recognizes local preferences (for hasRangedComments flag) - gr-file-list reacts to cursor hotkey only if there is no range selected (currently always false). - Remove dead code from GrDiffBuilderSideBySide, GrDiffBuilder, gr-diff-builder.html - Bugfix: GrDiffBuilder.prototype.getGroupsByLineRange handles one-line BOTH code sections correctly. Test updated as well. - Added utitily methods added to gr-diff-builder.html to reduce dependency on DOM structure and reduce amount of code copy-pasting: - renderLineRange, getContentByLine, etc - For gr-diff.js and gr-diff-comment-thread.js addDraft renamed to addOrEditDraft because that's what it does. - For both, addDraft method always creates a draft comment. - Added support for ranged comments in gr-diff, gr-diff-comment-thread. - Added mouseenter and mouseout events to gr-comment.js - Refactored gr-comment.js to reduce code copy-paste, unify event payload, and to eliminate need of accessing component instance for patchNum. Tests updated as well. - Refactored gr-diff.js UI data model update using gr-diff-builder.html utility methods to make code more readable. - Added support for creating ranged comments to gr-diff.js. - gr-selection-action-box now reacts to click and tap to create a comment. Change-Id: I01480a4c6f460774a8b2826915702800b3f81d25
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html index f15c654..11c160f 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -176,10 +176,12 @@ patch-range="[[patchRange]]" path="[[file.__path]]" prefs="[[_diffPrefs]]" + has-ranged-comments="[[_localPrefs.ranged_comments]]" project-config="[[projectConfig]]" view-mode="[[_userPrefs.diff_view]]"></gr-diff> </template> <gr-rest-api-interface id="restAPI"></gr-rest-api-interface> + <gr-storage id="storage"></gr-storage> <gr-diff-cursor id="cursor" fold-offset-top="[[topMargin]]"></gr-diff-cursor>
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js index 82c47d6..b9ad8c9 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -52,6 +52,7 @@ }, _diffPrefs: Object, _userPrefs: Object, + _localPrefs: Object, _showInlineDiffs: Boolean, }, @@ -82,6 +83,7 @@ }); })); + this._localPrefs = this.$.storage.getPreferences(); promises.push(this._getDiffPreferences().then(function(prefs) { this._diffPrefs = prefs; }.bind(this))); @@ -91,6 +93,10 @@ }.bind(this))); }, + get diffs() { + return Polymer.dom(this.root).querySelectorAll('gr-diff'); + }, + _getDiffPreferences: function() { return this.$.restAPI.getDiffPreferences(); }, @@ -122,7 +128,7 @@ }, _forEachDiff: function(fn) { - var diffs = Polymer.dom(this.root).querySelectorAll('gr-diff'); + var diffs = this.diffs; for (var i = 0; i < diffs.length; i++) { fn(diffs[i]); } @@ -252,7 +258,10 @@ } break; case 67: // 'c' - if (this._showInlineDiffs) { + var isRangeSelected = this.diffs.some(function(diff) { + return diff.isRangeSelected(); + }, this); + if (this._showInlineDiffs && !isRangeSelected) { e.preventDefault(); this._addDraftAtTarget(); }
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js index 7e8779f..8f1b6b6 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
@@ -20,8 +20,7 @@ GrDiffBuilderSideBySide.prototype = Object.create(GrDiffBuilder.prototype); GrDiffBuilderSideBySide.prototype.constructor = GrDiffBuilderSideBySide; - GrDiffBuilderSideBySide.prototype.buildSectionElement = function(group, - opt_beforeSection) { + GrDiffBuilderSideBySide.prototype.buildSectionElement = function(group) { var sectionEl = this._createElement('tbody', 'section'); sectionEl.classList.add(group.type); var pairs = group.getSideBySidePairs();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html index d9d6c8a..005947c 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
@@ -77,9 +77,84 @@ return null; }, + renderLineRange: function(startLine, endLine, opt_side) { + var groups = + this._builder.getGroupsByLineRange(startLine, endLine, opt_side); + groups.forEach(function(group) { + var newElement = this._builder.buildSectionElement(group); + var oldElement = group.element; + + // Transfer comment threads from existing section to new one. + var threads = Polymer.dom(newElement).querySelectorAll( + 'gr-diff-comment-thread'); + threads.forEach(function(threadEl) { + var lineEl = this.getLineElByChild(threadEl, oldElement); + if (!lineEl) { // New comment thread. + return; + } + var side = this.getSideByLineEl(lineEl); + var line = lineEl.getAttribute('data-value'); + var oldThreadEl = + this.getCommentThreadByLine(line, side, oldElement); + threadEl.parentNode.replaceChild(oldThreadEl, threadEl); + }, this); + + // Replace old group elements with new ones. + group.element.parentNode.replaceChild(newElement, group.element); + group.element = newElement; + }, this); + + this.async(function() { + this.fire('render'); + }, 1); + }, + + getContentByLine: function(lineNumber, opt_side, opt_root) { + var root = Polymer.dom(opt_root || this.diffElement); + var sideSelector = !!opt_side ? ('.' + opt_side) : ''; + return root.querySelector('td.lineNum[data-value="' + lineNumber + + '"]' + sideSelector + ' ~ td.content'); + }, + + getContentByLineEl: function(lineEl) { + var root = Polymer.dom(lineEl.parentElement); + var side = this.getSideByLineEl(lineEl); + var line = lineEl.getAttribute('data-value'); + return this.getContentByLine(line, side, root); + }, + + getLineElByNumber: function(lineNumber, opt_side) { + var sideSelector = !!opt_side ? ('.' + opt_side) : ''; + return this.diffElement.querySelector( + '.lineNum[data-value="' + lineNumber + '"]' + sideSelector); + }, + + getContentsByLineRange: function(startLine, endLine, opt_side) { + var groups = + this._builder.getGroupsByLineRange(startLine, endLine, opt_side); + // In each group, search Element for lines in range. + return groups.reduce((function(acc, group) { + for (var line = startLine; line <= endLine; line++) { + var content = + this.getContentByLine(line, opt_side, group.element); + if (content) { + acc.push(content); + } + } + return acc; + }).bind(this), []); + }, + + getCommentThreadByLine: function(lineNumber, opt_side, opt_root) { + var root = Polymer.dom(opt_root || this.diffElement); + var sideSelector = !!opt_side ? ('.' + opt_side) : '' + var content = this.getContentByLine(lineNumber, opt_side, opt_root); + return content.querySelector('gr-diff-comment-thread'); + }, + getSideByLineEl: function(lineEl) { return lineEl.classList.contains(GrDiffBuilder.Side.RIGHT) ? - GrDiffBuilder.Side.RIGHT : GrDiffBuilder.Side.LEFT; + GrDiffBuilder.Side.RIGHT : GrDiffBuilder.Side.LEFT; }, createCommentThread: function(changeNum, patchNum, path, side, @@ -106,12 +181,12 @@ newGroups.forEach(function(newGroup) { this._builder.emitGroup(newGroup, sectionEl); - }.bind(this)); + }, this); sectionEl.parentNode.removeChild(sectionEl); this.async(function() { this.fire('render'); - }.bind(this), 1); + }, 1); }, _getDiffBuilder: function(diff, comments, prefs) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js index f03687c..8acf936 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -65,8 +65,7 @@ } }; - GrDiffBuilder.prototype.buildSectionElement = function( - group, opt_beforeSection) { + GrDiffBuilder.prototype.buildSectionElement = function(group) { throw Error('Subclasses must implement buildGroupElement'); }; @@ -88,28 +87,53 @@ } }; - GrDiffBuilder.prototype.getSectionsByLineRange = function( + GrDiffBuilder.prototype.getGroupsByLineRange = function( startLine, endLine, opt_side) { - var sections = []; + var groups = []; for (var i = 0; i < this._groups.length; i++) { var group = this._groups[i]; if (group.lines.length === 0) { continue; } - var groupStartLine; - var groupEndLine; - if (opt_side === GrDiffBuilder.Side.LEFT) { - groupStartLine = group.lines[0].beforeNumber; - groupEndLine = group.lines[group.lines.length - 1].beforeNumber; - } else if (opt_side === GrDiffBuilder.Side.RIGHT) { - groupStartLine = group.lines[0].afterNumber; - groupEndLine = group.lines[group.lines.length - 1].afterNumber; + var groupStartLine = 0; + var groupEndLine = 0; + switch (group.type) { + case GrDiffGroup.Type.BOTH: + if (opt_side === GrDiffBuilder.Side.LEFT) { + groupStartLine = group.lines[0].beforeNumber; + groupEndLine = group.lines[group.lines.length - 1].beforeNumber; + } else if (opt_side === GrDiffBuilder.Side.RIGHT) { + groupStartLine = group.lines[0].afterNumber; + groupEndLine = group.lines[group.lines.length - 1].afterNumber; + } + break; + case GrDiffGroup.Type.DELTA: + if (opt_side === GrDiffBuilder.Side.LEFT && group.removes.length) { + groupStartLine = group.removes[0].beforeNumber; + groupEndLine = group.removes[group.removes.length - 1].beforeNumber; + } else if (group.adds.length) { + groupStartLine = group.adds[0].afterNumber; + groupEndLine = group.adds[group.adds.length - 1].afterNumber; + } + break; + } + if (groupStartLine === 0) { // Line was removed or added. + groupStartLine = groupEndLine; + } + if (groupEndLine === 0) { // Line was removed or added. + groupEndLine = groupStartLine; } if (startLine <= groupEndLine && endLine >= groupStartLine) { - sections.push(group.element); + groups.push(group); } } - return sections; + return groups; + }; + + GrDiffBuilder.prototype.getSectionsByLineRange = function( + startLine, endLine, opt_side) { + return this.getGroupsByLineRange(startLine, endLine, opt_side).map( + function(group) { return group.element; }); }; GrDiffBuilder.prototype._processContent = function(content, groups, context) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html index c47e05d..6cef374 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -562,11 +562,16 @@ context: -1 }; content = [ - {ab: []}, - {a: ['all work and no play make andybons a dull boy']}, - {ab: []}, - {b: ['elgoog elgoog elgoog']}, - {ab: []}, + { + a: ['all work and no play make andybons a dull boy'], + b: ['elgoog elgoog elgoog'] + }, + { + ab: [ + 'Non eram nescius, Brute, cum, quae summis ingeniis ', + 'exquisitaque doctrina philosophi Graeco sermone tractavissent', + ] + }, ]; outputEl = document.createElement('out'); builder = @@ -591,12 +596,23 @@ assert.equal(section.innerHTML, prevInnerHTML); }); - test('getSectionsByLineRange', function() { + test('getSectionsByLineRange one line', function() { var section = outputEl.querySelector('stub:nth-of-type(2)'); var sections = builder.getSectionsByLineRange(1, 1, 'left'); assert.equal(sections.length, 1); assert.strictEqual(sections[0], section); }); + + test('getSectionsByLineRange over diff', function() { + var section = [ + outputEl.querySelector('stub:nth-of-type(2)'), + outputEl.querySelector('stub:nth-of-type(3)'), + ]; + var sections = builder.getSectionsByLineRange(1, 2, 'left'); + assert.equal(sections.length, 2); + assert.strictEqual(sections[0], section[0]); + assert.strictEqual(sections[1], section[1]); + }); }); }); </script>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js index 7304ac7..8cc784f 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
@@ -51,16 +51,19 @@ }.bind(this)); }, - addDraft: function(opt_lineNum) { + addOrEditDraft: function(opt_lineNum) { var lastComment = this.comments[this.comments.length - 1]; if (lastComment && lastComment.__draft) { var commentEl = this._commentElWithDraftID( lastComment.id || lastComment.__draftID); commentEl.editing = true; - return; + } else { + this.addDraft(opt_lineNum); } + }, - var draft = this._newDraft(opt_lineNum); + addDraft: function(opt_lineNum, opt_range) { + var draft = this._newDraft(opt_lineNum, opt_range); draft.__editing = true; this.push('comments', draft); }, @@ -154,7 +157,7 @@ return d; }, - _newDraft: function(opt_lineNum) { + _newDraft: function(opt_lineNum, opt_range) { var d = { __draft: true, __draftID: Math.random().toString(36), @@ -165,20 +168,28 @@ if (opt_lineNum) { d.line = opt_lineNum; } + if (opt_range) { + d.range = { + start_line: opt_range.startLine, + start_character: opt_range.startChar, + end_line: opt_range.endLine, + end_character: opt_range.endChar, + }; + } return d; }, _handleCommentDiscard: function(e) { var diffCommentEl = Polymer.dom(e).rootTarget; - var idx = this._indexOf(diffCommentEl.comment, this.comments); + var comment = diffCommentEl.comment; + var idx = this._indexOf(comment, this.comments); if (idx == -1) { throw Error('Cannot find comment ' + JSON.stringify(diffCommentEl.comment)); } this.splice('comments', idx, 1); if (this.comments.length == 0) { - this.fire('thread-discard'); - return; + this.fire('thread-discard', {lastComment: comment}); } },
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html index 6e7a68a..c3b6233 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
@@ -114,7 +114,10 @@ display: block; } </style> - <div class="container" id="container"> + <div id="container" + class="container" + on-mouseenter="_handleMouseEnter" + on-mouseleave="_handleMouseLeave"> <div class="header" id="header"> <div class="headerLeft"> <span class="authorName">[[comment.author.name]]</span>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js index 7a15754..3baf200 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
@@ -50,6 +50,14 @@ * @event comment-update */ + /** + * @event comment-mouse-over + */ + + /** + * @event comment-mouse-out + */ + properties: { changeNum: String, comment: { @@ -90,7 +98,7 @@ ], detached: function() { - this.flushDebouncer('fire-update'); + this.cancelDebouncer('fire-update'); }, save: function() { @@ -118,7 +126,7 @@ } this.comment = comment; this.editing = false; - this.fire('comment-save', {comment: this.comment}); + this._fireSave(); return obj; }.bind(this)); }.bind(this)).catch(function(err) { @@ -129,11 +137,29 @@ _commentChanged: function(comment) { this.editing = !!comment.__editing; + if (this.editing) { // It's a new draft/reply, notify. + this._fireUpdate(); + } + }, + + _getEventPayload: function(opt_mixin) { + var payload = { + comment: this.comment, + patchNum: this.patchNum, + }; + for (var k in opt_mixin) { + payload[k] = opt_mixin[k]; + } + return payload; + }, + + _fireSave: function() { + this.fire('comment-save', this._getEventPayload()); }, _fireUpdate: function() { this.debounce('fire-update', function() { - this.fire('comment-update', {comment: this.comment}); + this.fire('comment-update', this._getEventPayload()); }, UPDATE_DEBOUNCE_INTERVAL); }, @@ -141,7 +167,7 @@ this.$.container.classList.toggle('draft', draft); }, - _editingChanged: function(editing) { + _editingChanged: function(editing, previousValue) { this.$.container.classList.toggle('editing', editing); if (editing) { var textarea = this.$.editTextarea.textarea; @@ -158,7 +184,10 @@ if (this.comment) { this.comment.__editing = this.editing; } - this._fireUpdate(); + if (editing != !!previousValue) { + // To prevent event firing on comment creation. + this._fireUpdate(); + } }, _computeLinkToComment: function(comment) { @@ -216,18 +245,18 @@ _handleReply: function(e) { this._preventDefaultAndBlur(e); - this.fire('reply', {comment: this.comment}, {bubbles: false}); + this.fire('reply', this._getEventPayload(), {bubbles: false}); }, _handleQuote: function(e) { this._preventDefaultAndBlur(e); - this.fire('reply', {comment: this.comment, quote: true}, - {bubbles: false}); + this.fire( + 'reply', this._getEventPayload({quote: true}), {bubbles: false}); }, _handleDone: function(e) { this._preventDefaultAndBlur(e); - this.fire('done', {comment: this.comment}, {bubbles: false}); + this.fire('done', this._getEventPayload(), {bubbles: false}); }, _handleEdit: function(e) { @@ -244,13 +273,17 @@ _handleCancel: function(e) { this._preventDefaultAndBlur(e); if (this.comment.message == null || this.comment.message.length == 0) { - this.fire('comment-discard', {comment: this.comment}); + this._fireDiscard(); return; } this._messageText = this.comment.message; this.editing = false; }, + _fireDiscard: function() { + this.fire('comment-discard', this._getEventPayload()); + }, + _handleDiscard: function(e) { this._preventDefaultAndBlur(e); if (!this.comment.__draft) { @@ -260,7 +293,7 @@ this.disabled = true; if (!this.comment.id) { this.disabled = false; - this.fire('comment-discard', {comment: this.comment}); + this._fireDiscard(); return; } @@ -269,7 +302,7 @@ this.disabled = false; if (!response.ok) { return response; } - this.fire('comment-discard', {comment: this.comment}); + this._fireDiscard(); }.bind(this)).catch(function(err) { this.disabled = false; throw err; @@ -308,5 +341,13 @@ this.set('comment.message', draft.message); } }, + + _handleMouseEnter: function(e) { + this.fire('comment-mouse-over', this._getEventPayload()); + }, + + _handleMouseLeave: function(e) { + this.fire('comment-mouse-out', this._getEventPayload()); + }, }); })();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html index d2262ab..de5eb48 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
@@ -225,6 +225,7 @@ line: 5, path: '/path/to/file', }, + patchNum: 1, }, ]); MockInteractions.tap(element.$$('.save')); @@ -247,6 +248,7 @@ path: '/path/to/file', updated: '2015-12-08 21:52:36.177000000', }, + patchNum: 1, }, ]); assert.isFalse(element.disabled, @@ -255,8 +257,8 @@ assert.isFalse(element.editing); }).then(function() { MockInteractions.tap(element.$$('.edit')); - element._messageText = 'You’ll be delivering a package to Chapek 9, a ' + - 'world where humans are killed on sight.'; + element._messageText = 'You’ll be delivering a package to Chapek 9, ' + + 'a world where humans are killed on sight.'; MockInteractions.tap(element.$$('.save')); assert.isTrue(element.disabled, 'Element should be disabled when updating draft.');
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js index 6eefe07..e3f0c0c 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
@@ -137,7 +137,6 @@ }, _getDiffPreferences: function() { - this._localPrefs = this.$.storage.getPreferences(); return this.$.restAPI.getDiffPreferences(); }, @@ -190,10 +189,12 @@ this.$.cursor.moveUp(); break; case 67: // 'c' - e.preventDefault(); - var line = this.$.cursor.getTargetLineElement(); - if (line) { - this.$.diff.addDraftAtLine(line); + if (!this.$.diff.isRangeSelected()) { + e.preventDefault(); + var line = this.$.cursor.getTargetLineElement(); + if (line) { + this.$.diff.addDraftAtLine(line); + } } break; case 219: // '[' @@ -288,6 +289,7 @@ var promises = []; + this._localPrefs = this.$.storage.getPreferences(); promises.push(this._getDiffPreferences().then(function(prefs) { this._prefs = prefs; }.bind(this)));
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js index 8c9b20d..88810c1 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -66,6 +66,7 @@ 'comment-discard': '_handleCommentDiscard', 'comment-update': '_handleCommentUpdate', 'comment-save': '_handleCommentSave', + 'create-comment': '_handleCreateComment', }, attached: function() { @@ -195,33 +196,59 @@ } }, - _addDraft: function(lineEl, opt_lineNum) { - var threadEl; + _handleCreateComment: function(e) { + var range = e.detail.range; + var diffSide = e.detail.side; + var line = range.endLine; + var lineEl = this.$.diffBuilder.getLineElByNumber(line, diffSide); + var contentEl = this.$.diffBuilder.getContentByLineEl(lineEl); + var patchNum = this._getPatchNumByLineAndContent(lineEl, contentEl); + var side = this._getSideByLineAndContent(lineEl, contentEl); + var threadEl = this._getOrCreateThreadAtLine(contentEl, patchNum, side); - // Does a thread already exist at this line? - var contentEl = lineEl.nextSibling; - while (contentEl && !contentEl.classList.contains('content')) { - contentEl = contentEl.nextSibling; - } - if (contentEl.childNodes.length > 0 && - contentEl.lastChild.nodeName === 'GR-DIFF-COMMENT-THREAD') { - threadEl = contentEl.lastChild; - } else { - var patchNum = this.patchRange.patchNum; - var side = 'REVISION'; - if (lineEl.classList.contains(DiffSide.LEFT) || - contentEl.classList.contains('remove')) { - if (this.patchRange.basePatchNum === 'PARENT') { - side = 'PARENT'; - } else { - patchNum = this.patchRange.basePatchNum; - } - } + threadEl.addDraft(line, range); + }, + + _addDraft: function(lineEl, opt_lineNum) { + var line = opt_lineNum || lineEl.getAttribute('data-value'); + var contentEl = this.$.diffBuilder.getContentByLineEl(lineEl); + var patchNum = this._getPatchNumByLineAndContent(lineEl, contentEl); + var side = this._getSideByLineAndContent(lineEl, contentEl); + var threadEl = this._getOrCreateThreadAtLine(contentEl, patchNum, side); + + threadEl.addOrEditDraft(opt_lineNum); + }, + + _getOrCreateThreadAtLine: function(contentEl, patchNum, side) { + var threadEl = contentEl.querySelector('gr-diff-comment-thread'); + + if (!threadEl) { threadEl = this.$.diffBuilder.createCommentThread( this.changeNum, patchNum, this.path, side, this.projectConfig); contentEl.appendChild(threadEl); } - threadEl.addDraft(opt_lineNum); + + return threadEl; + }, + + _getPatchNumByLineAndContent: function(lineEl, contentEl) { + var patchNum = this.patchRange.patchNum; + if ((lineEl.classList.contains(DiffSide.LEFT) || + contentEl.classList.contains('remove')) && + this.patchRange.basePatchNum !== 'PARENT') { + patchNum = this.patchRange.basePatchNum; + } + return patchNum; + }, + + _getSideByLineAndContent: function(lineEl, contentEl) { + var side = 'REVISION'; + if ((lineEl.classList.contains(DiffSide.LEFT) || + contentEl.classList.contains('remove')) && + this.patchRange.basePatchNum === 'PARENT') { + side = 'PARENT'; + } + return side; }, _handleThreadDiscard: function(e) { @@ -231,7 +258,7 @@ _handleCommentDiscard: function(e) { var comment = e.detail.comment; - this._removeComment(comment, e.target.patchNum); + this._removeComment(comment, e.detail.patchNum); }, _removeComment: function(comment, opt_patchNum) { @@ -250,14 +277,14 @@ _handleCommentSave: function(e) { var comment = e.detail.comment; - var side = this._findCommentSide(comment, e.target.patchNum); + var side = this._findCommentSide(comment, e.detail.patchNum); var idx = this._findDraftIndex(comment, side); this.set(['_comments', side, idx], comment); }, _handleCommentUpdate: function(e) { var comment = e.detail.comment; - var side = this._findCommentSide(comment, e.target.patchNum); + var side = this._findCommentSide(comment, e.detail.patchNum); var idx = this._findCommentIndex(comment, side); if (idx === -1) { idx = this._findDraftIndex(comment, side);
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.html b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.html index f9174ac..6f95789 100644 --- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.html +++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.html
@@ -26,6 +26,7 @@ background-color: #fff; border: 1px solid #000; border-radius: .5em; + cursor: pointer; padding: .3em; position: absolute; }
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js index 0a85096..64e2f4a 100644 --- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js +++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js
@@ -17,6 +17,12 @@ Polymer({ is: 'gr-selection-action-box', + /** + * Fired when the comment creation action was taken (hotkey, click). + * + * @event create-comment + */ + properties: { keyEventTarget: { type: Object, @@ -41,6 +47,10 @@ Gerrit.KeyboardShortcutBehavior, ], + listeners: { + 'tap': '_handleTap', + }, + placeAbove: function(el) { var rect = this._getTargetBoundingRect(el); var boxRect = this.getBoundingClientRect(); @@ -53,7 +63,7 @@ _getTargetBoundingRect: function(el) { var rect; - if (!(el instanceof Element)) { + if (el instanceof Text) { var range = document.createRange(); range.selectNode(el); rect = range.getBoundingClientRect(); @@ -68,8 +78,16 @@ if (this.shouldSupressKeyboardShortcut(e)) { return; } if (e.keyCode === 67) { // 'c' e.preventDefault(); - this.fire('create-comment', {side: this.side, range: this.range}); + this._fireCreateComment(); }; }, + + _handleTap: function() { + this._fireCreateComment(); + }, + + _fireCreateComment: function() { + this.fire('create-comment', {side: this.side, range: this.range}); + }, }); })();