From 4310c4ed5387d87c784ecace359c7495459a4e72 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Mar 2015 18:41:47 -0700 Subject: [PATCH] Track a "Done" state on inline comments Summary: Ref T1460. This just barely works, but throwing it up in case any of it sounds mechanically crazy before we build integrations/UI/etc. Specifically, these are the behaviors: - You can mark your own draft comments as "done" before you submit them. The intent is to let reviewers mark their stuff advisory/minor/not-important before they submit it, to hint to authors that they don't expect the feedback to necessarily be addressed (maybe it's a joke, maybe it's just discussion, maybe it's "consider.."). - You can mark others' published comments as "done" if you're the revision/commit author. The intent is to keep this lightweight by not requiring an audit trail of who marked what done when. If anyone could mark anything done, we'd have to have some way to show who marked stuff. - When you mark stuff done (or unmark it), it goes into a "draft" state, where you see the change but others don't see it yet. The intent is twofold: - Be consistent with how inlines work. - Allow us to publish a "epriestley updated this revision + epriestley marked 15 inlines as done" story later if we want. This seems more useful than publishing 15 "epriestley marked one thing as done" stories. - The actual bit where done-ness publishes isn't implemented. - UI is bare bones. - No integration with the rest of the UI yet. Test Plan: Clicked some checkboxes. Reviewers: chad, btrahan Reviewed By: btrahan Subscribers: paulshen, chasemp, epriestley Maniphest Tasks: T1460 Differential Revision: https://secure.phabricator.com/D12033 --- resources/celerity/map.php | 48 +++++++++---------- .../storage/PhabricatorAuditInlineComment.php | 9 ++++ .../DifferentialChangesetViewController.php | 29 +++++++---- ...ifferentialInlineCommentEditController.php | 40 ++++++++++++++++ .../parser/DifferentialChangesetParser.php | 13 ++++- .../DifferentialChangesetHTMLRenderer.php | 4 +- .../render/DifferentialChangesetRenderer.php | 12 +++++ .../storage/DifferentialInlineComment.php | 9 ++++ .../controller/DiffusionDiffController.php | 15 ++++-- .../DiffusionInlineCommentController.php | 25 ++++++++++ .../PhabricatorInlineCommentController.php | 29 ++++++++++- ...bricatorInlineCommentPreviewController.php | 19 ++++---- .../PhabricatorInlineCommentInterface.php | 8 ++++ .../view/PHUIDiffInlineCommentDetailView.php | 38 +++++++++++++++ .../differential/changeset-view.css | 11 +++++ .../DifferentialInlineCommentEditor.js | 27 +++++++++-- .../behavior-edit-inline-comments.js | 22 +++++++-- 17 files changed, 299 insertions(+), 59 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index b1a695166a..10afe8a170 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,8 +10,8 @@ return array( 'core.pkg.css' => '404f1f98', 'core.pkg.js' => '75599122', 'darkconsole.pkg.js' => '8ab24e01', - 'differential.pkg.css' => '1940be3f', - 'differential.pkg.js' => '85fd84c6', + 'differential.pkg.css' => '686ac058', + 'differential.pkg.js' => 'e1dd7634', 'diffusion.pkg.css' => '591664fa', 'diffusion.pkg.js' => 'bfc0737b', 'maniphest.pkg.css' => '68d4dd3d', @@ -55,7 +55,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '17937d22', 'rsrc/css/application/diff/inline-comment-summary.css' => 'eb5f8e8c', 'rsrc/css/application/differential/add-comment.css' => 'c478bcaa', - 'rsrc/css/application/differential/changeset-view.css' => '6a8b172a', + 'rsrc/css/application/differential/changeset-view.css' => '79c27a4c', 'rsrc/css/application/differential/core.css' => '7ac3cabc', 'rsrc/css/application/differential/results-table.css' => '181aa9d9', 'rsrc/css/application/differential/revision-comment.css' => '48186045', @@ -364,13 +364,13 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63', 'rsrc/js/application/differential/ChangesetViewManager.js' => '88be0133', - 'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => '0286a1db', + 'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => 'f2431bc1', 'rsrc/js/application/differential/behavior-add-reviewers-and-ccs.js' => 'e10f8e18', 'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d', 'rsrc/js/application/differential/behavior-comment-preview.js' => '8e1389b5', 'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1', 'rsrc/js/application/differential/behavior-dropdown-menus.js' => '2035b9cb', - 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => 'a48aa699', + 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => 'e723c323', 'rsrc/js/application/differential/behavior-keyboard-nav.js' => '2c426492', 'rsrc/js/application/differential/behavior-populate.js' => '8694b1df', 'rsrc/js/application/differential/behavior-show-field-details.js' => 'bba9eedf', @@ -520,9 +520,9 @@ return array( 'conpherence-thread-manager' => '24561adb', 'conpherence-update-css' => '1099a660', 'conpherence-widget-pane-css' => '9efbfed0', - 'differential-changeset-view-css' => '6a8b172a', + 'differential-changeset-view-css' => '79c27a4c', 'differential-core-view-css' => '7ac3cabc', - 'differential-inline-comment-editor' => '0286a1db', + 'differential-inline-comment-editor' => 'f2431bc1', 'differential-results-table-css' => '181aa9d9', 'differential-revision-add-comment-css' => 'c478bcaa', 'differential-revision-comment-css' => '48186045', @@ -570,7 +570,7 @@ return array( 'javelin-behavior-differential-comment-jump' => '4fdb476d', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', 'javelin-behavior-differential-dropdown-menus' => '2035b9cb', - 'javelin-behavior-differential-edit-inline-comments' => 'a48aa699', + 'javelin-behavior-differential-edit-inline-comments' => 'e723c323', 'javelin-behavior-differential-feedback-preview' => '8e1389b5', 'javelin-behavior-differential-keyboard-navigation' => '2c426492', 'javelin-behavior-differential-populate' => '8694b1df', @@ -842,14 +842,6 @@ return array( 'javelin-behavior-device', 'phabricator-title', ), - '0286a1db' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-request', - 'javelin-workflow', - ), '029a133d' => array( 'aphront-dialog-view-css', ), @@ -1616,14 +1608,6 @@ return array( 'javelin-vector', 'javelin-magical-init', ), - 'a48aa699' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-util', - 'javelin-vector', - 'differential-inline-comment-editor', - ), 'a80d0378' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1886,6 +1870,14 @@ return array( 'javelin-behavior-device', 'phabricator-keyboard-shortcut', ), + 'e723c323' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-util', + 'javelin-vector', + 'differential-inline-comment-editor', + ), 'e9581f08' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1913,6 +1905,14 @@ return array( 'javelin-install', 'javelin-util', ), + 'f2431bc1' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-request', + 'javelin-workflow', + ), 'f24f3253' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/audit/storage/PhabricatorAuditInlineComment.php b/src/applications/audit/storage/PhabricatorAuditInlineComment.php index 5483c58494..1e1b47f73e 100644 --- a/src/applications/audit/storage/PhabricatorAuditInlineComment.php +++ b/src/applications/audit/storage/PhabricatorAuditInlineComment.php @@ -299,6 +299,15 @@ final class PhabricatorAuditInlineComment return $this->proxy->getIsDeleted(); } + public function setFixedState($state) { + $this->proxy->setFixedState($state); + return $this; + } + + public function getFixedState() { + return $this->proxy->getFixedState(); + } + /* -( PhabricatorMarkupInterface Implementation )-------------------------- */ diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index b13cee5d28..f8ca82b111 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -8,8 +8,9 @@ final class DifferentialChangesetViewController extends DifferentialController { public function processRequest() { $request = $this->getRequest(); + $viewer = $this->getViewer(); - $author_phid = $request->getUser()->getPHID(); + $author_phid = $viewer->getPHID(); $rendering_reference = $request->getStr('ref'); $parts = explode('/', $rendering_reference); @@ -29,7 +30,7 @@ final class DifferentialChangesetViewController extends DifferentialController { } $changesets = id(new DifferentialChangesetQuery()) - ->setViewer($request->getUser()) + ->setViewer($viewer) ->withIDs($load_ids) ->needHunks(true) ->execute(); @@ -191,7 +192,7 @@ final class DifferentialChangesetViewController extends DifferentialController { $parser->setHandles($handles); $engine = new PhabricatorMarkupEngine(); - $engine->setViewer($request->getUser()); + $engine->setViewer($viewer); foreach ($inlines as $inline) { $engine->addObject( @@ -201,10 +202,25 @@ final class DifferentialChangesetViewController extends DifferentialController { $engine->process(); + $diff = $changeset->getDiff(); + $revision_id = $diff->getRevisionID(); + + $can_mark = false; + if ($revision_id) { + $revision = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->withIDs(array($revision_id)) + ->executeOne(); + if ($revision) { + $can_mark = ($revision->getAuthorPHID() == $viewer->getPHID()); + } + } + $parser - ->setUser($request->getUser()) + ->setUser($viewer) ->setMarkupEngine($engine) ->setShowEditAndReplyLinks(true) + ->setCanMarkDone($can_mark) ->setRange($range_s, $range_e) ->setMask($mask); @@ -221,8 +237,6 @@ final class DifferentialChangesetViewController extends DifferentialController { ->setUndoTemplates($parser->getRenderer()->renderUndoTemplates()); } - $diff = $changeset->getDiff(); - $detail = id(new DifferentialChangesetListView()) ->setUser($this->getViewer()) ->setChangesets(array($changeset)) @@ -233,7 +247,6 @@ final class DifferentialChangesetViewController extends DifferentialController { ->setTitle(pht('Standalone View')) ->setParser($parser); - $revision_id = $diff->getRevisionID(); if ($revision_id) { $detail->setInlineCommentControllerURI( '/differential/comment/inline/edit/'.$revision_id.'/'); @@ -280,7 +293,7 @@ final class DifferentialChangesetViewController extends DifferentialController { DifferentialChangeset $changeset, $is_new) { - $viewer = $this->getRequest()->getUser(); + $viewer = $this->getViewer(); if ($is_new) { $key = 'raw:new:phid'; diff --git a/src/applications/differential/controller/DifferentialInlineCommentEditController.php b/src/applications/differential/controller/DifferentialInlineCommentEditController.php index 89d5b5e428..4c411f9f84 100644 --- a/src/applications/differential/controller/DifferentialInlineCommentEditController.php +++ b/src/applications/differential/controller/DifferentialInlineCommentEditController.php @@ -57,6 +57,46 @@ final class DifferentialInlineCommentEditController return $inline; } + protected function loadCommentForDone($id) { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $inline = $this->loadComment($id); + if (!$inline) { + throw new Exception(pht('Unable to load inline "%d".', $id)); + } + + $changeset = id(new DifferentialChangesetQuery()) + ->setViewer($viewer) + ->withIDs(array($inline->getChangesetID())) + ->executeOne(); + if (!$changeset) { + throw new Exception(pht('Unable to load changeset.')); + } + + $diff = id(new DifferentialDiffQuery()) + ->setViewer($viewer) + ->withIDs(array($changeset->getDiffID())) + ->executeOne(); + if (!$diff) { + throw new Exception(pht('Unable to load diff.')); + } + + $revision = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->withIDs(array($diff->getRevisionID())) + ->executeOne(); + if (!$revision) { + throw new Exception(pht('Unable to load revision.')); + } + + if ($revision->getAuthorPHID() !== $viewer->getPHID()) { + throw new Exception(pht('You are not the revision owner.')); + } + + return $inline; + } + private function canEditInlineComment( PhabricatorUser $user, DifferentialInlineComment $inline) { diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 51edd01336..3c78415152 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -44,6 +44,7 @@ final class DifferentialChangesetParser { private $characterEncoding; private $highlightAs; private $showEditAndReplyLinks = true; + private $canMarkDone; private $rangeStart; private $rangeEnd; @@ -111,6 +112,15 @@ final class DifferentialChangesetParser { return $this->disableCache; } + public function setCanMarkDone($can_mark_done) { + $this->canMarkDone = $can_mark_done; + return $this; + } + + public function getCanMarkDone() { + return $this->canMarkDone; + } + public static function getDefaultRendererForViewer(PhabricatorUser $viewer) { $prefs = $viewer->loadPreferences(); $pref_unified = PhabricatorUserPreferences::PREFERENCE_DIFF_UNIFIED; @@ -819,7 +829,8 @@ final class DifferentialChangesetParser { ->setOldLines($this->old) ->setNewLines($this->new) ->setOriginalCharacterEncoding($encoding) - ->setShowEditAndReplyLinks($this->getShowEditAndReplyLinks()); + ->setShowEditAndReplyLinks($this->getShowEditAndReplyLinks()) + ->setCanMarkDone($this->getCanMarkDone()); $shield = null; if ($this->isTopLevel && !$this->comments) { diff --git a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php index 81cbfde914..3eef085555 100644 --- a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php @@ -435,6 +435,7 @@ abstract class DifferentialChangesetHTMLRenderer ($comment->isDraft()) && $this->getShowEditAndReplyLinks(); $allow_reply = (bool)$user && $this->getShowEditAndReplyLinks(); + $allow_done = !$comment->isDraft() && $this->getCanMarkDone(); return id(new PHUIDiffInlineCommentDetailView()) ->setInlineComment($comment) @@ -442,7 +443,8 @@ abstract class DifferentialChangesetHTMLRenderer ->setHandles($this->getHandles()) ->setMarkupEngine($this->getMarkupEngine()) ->setEditable($edit) - ->setAllowReply($allow_reply); + ->setAllowReply($allow_reply) + ->setCanMarkDone($allow_done); } diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index 8edc301b79..7fa5b37f67 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -30,6 +30,7 @@ abstract class DifferentialChangesetRenderer { private $depths; private $originalCharacterEncoding; private $showEditAndReplyLinks; + private $canMarkDone; private $oldFile = false; private $newFile = false; @@ -289,6 +290,7 @@ abstract class DifferentialChangesetRenderer { $this->renderPropertyChangeHeader = $should_render; return $this; } + private function shouldRenderPropertyChangeHeader() { return $this->renderPropertyChangeHeader; } @@ -297,10 +299,20 @@ abstract class DifferentialChangesetRenderer { $this->isTopLevel = $is; return $this; } + private function getIsTopLevel() { return $this->isTopLevel; } + public function setCanMarkDone($can_mark_done) { + $this->canMarkDone = $can_mark_done; + return $this; + } + + public function getCanMarkDone() { + return $this->canMarkDone; + } + final public function renderChangesetTable($content) { $props = null; if ($this->shouldRenderPropertyChangeHeader()) { diff --git a/src/applications/differential/storage/DifferentialInlineComment.php b/src/applications/differential/storage/DifferentialInlineComment.php index 711e50020f..52ab4a2944 100644 --- a/src/applications/differential/storage/DifferentialInlineComment.php +++ b/src/applications/differential/storage/DifferentialInlineComment.php @@ -216,6 +216,15 @@ final class DifferentialInlineComment return $this->proxy->getIsDeleted(); } + public function setFixedState($state) { + $this->proxy->setFixedState($state); + return $this; + } + + public function getFixedState() { + return $this->proxy->getFixedState(); + } + /* -( PhabricatorMarkupInterface Implementation )-------------------------- */ diff --git a/src/applications/diffusion/controller/DiffusionDiffController.php b/src/applications/diffusion/controller/DiffusionDiffController.php index 52ac964e0a..502c85c098 100644 --- a/src/applications/diffusion/controller/DiffusionDiffController.php +++ b/src/applications/diffusion/controller/DiffusionDiffController.php @@ -26,7 +26,7 @@ final class DiffusionDiffController extends DiffusionController { $this->setDiffusionRequest($drequest); $drequest = $this->getDiffusionRequest(); - $user = $request->getUser(); + $viewer = $this->getViewer(); if (!$request->isAjax()) { @@ -70,7 +70,7 @@ final class DiffusionDiffController extends DiffusionController { } $parser = new DifferentialChangesetParser(); - $parser->setUser($user); + $parser->setUser($viewer); $parser->setChangeset($changeset); $parser->setRenderingReference($drequest->generateURI( array( @@ -84,19 +84,24 @@ final class DiffusionDiffController extends DiffusionController { $parser->setCoverage($coverage); } + $commit = $drequest->loadCommit(); + $pquery = new DiffusionPathIDQuery(array($changeset->getFilename())); $ids = $pquery->loadPathIDs(); $path_id = $ids[$changeset->getFilename()]; $parser->setLeftSideCommentMapping($path_id, false); $parser->setRightSideCommentMapping($path_id, true); + $parser->setCanMarkDone( + ($commit->getAuthorPHID()) && + ($viewer->getPHID() == $commit->getAuthorPHID())); $parser->setWhitespaceMode( DifferentialChangesetParser::WHITESPACE_SHOW_ALL); $inlines = PhabricatorAuditInlineComment::loadDraftAndPublishedComments( - $user, - $drequest->loadCommit()->getPHID(), + $viewer, + $commit->getPHID(), $path_id); if ($inlines) { @@ -110,7 +115,7 @@ final class DiffusionDiffController extends DiffusionController { } $engine = new PhabricatorMarkupEngine(); - $engine->setViewer($user); + $engine->setViewer($viewer); foreach ($inlines as $inline) { $engine->addObject( diff --git a/src/applications/diffusion/controller/DiffusionInlineCommentController.php b/src/applications/diffusion/controller/DiffusionInlineCommentController.php index 62e70dde6c..9cdb3fc349 100644 --- a/src/applications/diffusion/controller/DiffusionInlineCommentController.php +++ b/src/applications/diffusion/controller/DiffusionInlineCommentController.php @@ -58,6 +58,31 @@ final class DiffusionInlineCommentController return $inline; } + protected function loadCommentForDone($id) { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $inline = $this->loadComment($id); + if (!$inline) { + throw new Exception(pht('Failed to load comment "%d".', $id)); + } + + $commit = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withPHIDs(array($inline->getCommitPHID())) + ->exeucteOne(); + if (!$commit) { + throw new Exception(pht('Failed to load commit.')); + } + + if ((!$commit->getAuthorPHID()) || + ($commit->getAuthorPHID() != $viewer->getPHID())) { + throw new Exception(pht('You can not mark this comment as complete.')); + } + + return $inline; + } + private function canEditInlineComment( PhabricatorUser $user, PhabricatorAuditInlineComment $inline) { diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index d6f6db44b6..3176a72c86 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -6,6 +6,7 @@ abstract class PhabricatorInlineCommentController abstract protected function createComment(); abstract protected function loadComment($id); abstract protected function loadCommentForEdit($id); + abstract protected function loadCommentForDone($id); abstract protected function loadCommentByPHID($phid); abstract protected function deleteComment( PhabricatorInlineCommentInterface $inline); @@ -81,6 +82,31 @@ abstract class PhabricatorInlineCommentController $op = $this->getOperation(); switch ($op) { + case 'done': + if (!$request->validateCSRF()) { + return new Aphront404Response(); + } + $inline = $this->loadCommentForDone($this->getCommentID()); + + switch ($inline->getFixedState()) { + case PhabricatorInlineCommentInterface::STATE_DRAFT: + $next_state = PhabricatorInlineCommentInterface::STATE_UNDONE; + break; + case PhabricatorInlineCommentInterface::STATE_UNDRAFT: + $next_state = PhabricatorInlineCommentInterface::STATE_DONE; + break; + case PhabricatorInlineCommentInterface::STATE_DONE: + $next_state = PhabricatorInlineCommentInterface::STATE_UNDRAFT; + break; + default: + case PhabricatorInlineCommentInterface::STATE_UNDONE: + $next_state = PhabricatorInlineCommentInterface::STATE_DRAFT; + break; + } + + $inline->setFixedState($next_state)->save(); + + return $this->buildEmptyResponse(); case 'delete': case 'undelete': case 'refdelete': @@ -286,7 +312,8 @@ abstract class PhabricatorInlineCommentController ->setIsOnRight($on_right) ->setMarkupEngine($engine) ->setHandles($handles) - ->setEditable(true); + ->setEditable(true) + ->setCanMarkDone(false); $view = $this->buildScaffoldForView($view); diff --git a/src/infrastructure/diff/PhabricatorInlineCommentPreviewController.php b/src/infrastructure/diff/PhabricatorInlineCommentPreviewController.php index 067e90cff9..21f8a1daa1 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentPreviewController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentPreviewController.php @@ -7,13 +7,13 @@ abstract class PhabricatorInlineCommentPreviewController public function processRequest() { $request = $this->getRequest(); - $user = $request->getUser(); + $viewer = $request->getUser(); $inlines = $this->loadInlineComments(); assert_instances_of($inlines, 'PhabricatorInlineCommentInterface'); $engine = new PhabricatorMarkupEngine(); - $engine->setViewer($user); + $engine->setViewer($viewer); foreach ($inlines as $inline) { $engine->addObject( $inline, @@ -21,17 +21,18 @@ abstract class PhabricatorInlineCommentPreviewController } $engine->process(); - $phids = array($user->getPHID()); + $phids = array($viewer->getPHID()); $handles = $this->loadViewerHandles($phids); $views = array(); foreach ($inlines as $inline) { - $view = new PHUIDiffInlineCommentDetailView(); - $view->setInlineComment($inline); - $view->setMarkupEngine($engine); - $view->setHandles($handles); - $view->setEditable(false); - $view->setPreview(true); + $view = id(new PHUIDiffInlineCommentDetailView()) + ->setInlineComment($inline) + ->setMarkupEngine($engine) + ->setHandles($handles) + ->setEditable(false) + ->setPreview(true) + ->setCanMarkDone(false); $views[] = $view->render(); } $views = phutil_implode_html("\n", $views); diff --git a/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php b/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php index ba47295cf6..d123f82c51 100644 --- a/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php +++ b/src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php @@ -7,6 +7,11 @@ interface PhabricatorInlineCommentInterface extends PhabricatorMarkupInterface { const MARKUP_FIELD_BODY = 'markup:body'; + const STATE_UNDONE = 'undone'; + const STATE_DRAFT = 'draft'; + const STATE_UNDRAFT = 'undraft'; + const STATE_DONE = 'done'; + public function setChangesetID($id); public function getChangesetID(); @@ -28,6 +33,9 @@ interface PhabricatorInlineCommentInterface extends PhabricatorMarkupInterface { public function setIsDeleted($deleted); public function getIsDeleted(); + public function setFixedState($state); + public function getFixedState(); + public function setContent($content); public function getContent(); diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php index 1a11fd295c..962d2c48bd 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -10,6 +10,7 @@ final class PHUIDiffInlineCommentDetailView private $preview; private $allowReply; private $renderer; + private $canMarkDone; public function setInlineComment(PhabricatorInlineCommentInterface $comment) { $this->inlineComment = $comment; @@ -51,6 +52,15 @@ final class PHUIDiffInlineCommentDetailView return $this->renderer; } + public function setCanMarkDone($can_mark_done) { + $this->canMarkDone = $can_mark_done; + return $this; + } + + public function getCanMarkDone() { + return $this->canMarkDone; + } + public function render() { $inline = $this->inlineComment; @@ -186,6 +196,34 @@ final class PHUIDiffInlineCommentDetailView pht('Delete')); } + if (!$is_synthetic) { + switch ($inline->getFixedState()) { + case PhabricatorInlineCommentInterface::STATE_DRAFT: + $is_done = ($this->getCanMarkDone()); + break; + case PhabricatorInlineCommentInterface::STATE_UNDRAFT: + $is_done = !($this->getCanMarkDone()); + break; + case PhabricatorInlineCommentInterface::STATE_DONE: + $is_done = true; + break; + default: + case PhabricatorInlineCommentInterface::STATE_UNDONE: + $is_done = false; + break; + } + + $links[] = javelin_tag( + 'input', + array( + 'type' => 'checkbox', + 'checked' => ($is_done ? 'checked' : null), + 'disabled' => ($this->getCanMarkDone() ? null : 'disabled'), + 'class' => 'differential-inline-done', + 'sigil' => 'differential-inline-done', + )); + } + if ($links) { $links = phutil_tag( 'span', diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index c77e807bc3..5678168076 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -336,6 +336,17 @@ td.cov-I { font-style: normal; } +input.differential-inline-done[type="checkbox"] { + margin: 0; + display: inline; + cursor: pointer; +} + +input.differential-inline-done[disabled="disabled"] { + cursor: auto; +} + + .differential-inline-comment-edit-body .aphront-form-input { margin: 0; width: 100%; diff --git a/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js b/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js index 6055c8ff52..d0c60f62fb 100644 --- a/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js +++ b/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js @@ -256,7 +256,6 @@ JX.install('DifferentialInlineCommentEditor', { var data = this._buildRequestData(); var op = this.getOperation(); - if (op == 'delete' || op == 'refdelete' || op == 'undelete') { this._setRowState('loading'); @@ -285,14 +284,32 @@ JX.install('DifferentialInlineCommentEditor', { deleteByID: function(id) { var data = { op: 'refdelete', - changesetID: id + id: id }; new JX.Workflow(this._uri, data) - .setHandler(function() { - JX.Stratcom.invoke('differential-inline-comment-update'); - }) + .setHandler(JX.bind(this, function() { + this._didUpdate(); + })) .start(); + }, + + toggleCheckbox: function(id, checkbox) { + var data = { + op: 'done', + id: id + }; + + new JX.Workflow(this._uri, data) + .setHandler(JX.bind(this, function() { + checkbox.checked = !checkbox.checked; + this._didUpdate(); + })) + .start(); + }, + + _didUpdate: function() { + JX.Stratcom.invoke('differential-inline-comment-update'); } }, diff --git a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js index 0c27a7472a..0a572a999a 100644 --- a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js +++ b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js @@ -326,12 +326,24 @@ JX.behavior('differential-edit-inline-comments', function(config) { } if (!found) { - new JX.DifferentialInlineCommentEditor(config.uri) - .deleteByID(data.id); - return; + switch (op) { + case 'delete': + new JX.DifferentialInlineCommentEditor(config.uri) + .deleteByID(data.id); + return; + } } - op = 'refdelete'; + if (op == 'delete') { + op = 'refdelete'; + } + } + + if (op == 'done') { + var checkbox = JX.DOM.find(node, 'input', 'differential-inline-done'); + new JX.DifferentialInlineCommentEditor(config.uri) + .toggleCheckbox(data.id, checkbox); + return; } var original = data.original; @@ -368,7 +380,7 @@ JX.behavior('differential-edit-inline-comments', function(config) { set_link_state(true); }; - for (var op in {'edit' : 1, 'delete' : 1, 'reply' : 1}) { + for (var op in {'edit': 1, 'delete': 1, 'reply': 1, 'done': 1}) { JX.Stratcom.listen( 'click', ['differential-inline-comment', 'differential-inline-' + op],