Page MenuHomePhabricator

Remove "willRenderTimeline()" from ApplicationTransactionInterface
ClosedPublic

Authored by epriestley on Dec 20 2018, 6:12 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 3 2024, 8:29 PM
Unknown Object (File)
Jan 25 2024, 1:40 AM
Unknown Object (File)
Dec 22 2023, 1:03 AM
Unknown Object (File)
Nov 30 2023, 3:29 AM
Unknown Object (File)
Nov 25 2023, 8:32 PM
Unknown Object (File)
Nov 16 2023, 10:14 AM
Unknown Object (File)
Nov 4 2023, 1:20 AM
Unknown Object (File)
Nov 3 2023, 9:13 PM
Subscribers
None

Details

Summary

Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.

PhabricatorApplicationTransactionInterface currently requires you to implement willRenderTimeline(). Almost every object just implements this as return $timeline; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.

The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.

Try to clean this up:

  • Stop requiring willRenderTimeline() to be implemented.
  • Stop requiring getApplicationTransactionViewObject() to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
  • Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
  • If objects want to customize timeline rendering, they now implement PhabricatorTimelineInterface and provide a TimelineEngine which gets a nice formal stack.

This leaves a lot of empty willRenderTimeline() implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.

Test Plan
  • Viewed audits, revisions, and mocks with inline comments.
  • Used "Show Older" to page a revision back in history (this is relevant for "View Data").
  • Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

  • Slight cleanup.
  • Also tested ConfigSettings History. This is a slightly odd page that renders transactions for multiple objects in a single timeline.
This revision is now accepted and ready to land.Dec 20 2018, 8:25 PM
This revision was automatically updated to reflect the committed changes.