Skip to content

Commit 41379f3

Browse files
author
epriestley
committed
Move inline replies to new code and remove DifferentialInlineEditor
Summary: Ref T12616. This moves "reply" to the new stuff and deletes DifferentialInlineEditor, which no longer does anything. (This breaks some keyboard shortcuts, but I'll rebase D17859 shortly.) Test Plan: Replied to inlines; things seemed to work properly. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12616 Differential Revision: https://secure.phabricator.com/D17894
1 parent 58dded5 commit 41379f3

File tree

8 files changed

+104
-522
lines changed

8 files changed

+104
-522
lines changed

‎resources/celerity/map.php

Lines changed: 32 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
'core.pkg.js' => '2ff7879f',
1414
'darkconsole.pkg.js' => '1f9a31bc',
1515
'differential.pkg.css' => '58712637',
16-
'differential.pkg.js' => '3a7c5866',
16+
'differential.pkg.js' => '6375358e',
1717
'diffusion.pkg.css' => 'b93d9b8c',
1818
'diffusion.pkg.js' => '84c8f8fd',
1919
'favicon.ico' => '30672e08',
@@ -390,15 +390,14 @@
390390
'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '408bf173',
391391
'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375',
392392
'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
393-
'rsrc/js/application/diff/DiffChangeset.js' => '454cfe59',
394-
'rsrc/js/application/diff/DiffChangesetList.js' => '50bc5b50',
395-
'rsrc/js/application/diff/DiffInline.js' => '38a957be',
393+
'rsrc/js/application/diff/DiffChangeset.js' => '4c9c47ad',
394+
'rsrc/js/application/diff/DiffChangesetList.js' => '589a30aa',
395+
'rsrc/js/application/diff/DiffInline.js' => '98c12b2f',
396396
'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
397-
'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => '9ed8d2b6',
398397
'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d',
399398
'rsrc/js/application/differential/behavior-comment-preview.js' => 'b064af76',
400399
'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1',
401-
'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '995c805a',
400+
'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '89d11432',
402401
'rsrc/js/application/differential/behavior-keyboard-nav.js' => '92904457',
403402
'rsrc/js/application/differential/behavior-populate.js' => '5e41c819',
404403
'rsrc/js/application/differential/behavior-toggle-files.js' => 'ca3f91eb',
@@ -572,7 +571,6 @@
572571
'd3' => 'a11a5ff2',
573572
'differential-changeset-view-css' => '41af6d25',
574573
'differential-core-view-css' => '5b7b8ff4',
575-
'differential-inline-comment-editor' => '9ed8d2b6',
576574
'differential-revision-add-comment-css' => 'c47f8c40',
577575
'differential-revision-comment-css' => '14b8565a',
578576
'differential-revision-history-css' => '0e8eb855',
@@ -624,7 +622,7 @@
624622
'javelin-behavior-diff-preview-link' => '051c7832',
625623
'javelin-behavior-differential-comment-jump' => '4fdb476d',
626624
'javelin-behavior-differential-diff-radios' => 'e1ff79b1',
627-
'javelin-behavior-differential-edit-inline-comments' => '995c805a',
625+
'javelin-behavior-differential-edit-inline-comments' => '89d11432',
628626
'javelin-behavior-differential-feedback-preview' => 'b064af76',
629627
'javelin-behavior-differential-keyboard-navigation' => '92904457',
630628
'javelin-behavior-differential-populate' => '5e41c819',
@@ -785,9 +783,9 @@
785783
'phabricator-darklog' => 'c8e1ffe3',
786784
'phabricator-darkmessage' => 'c48cccdd',
787785
'phabricator-dashboard-css' => 'fe5b1869',
788-
'phabricator-diff-changeset' => '454cfe59',
789-
'phabricator-diff-changeset-list' => '50bc5b50',
790-
'phabricator-diff-inline' => '38a957be',
786+
'phabricator-diff-changeset' => '4c9c47ad',
787+
'phabricator-diff-changeset-list' => '589a30aa',
788+
'phabricator-diff-inline' => '98c12b2f',
791789
'phabricator-drag-and-drop-file-upload' => '58dea2fa',
792790
'phabricator-draggable-list' => 'bea6e7f4',
793791
'phabricator-fatal-config-template-css' => '8f18fa41',
@@ -1132,9 +1130,6 @@
11321130
'javelin-dom',
11331131
'javelin-workflow',
11341132
),
1135-
'38a957be' => array(
1136-
'javelin-dom',
1137-
),
11381133
'3ab51e2c' => array(
11391134
'javelin-behavior',
11401135
'javelin-behavior-device',
@@ -1214,17 +1209,6 @@
12141209
'javelin-behavior',
12151210
'javelin-dom',
12161211
),
1217-
'454cfe59' => array(
1218-
'javelin-dom',
1219-
'javelin-util',
1220-
'javelin-stratcom',
1221-
'javelin-install',
1222-
'javelin-workflow',
1223-
'javelin-router',
1224-
'javelin-behavior-device',
1225-
'javelin-vector',
1226-
'phabricator-diff-inline',
1227-
),
12281212
'469c0d9e' => array(
12291213
'javelin-behavior',
12301214
'javelin-dom',
@@ -1286,6 +1270,17 @@
12861270
'javelin-uri',
12871271
'phabricator-notification',
12881272
),
1273+
'4c9c47ad' => array(
1274+
'javelin-dom',
1275+
'javelin-util',
1276+
'javelin-stratcom',
1277+
'javelin-install',
1278+
'javelin-workflow',
1279+
'javelin-router',
1280+
'javelin-behavior-device',
1281+
'javelin-vector',
1282+
'phabricator-diff-inline',
1283+
),
12891284
'4d863052' => array(
12901285
'javelin-dom',
12911286
'javelin-util',
@@ -1312,9 +1307,6 @@
13121307
'javelin-typeahead-source',
13131308
'javelin-util',
13141309
),
1315-
'50bc5b50' => array(
1316-
'javelin-install',
1317-
),
13181310
'519705ea' => array(
13191311
'javelin-install',
13201312
'javelin-dom',
@@ -1356,6 +1348,9 @@
13561348
'javelin-vector',
13571349
'javelin-dom',
13581350
),
1351+
'589a30aa' => array(
1352+
'javelin-install',
1353+
),
13591354
'58dea2fa' => array(
13601355
'javelin-install',
13611356
'javelin-util',
@@ -1596,6 +1591,13 @@
15961591
'phabricator-draggable-list',
15971592
'javelin-workboard-column',
15981593
),
1594+
'89d11432' => array(
1595+
'javelin-behavior',
1596+
'javelin-stratcom',
1597+
'javelin-dom',
1598+
'javelin-util',
1599+
'javelin-vector',
1600+
),
15991601
'8a41885b' => array(
16001602
'javelin-install',
16011603
'javelin-dom',
@@ -1675,13 +1677,8 @@
16751677
'javelin-dom',
16761678
'javelin-reactor-dom',
16771679
),
1678-
'995c805a' => array(
1679-
'javelin-behavior',
1680-
'javelin-stratcom',
1680+
'98c12b2f' => array(
16811681
'javelin-dom',
1682-
'javelin-util',
1683-
'javelin-vector',
1684-
'differential-inline-comment-editor',
16851682
),
16861683
'9a6dd75c' => array(
16871684
'javelin-behavior',
@@ -1706,14 +1703,6 @@
17061703
'9d9685d6' => array(
17071704
'phui-oi-list-view-css',
17081705
),
1709-
'9ed8d2b6' => array(
1710-
'javelin-dom',
1711-
'javelin-util',
1712-
'javelin-stratcom',
1713-
'javelin-install',
1714-
'javelin-request',
1715-
'javelin-workflow',
1716-
),
17171706
'9f36c42d' => array(
17181707
'javelin-behavior',
17191708
'javelin-stratcom',
@@ -2456,10 +2445,10 @@
24562445
'javelin-behavior-phabricator-object-selector',
24572446
'javelin-behavior-repository-crossreference',
24582447
'javelin-behavior-load-blame',
2459-
'differential-inline-comment-editor',
24602448
'javelin-behavior-differential-toggle-files',
24612449
'javelin-behavior-differential-user-select',
24622450
'javelin-behavior-aphront-more',
2451+
'phabricator-diff-inline',
24632452
'phabricator-diff-changeset',
24642453
'phabricator-diff-changeset-list',
24652454
),

‎resources/celerity/packages.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -203,10 +203,11 @@
203203
'javelin-behavior-repository-crossreference',
204204
'javelin-behavior-load-blame',
205205

206-
'differential-inline-comment-editor',
207206
'javelin-behavior-differential-toggle-files',
208207
'javelin-behavior-differential-user-select',
209208
'javelin-behavior-aphront-more',
209+
210+
'phabricator-diff-inline',
210211
'phabricator-diff-changeset',
211212
'phabricator-diff-changeset-list',
212213
),

‎src/infrastructure/diff/PhabricatorInlineCommentController.php

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -94,19 +94,6 @@ public function processRequest() {
9494

9595
$op = $this->getOperation();
9696
switch ($op) {
97-
case 'busy':
98-
if ($request->isFormPost()) {
99-
return new AphrontAjaxResponse();
100-
}
101-
102-
return $this->newDialog()
103-
->setTitle(pht('Already Editing'))
104-
->appendParagraph(
105-
pht(
106-
'You are already editing an inline comment. Finish editing '.
107-
'your current comment before adding new comments.'))
108-
->addCancelButton('/')
109-
->addSubmitButton(pht('Jump to Inline'));
11097
case 'hide':
11198
case 'show':
11299
if (!$request->validateCSRF()) {

‎webroot/rsrc/js/application/diff/DiffChangeset.js

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,18 @@ JX.install('DiffChangeset', {
433433
return inline;
434434
},
435435

436+
newInlineReply: function(original) {
437+
var inline = new JX.DiffInline()
438+
.setChangeset(this)
439+
.bindToReply(original);
440+
441+
this._inlines.push(inline);
442+
443+
inline.create();
444+
445+
return inline;
446+
},
447+
436448
getInlineByID: function(id) {
437449
// TODO: Currently, this will only find inlines which the user has
438450
// already interacted with! Inlines are built lazily as events arrive.

‎webroot/rsrc/js/application/diff/DiffChangesetList.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,12 @@ JX.install('DiffChangesetList', {
4141
'click',
4242
['differential-inline-comment', 'differential-inline-delete'],
4343
ondelete);
44+
45+
var onreply = JX.bind(this, this._ifawake, this._onaction, 'reply');
46+
JX.Stratcom.listen(
47+
'click',
48+
['differential-inline-comment', 'differential-inline-reply'],
49+
onreply);
4450
},
4551

4652
properties: {
@@ -410,6 +416,9 @@ JX.install('DiffChangesetList', {
410416
case 'delete':
411417
inline.delete(is_ref);
412418
break;
419+
case 'reply':
420+
inline.reply();
421+
break;
413422
}
414423
},
415424

‎webroot/rsrc/js/application/diff/DiffInline.js

Lines changed: 49 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ JX.install('DiffInline', {
2424
_displaySide: null,
2525
_isNewFile: null,
2626
_undoRow: null,
27+
_replyToCommentPHID: null,
2728

2829
_isDeleted: false,
2930
_isInvisible: false,
@@ -66,32 +67,50 @@ JX.install('DiffInline', {
6667
},
6768

6869
bindToRange: function(data) {
69-
this._id = null;
70-
this._phid = null;
71-
72-
this._hidden = false;
73-
7470
this._displaySide = data.displaySide;
7571
this._number = data.number;
7672
this._length = data.length;
7773
this._isNewFile = data.isNewFile;
7874
this._changesetID = data.changesetID;
7975

76+
// Insert the comment after any other comments which already appear on
77+
// the same row.
78+
var parent_row = JX.DOM.findAbove(data.target, 'tr');
79+
var target_row = parent_row.nextSibling;
80+
while (target_row && JX.Stratcom.hasSigil(target_row, 'inline-row')) {
81+
target_row = target_row.nextSibling;
82+
}
83+
8084
var row = this._newRow();
81-
JX.Stratcom.getData(row).inline = this;
82-
this._row = row;
85+
parent_row.parentNode.insertBefore(row, target_row);
8386

8487
this.setInvisible(true);
8588

86-
// Insert the comment after any other comments which already appear on
87-
// the same row.
88-
var parent_row = JX.DOM.findAbove(data.target, 'tr');
89+
return this;
90+
},
91+
92+
bindToReply: function(inline) {
93+
this._displaySide = inline._displaySide;
94+
this._number = inline._number;
95+
this._length = inline._length;
96+
this._isNewFile = inline._isNewFile;
97+
this._changesetID = inline._changesetID;
98+
99+
this._replyToCommentPHID = inline._phid;
100+
101+
// TODO: This should insert correctly into the thread, not just at the
102+
// bottom.
103+
var parent_row = inline._row;
89104
var target_row = parent_row.nextSibling;
90105
while (target_row && JX.Stratcom.hasSigil(target_row, 'inline-row')) {
91106
target_row = target_row.nextSibling;
92107
}
108+
109+
var row = this._newRow();
93110
parent_row.parentNode.insertBefore(row, target_row);
94111

112+
this.setInvisible(true);
113+
95114
return this;
96115
},
97116

@@ -100,7 +119,16 @@ JX.install('DiffInline', {
100119
sigil: 'inline-row'
101120
};
102121

103-
return JX.$N('tr', attributes);
122+
var row = JX.$N('tr', attributes);
123+
124+
JX.Stratcom.getData(row).inline = this;
125+
this._row = row;
126+
127+
this._id = null;
128+
this._phid = null;
129+
this._hidden = false;
130+
131+
return row;
104132
},
105133

106134
setHidden: function(hidden) {
@@ -172,6 +200,11 @@ JX.install('DiffInline', {
172200
.send();
173201
},
174202

203+
reply: function() {
204+
var changeset = this.getChangeset();
205+
return changeset.newInlineReply(this);
206+
},
207+
175208
edit: function() {
176209
var uri = this._getInlineURI();
177210
var handler = JX.bind(this, this._oneditresponse);
@@ -238,6 +271,10 @@ JX.install('DiffInline', {
238271
return this._changesetID;
239272
},
240273

274+
getReplyToCommentPHID: function() {
275+
return this._replyToCommentPHID;
276+
},
277+
241278
setDeleted: function(deleted) {
242279
this._isDeleted = deleted;
243280
this._redraw();
@@ -266,7 +303,7 @@ JX.install('DiffInline', {
266303
length: this.getLineLength(),
267304
is_new: this.isNewFile(),
268305
changesetID: this.getChangesetID(),
269-
replyToCommentPHID: ''
306+
replyToCommentPHID: this.getReplyToCommentPHID() || '',
270307
};
271308
},
272309

0 commit comments

Comments
 (0)