Page MenuHomePhabricator

Render Remarkup poorly in Phame Feed stories
ClosedPublic

Authored by chad on Dec 2 2015, 9:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 7, 4:58 PM
Unknown Object (File)
Tue, Mar 5, 4:09 PM
Unknown Object (File)
Tue, Mar 5, 2:29 AM
Unknown Object (File)
Tue, Mar 5, 2:29 AM
Unknown Object (File)
Tue, Mar 5, 2:28 AM
Unknown Object (File)
Fri, Mar 1, 3:51 PM
Unknown Object (File)
Fri, Mar 1, 3:28 PM
Unknown Object (File)
Feb 26 2024, 9:20 PM

Details

Summary

Seeing if this is the correct path, then will apply in Pholio, Ponder.

Test Plan

epriestley

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 9240
Build 10947: Run Core Tests
Build 10946: arc lint + arc unit

Event Timeline

chad retitled this revision from to Render Remarkup poorly in Phame Feed stories.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added a reviewer: epriestley.

Try something like this instead, maybe?

In PhabricatorApplicationTransaction->getBodyForFeed(), add a clause near the top like this:

$remarkup = $this->getRemarkupBodyForFeed();
if ($remarkup !== null) {
  summarize;
  return new PHUIRemarkupView(...);
}

Then add a new method:

protected function getMarkupBodyForFeed() {
  return null;
}

Then rename PhameTransaction->getBodyForFeed() to getRemarkupBodyForFeed() and just have it return $text if it finds some text.

So the overall behavior is:

  • getBodyForFeed(): Look for some remarkup by calling getRemarkupBodyForFeed(). If it finds some, summarize and render it and use that. Otherwise, do other stuff.
  • getRemarkupBodyForFeed(): Return the relevant raw remarkup for this story, if some exists.

Then we can clean this up more later in T9789 etc.

Why do we need getBodyForFeed() and getRemarkupBodyForFeed()? This feels very verbose.

Because not all things which return a body can currently return raw remarkup.

Particularly, comments can return rendered remarkup much more efficiently than raw remarkup (the remarkup has already been batch-rendered earlier in the pipeline).

We could clean this up now, but we'd have to take a performance hit to do so, and no version of this will be particularly clean relative to T9789.

Even in the future, I think some stories are likely to contain bodies which are not remarkup. One example is that I'd like to show better human-readable diffs for "edited description of" stories; they would be HTML but not remarkup. I could also imagine "Edit profile picture" stories having non-remarkup bodies, etc.

At least since this is Phame/Pholio/Ponder, I don't expect a large number of stories.

We could also not do the getRemarkupBodyForFeed() thing and put some setSummarize(true) on PHUIRemarkupView and just have these things all do return new PHUIRemarkupView(...)->setSummarize(true), but that's doubling down on my ability to make that render efficiently later. I'm pretty sure I can, it's just a lot of eggs to put in the "anticipated performance improvement" basket.

chad edited edge metadata.
  • changes
epriestley edited edge metadata.
This revision is now accepted and ready to land.Dec 2 2015, 10:15 PM
This revision was automatically updated to reflect the committed changes.