@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Formally track "initial", "committed", and "active" states for inline comments
Summary:
Ref T13559. Various client decisions depend on the "initial" or "committed" states of inline comments. Previously, these were informally constructed from "mostly similar" available values, or glossed over in some cases.
On the server, save the initial state when creating a comment. Save the committed state when applying a "save" operation. Send all three states to the client.
On the client, load and track all three states explicitly.
Test Plan: Created inlines, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21653
+158
-71
+7
-7
resources/celerity/map.php
+7
-7
resources/celerity/map.php
···
13
13
'core.pkg.js' => '68f29322',
14
14
'dark-console.pkg.js' => '187792c2',
15
15
'differential.pkg.css' => 'ffb69e3d',
16
-
'differential.pkg.js' => 'fbde899f',
16
+
'differential.pkg.js' => '59453886',
17
17
'diffusion.pkg.css' => '42c75c37',
18
18
'diffusion.pkg.js' => '78c9885d',
19
19
'maniphest.pkg.css' => '35995d6d',
···
385
385
'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
386
386
'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75',
387
387
'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
388
-
'rsrc/js/application/diff/DiffInline.js' => '62fff8eb',
388
+
'rsrc/js/application/diff/DiffInline.js' => '26664c24',
389
389
'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d',
390
390
'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
391
391
'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
···
788
788
'phabricator-dashboard-css' => '5a205b9d',
789
789
'phabricator-diff-changeset' => 'd7d3ba75',
790
790
'phabricator-diff-changeset-list' => 'cc2c5de5',
791
-
'phabricator-diff-inline' => '62fff8eb',
791
+
'phabricator-diff-inline' => '26664c24',
792
792
'phabricator-diff-inline-content-state' => '68e6339d',
793
793
'phabricator-diff-path-view' => '8207abf9',
794
794
'phabricator-diff-tree-view' => '5d83623b',
···
1162
1162
'javelin-json',
1163
1163
'phabricator-prefab',
1164
1164
),
1165
+
'26664c24' => array(
1166
+
'javelin-dom',
1167
+
'phabricator-diff-inline-content-state',
1168
+
),
1165
1169
'289bf236' => array(
1166
1170
'javelin-install',
1167
1171
'javelin-util',
···
1531
1535
'javelin-vector',
1532
1536
'javelin-request',
1533
1537
'javelin-uri',
1534
-
),
1535
-
'62fff8eb' => array(
1536
-
'javelin-dom',
1537
-
'phabricator-diff-inline-content-state',
1538
1538
),
1539
1539
'65bb0011' => array(
1540
1540
'javelin-behavior',
+49
-44
src/infrastructure/diff/PhabricatorInlineCommentController.php
+49
-44
src/infrastructure/diff/PhabricatorInlineCommentController.php
···
180
180
$this->saveComment($inline);
181
181
182
182
return $this->buildEmptyResponse();
183
-
case 'edit':
184
183
case 'save':
185
184
$inline = $this->loadCommentByIDForEdit($this->getCommentID());
186
-
if ($op === 'save') {
187
-
$this->updateCommentContentState($inline);
188
185
189
-
$inline
190
-
->setIsEditing(false)
191
-
->setIsDeleted(0);
186
+
$this->updateCommentContentState($inline);
192
187
193
-
$this->saveComment($inline);
188
+
$inline
189
+
->setIsEditing(false)
190
+
->setIsDeleted(0);
194
191
195
-
return $this->buildRenderedCommentResponse(
196
-
$inline,
197
-
$this->getIsOnRight());
198
-
} else {
199
-
// NOTE: At time of writing, the "editing" state of inlines is
200
-
// preserved by simulating a click on "Edit" when the inline loads.
192
+
// Since we're saving the comment, update the committed state.
193
+
$active_state = $inline->getContentState();
194
+
$inline->setCommittedContentState($active_state);
195
+
196
+
$this->saveComment($inline);
197
+
198
+
return $this->buildRenderedCommentResponse(
199
+
$inline,
200
+
$this->getIsOnRight());
201
+
case 'edit':
202
+
$inline = $this->loadCommentByIDForEdit($this->getCommentID());
203
+
204
+
// NOTE: At time of writing, the "editing" state of inlines is
205
+
// preserved by simulating a click on "Edit" when the inline loads.
201
206
202
-
// In this case, we don't want to "saveComment()", because it
203
-
// recalculates object drafts and purges versioned drafts.
207
+
// In this case, we don't want to "saveComment()", because it
208
+
// recalculates object drafts and purges versioned drafts.
204
209
205
-
// The recalculation is merely unnecessary (state doesn't change)
206
-
// but purging drafts means that loading a page and then closing it
207
-
// discards your drafts.
210
+
// The recalculation is merely unnecessary (state doesn't change)
211
+
// but purging drafts means that loading a page and then closing it
212
+
// discards your drafts.
208
213
209
-
// To avoid the purge, only invoke "saveComment()" if we actually
210
-
// have changes to apply.
214
+
// To avoid the purge, only invoke "saveComment()" if we actually
215
+
// have changes to apply.
211
216
212
-
$is_dirty = false;
213
-
if (!$inline->getIsEditing()) {
214
-
$inline
215
-
->setIsDeleted(0)
216
-
->setIsEditing(true);
217
+
$is_dirty = false;
218
+
if (!$inline->getIsEditing()) {
219
+
$inline
220
+
->setIsDeleted(0)
221
+
->setIsEditing(true);
217
222
218
-
$is_dirty = true;
219
-
}
223
+
$is_dirty = true;
224
+
}
220
225
221
-
if ($this->hasContentState()) {
222
-
$this->updateCommentContentState($inline);
223
-
$is_dirty = true;
224
-
} else {
225
-
PhabricatorInlineComment::loadAndAttachVersionedDrafts(
226
-
$viewer,
227
-
array($inline));
228
-
}
226
+
if ($this->hasContentState()) {
227
+
$this->updateCommentContentState($inline);
228
+
$is_dirty = true;
229
+
} else {
230
+
PhabricatorInlineComment::loadAndAttachVersionedDrafts(
231
+
$viewer,
232
+
array($inline));
233
+
}
229
234
230
-
if ($is_dirty) {
231
-
$this->saveComment($inline);
232
-
}
235
+
if ($is_dirty) {
236
+
$this->saveComment($inline);
233
237
}
234
238
235
239
$edit_dialog = $this->buildEditDialog($inline)
···
333
337
$state = $inline->getContentState();
334
338
$default_suggestion = $inline->getDefaultSuggestionText();
335
339
$state->setContentSuggestionText($default_suggestion);
340
+
341
+
$inline->setInitialContentState($state);
336
342
$inline->setContentState($state);
343
+
337
344
$inline->setIsDeleted(0);
338
345
339
346
$this->saveComment($inline);
···
461
468
PhabricatorInlineComment $inline,
462
469
$view,
463
470
$is_edit) {
471
+
$viewer = $this->getViewer();
464
472
465
473
if ($inline->getReplyToCommentPHID()) {
466
474
$can_suggest = false;
···
469
477
}
470
478
471
479
if ($is_edit) {
472
-
$viewer = $this->getViewer();
473
-
$content_state = $inline->getContentStateForEdit($viewer);
480
+
$state = $inline->getContentStateMapForEdit($viewer);
474
481
} else {
475
-
$content_state = $inline->getContentState();
482
+
$state = $inline->getContentStateMap();
476
483
}
477
-
478
-
$state_map = $content_state->newStorageMap();
479
484
480
485
$response = array(
481
486
'inline' => array(
482
487
'id' => $inline->getID(),
483
-
'contentState' => $state_map,
488
+
'state' => $state,
484
489
'canSuggestEdit' => $can_suggest,
485
490
),
486
491
'view' => hsprintf('%s', $view),
+80
-8
src/infrastructure/diff/interface/PhabricatorInlineComment.php
+80
-8
src/infrastructure/diff/interface/PhabricatorInlineComment.php
···
336
336
return $this->newContentState()->readFromRequest($request);
337
337
}
338
338
339
+
public function getInitialContentState() {
340
+
return $this->getNamedContentState('inline.state.initial');
341
+
}
342
+
343
+
public function setInitialContentState(
344
+
PhabricatorInlineCommentContentState $state) {
345
+
return $this->setNamedContentState('inline.state.initial', $state);
346
+
}
347
+
348
+
public function getCommittedContentState() {
349
+
return $this->getNamedContentState('inline.state.committed');
350
+
}
351
+
352
+
public function setCommittedContentState(
353
+
PhabricatorInlineCommentContentState $state) {
354
+
return $this->setNamedContentState('inline.state.committed', $state);
355
+
}
356
+
339
357
public function getContentState() {
340
-
$state = $this->newContentState();
358
+
$state = $this->getNamedContentState('inline.state');
341
359
342
-
$storage = $this->getStorageObject();
343
-
$storage_map = $storage->getAttribute('inline.state');
344
-
if (is_array($storage_map)) {
345
-
$state->readStorageMap($storage_map);
360
+
if (!$state) {
361
+
$state = $this->newContentState();
346
362
}
347
363
348
364
$state->setContentText($this->getContent());
···
351
367
}
352
368
353
369
public function setContentState(PhabricatorInlineCommentContentState $state) {
370
+
$this->setContent($state->getContentText());
371
+
372
+
return $this->setNamedContentState('inline.state', $state);
373
+
}
374
+
375
+
private function getNamedContentState($key) {
354
376
$storage = $this->getStorageObject();
355
-
$storage_map = $state->newStorageMap();
356
-
$storage->setAttribute('inline.state', $storage_map);
377
+
378
+
$storage_map = $storage->getAttribute($key);
379
+
if (!is_array($storage_map)) {
380
+
return null;
381
+
}
357
382
358
-
$this->setContent($state->getContentText());
383
+
$state = $this->newContentState();
384
+
$state->readStorageMap($storage_map);
385
+
return $state;
386
+
}
387
+
388
+
private function setNamedContentState(
389
+
$key,
390
+
PhabricatorInlineCommentContentState $state) {
391
+
392
+
$storage = $this->getStorageObject();
393
+
$storage_map = $state->newStorageMap();
394
+
$storage->setAttribute($key, $storage_map);
359
395
360
396
return $this;
361
397
}
362
398
363
399
public function getInlineContext() {
364
400
return $this->getStorageObject()->getInlineContext();
401
+
}
402
+
403
+
public function getContentStateMapForEdit(PhabricatorUser $viewer) {
404
+
return $this->getWireContentStateMap(true, $viewer);
405
+
}
406
+
407
+
public function getContentStateMap() {
408
+
return $this->getWireContentStateMap(false, null);
409
+
}
410
+
411
+
private function getWireContentStateMap(
412
+
$is_edit,
413
+
PhabricatorUser $viewer = null) {
414
+
415
+
$initial_state = $this->getInitialContentState();
416
+
$committed_state = $this->getCommittedContentState();
417
+
418
+
if ($is_edit) {
419
+
$active_state = $this->getContentStateForEdit($viewer);
420
+
} else {
421
+
$active_state = $this->getContentState();
422
+
}
423
+
424
+
return array(
425
+
'initial' => $this->getWireContentState($initial_state),
426
+
'committed' => $this->getWireContentState($committed_state),
427
+
'active' => $this->getWireContentState($active_state),
428
+
);
429
+
}
430
+
431
+
private function getWireContentState($content_state) {
432
+
if ($content_state === null) {
433
+
return null;
434
+
}
435
+
436
+
return $content_state->newStorageMap();
365
437
}
366
438
367
439
public function getDefaultSuggestionText() {
+1
-1
src/infrastructure/diff/view/PHUIDiffInlineCommentView.php
+1
-1
src/infrastructure/diff/view/PHUIDiffInlineCommentView.php
+21
-11
webroot/rsrc/js/application/diff/DiffInline.js
+21
-11
webroot/rsrc/js/application/diff/DiffInline.js
···
8
8
JX.install('DiffInline', {
9
9
10
10
construct : function() {
11
-
this._activeContentState = new JX.DiffInlineContentState();
12
-
this._committedContentState = new JX.DiffInlineContentState();
11
+
this._state = {};
13
12
},
14
13
15
14
members: {
···
56
55
_isSelected: false,
57
56
_canSuggestEdit: false,
58
57
59
-
_committedContentState: null,
60
-
_activeContentState: null,
58
+
_state: null,
61
59
62
60
bindToRow: function(row) {
63
61
this._row = row;
···
602
600
_readInlineState: function(state) {
603
601
this._id = state.id;
604
602
605
-
// TODO: This is not the correct content state after a reload: it is
606
-
// the draft state.
607
-
this._getCommittedContentState().readWireFormat(state.contentState);
608
-
609
-
this._getActiveContentState().readWireFormat(state.contentState);
603
+
this._state = {
604
+
initial: this._newContentStateFromWireFormat(state.state.initial),
605
+
committed: this._newContentStateFromWireFormat(state.state.committed),
606
+
active: this._newContentStateFromWireFormat(state.state.active)
607
+
};
610
608
611
609
this._canSuggestEdit = state.canSuggestEdit;
610
+
},
611
+
612
+
_newContentStateFromWireFormat: function(map) {
613
+
if (map === null) {
614
+
return null;
615
+
}
616
+
617
+
return new JX.DiffInlineContentState().readWireFormat(map);
612
618
},
613
619
614
620
_ondeleteresponse: function() {
···
787
793
},
788
794
789
795
_getActiveContentState: function() {
790
-
var state = this._activeContentState;
796
+
var state = this._state.active;
791
797
792
798
if (this._editRow) {
793
799
state.readForm(this._editRow);
···
797
803
},
798
804
799
805
_getCommittedContentState: function() {
800
-
return this._committedContentState;
806
+
return this._state.committed;
807
+
},
808
+
809
+
_getInitialContentState: function() {
810
+
return this._state.initial;
801
811
},
802
812
803
813
setHasSuggestion: function(has_suggestion) {