Skip to content

Commit d2d7e7f

Browse files
author
epriestley
committed
Clean up Diffusion behaviors for inline edit suggestions
Summary: Ref T13513. For now, I'm not supporting inline edit suggestions in Diffusion, although it's likely not difficult to do so in the future. Clean up some of the code so that plain ol' inlines work the same way they did before. Test Plan: - Created, edited, reloaded, submitted inlines in Diffusion: familiar old behavior. - Created, edited, reloaded, submitted inlines with suggestions in Differential: familiar new behavior. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21278
1 parent 10f2413 commit d2d7e7f

File tree

5 files changed

+91
-73
lines changed

5 files changed

+91
-73
lines changed

‎resources/celerity/map.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
'core.pkg.js' => '845355f4',
1414
'dark-console.pkg.js' => '187792c2',
1515
'differential.pkg.css' => '5c459f92',
16-
'differential.pkg.js' => '256a327a',
16+
'differential.pkg.js' => '24616785',
1717
'diffusion.pkg.css' => '42c75c37',
1818
'diffusion.pkg.js' => 'a98c0bf7',
1919
'maniphest.pkg.css' => '35995d6d',
@@ -381,7 +381,7 @@
381381
'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
382382
'rsrc/js/application/diff/DiffChangeset.js' => '6e5e03d2',
383383
'rsrc/js/application/diff/DiffChangesetList.js' => 'b51ba93a',
384-
'rsrc/js/application/diff/DiffInline.js' => '829b88bf',
384+
'rsrc/js/application/diff/DiffInline.js' => '008b6a15',
385385
'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
386386
'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
387387
'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -776,7 +776,7 @@
776776
'phabricator-dashboard-css' => '5a205b9d',
777777
'phabricator-diff-changeset' => '6e5e03d2',
778778
'phabricator-diff-changeset-list' => 'b51ba93a',
779-
'phabricator-diff-inline' => '829b88bf',
779+
'phabricator-diff-inline' => '008b6a15',
780780
'phabricator-diff-path-view' => '8207abf9',
781781
'phabricator-diff-tree-view' => '5d83623b',
782782
'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -917,6 +917,9 @@
917917
'unhandled-exception-css' => '9ecfc00d',
918918
),
919919
'requires' => array(
920+
'008b6a15' => array(
921+
'javelin-dom',
922+
),
920923
'0116d3e8' => array(
921924
'javelin-behavior',
922925
'javelin-dom',
@@ -1639,9 +1642,6 @@
16391642
'8207abf9' => array(
16401643
'javelin-dom',
16411644
),
1642-
'829b88bf' => array(
1643-
'javelin-dom',
1644-
),
16451645
83754533 => array(
16461646
'javelin-install',
16471647
'javelin-util',

‎src/applications/differential/query/DifferentialDiffInlineCommentQuery.php‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,4 +67,78 @@ protected function loadHiddenCommentIDs(
6767
return $id_map;
6868
}
6969

70+
protected function newInlineContextMap(array $inlines) {
71+
$viewer = $this->getViewer();
72+
73+
$map = array();
74+
75+
foreach ($inlines as $key => $inline) {
76+
$changeset = id(new DifferentialChangesetQuery())
77+
->setViewer($viewer)
78+
->withIDs(array($inline->getChangesetID()))
79+
->needHunks(true)
80+
->executeOne();
81+
if (!$changeset) {
82+
continue;
83+
}
84+
85+
$hunks = $changeset->getHunks();
86+
87+
$is_simple =
88+
(count($hunks) === 1) &&
89+
((int)head($hunks)->getOldOffset() <= 1) &&
90+
((int)head($hunks)->getNewOffset() <= 1);
91+
92+
if (!$is_simple) {
93+
continue;
94+
}
95+
96+
if ($inline->getIsNewFile()) {
97+
$vector = $changeset->getNewStatePathVector();
98+
$filename = last($vector);
99+
$corpus = $changeset->makeNewFile();
100+
} else {
101+
$vector = $changeset->getOldStatePathVector();
102+
$filename = last($vector);
103+
$corpus = $changeset->makeOldFile();
104+
}
105+
106+
$corpus = phutil_split_lines($corpus);
107+
108+
// Adjust the line number into a 0-based offset.
109+
$offset = $inline->getLineNumber();
110+
$offset = $offset - 1;
111+
112+
// Adjust the inclusive range length into a row count.
113+
$length = $inline->getLineLength();
114+
$length = $length + 1;
115+
116+
$head_min = max(0, $offset - 3);
117+
$head_max = $offset;
118+
$head_len = $head_max - $head_min;
119+
120+
if ($head_len) {
121+
$head = array_slice($corpus, $head_min, $head_len, true);
122+
$head = $this->simplifyContext($head, true);
123+
} else {
124+
$head = array();
125+
}
126+
127+
$body = array_slice($corpus, $offset, $length, true);
128+
129+
$tail = array_slice($corpus, $offset + $length, 3, true);
130+
$tail = $this->simplifyContext($tail, false);
131+
132+
$context = id(new PhabricatorDiffInlineCommentContext())
133+
->setFilename($filename)
134+
->setHeadLines($head)
135+
->setBodyLines($body)
136+
->setTailLines($tail);
137+
138+
$map[$key] = $context;
139+
}
140+
141+
return $map;
142+
}
143+
70144
}

‎src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,4 +66,8 @@ protected function loadHiddenCommentIDs(
6666
return array();
6767
}
6868

69+
protected function newInlineContextMap(array $inlines) {
70+
return array();
71+
}
72+
6973
}

‎src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php‎

Lines changed: 7 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ abstract protected function loadHiddenCommentIDs(
1818
$viewer_phid,
1919
array $comments);
2020

21+
abstract protected function newInlineContextMap(array $inlines);
22+
2123
final public function withFixedStates(array $states) {
2224
$this->fixedStates = $states;
2325
return $this;
@@ -265,80 +267,20 @@ protected function didFilterPage(array $inlines) {
265267
$need_context[] = $inline;
266268
}
267269

268-
foreach ($need_context as $inline) {
269-
$changeset = id(new DifferentialChangesetQuery())
270-
->setViewer($viewer)
271-
->withIDs(array($inline->getChangesetID()))
272-
->needHunks(true)
273-
->executeOne();
274-
if (!$changeset) {
275-
$inline->attachInlineContext(null);
276-
continue;
277-
}
278-
279-
$hunks = $changeset->getHunks();
280-
281-
$is_simple =
282-
(count($hunks) === 1) &&
283-
((int)head($hunks)->getOldOffset() <= 1) &&
284-
((int)head($hunks)->getNewOffset() <= 1);
285-
286-
if (!$is_simple) {
287-
$inline->attachInlineContext(null);
288-
continue;
289-
}
290-
291-
if ($inline->getIsNewFile()) {
292-
$vector = $changeset->getNewStatePathVector();
293-
$filename = last($vector);
294-
$corpus = $changeset->makeNewFile();
295-
} else {
296-
$vector = $changeset->getOldStatePathVector();
297-
$filename = last($vector);
298-
$corpus = $changeset->makeOldFile();
299-
}
300-
301-
$corpus = phutil_split_lines($corpus);
302-
303-
// Adjust the line number into a 0-based offset.
304-
$offset = $inline->getLineNumber();
305-
$offset = $offset - 1;
306-
307-
// Adjust the inclusive range length into a row count.
308-
$length = $inline->getLineLength();
309-
$length = $length + 1;
270+
if ($need_context) {
271+
$context_map = $this->newInlineContextMap($need_context);
310272

311-
$head_min = max(0, $offset - 3);
312-
$head_max = $offset;
313-
$head_len = $head_max - $head_min;
314-
315-
if ($head_len) {
316-
$head = array_slice($corpus, $head_min, $head_len, true);
317-
$head = $this->simplifyContext($head, true);
318-
} else {
319-
$head = array();
273+
foreach ($need_context as $key => $inline) {
274+
$inline->attachInlineContext(idx($context_map, $key));
320275
}
321-
322-
$body = array_slice($corpus, $offset, $length, true);
323-
324-
$tail = array_slice($corpus, $offset + $length, 3, true);
325-
$tail = $this->simplifyContext($tail, false);
326-
327-
$context = id(new PhabricatorDiffInlineCommentContext())
328-
->setFilename($filename)
329-
->setHeadLines($head)
330-
->setBodyLines($body)
331-
->setTailLines($tail);
332-
333-
$inline->attachInlineContext($context);
334276
}
335277

336278
}
337279

338280
return $inlines;
339281
}
340282

341-
private function simplifyContext(array $lines, $is_head) {
283+
final protected function simplifyContext(array $lines, $is_head) {
342284
// We want to provide the smallest amount of context we can while still
343285
// being useful, since the actual code is visible nearby and showing a
344286
// ton of context is silly.

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -668,8 +668,6 @@ JX.install('DiffInline', {
668668
this._editRow = this._drawRows(rows, null, 'edit');
669669

670670
this._drawSuggestionState(this._editRow);
671-
JX.log(this._originalState);
672-
673671
this.setHasSuggestion(this._originalState.hasSuggestion);
674672
},
675673

0 commit comments

Comments
 (0)