@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Fix left/right detection of inline comments in unified view
Summary:
Ref T2009. Currently, the code figures out if a comment is on the left or right by looking at the `<th />` preceeding the enclosing `<td />`.
This gets the right result in 2-up, but in 1-up rows are always `<th />`, `<th />`, `<td />`, so it always detects every inline as being in the new file.
Because "old" and "new" cells aren't inherently distingushable in the 1up view, we can't use a DOM test for this at all. Instead, just track this state explicitly.
Test Plan:
- Made left/right comments in 1up view and 2up view.
- Viewed them in 1up and 2up views.
- Hovered in 1up and 2up views.
- Diff-of-diff'd and reviewed old/new comments, then made some more.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12011
+27
-23
+11
-11
resources/celerity/map.php
+11
-11
resources/celerity/map.php
···
11
11
'core.pkg.js' => '5a1c336d',
12
12
'darkconsole.pkg.js' => '8ab24e01',
13
13
'differential.pkg.css' => '5f5d3a4c',
14
-
'differential.pkg.js' => '58dae818',
14
+
'differential.pkg.js' => '6b52883b',
15
15
'diffusion.pkg.css' => '591664fa',
16
16
'diffusion.pkg.js' => 'bfc0737b',
17
17
'maniphest.pkg.css' => '68d4dd3d',
···
367
367
'rsrc/js/application/differential/behavior-comment-preview.js' => '6932def3',
368
368
'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1',
369
369
'rsrc/js/application/differential/behavior-dropdown-menus.js' => '2035b9cb',
370
-
'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '334267b3',
370
+
'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '1360cac8',
371
371
'rsrc/js/application/differential/behavior-keyboard-nav.js' => '2c426492',
372
372
'rsrc/js/application/differential/behavior-populate.js' => '8694b1df',
373
373
'rsrc/js/application/differential/behavior-show-field-details.js' => 'bba9eedf',
···
569
569
'javelin-behavior-differential-comment-jump' => '4fdb476d',
570
570
'javelin-behavior-differential-diff-radios' => 'e1ff79b1',
571
571
'javelin-behavior-differential-dropdown-menus' => '2035b9cb',
572
-
'javelin-behavior-differential-edit-inline-comments' => '334267b3',
572
+
'javelin-behavior-differential-edit-inline-comments' => '1360cac8',
573
573
'javelin-behavior-differential-feedback-preview' => '6932def3',
574
574
'javelin-behavior-differential-keyboard-navigation' => '2c426492',
575
575
'javelin-behavior-differential-populate' => '8694b1df',
···
889
889
'javelin-install',
890
890
'javelin-util',
891
891
),
892
+
'1360cac8' => array(
893
+
'javelin-behavior',
894
+
'javelin-stratcom',
895
+
'javelin-dom',
896
+
'javelin-util',
897
+
'javelin-vector',
898
+
'differential-inline-comment-editor',
899
+
),
892
900
'13c739ea' => array(
893
901
'javelin-behavior',
894
902
'javelin-stratcom',
···
1026
1034
),
1027
1035
'331b1611' => array(
1028
1036
'javelin-install',
1029
-
),
1030
-
'334267b3' => array(
1031
-
'javelin-behavior',
1032
-
'javelin-stratcom',
1033
-
'javelin-dom',
1034
-
'javelin-util',
1035
-
'javelin-vector',
1036
-
'differential-inline-comment-editor',
1037
1037
),
1038
1038
'3ab51e2c' => array(
1039
1039
'javelin-behavior',
+1
src/infrastructure/diff/PhabricatorInlineCommentController.php
+1
src/infrastructure/diff/PhabricatorInlineCommentController.php
+1
src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
+1
src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php
+12
-1
src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
+12
-1
src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php
···
10
10
private $number;
11
11
private $length;
12
12
private $renderer;
13
+
private $isNewFile;
14
+
15
+
public function setIsNewFile($is_new_file) {
16
+
$this->isNewFile = $is_new_file;
17
+
return $this;
18
+
}
19
+
20
+
public function getIsNewFile() {
21
+
return $this->isNewFile;
22
+
}
13
23
14
24
public function getIsOnRight() {
15
25
return $this->onRight;
···
160
170
'class' => 'differential-inline-comment-edit',
161
171
'sigil' => 'differential-inline-comment',
162
172
'meta' => array(
163
-
'on_right' => $this->onRight,
173
+
'on_right' => $this->getIsOnRight(),
174
+
'isNewFile' => (bool)$this->getIsNewFile(),
164
175
'number' => $this->number,
165
176
'length' => $this->length,
166
177
),
+2
-11
webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js
+2
-11
webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js
···
269
269
} else {
270
270
root = e.getNode('differential-changeset');
271
271
if (root) {
272
-
273
272
var data = e.getNodeData('differential-inline-comment');
274
273
var change = e.getNodeData('differential-changeset');
275
274
276
-
var id_part = data.on_right ? change.right : change.left;
277
-
278
-
// NOTE: We can't just look for 'tag:td' because the event might be
279
-
// inside a table which is inside an inline comment.
280
-
var comment = e.getNode('differential-inline-comment');
281
-
var td = JX.DOM.findAbove(comment, 'td');
282
-
var th = td.previousSibling;
283
-
284
-
// TODO: For one-up views, this is incorrect!
285
-
var new_part = isNewFile(th) ? 'N' : 'O';
275
+
var id_part = data.on_right ? change.right : change.left;
276
+
var new_part = data.isNewFile ? 'N' : 'O';
286
277
var prefix = 'C' + id_part + new_part + 'L';
287
278
288
279
origin = JX.$(prefix + data.number);