Skip to content

Commit fe501bd

Browse files
author
epriestley
committed
Save drafts for inline comments currently being edited
Summary: Ref T13513. As users type text into inline comments, save the comment state as a draft on the server. This has some rough edges, particularly around previews, but mostly works. See T13513 for notes. Test Plan: Started an inline, typed some text, waited a second, reloaded the page, saw an editing inline with the saved text. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21216
1 parent 27b7ba8 commit fe501bd

File tree

15 files changed

+283
-51
lines changed

15 files changed

+283
-51
lines changed

‎resources/celerity/map.php‎

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
'core.pkg.js' => '632fb8f5',
1414
'dark-console.pkg.js' => '187792c2',
1515
'differential.pkg.css' => '2d70b7b9',
16-
'differential.pkg.js' => '4287e51f',
16+
'differential.pkg.js' => '4d375e61',
1717
'diffusion.pkg.css' => '42c75c37',
1818
'diffusion.pkg.js' => 'a98c0bf7',
1919
'maniphest.pkg.css' => '35995d6d',
@@ -380,8 +380,8 @@
380380
'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
381381
'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
382382
'rsrc/js/application/diff/DiffChangeset.js' => 'a49dc31e',
383-
'rsrc/js/application/diff/DiffChangesetList.js' => '10726e6a',
384-
'rsrc/js/application/diff/DiffInline.js' => '7f804f2b',
383+
'rsrc/js/application/diff/DiffChangesetList.js' => '6992b85c',
384+
'rsrc/js/application/diff/DiffInline.js' => 'a39fd98e',
385385
'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
386386
'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
387387
'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17',
@@ -462,7 +462,7 @@
462462
'rsrc/js/core/MultirowRowManager.js' => '5b54c823',
463463
'rsrc/js/core/Notification.js' => 'a9b91e3f',
464464
'rsrc/js/core/Prefab.js' => '5793d835',
465-
'rsrc/js/core/ShapedRequest.js' => 'abf88db8',
465+
'rsrc/js/core/ShapedRequest.js' => '995f5102',
466466
'rsrc/js/core/TextAreaUtils.js' => 'f340a484',
467467
'rsrc/js/core/Title.js' => '43bc9360',
468468
'rsrc/js/core/ToolTip.js' => '83754533',
@@ -777,8 +777,8 @@
777777
'phabricator-darkmessage' => '26cd4b73',
778778
'phabricator-dashboard-css' => '5a205b9d',
779779
'phabricator-diff-changeset' => 'a49dc31e',
780-
'phabricator-diff-changeset-list' => '10726e6a',
781-
'phabricator-diff-inline' => '7f804f2b',
780+
'phabricator-diff-changeset-list' => '6992b85c',
781+
'phabricator-diff-inline' => 'a39fd98e',
782782
'phabricator-diff-path-view' => '8207abf9',
783783
'phabricator-diff-tree-view' => '5d83623b',
784784
'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -800,7 +800,7 @@
800800
'phabricator-prefab' => '5793d835',
801801
'phabricator-remarkup-css' => 'c286eaef',
802802
'phabricator-search-results-css' => '9ea70ace',
803-
'phabricator-shaped-request' => 'abf88db8',
803+
'phabricator-shaped-request' => '995f5102',
804804
'phabricator-slowvote-css' => '1694baed',
805805
'phabricator-source-code-view-css' => '03d7ac28',
806806
'phabricator-standard-page-view' => 'a374f94c',
@@ -1022,11 +1022,6 @@
10221022
'javelin-workflow',
10231023
'phuix-icon-view',
10241024
),
1025-
'10726e6a' => array(
1026-
'javelin-install',
1027-
'phuix-button-view',
1028-
'phabricator-diff-tree-view',
1029-
),
10301025
'111bfd2d' => array(
10311026
'javelin-install',
10321027
),
@@ -1519,6 +1514,11 @@
15191514
'javelin-install',
15201515
'javelin-dom',
15211516
),
1517+
'6992b85c' => array(
1518+
'javelin-install',
1519+
'phuix-button-view',
1520+
'phabricator-diff-tree-view',
1521+
),
15221522
'6a1583a8' => array(
15231523
'javelin-behavior',
15241524
'javelin-history',
@@ -1626,9 +1626,6 @@
16261626
'javelin-install',
16271627
'javelin-dom',
16281628
),
1629-
'7f804f2b' => array(
1630-
'javelin-dom',
1631-
),
16321629
'80bff3af' => array(
16331630
'javelin-install',
16341631
'javelin-typeahead-source',
@@ -1797,6 +1794,12 @@
17971794
'javelin-request',
17981795
'javelin-util',
17991796
),
1797+
'995f5102' => array(
1798+
'javelin-install',
1799+
'javelin-util',
1800+
'javelin-request',
1801+
'javelin-router',
1802+
),
18001803
'9aae2b66' => array(
18011804
'javelin-install',
18021805
'javelin-util',
@@ -1838,6 +1841,9 @@
18381841
'javelin-workflow',
18391842
'phabricator-draggable-list',
18401843
),
1844+
'a39fd98e' => array(
1845+
'javelin-dom',
1846+
),
18411847
'a4356cde' => array(
18421848
'javelin-install',
18431849
'javelin-dom',
@@ -1916,12 +1922,6 @@
19161922
'javelin-dom',
19171923
'phabricator-notification',
19181924
),
1919-
'abf88db8' => array(
1920-
'javelin-install',
1921-
'javelin-util',
1922-
'javelin-request',
1923-
'javelin-router',
1924-
),
19251925
'ad258e28' => array(
19261926
'javelin-behavior',
19271927
'javelin-dom',

‎src/applications/audit/editor/PhabricatorAuditEditor.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,13 @@ protected function applyCustomInternalTransaction(
105105

106106
switch ($xaction->getTransactionType()) {
107107
case PhabricatorAuditActionConstants::INLINE:
108-
$xaction->getComment()->setAttribute('editing', false);
108+
$comment = $xaction->getComment();
109+
110+
$comment->setAttribute('editing', false);
111+
112+
PhabricatorVersionedDraft::purgeDrafts(
113+
$comment->getPHID(),
114+
$this->getActingAsPHID());
109115
return;
110116
case PhabricatorAuditTransaction::TYPE_COMMIT:
111117
return;

‎src/applications/audit/storage/PhabricatorAuditInlineComment.php‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -111,13 +111,6 @@ public static function loadDraftAndPublishedComments(
111111
$viewer->getPHID());
112112
}
113113

114-
foreach ($inlines as $key => $inline) {
115-
$is_draft = !$inline->getTransactionPHID();
116-
if ($is_draft && $inline->isEmptyInlineComment()) {
117-
unset($inlines[$key]);
118-
}
119-
}
120-
121114
return self::buildProxies($inlines);
122115
}
123116

‎src/applications/differential/controller/DifferentialChangesetViewController.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,6 @@ public function handleRequest(AphrontRequest $request) {
197197
$query = id(new DifferentialInlineCommentQuery())
198198
->setViewer($viewer)
199199
->needHidden(true)
200-
->withEmptyInlineComments(false)
201200
->withRevisionPHIDs(array($revision->getPHID()));
202201
$inlines = $query->execute();
203202
$inlines = $query->adjustInlinesForChangesets(

‎src/applications/differential/editor/DifferentialTransactionEditor.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,13 @@ protected function applyCustomInternalTransaction(
112112

113113
switch ($xaction->getTransactionType()) {
114114
case DifferentialTransaction::TYPE_INLINE:
115-
$xaction->getComment()->setAttribute('editing', false);
115+
$comment = $xaction->getComment();
116+
117+
$comment->setAttribute('editing', false);
118+
119+
PhabricatorVersionedDraft::purgeDrafts(
120+
$comment->getPHID(),
121+
$this->getActingAsPHID());
116122
return;
117123
}
118124

‎src/applications/differential/parser/DifferentialChangesetParser.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -752,6 +752,8 @@ public function render(
752752
$range_len = null,
753753
$mask_force = array()) {
754754

755+
$viewer = $this->getViewer();
756+
755757
$renderer = $this->getRenderer();
756758
if (!$renderer) {
757759
$renderer = $this->newRenderer();
@@ -853,6 +855,16 @@ public function render(
853855

854856
$has_document_engine = ($engine_blocks !== null);
855857

858+
// Remove empty comments that don't have any unsaved draft data.
859+
PhabricatorInlineComment::loadAndAttachVersionedDrafts(
860+
$viewer,
861+
$this->comments);
862+
foreach ($this->comments as $key => $comment) {
863+
if ($comment->isVoidComment($viewer)) {
864+
unset($this->comments[$key]);
865+
}
866+
}
867+
856868
// See T13515. Sometimes, we collapse file content by default: for
857869
// example, if the file is marked as containing generated code.
858870

@@ -1050,6 +1062,7 @@ public function render(
10501062
}
10511063
}
10521064
}
1065+
10531066
$renderer
10541067
->setOldComments($old_comments)
10551068
->setNewComments($new_comments);

‎src/applications/differential/query/DifferentialInlineCommentQuery.php‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ final class DifferentialInlineCommentQuery
1616
private $revisionPHIDs;
1717
private $deletedDrafts;
1818
private $needHidden;
19-
private $withEmpty;
2019

2120
public function setViewer(PhabricatorUser $viewer) {
2221
$this->viewer = $viewer;
@@ -62,11 +61,6 @@ public function needHidden($need) {
6261
return $this;
6362
}
6463

65-
public function withEmptyInlineComments($empty) {
66-
$this->withEmpty = $empty;
67-
return $this;
68-
}
69-
7064
public function execute() {
7165
$table = new DifferentialTransactionComment();
7266
$conn_r = $table->establishConnection('r');
@@ -80,15 +74,6 @@ public function execute() {
8074

8175
$comments = $table->loadAllFromArray($data);
8276

83-
if ($this->withEmpty !== null) {
84-
$want_empty = (bool)$this->withEmpty;
85-
foreach ($comments as $key => $value) {
86-
if ($value->isEmptyInlineComment() !== $want_empty) {
87-
unset($comments[$key]);
88-
}
89-
}
90-
}
91-
9277
if ($this->needHidden) {
9378
$viewer_phid = $this->getViewer()->getPHID();
9479
if ($viewer_phid && $comments) {

‎src/applications/differential/query/DifferentialTransactionQuery.php‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,24 @@ public static function loadUnsubmittedInlineComments(
2020
->needReplyToComments(true)
2121
->execute();
2222

23-
// Don't count empty inlines when considering draft state.
2423
foreach ($inlines as $key => $inline) {
25-
if ($inline->isEmptyInlineComment()) {
24+
$inlines[$key] = DifferentialInlineComment::newFromModernComment(
25+
$inline);
26+
}
27+
28+
PhabricatorInlineComment::loadAndAttachVersionedDrafts(
29+
$viewer,
30+
$inlines);
31+
32+
// Don't count void inlines when considering draft state.
33+
foreach ($inlines as $key => $inline) {
34+
if ($inline->isVoidComment($viewer)) {
2635
unset($inlines[$key]);
2736
}
2837
}
2938

39+
$inlines = mpull($inlines, 'getStorageObject');
40+
3041
return $inlines;
3142
}
3243

‎src/applications/draft/storage/PhabricatorVersionedDraft.php‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,23 @@ public function getProperty($key, $default = null) {
3535
return idx($this->properties, $key, $default);
3636
}
3737

38+
public static function loadDrafts(
39+
array $object_phids,
40+
$viewer_phid) {
41+
42+
$rows = id(new self())->loadAllWhere(
43+
'objectPHID IN (%Ls) AND authorPHID = %s ORDER BY version ASC',
44+
$object_phids,
45+
$viewer_phid);
46+
47+
$map = array();
48+
foreach ($rows as $row) {
49+
$map[$row->getObjectPHID()] = $row;
50+
}
51+
52+
return $map;
53+
}
54+
3855
public static function loadDraft(
3956
$object_phid,
4057
$viewer_phid) {

‎src/infrastructure/diff/PhabricatorInlineCommentController.php‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,11 +192,15 @@ public function processRequest() {
192192
->setIsEditing(false);
193193

194194
$this->saveComment($inline);
195+
$this->purgeVersionedDrafts($inline);
196+
195197
return $this->buildRenderedCommentResponse(
196198
$inline,
197199
$this->getIsOnRight());
198200
} else {
199201
$this->deleteComment($inline);
202+
$this->purgeVersionedDrafts($inline);
203+
200204
return $this->buildEmptyResponse();
201205
}
202206
} else {
@@ -235,6 +239,23 @@ public function processRequest() {
235239
$this->saveComment($inline);
236240
}
237241

242+
$this->purgeVersionedDrafts($inline);
243+
244+
return $this->buildEmptyResponse();
245+
case 'draft':
246+
$inline = $this->loadCommentForEdit($this->getCommentID());
247+
248+
$versioned_draft = PhabricatorVersionedDraft::loadOrCreateDraft(
249+
$inline->getPHID(),
250+
$viewer->getPHID(),
251+
$inline->getID());
252+
253+
$text = $this->getCommentText();
254+
255+
$versioned_draft
256+
->setProperty('inline.text', $text)
257+
->save();
258+
238259
return $this->buildEmptyResponse();
239260
case 'new':
240261
case 'reply':
@@ -405,4 +426,13 @@ private function newInlineResponse(
405426
->setContent($response);
406427
}
407428

429+
private function purgeVersionedDrafts(
430+
PhabricatorInlineComment $inline) {
431+
$viewer = $this->getViewer();
432+
PhabricatorVersionedDraft::purgeDrafts(
433+
$inline->getPHID(),
434+
$viewer->getPHID());
435+
}
436+
437+
408438
}

0 commit comments

Comments
 (0)