@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
Improve sequencing of various content/header checks in abstract block diffs
Summary:
Ref T13425. Some diff checks currently sequence incorrectly:
- When we're rendering block lists, syntax highlighting isn't relevant.
- The "large change" guard can prevent rendering of otherwise-renderable changes.
- Actual errors in the document engine (like bad JSON in a ".ipynb" file) aren't surfaced properly.
Improve sequencing somewhat to resolve these issues.
Test Plan:
- Viewed a notebook, no longer saw a "highlighting disabled" warning.
- Forced a notebook to fail, got a useful inline error instead of a popup dialog error.
- Forced a notebook to have a large number of differences, got a rendering out of it.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20843
+72
-17
+8
-4
src/applications/differential/parser/DifferentialChangesetParser.php
+8
-4
src/applications/differential/parser/DifferentialChangesetParser.php
···
868
868
->setHighlightingDisabled($this->highlightingDisabled)
869
869
->setDepthOnlyLines($this->getDepthOnlyLines());
870
870
871
+
$engine_blocks = $this->newDocumentEngineBlocks();
872
+
$has_document_engine = ($engine_blocks !== null);
873
+
871
874
$shield = null;
872
-
if ($this->isTopLevel && !$this->comments) {
875
+
if ($this->isTopLevel && !$this->comments && !$has_document_engine) {
873
876
if ($this->isGenerated()) {
874
877
$shield = $renderer->renderShield(
875
878
pht(
···
1024
1027
->setOldComments($old_comments)
1025
1028
->setNewComments($new_comments);
1026
1029
1027
-
$engine_blocks = $this->newDocumentEngineChangesetView();
1028
1030
if ($engine_blocks !== null) {
1029
1031
$reference = $this->getRenderingReference();
1030
1032
$parts = explode('/', $reference);
···
1040
1042
if (!$vs) {
1041
1043
$vs = $id;
1042
1044
}
1045
+
1046
+
$renderer->setDocumentEngineBlocks($engine_blocks);
1043
1047
1044
1048
return $renderer->renderDocumentEngineBlocks(
1045
1049
$engine_blocks,
···
1653
1657
return $prefix.$line;
1654
1658
}
1655
1659
1656
-
private function newDocumentEngineChangesetView() {
1660
+
private function newDocumentEngineBlocks() {
1657
1661
$changeset = $this->changeset;
1658
1662
$viewer = $this->getViewer();
1659
1663
···
1724
1728
}
1725
1729
1726
1730
if ($document_engine) {
1727
-
return $document_engine->newDiffView(
1731
+
return $document_engine->newEngineBlocks(
1728
1732
$old_ref,
1729
1733
$new_ref);
1730
1734
}
+14
-5
src/applications/differential/render/DifferentialChangesetHTMLRenderer.php
+14
-5
src/applications/differential/render/DifferentialChangesetHTMLRenderer.php
···
270
270
}
271
271
}
272
272
273
-
if ($this->getHighlightingDisabled()) {
274
-
$messages[] = pht(
275
-
'This file is larger than %s, so syntax highlighting is '.
276
-
'disabled by default.',
277
-
phutil_format_bytes(DifferentialChangesetParser::HIGHLIGHT_BYTE_LIMIT));
273
+
$blocks = $this->getDocumentEngineBlocks();
274
+
if ($blocks) {
275
+
foreach ($blocks->getMessages() as $message) {
276
+
$messages[] = $message;
277
+
}
278
+
} else {
279
+
if ($this->getHighlightingDisabled()) {
280
+
$byte_limit = DifferentialChangesetParser::HIGHLIGHT_BYTE_LIMIT;
281
+
$byte_limit = phutil_format_bytes($byte_limit);
282
+
$messages[] = pht(
283
+
'This file is larger than %s, so syntax highlighting is '.
284
+
'disabled by default.',
285
+
$byte_limit);
286
+
}
278
287
}
279
288
280
289
return $this->formatHeaderMessages($messages);
+21
src/applications/differential/render/DifferentialChangesetRenderer.php
+21
src/applications/differential/render/DifferentialChangesetRenderer.php
···
35
35
private $highlightingDisabled;
36
36
private $scopeEngine = false;
37
37
private $depthOnlyLines;
38
+
private $documentEngineBlocks;
38
39
39
40
private $oldFile = false;
40
41
private $newFile = false;
···
239
240
return $this->oldChangesetID;
240
241
}
241
242
243
+
public function setDocumentEngineBlocks(
244
+
PhabricatorDocumentEngineBlocks $blocks) {
245
+
$this->documentEngineBlocks = $blocks;
246
+
return $this;
247
+
}
248
+
249
+
public function getDocumentEngineBlocks() {
250
+
return $this->documentEngineBlocks;
251
+
}
252
+
242
253
public function setNewComments(array $new_comments) {
243
254
foreach ($new_comments as $line_number => $comments) {
244
255
assert_instances_of($comments, 'PhabricatorInlineCommentInterface');
···
355
366
$notice = null;
356
367
if ($this->getIsTopLevel()) {
357
368
$force = (!$content && !$props);
369
+
370
+
// If we have DocumentEngine messages about the blocks, assume they
371
+
// explain why there's no content.
372
+
$blocks = $this->getDocumentEngineBlocks();
373
+
if ($blocks) {
374
+
if ($blocks->getMessages()) {
375
+
$force = false;
376
+
}
377
+
}
378
+
358
379
$notice = $this->renderChangeTypeHeader($force);
359
380
}
360
381
+14
src/applications/files/diff/PhabricatorDocumentEngineBlocks.php
+14
src/applications/files/diff/PhabricatorDocumentEngineBlocks.php
···
4
4
extends Phobject {
5
5
6
6
private $lists = array();
7
+
private $messages = array();
8
+
9
+
public function addMessage($message) {
10
+
$this->messages[] = $message;
11
+
return $this;
12
+
}
13
+
14
+
public function getMessages() {
15
+
return $this->messages;
16
+
}
7
17
8
18
public function addBlockList(PhabricatorDocumentRef $ref, array $blocks) {
9
19
assert_instances_of($blocks, 'PhabricatorDocumentEngineBlock');
···
19
29
public function newTwoUpLayout() {
20
30
$rows = array();
21
31
$lists = $this->lists;
32
+
33
+
if (count($lists) != 2) {
34
+
return array();
35
+
}
22
36
23
37
$specs = array();
24
38
foreach ($this->lists as $list) {
+1
-1
src/applications/files/document/PhabricatorDocumentEngine.php
+1
-1
src/applications/files/document/PhabricatorDocumentEngine.php
+1
-1
src/applications/files/document/PhabricatorImageDocumentEngine.php
+1
-1
src/applications/files/document/PhabricatorImageDocumentEngine.php
+13
-6
src/applications/files/document/PhabricatorJupyterDocumentEngine.php
+13
-6
src/applications/files/document/PhabricatorJupyterDocumentEngine.php
···
41
41
return true;
42
42
}
43
43
44
-
public function newDiffView(
44
+
public function newEngineBlocks(
45
45
PhabricatorDocumentRef $uref,
46
46
PhabricatorDocumentRef $vref) {
47
47
48
-
$u_blocks = $this->newDiffBlocks($uref);
49
-
$v_blocks = $this->newDiffBlocks($vref);
48
+
$blocks = new PhabricatorDocumentEngineBlocks();
49
+
50
+
try {
51
+
$u_blocks = $this->newDiffBlocks($uref);
52
+
$v_blocks = $this->newDiffBlocks($vref);
53
+
54
+
$blocks->addBlockList($uref, $u_blocks);
55
+
$blocks->addBlockList($vref, $v_blocks);
56
+
} catch (Exception $ex) {
57
+
$blocks->addMessage($ex->getMessage());
58
+
}
50
59
51
-
return id(new PhabricatorDocumentEngineBlocks())
52
-
->addBlockList($uref, $u_blocks)
53
-
->addBlockList($vref, $v_blocks);
60
+
return $blocks;
54
61
}
55
62
56
63
private function newDiffBlocks(PhabricatorDocumentRef $ref) {