Page MenuHomePhabricator

Always run "behavior-populate" before "behavior-show-more"
ClosedPublic

Authored by epriestley on Mar 5 2015, 3:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 20, 7:38 PM
Unknown Object (File)
Feb 4 2024, 6:48 PM
Unknown Object (File)
Jan 29 2024, 9:39 PM
Unknown Object (File)
Jan 20 2024, 2:21 PM
Unknown Object (File)
Jan 20 2024, 3:42 AM
Unknown Object (File)
Jan 20 2024, 3:42 AM
Unknown Object (File)
Jan 20 2024, 2:44 AM
Unknown Object (File)
Jan 10 2024, 4:55 AM
Subscribers

Details

Summary

Ref T2009. This clears the stage for D11977.

Specifically, D11977 moves "show context" logic into ChangesetViewManager, but those objects won't exist if we don't run "behavior-populate" first.

Generally, this increases consistency across changeset views -- which is still very low overall, but getting slightly better.

Both of these should probably move up more and use ChangesetListView, but we don't need to do that quite yet.

Test Plan
  • Took changeset actions in Phriction diff view.
  • Took changeset actions in Differential standalone view.
  • Took changeset actions in normal Differential view.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Always run "behavior-populate" before "behavior-show-more".
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
btrahan edited edge metadata.

Curiosities for me inline

src/applications/differential/controller/DifferentialChangesetViewController.php
240–252

Each behavior runs asynchronously right? So we don't necessary guarantee populate finishes before show-more?

src/applications/phriction/controller/PhrictionDiffController.php
62

does this fix a bug or something?

This revision is now accepted and ready to land.Mar 5 2015, 8:57 PM
src/applications/differential/controller/DifferentialChangesetViewController.php
240–252

I think they execute in the order they were initialized, although that's probably not good to rely on.

In this diff, it's OK if they run out of order because all show-more does is install a listener, and doesn't actually do anything interesting. So it could only error if an event fired between the listener being installed and the populate behavior running, which isn't possible today and is vanishingly unlikely to be possible even in some future world where the rules change, since you'd have to click really really fast. (In most cases, behavior-populate has to run before the links even appear on the page in order to fire the event.)

And, yeah, mooted by the next change.

src/applications/phriction/controller/PhrictionDiffController.php
62

Adding the "ChangesetDetailView" below makes the diff render with a header. Since we don't set a name, it uses a default name like /left/diff or something though if we don't make this change. It seemed reasonable to just use the page title instead of adding an option to hide the name.

(We don't show the "View Options" menu right now, but I'd like to at some point, and it would look really awkward if we hid the name since it would sort of just be floating there.)

This revision was automatically updated to reflect the committed changes.