Page MenuHomePhabricator

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

Description

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.


Done

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
D21858
D21849
D21849
D21848
D21847
D21846
D21845
D21844
D21843
D21842
D21841
D21840
D21839
D21838
D21837
D21836
D21835
D21833
D21832
D21834
D21831

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.