Page MenuHomePhabricator

Make "editing" state persistent for inline comments
ClosedPublic

Authored by epriestley on Apr 29 2020, 2:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 4 2024, 12:08 AM
Unknown Object (File)
Jan 25 2024, 5:06 AM
Unknown Object (File)
Jan 25 2024, 4:48 AM
Unknown Object (File)
Jan 25 2024, 4:32 AM
Unknown Object (File)
Jan 25 2024, 4:18 AM
Unknown Object (File)
Jan 25 2024, 1:43 AM
Unknown Object (File)
Jan 25 2024, 12:42 AM
Unknown Object (File)
Jan 17 2024, 6:33 PM
Subscribers

Details

Summary

Ref T13513. This is mostly an infrastructure cleanup change.

In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.


Inline comments are stored as transaction comments in the PhabricatorAuditTransactionComment and DifferentialTransactionComment classes.

On top of these two storage classes sit PhabricatorAuditInlineComment and DifferentialInlineComment. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.

Part of the reason they're so copy/pastey is that they implement a parent Interface. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).

To simplify this:

  • Make PhabricatorInlineCommentInterface an abstract base class instead.
  • Lift as much code out of the Audit and Differential subclasses as possible.
  • Delete methods which no longer have callers, or have only trivial callers.

Inline comments have two View rendering classes, DetailView and EditView. They share very little code.

Partly, this is because EditView does not take an $inline object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple <form />.

These classes can be significantly simplified by passing an $inline to the EditView, instead of individually setting all the properties on the View itself. This allows the DetailView and EditView classes to share a lot of code.

The EditView can not fully render its content. Move the content rendering code into the view.


Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.

This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in EditView and allows the "create" and "reply" code to be merged in PhabricatorInlineCommentController.


Client-side inline events are currently handled through a mixture of ChangesetList listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.

Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.

This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in _drawRows(), but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.


Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".

Test Plan
  • Created comments on either side of a diff.
  • Edited a comment, reloaded, saw edit stick.
  • Saved comments, reloaded, saw save stick.
  • Edited a comment, typed text, cancelled, "unedited" to get state back.
  • Created a comment, typed text, cancelled, "unedited" to get state back.
  • Deleted a comment, "undeleted" to get state back.

Weirdness / known issues:

  • Drafts don't autosave yet.
  • Fixed in D21187:
    • When you create an empty comment then reload, you get an empty editor. This is a bit silly.
    • "Cancel" does not save state, but should, once drafts autosave.
  • Mostly fixed in D21188:
    • "Editing" comments aren't handled specially by the overall submission flow.
    • "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

  • Don't process inline actions if the changeset list is asleep (paged out with Quicksand).
  • Remove some debugging code.

One thought here for Test Plan items: performing edits with the revision loaded in two separate browser tabs. I don't think UX here is important, but probably important to make sure there's no data loss.

  • Fix the "Create > Type Text > Cancel > Undo" workflow.

One thought here for Test Plan items: performing edits with the revision loaded in two separate browser tabs.

This is "probably" safe in all cases right now, but I'll run through this once the draft stuff is in place since that definitely creates theoretical peril here.

There are currently (in the general case, for any type of object) some cases where data is destroyed:

Window AWindow B
Load Revision
Add Draft Comment "Quack"
Load Revision
Begin Editing Comment "Quack"
Begin Editing Comment "Quack"
Type "Very Important Insight"
Type "Moo"
Save Comment
Reload Page

In this case, the comment now reads "Moo", and the text "Very Important Insight" is discarded -- but the user never explicitly deleted it or typed over it.

I think this could only be fixed by allowing drafts to "branch", and prompting the user to merge all "branch heads" when they save, e.g.:

+---------------------------------------------------+
| You have more than one draft of this content:     |
|                                                   |
| * [     This Window    ] "Moo"                    |
|   [ Window 2 on Safari ] "Very Important Insight" |
|                                                   |
+---------------------------------------------------+--------------------------------+
|  [ Resolve Conflicts ][ Save Selected Draft, Permanently Discarding Other Drafts ] |
+---------------------------------------------------+--------------------------------+

I think this is feasible to build technically, although whatever screen "Resolve Conflicts" brings you to would also suffer from this same problem (you could make conflicting edits to the merge resolution in two different windows). I think it's probably overkill, though.

My sense is that the draft behavior is generally "good enough" after D14640, which roughly added these rules:

  • Ignore drafts sent from windows which loaded an older version of the comment/object than the version on the server. (This mostly prevents a leftover window from writing an out-of-date draft.)
  • When a comment/object is saved, delete all outstanding drafts.

I expect draft stuff here will use the same infrastructure so it should get similar behavior: not data-preserving in all cases, but rarely does anything surprising.

This revision was not accepted when it landed; it landed in state Needs Review.May 4 2020, 8:10 PM
This revision was automatically updated to reflect the committed changes.