Make "editing" state persistent for inline comments
b48a22bf50a3
Actions

Description

Make "editing" state persistent for inline comments

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.