Page MenuHomePhabricator

Improve Remarkup file reference attachment behaviors after removal of "attach by default"
Open, NormalPublic


After T13603, referencing a file in Remarkup no longer automatically attaches it to the object where it is referenced. This breaks some workflows, and interacts with some existing behavior and code:

See also PHI2217.

Remove "getRemarkupBlocks()"

Remarkup blocks are extracted from transactions with "newRemarkupChanges()", except that two callsites use the obsolete "getRemarkupBlocks()" pathway: one in Audit, one in CustomFields. These may both be difficult to get rid of.

Remove Old Drafts

"PhabricatorApplicationTransactionCommentView->setDraft()" has exactly two callers: one in Slowvote and one in Pholio. These are best removed by updating both applications to EditEngine, which is a complicated change. Some attachment behavior in both applications is broken until this is completed.

The Conpherence "Durable Column View" also uses old drafts. This could probably just be removed.

Permanent Attachment Type

If the UI gets explicit attachment management, it will become possible to detach "innate" files (like images in a Pholio version). This can break objects. These relationships should likely be marked "Permanent", e.g. "attached, and users can't detach them since they're fundamental to the object".

What should API callers do?

API callers can't currently provide an "attach" transaction and also can't provide Remarkup metadata. Do they need to be able to do one or the other?

Phriction Does Not Automatically Attach Files

Editing a Phriction document does not properly attach files.


Attachments not mentioned

If you drag-and-drop a file into an object, then delete the reference from the text, we shouldn't attach the file (even though it will still be in the metadata attachment list).

References in Quotes

We should stop extracting file references from quoted blocks.

Revisions and Commits

rP Phabricator

Event Timeline

epriestley triaged this task as Normal priority.May 19 2022, 8:08 PM
epriestley created this task.

"PhabricatorApplicationTransactionCommentView->setDraft()" has exactly two callers: one in Slowvote...

Slowvote is nontrivial to convert to EditEngine because there is currently no "EditField" which can render a reasonable control for providing an arbitrary number of responses. However, this is probably not terribly difficult to build, particularly because just rendering 10 static inputs (like the UI currently does) is ultimately reasonable. Some future change could progressively enhance this with Javascript to add "More/Fewer Responses" or whatever.

Slowvote is also very old, and has more than a handful of outdated mechanisms -- like internal numeric constants, on-object mail keys, an ad-hoc PHID key, an absolutely minimal Conduit implementation, etc.

  • Inline Comments: See PHI2217. See PHI2206. Inline comments don't currently attach images.
  • Destroying Edges: See PHI2201. See PHI2217. Destroying files that have old edge types may raise warnings. The destruction still works, although it doesn't hit the edges.
  • Destroying Files: See PHI2217. Destroying an object doesn't destroy corresponding FileAttachment objects, so you can end up with ghosts in the UI.