From 2c06815edb0fe3f0cc374fc7986d5a4cfb90ab13 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 25 Sep 2019 19:39:45 -0700 Subject: [PATCH] When rendering Jupyter notebook diffs, split code inputs into individual blocks Summary: Ref T13425. Currently, code inputs and all outputs are grouped into a single block. This is fine for display notebooks but not great for diffing notebooks. Instead, split source code input into individual lines with one line per block, and each output into its own block. This allows you to leave actual line-by-line inlines on source, and comment on outputs individually. Test Plan: {F6888583} Maniphest Tasks: T13425 Differential Revision: https://secure.phabricator.com/D20840 --- resources/celerity/map.php | 42 +-- .../DifferentialChangesetTwoUpRenderer.php | 6 + .../PhabricatorJupyterDocumentEngine.php | 254 +++++++++++++++--- .../differential/changeset-view.css | 5 + webroot/rsrc/css/core/syntax.css | 4 + .../rsrc/css/phui/phui-property-list-view.css | 44 ++- .../js/application/diff/DiffChangesetList.js | 45 ++-- 7 files changed, 300 insertions(+), 100 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index ae52bfe21f..ad7675abff 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,10 +9,10 @@ return array( 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '6a8c9533', + 'core.pkg.css' => '19ec9519', 'core.pkg.js' => '6e5c894f', - 'differential.pkg.css' => 'abd2c0d8', - 'differential.pkg.js' => '68fa36fc', + 'differential.pkg.css' => '607c84be', + 'differential.pkg.js' => 'a0212a0b', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => 'a98c0bf7', 'maniphest.pkg.css' => '35995d6d', @@ -61,7 +61,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '5a205b9d', 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', - 'rsrc/css/application/differential/changeset-view.css' => '5dda5e53', + 'rsrc/css/application/differential/changeset-view.css' => '489b6995', 'rsrc/css/application/differential/core.css' => '7300a73e', 'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', @@ -113,7 +113,7 @@ return array( 'rsrc/css/application/uiexample/example.css' => 'b4795059', 'rsrc/css/core/core.css' => '1b29ed61', 'rsrc/css/core/remarkup.css' => 'f06cc20e', - 'rsrc/css/core/syntax.css' => '4234f572', + 'rsrc/css/core/syntax.css' => '220b85f9', 'rsrc/css/core/z-index.css' => '99c0f5eb', 'rsrc/css/diviner/diviner-shared.css' => '4bd263b0', 'rsrc/css/font/font-awesome.css' => '3883938a', @@ -169,7 +169,7 @@ return array( 'rsrc/css/phui/phui-pager.css' => 'd022c7ad', 'rsrc/css/phui/phui-pinboard-view.css' => '1f08f5d8', 'rsrc/css/phui/phui-policy-section-view.css' => '139fdc64', - 'rsrc/css/phui/phui-property-list-view.css' => '34180764', + 'rsrc/css/phui/phui-property-list-view.css' => 'ad841f1c', 'rsrc/css/phui/phui-remarkup-preview.css' => '91767007', 'rsrc/css/phui/phui-segment-bar-view.css' => '5166b370', 'rsrc/css/phui/phui-spacing.css' => 'b05cadc3', @@ -377,7 +377,7 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8', 'rsrc/js/application/diff/DiffChangeset.js' => 'a31ffc00', - 'rsrc/js/application/diff/DiffChangesetList.js' => '22f6bb51', + 'rsrc/js/application/diff/DiffChangesetList.js' => '0f5c016d', 'rsrc/js/application/diff/DiffInline.js' => 'a4a14a94', 'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17', 'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd', @@ -554,7 +554,7 @@ return array( 'conpherence-thread-manager' => 'aec8e38c', 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => '9d068042', - 'differential-changeset-view-css' => '5dda5e53', + 'differential-changeset-view-css' => '489b6995', 'differential-core-view-css' => '7300a73e', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', @@ -774,7 +774,7 @@ return array( 'phabricator-darkmessage' => '26cd4b73', 'phabricator-dashboard-css' => '5a205b9d', 'phabricator-diff-changeset' => 'a31ffc00', - 'phabricator-diff-changeset-list' => '22f6bb51', + 'phabricator-diff-changeset-list' => '0f5c016d', 'phabricator-diff-inline' => 'a4a14a94', 'phabricator-drag-and-drop-file-upload' => '4370900d', 'phabricator-draggable-list' => 'c9ad6f70', @@ -865,7 +865,7 @@ return array( 'phui-pager-css' => 'd022c7ad', 'phui-pinboard-view-css' => '1f08f5d8', 'phui-policy-section-view-css' => '139fdc64', - 'phui-property-list-view-css' => '34180764', + 'phui-property-list-view-css' => 'ad841f1c', 'phui-remarkup-preview-css' => '91767007', 'phui-segment-bar-view-css' => '5166b370', 'phui-spacing-css' => 'b05cadc3', @@ -900,7 +900,7 @@ return array( 'sprite-login-css' => '18b368a6', 'sprite-tokens-css' => 'f1896dc5', 'syntax-default-css' => '055fc231', - 'syntax-highlighting-css' => '4234f572', + 'syntax-highlighting-css' => '220b85f9', 'tokens-css' => 'ce5a50bd', 'trigger-rule' => '41b7b4f6', 'trigger-rule-control' => '5faf27b9', @@ -1005,6 +1005,10 @@ return array( 'javelin-workflow', 'phuix-icon-view', ), + '0f5c016d' => array( + 'javelin-install', + 'phuix-button-view', + ), '111bfd2d' => array( 'javelin-install', ), @@ -1065,6 +1069,9 @@ return array( 'javelin-behavior', 'javelin-request', ), + '220b85f9' => array( + 'syntax-default-css', + ), '225bbb98' => array( 'javelin-install', 'javelin-reactor', @@ -1075,10 +1082,6 @@ return array( 'javelin-typeahead-source', 'javelin-util', ), - '22f6bb51' => array( - 'javelin-install', - 'phuix-button-view', - ), 23387297 => array( 'javelin-install', 'javelin-util', @@ -1260,9 +1263,6 @@ return array( 'javelin-behavior', 'javelin-uri', ), - '4234f572' => array( - 'syntax-default-css', - ), '4370900d' => array( 'javelin-install', 'javelin-util', @@ -1303,6 +1303,9 @@ return array( 'javelin-dom', 'phabricator-draggable-list', ), + '489b6995' => array( + 'phui-inline-comment-view-css', + ), '48fe33d0' => array( 'javelin-behavior', 'javelin-dom', @@ -1457,9 +1460,6 @@ return array( 'javelin-dom', 'phuix-dropdown-menu', ), - '5dda5e53' => array( - 'phui-inline-comment-view-css', - ), '5faf27b9' => array( 'phuix-form-control-view', ), diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index 625626c4bb..8285baa0ad 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -387,6 +387,8 @@ final class DifferentialChangesetTwoUpRenderer $old_classes[] = 'old-full'; } + $old_classes[] = 'diff-flush'; + $old_classes = implode(' ', $old_classes); } else { $old_content = null; @@ -404,6 +406,8 @@ final class DifferentialChangesetTwoUpRenderer $new_classes[] = 'new-full'; } + $new_classes[] = 'diff-flush'; + $new_classes = implode(' ', $new_classes); } else { $new_content = null; @@ -451,6 +455,7 @@ final class DifferentialChangesetTwoUpRenderer 'td', array( 'class' => $old_classes, + 'data-copy-mode' => 'copy-l', ), $old_content); @@ -479,6 +484,7 @@ final class DifferentialChangesetTwoUpRenderer array( 'class' => $new_classes, 'colspan' => '2', + 'data-copy-mode' => 'copy-r', ), $new_content); diff --git a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php index b4a0895c12..9836797f84 100644 --- a/src/applications/files/document/PhabricatorJupyterDocumentEngine.php +++ b/src/applications/files/document/PhabricatorJupyterDocumentEngine.php @@ -57,11 +57,7 @@ final class PhabricatorJupyterDocumentEngine $viewer = $this->getViewer(); $content = $ref->loadData(); - $data = phutil_json_decode($content); - $cells = idx($data, 'cells'); - if (!is_array($cells)) { - throw new Exception('Missing "cells".'); - } + $cells = $this->newCells($content, true); $idx = 1; $blocks = array(); @@ -82,8 +78,20 @@ final class PhabricatorJupyterDocumentEngine ), $notebook_table); + // When the cell is a source code line, we can hash just the raw + // input rather than all the cell metadata. + + switch (idx($cell, 'cell_type')) { + case 'code/line': + $hash_input = $cell['raw']; + break; + default: + $hash_input = serialize($cell); + break; + } + $hash = PhabricatorHash::digestWithNamedKey( - serialize($cell), + $hash_input, 'document-engine.content-digest'); $blocks[] = id(new PhabricatorDocumentEngineBlock()) @@ -101,10 +109,39 @@ final class PhabricatorJupyterDocumentEngine $viewer = $this->getViewer(); $content = $ref->loadData(); + try { + $cells = $this->newCells($content, false); + } catch (Exception $ex) { + return $this->newMessage($ex->getMessage()); + } + + $rows = array(); + foreach ($cells as $cell) { + $rows[] = $this->renderJupyterCell($viewer, $cell); + } + + $notebook_table = phutil_tag( + 'table', + array( + 'class' => 'jupyter-notebook', + ), + $rows); + + $container = phutil_tag( + 'div', + array( + 'class' => 'document-engine-jupyter', + ), + $notebook_table); + + return $container; + } + + private function newCells($content, $for_diff) { try { $data = phutil_json_decode($content); } catch (PhutilJSONParserException $ex) { - return $this->newMessage( + throw new Exception( pht( 'This is not a valid JSON document and can not be rendered as '. 'a Jupyter notebook: %s.', @@ -112,7 +149,7 @@ final class PhabricatorJupyterDocumentEngine } if (!is_array($data)) { - return $this->newMessage( + throw new Exception( pht( 'This document does not encode a valid JSON object and can not '. 'be rendered as a Jupyter notebook.')); @@ -121,14 +158,14 @@ final class PhabricatorJupyterDocumentEngine $nbformat = idx($data, 'nbformat'); if (!strlen($nbformat)) { - return $this->newMessage( + throw new Exception( pht( 'This document is missing an "nbformat" field. Jupyter notebooks '. 'must have this field.')); } if ($nbformat !== 4) { - return $this->newMessage( + throw new Exception( pht( 'This Jupyter notebook uses an unsupported version of the file '. 'format (found version %s, expected version 4).', @@ -137,39 +174,80 @@ final class PhabricatorJupyterDocumentEngine $cells = idx($data, 'cells'); if (!is_array($cells)) { - return $this->newMessage( + throw new Exception( pht( 'This Jupyter notebook does not specify a list of "cells".')); } if (!$cells) { - return $this->newMessage( + throw new Exception( pht( 'This Jupyter notebook does not specify any notebook cells.')); } - $rows = array(); - foreach ($cells as $cell) { - $rows[] = $this->renderJupyterCell($viewer, $cell); + if (!$for_diff) { + return $cells; } - $notebook_table = phutil_tag( - 'table', - array( - 'class' => 'jupyter-notebook', - ), - $rows); + // If we're extracting cells to build a diff view, split code cells into + // individual lines and individual outputs. We want users to be able to + // add inline comments to each line and each output block. - $container = phutil_tag( - 'div', - array( - 'class' => 'document-engine-jupyter', - ), - $notebook_table); + $results = array(); + foreach ($cells as $cell) { + $cell_type = idx($cell, 'cell_type'); - return $container; + if ($cell_type !== 'code') { + $results[] = $cell; + continue; + } + + $label = $this->newCellLabel($cell); + + $lines = idx($cell, 'source'); + if (!is_array($lines)) { + $lines = array(); + } + + $content = $this->highlightLines($lines); + + $count = count($lines); + for ($ii = 0; $ii < $count; $ii++) { + $is_head = ($ii === 0); + $is_last = ($ii === ($count - 1)); + + if ($is_head) { + $line_label = $label; + } else { + $line_label = null; + } + + $results[] = array( + 'cell_type' => 'code/line', + 'label' => $line_label, + 'raw' => $lines[$ii], + 'display' => idx($content, $ii), + 'head' => $is_head, + 'last' => $is_last, + ); + } + + $outputs = array(); + $output_list = idx($cell, 'outputs'); + if (is_array($output_list)) { + foreach ($output_list as $output) { + $results[] = array( + 'cell_type' => 'code/output', + 'output' => $output, + ); + } + } + } + + return $results; } + private function renderJupyterCell( PhabricatorUser $viewer, array $cell) { @@ -177,13 +255,24 @@ final class PhabricatorJupyterDocumentEngine list($label, $content) = $this->renderJupyterCellContent($viewer, $cell); $label_cell = phutil_tag( - 'th', - array(), + 'td', + array( + 'class' => 'jupyter-label', + ), $label); + $classes = null; + switch (idx($cell, 'cell_type')) { + case 'code/line': + $classes = 'jupyter-cell-flush'; + break; + } + $content_cell = phutil_tag( 'td', - array(), + array( + 'class' => $classes, + ), $content); return phutil_tag( @@ -204,7 +293,11 @@ final class PhabricatorJupyterDocumentEngine case 'markdown': return $this->newMarkdownCell($cell); case 'code': - return $this->newCodeCell($cell); + return $this->newCodeCell($cell); + case 'code/line': + return $this->newCodeLineCell($cell); + case 'code/output': + return $this->newCodeOutputCell($cell); } return $this->newRawCell(id(new PhutilJSON())->encodeFormatted($cell)); @@ -243,23 +336,14 @@ final class PhabricatorJupyterDocumentEngine } private function newCodeCell(array $cell) { - $execution_count = idx($cell, 'execution_count'); - if ($execution_count) { - $label = 'In ['.$execution_count.']:'; - } else { - $label = null; - } + $label = $this->newCellLabel($cell); $content = idx($cell, 'source'); if (!is_array($content)) { $content = array(); } - $content = implode('', $content); - - $content = PhabricatorSyntaxHighlighter::highlightWithLanguage( - 'py', - $content); + $content = $this->highlightLines($content); $outputs = array(); $output_list = idx($cell, 'outputs'); @@ -275,7 +359,9 @@ final class PhabricatorJupyterDocumentEngine phutil_tag( 'div', array( - 'class' => 'jupyter-cell-code PhabricatorMonospaced remarkup-code', + 'class' => + 'jupyter-cell-code jupyter-cell-code-block '. + 'PhabricatorMonospaced remarkup-code', ), array( $content, @@ -285,6 +371,45 @@ final class PhabricatorJupyterDocumentEngine ); } + private function newCodeLineCell(array $cell) { + $classes = array(); + $classes[] = 'PhabricatorMonospaced'; + $classes[] = 'remarkup-code'; + $classes[] = 'jupyter-cell-code'; + $classes[] = 'jupyter-cell-code-line'; + + if ($cell['head']) { + $classes[] = 'jupyter-cell-code-head'; + } + + if ($cell['last']) { + $classes[] = 'jupyter-cell-code-last'; + } + + $classes = implode(' ', $classes); + + return array( + $cell['label'], + array( + phutil_tag( + 'div', + array( + 'class' => $classes, + ), + array( + $cell['display'], + )), + ), + ); + } + + private function newCodeOutputCell(array $cell) { + return array( + null, + $this->newOutput($cell['output']), + ); + } + private function newOutput(array $output) { if (!is_array($output)) { return pht(''); @@ -371,4 +496,45 @@ final class PhabricatorJupyterDocumentEngine $content); } + private function newCellLabel(array $cell) { + $execution_count = idx($cell, 'execution_count'); + if ($execution_count) { + $label = 'In ['.$execution_count.']:'; + } else { + $label = null; + } + + return $label; + } + + private function highlightLines(array $lines) { + $head = head($lines); + $matches = null; + if (preg_match('/^%%(.*)$/', $head, $matches)) { + $restore = array_shift($lines); + $lang = $matches[1]; + } else { + $restore = null; + $lang = 'py'; + } + + $content = PhabricatorSyntaxHighlighter::highlightWithLanguage( + $lang, + implode('', $lines)); + $content = phutil_split_lines($content); + + if ($restore !== null) { + $language_tag = phutil_tag( + 'span', + array( + 'class' => 'language-tag', + ), + $restore); + + array_unshift($content, $language_tag); + } + + return $content; + } + } diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 5aa4b60981..06d5a50721 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -63,6 +63,11 @@ padding: 1px 8px; } +.differential-diff td.diff-flush { + padding-top: 0; + padding-bottom: 0; +} + .device .differential-diff td { padding: 1px 4px; } diff --git a/webroot/rsrc/css/core/syntax.css b/webroot/rsrc/css/core/syntax.css index 90f2981ba6..0114d3fb24 100644 --- a/webroot/rsrc/css/core/syntax.css +++ b/webroot/rsrc/css/core/syntax.css @@ -6,6 +6,10 @@ color: #aa0066; } +.remarkup-code .language-tag { + color: {$lightgreytext}; +} + .remarkup-code td > span { display: inline; word-break: break-all; diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css index 77198be72d..61be658feb 100644 --- a/webroot/rsrc/css/phui/phui-property-list-view.css +++ b/webroot/rsrc/css/phui/phui-property-list-view.css @@ -298,22 +298,56 @@ div.phui-property-list-stacked .phui-property-list-properties .jupyter-cell-code { white-space: pre-wrap; + word-break: break-word; background: {$lightgreybackground}; - padding: 8px; - border: 1px solid {$lightgreyborder}; border-radius: 2px; + border-color: {$lightgreyborder}; + border-style: solid; +} + +.jupyter-cell-code-block { + padding: 8px; + border-width: 1px; +} + +.jupyter-cell-code-line { + padding: 2px 8px; + border-width: 0 1px; +} + +.jupyter-cell-code-head { + border-top-width: 1px; + margin-top: 4px; + padding-top: 8px; +} + +.jupyter-cell-code-last { + border-bottom-width: 1px; + margin-bottom: 4px; + padding-bottom: 8px; } -.jupyter-notebook > tbody > tr > th, .jupyter-notebook > tbody > tr > td { padding: 8px; } -.jupyter-notebook > tbody > tr > th { +.jupyter-notebook > tbody > tr > td.jupyter-cell-flush { + padding-top: 0; + padding-bottom: 0; +} + +.jupyter-notebook, +.jupyter-notebook > tbody > tr > td { + width: 100%; +} + +.jupyter-notebook > tbody > tr > td.jupyter-label { white-space: nowrap; text-align: right; - min-width: 48px; + min-width: 56px; font-weight: bold; + width: auto; + padding: 8px 8px 0; } .jupyter-output { diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 3240b30290..341590ea85 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -1195,41 +1195,26 @@ JX.install('DiffChangesetList', { bot = tmp; } - // Find the leftmost cell that we're going to highlight: this is the next - // in the row that does not have a "data-n" (line number) - // attribute. In 2up views, it should be directly adjacent. In - // 1up views, we may have to skip over the other line number column. - var l = top; - while (l.nextSibling && l.getAttribute('data-n')) { - l = l.nextSibling; - } - - // Find the rightmost cell that we're going to highlight: this is the - // farthest consecutive, adjacent in the row that does not have - // a "data-n" (line number) attribute. Sometimes the left and right nodes - // are the same (left side of 2up view); sometimes we're going to - // highlight several nodes (copy + code + coverage). - var r = l; - while (true) { - // No more cells in the row, so we can't keep expanding. - if (!r.nextSibling) { - break; - } - - if (r.nextSibling.getAttribute('data-n')) { - break; - } + // Find the leftmost cell that we're going to highlight. This is the + // next sibling with a "data-copy-mode" attribute, which is a marker + // for the cell with actual content in it. + var content_cell = top; + while (content_cell && !content_cell.getAttribute('data-copy-mode')) { + content_cell = content_cell.nextSibling; + } - r = r.nextSibling; + // If we didn't find a cell to highlight, don't highlight anything. + if (!content_cell) { + return; } - var pos = JX.$V(l) - .add(JX.Vector.getAggregateScrollForNode(l)); + var pos = JX.$V(content_cell) + .add(JX.Vector.getAggregateScrollForNode(content_cell)); - var dim = JX.$V(r) - .add(JX.Vector.getAggregateScrollForNode(r)) + var dim = JX.$V(content_cell) + .add(JX.Vector.getAggregateScrollForNode(content_cell)) .add(-pos.x, -pos.y) - .add(JX.Vector.getDim(r)); + .add(JX.Vector.getDim(content_cell)); var bpos = JX.$V(bot) .add(JX.Vector.getAggregateScrollForNode(bot)); -- 2.51.2