Page MenuHomePhabricator

Render Remarkup poorly in Phame Feed stories
ClosedPublic

Authored by chad on Dec 2 2015, 9:47 PM.

Details

Summary

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

Test Plan

epriestley

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

chad updated this revision to Diff 35429.Dec 2 2015, 9:47 PM
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.
epriestley edited edge metadata.Dec 2 2015, 9:53 PM

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.

chad added a comment.Dec 2 2015, 10:05 PM

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 updated this revision to Diff 35431.Dec 2 2015, 10:14 PM
chad edited edge metadata.
  • changes
epriestley accepted this revision.Dec 2 2015, 10:15 PM
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.