From e5c0bfc8e30876bccc43cdd9874c37ebec9e9da5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 6 Jan 2012 07:36:55 -0800 Subject: [PATCH] Link to undisplayed inline comments Summary: We currently don't link to comments which aren't visible. Link to the appropriate diff in a new window, indicating where the comment lives. Test Plan: Clicked visible, not-so-visible comments. Reviewers: btrahan, jungejason, davidreuss Reviewed By: btrahan CC: aran, btrahan, epriestley Maniphest Tasks: T555, T449 Differential Revision: https://secure.phabricator.com/D1333 --- .../DifferentialRevisionViewController.php | 1 + .../DifferentialRevisionCommentView.php | 61 +++++++++++++++++-- .../DifferentialRevisionCommentListView.php | 10 ++- .../differential/revision-comment.css | 5 ++ 4 files changed, 72 insertions(+), 5 deletions(-) diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index bf64868591..ecb531f476 100644 --- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php @@ -210,6 +210,7 @@ class DifferentialRevisionViewController extends DifferentialController { $comment_view->setChangesets($all_changesets); $comment_view->setUser($user); $comment_view->setTargetDiff($target); + $comment_view->setVersusDiffID($diff_vs); $changeset_view = new DifferentialChangesetListView(); $changeset_view->setChangesets($visible_changesets); diff --git a/src/applications/differential/view/revisioncomment/DifferentialRevisionCommentView.php b/src/applications/differential/view/revisioncomment/DifferentialRevisionCommentView.php index c905769c89..f5d57e1771 100644 --- a/src/applications/differential/view/revisioncomment/DifferentialRevisionCommentView.php +++ b/src/applications/differential/view/revisioncomment/DifferentialRevisionCommentView.php @@ -27,6 +27,7 @@ final class DifferentialRevisionCommentView extends AphrontView { private $target; private $commentNumber; private $user; + private $versusDiffID; public function setComment($comment) { $this->comment = $comment; @@ -61,6 +62,12 @@ final class DifferentialRevisionCommentView extends AphrontView { public function setTargetDiff($target) { $this->target = $target; + return $this; + } + + public function setVersusDiffID($diff_vs) { + $this->versusDiffID = $diff_vs; + return $this; } public function setCommentNumber($comment_number) { @@ -181,15 +188,60 @@ final class DifferentialRevisionCommentView extends AphrontView { ($inline->getLineNumber() + $inline->getLineLength()); } - if (!$this->target || - $changeset->getDiffID() === $this->target->getID()) { + $on_target = ($this->target) && + ($this->target->getID() == $changeset->getDiffID()); + + $is_visible = false; + if ($inline->getIsNewFile()) { + // This comment is on the right side of the versus diff, and visible + // on the left side of the page. + if ($this->versusDiffID) { + if ($changeset->getDiffID() == $this->versusDiffID) { + $is_visible = true; + } + } + + // This comment is on the right side of the target diff, and visible + // on the right side of the page. + if ($on_target) { + $is_visible = true; + } + } else { + // Ths comment is on the left side of the target diff, and visible + // on the left side of the page. + if (!$this->versusDiffID) { + if ($on_target) { + $is_visible = true; + } + } + + // TODO: We still get one edge case wrong here, when we have a + // versus diff and the file didn't exist in the old version. The + // comment is visible because we show the left side of the target + // diff when there's no corresponding file in the versus diff, but + // we incorrectly link it off-page. + } + + $where = null; + if ($is_visible) { $lines = phutil_render_tag( 'a', array( - 'href' => '#inline-'.$inline->getID(), - 'class' => 'num', + 'href' => '#inline-'.$inline->getID(), + 'class' => 'num', ), $lines); + } else { + $diff_id = $changeset->getDiffID(); + $lines = phutil_render_tag( + 'a', + array( + 'href' => '?id='.$diff_id.'#inline-'.$inline->getID(), + 'class' => 'num', + 'target' => '_blank', + ), + $lines." \xE2\x86\x97"); + $where = '(On Diff #'.$diff_id.')'; } $inline_content = $inline->getContent(); @@ -212,6 +264,7 @@ final class DifferentialRevisionCommentView extends AphrontView { $inline_render[] = ''. ''.$lines.''. + ''.$where.''. ''. '
'. $inline_content. diff --git a/src/applications/differential/view/revisioncommentlist/DifferentialRevisionCommentListView.php b/src/applications/differential/view/revisioncommentlist/DifferentialRevisionCommentListView.php index d03486e94f..7cd331ce06 100644 --- a/src/applications/differential/view/revisioncommentlist/DifferentialRevisionCommentListView.php +++ b/src/applications/differential/view/revisioncommentlist/DifferentialRevisionCommentListView.php @@ -1,7 +1,7 @@ comments = $comments; @@ -52,6 +53,12 @@ final class DifferentialRevisionCommentListView extends AphrontView { public function setTargetDiff(DifferentialDiff $target) { $this->target = $target; + return $this; + } + + public function setVersusDiffID($diff_vs) { + $this->versusDiffID = $diff_vs; + return $this; } public function render() { @@ -75,6 +82,7 @@ final class DifferentialRevisionCommentListView extends AphrontView { $view->setInlineComments(idx($inlines, $comment->getID(), array())); $view->setChangesets($this->changesets); $view->setTargetDiff($this->target); + $view->setVersusDiffID($this->versusDiffID); $view->setCommentNumber($num++); $html[] = $view->render(); diff --git a/webroot/rsrc/css/application/differential/revision-comment.css b/webroot/rsrc/css/application/differential/revision-comment.css index 65ed1f97e0..21612bc57b 100644 --- a/webroot/rsrc/css/application/differential/revision-comment.css +++ b/webroot/rsrc/css/application/differential/revision-comment.css @@ -191,6 +191,11 @@ width: 70px; /* Need lots of width for 23,950-23,951 */ } +.differential-inline-summary td.inline-which-diff { + padding: 0 8px 0 0; + color: #666666; +} + .differential-inline-summary td.inline-line-number a:hover { background: #3b5998; color: white;