Page MenuHomePhabricator

Separate workboard view state (ordering, filtering, hidden columns) from the View controller
ClosedPublic

Authored by epriestley on Jun 29 2019, 10:11 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 14, 5:20 PM
Unknown Object (File)
Feb 12 2024, 3:19 PM
Unknown Object (File)
Feb 3 2024, 10:44 PM
Unknown Object (File)
Jan 6 2024, 1:57 PM
Unknown Object (File)
Dec 25 2023, 3:22 PM
Unknown Object (File)
Dec 22 2023, 2:32 AM
Unknown Object (File)
Dec 22 2023, 2:32 AM
Unknown Object (File)
Dec 22 2023, 2:32 AM
Subscribers
None

Details

Summary

Depends on D20627. Ref T4900. If a user orders a board by "Sort by Title", then toggles the visibility of hidden columns, we want to keep the board sorted by title. To accomplish this, we pass the board state around to all the workflows here.

Pull the "bag of state properties" code out of the View controller. This class basically:

  • reads state from a request (order, hidden, filter);
  • manages defaults;
  • provides the application with the current settings; and
  • generates URIs with "?order=X&hidden=Y&filter=Z" to preserve state.

This is still a little questionable/transitional since some of the controllers need more cleanup.

Test Plan

Toggled state, order, filters, clicked around various workflows and saw the filters preserved.

A lot of these workflows are pretty serious edge cases. For example, here's a feature this implements:

  • Changed workboard order to "Title".
  • Selected "Bulk Edit Tasks..." in an empty column and command-clicked it to open the link in a new window.
  • Hovered over "Cancel".
  • Saw the link properly generate with "?order=title", preserving the order.

Diff Detail

Repository
rP Phabricator
Branch
refresh3
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 23097
Build 31714: Run Core Tests
Build 31713: arc lint + arc unit

Event Timeline

amckinley added inline comments.
src/applications/project/controller/PhabricatorProjectBoardViewController.php
56

I'm pretty sure this is now an unneeded repeat of line 49?

src/applications/project/state/PhabricatorWorkboardViewState.php
49

Shouldn't this be urisprintf('%s%p')?

57–81

This is just flirting with being enough duplicated code to factor out. Do you think we'll ever add more query params with almost-identical implementations? I can see this is a cut/paste from the old version, so maybe not worth messing with right now anyway.

This revision is now accepted and ready to land.Jul 2 2019, 6:26 PM
src/applications/project/state/PhabricatorWorkboardViewState.php
57–81

Yeah -- this gets (moderately) simpler later in the change sequence, at least.

I don't think we have more of this coming, but who knows what the future holds...

src/applications/project/controller/PhabricatorProjectBoardViewController.php
56

Oh, they're not always the same ($saved vs $state) and line 49 moves out of the controller somewhere down the line.

  • Use "%p" in urisprintf().