From cd63d9b2ce26a74c315f9c3ebc2dcdffdd0d578e Mon Sep 17 00:00:00 2001 From: vrana Date: Fri, 4 May 2012 17:41:06 -0700 Subject: [PATCH] Use subtler highlighting for copied and moved code Summary: The highlighting is distracting according to Nick Shrock and others. Real designer, Lee Byron, helped me with this. It also gives us unagressive target for jumping to the source line in future. Another feature I will probably implement is highlighting also the source of copies/moves. I will use the right side of the left column for it. Test Plan: Hover copied notifier. Hover coverage notifier. I've also checked that this doesn't break our super-flaky old/new code JavaScript detector. Reviewers: epriestley Reviewed By: epriestley CC: aran, Koolvin, leebyron, schrockn Differential Revision: https://secure.phabricator.com/D2403 --- src/__celerity_resource_map__.php | 74 +++++++++---------- .../changeset/DifferentialChangesetParser.php | 30 ++++++-- .../differential/changeset-view.css | 23 ++++-- .../differential/behavior-populate.js | 16 +++- 4 files changed, 87 insertions(+), 56 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 24c0be6e5c..6e2ef64d47 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -538,7 +538,7 @@ celerity_register_resource_map(array( ), 'differential-changeset-view-css' => array( - 'uri' => '/res/4ce438cd/rsrc/css/application/differential/changeset-view.css', + 'uri' => '/res/8e2ace51/rsrc/css/application/differential/changeset-view.css', 'type' => 'css', 'requires' => array( @@ -969,7 +969,7 @@ celerity_register_resource_map(array( ), 'javelin-behavior-differential-populate' => array( - 'uri' => '/res/c0979571/rsrc/js/application/differential/behavior-populate.js', + 'uri' => '/res/a955bf2c/rsrc/js/application/differential/behavior-populate.js', 'type' => 'js', 'requires' => array( @@ -2510,7 +2510,7 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/0c96375e/core.pkg.js', 'type' => 'js', ), - '2debe0e0' => + 'd9299c35' => array( 'name' => 'differential.pkg.css', 'symbols' => @@ -2529,10 +2529,10 @@ celerity_register_resource_map(array( 11 => 'differential-local-commits-view-css', 12 => 'inline-comment-summary-css', ), - 'uri' => '/res/pkg/2debe0e0/differential.pkg.css', + 'uri' => '/res/pkg/d9299c35/differential.pkg.css', 'type' => 'css', ), - '5b7b36d7' => + 'dc7ca445' => array( 'name' => 'differential.pkg.js', 'symbols' => @@ -2556,7 +2556,7 @@ celerity_register_resource_map(array( 16 => 'javelin-behavior-differential-dropdown-menus', 17 => 'javelin-behavior-buoyant', ), - 'uri' => '/res/pkg/5b7b36d7/differential.pkg.js', + 'uri' => '/res/pkg/dc7ca445/differential.pkg.js', 'type' => 'js', ), 'c8ce2d88' => @@ -2652,7 +2652,7 @@ celerity_register_resource_map(array( 'aphront-dialog-view-css' => '2b054c5c', 'aphront-error-view-css' => '2b054c5c', 'aphront-form-view-css' => '2b054c5c', - 'aphront-headsup-action-list-view-css' => '2debe0e0', + 'aphront-headsup-action-list-view-css' => 'd9299c35', 'aphront-headsup-view-css' => '2b054c5c', 'aphront-list-filter-view-css' => '2b054c5c', 'aphront-pager-view-css' => '2b054c5c', @@ -2662,36 +2662,36 @@ celerity_register_resource_map(array( 'aphront-tokenizer-control-css' => '2b054c5c', 'aphront-tooltip-css' => '2b054c5c', 'aphront-typeahead-control-css' => '2b054c5c', - 'differential-changeset-view-css' => '2debe0e0', - 'differential-core-view-css' => '2debe0e0', - 'differential-inline-comment-editor' => '5b7b36d7', - 'differential-local-commits-view-css' => '2debe0e0', - 'differential-results-table-css' => '2debe0e0', - 'differential-revision-add-comment-css' => '2debe0e0', - 'differential-revision-comment-css' => '2debe0e0', - 'differential-revision-comment-list-css' => '2debe0e0', - 'differential-revision-history-css' => '2debe0e0', - 'differential-table-of-contents-css' => '2debe0e0', + 'differential-changeset-view-css' => 'd9299c35', + 'differential-core-view-css' => 'd9299c35', + 'differential-inline-comment-editor' => 'dc7ca445', + 'differential-local-commits-view-css' => 'd9299c35', + 'differential-results-table-css' => 'd9299c35', + 'differential-revision-add-comment-css' => 'd9299c35', + 'differential-revision-comment-css' => 'd9299c35', + 'differential-revision-comment-list-css' => 'd9299c35', + 'differential-revision-history-css' => 'd9299c35', + 'differential-table-of-contents-css' => 'd9299c35', 'diffusion-commit-view-css' => 'c8ce2d88', 'diffusion-icons-css' => 'c8ce2d88', - 'inline-comment-summary-css' => '2debe0e0', + 'inline-comment-summary-css' => 'd9299c35', 'javelin-behavior' => '8a5de8a3', 'javelin-behavior-aphront-basic-tokenizer' => '97f65640', - 'javelin-behavior-aphront-drag-and-drop' => '5b7b36d7', - 'javelin-behavior-aphront-drag-and-drop-textarea' => '5b7b36d7', + 'javelin-behavior-aphront-drag-and-drop' => 'dc7ca445', + 'javelin-behavior-aphront-drag-and-drop-textarea' => 'dc7ca445', 'javelin-behavior-aphront-form-disable-on-submit' => '0c96375e', 'javelin-behavior-audit-preview' => '5e68be89', - 'javelin-behavior-buoyant' => '5b7b36d7', - 'javelin-behavior-differential-accept-with-errors' => '5b7b36d7', - 'javelin-behavior-differential-add-reviewers-and-ccs' => '5b7b36d7', - 'javelin-behavior-differential-comment-jump' => '5b7b36d7', - 'javelin-behavior-differential-diff-radios' => '5b7b36d7', - 'javelin-behavior-differential-dropdown-menus' => '5b7b36d7', - 'javelin-behavior-differential-edit-inline-comments' => '5b7b36d7', - 'javelin-behavior-differential-feedback-preview' => '5b7b36d7', - 'javelin-behavior-differential-keyboard-navigation' => '5b7b36d7', - 'javelin-behavior-differential-populate' => '5b7b36d7', - 'javelin-behavior-differential-show-more' => '5b7b36d7', + 'javelin-behavior-buoyant' => 'dc7ca445', + 'javelin-behavior-differential-accept-with-errors' => 'dc7ca445', + 'javelin-behavior-differential-add-reviewers-and-ccs' => 'dc7ca445', + 'javelin-behavior-differential-comment-jump' => 'dc7ca445', + 'javelin-behavior-differential-diff-radios' => 'dc7ca445', + 'javelin-behavior-differential-dropdown-menus' => 'dc7ca445', + 'javelin-behavior-differential-edit-inline-comments' => 'dc7ca445', + 'javelin-behavior-differential-feedback-preview' => 'dc7ca445', + 'javelin-behavior-differential-keyboard-navigation' => 'dc7ca445', + 'javelin-behavior-differential-populate' => 'dc7ca445', + 'javelin-behavior-differential-show-more' => 'dc7ca445', 'javelin-behavior-diffusion-commit-graph' => '5e68be89', 'javelin-behavior-diffusion-pull-lastmodified' => '5e68be89', 'javelin-behavior-maniphest-batch-selector' => '7707de41', @@ -2701,12 +2701,12 @@ celerity_register_resource_map(array( 'javelin-behavior-maniphest-transaction-preview' => '7707de41', 'javelin-behavior-phabricator-autofocus' => '0c96375e', 'javelin-behavior-phabricator-keyboard-shortcuts' => '0c96375e', - 'javelin-behavior-phabricator-object-selector' => '5b7b36d7', + 'javelin-behavior-phabricator-object-selector' => 'dc7ca445', 'javelin-behavior-phabricator-oncopy' => '0c96375e', 'javelin-behavior-phabricator-tooltips' => '0c96375e', 'javelin-behavior-phabricator-watch-anchor' => '0c96375e', 'javelin-behavior-refresh-csrf' => '0c96375e', - 'javelin-behavior-repository-crossreference' => '5b7b36d7', + 'javelin-behavior-repository-crossreference' => 'dc7ca445', 'javelin-behavior-workflow' => '0c96375e', 'javelin-dom' => '8a5de8a3', 'javelin-event' => '8a5de8a3', @@ -2728,23 +2728,23 @@ celerity_register_resource_map(array( 'maniphest-task-summary-css' => '7839ae2d', 'maniphest-transaction-detail-css' => '7839ae2d', 'phabricator-app-buttons-css' => '2b054c5c', - 'phabricator-content-source-view-css' => '2debe0e0', + 'phabricator-content-source-view-css' => 'd9299c35', 'phabricator-core-buttons-css' => '2b054c5c', 'phabricator-core-css' => '2b054c5c', 'phabricator-directory-css' => '2b054c5c', - 'phabricator-drag-and-drop-file-upload' => '5b7b36d7', + 'phabricator-drag-and-drop-file-upload' => 'dc7ca445', 'phabricator-dropdown-menu' => '0c96375e', 'phabricator-flag-css' => '2b054c5c', 'phabricator-jump-nav' => '2b054c5c', 'phabricator-keyboard-shortcut' => '0c96375e', 'phabricator-keyboard-shortcut-manager' => '0c96375e', 'phabricator-menu-item' => '0c96375e', - 'phabricator-object-selector-css' => '2debe0e0', + 'phabricator-object-selector-css' => 'd9299c35', 'phabricator-paste-file-upload' => '0c96375e', 'phabricator-prefab' => '0c96375e', 'phabricator-project-tag-css' => '7839ae2d', 'phabricator-remarkup-css' => '2b054c5c', - 'phabricator-shaped-request' => '5b7b36d7', + 'phabricator-shaped-request' => 'dc7ca445', 'phabricator-standard-page-view' => '2b054c5c', 'phabricator-tooltip' => '0c96375e', 'phabricator-transaction-view-css' => '2b054c5c', diff --git a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php index da14607e0b..de0e01fca9 100644 --- a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php @@ -1396,6 +1396,7 @@ final class DifferentialChangesetParser { $o_attr = null; } + $n_copy = ''; if (isset($this->new[$ii])) { $n_num = $this->new[$ii]['line']; @@ -1417,8 +1418,17 @@ final class DifferentialChangesetParser { if ($this->new[$ii]['type']) { if ($this->new[$ii]['type'] == '\\') { $n_text = $this->new[$ii]['text']; - $n_attr = ' class="comment"'; - } else if (isset($copy_lines[$n_num])) { + $n_class = 'comment'; + } else if (empty($this->old[$ii])) { + $n_class = 'new new-full'; + } else { + $n_class = 'new'; + } + $n_attr = ' class="'.$n_class.'"'; + + if ($this->new[$ii]['type'] == '\\' || !isset($copy_lines[$n_num])) { + $n_copy = ''; + } else { list($orig_file, $orig_line, $orig_type) = $copy_lines[$n_num]; $title = ($orig_type == '-' ? 'Moved' : 'Copied').' from '; if ($orig_file == '') { @@ -1430,12 +1440,15 @@ final class DifferentialChangesetParser { dirname('/'.$orig_file); } $class = ($orig_type == '-' ? 'new-move' : 'new-copy'); - $n_attr = - ' class="'.$class.'" title="'.phutil_escape_html($title).'"'; - } else if (empty($this->old[$ii])) { - $n_attr = ' class="new new-full"'; - } else { - $n_attr = ' class="new"'; + $n_copy = javelin_render_tag( + 'td', + array( + 'meta' => array( + 'msg' => $title, + ), + 'class' => 'copy '.$class, + ), + ''); } } } else { @@ -1470,6 +1483,7 @@ final class DifferentialChangesetParser { ''.$o_num.''. ''.$o_text.''. ''.$n_num.''. + $n_copy. // NOTE: This is a unicode zero-width space, which we use as a hint // when intercepting 'copy' events to make sure sensible text ends // up on the clipboard. See the 'phabricator-oncopy' behavior. diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 263f8db210..21c4181034 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -80,19 +80,26 @@ background: #ffaaaa; } -.differential-diff td.new-copy { - background: #ffffaa; -} - -.differential-diff td.new-move { - background: #ffddaa; -} - .differential-diff td.new-full, .differential-diff td.new span.bright { background: #aaffaa; } +.differential-diff td.copy { + width: 6px; + padding: 0; +} + +.differential-diff td.new-copy, +.differential-diff td.new-copy span.bright { + background: #ffffaa; +} + +.differential-diff td.new-move, +.differential-diff td.new-move span.bright { + background: #eeee44; +} + .differential-diff td.comment { background: #dddddd; } diff --git a/webroot/rsrc/js/application/differential/behavior-populate.js b/webroot/rsrc/js/application/differential/behavior-populate.js index 96f1a40b6f..2c718dfe40 100644 --- a/webroot/rsrc/js/application/differential/behavior-populate.js +++ b/webroot/rsrc/js/application/differential/behavior-populate.js @@ -65,7 +65,7 @@ JX.behavior('differential-populate', function(config) { // NOTE: Using className is not best practice, but the diff UI is perf // sensitive. - if (!t.className.match(/cov/)) { + if (!t.className.match(/cov|copy/)) { return; } @@ -78,6 +78,8 @@ JX.behavior('differential-populate', function(config) { } else { highlight_class = null; var msg; + var align = 'E'; + var sibling = 'previousSibling'; if (t.className.match(/cov-C/)) { msg = 'Covered'; highlight_class = 'source-cov-C'; @@ -87,14 +89,22 @@ JX.behavior('differential-populate', function(config) { } else if (t.className.match(/cov-N/)) { msg = 'Not Executable'; highlight_class = 'source-cov-N'; + } else { + var match = /new-copy|new-move/.exec(t.className); + if (match) { + align = 'N'; // TODO: 'W' + sibling = 'nextSibling'; + msg = JX.Stratcom.getData(t).msg; + highlight_class = match[0]; + } } if (msg) { - JX.Tooltip.show(t, 120, 'E', msg); + JX.Tooltip.show(t, 120, align, msg); } if (highlight_class) { - highlighted = t.previousSibling; + highlighted = t[sibling]; JX.DOM.alterClass(highlighted, highlight_class, true); } }