Summary:
Ref T12634. Using keyboard shortcuts in Differential currently relies on focus behavior in `KeyboardShortcutManager`.
This possibly made sense long ago, but no longer does, and leads to a whole slew of bugs where the reticle doesn't interact properly with anything else.
Move it to Differential so it can be made reasonably aware of edit operations, Quicksand navigation, etc.
This just moves the code; future diffs will actually fix bugs.
Test Plan: Used "n", "j", etc., saw the same behavior as before.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12634
Differential Revision: https://secure.phabricator.com/D17899
Summary: Restricts the view of the membership privileges to just the Members page itself, and not other pages like Home/Details.
Test Plan: Test Home, Test Members, see correct layouts.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17896
Summary: This UI can use the setDrag call to reduce clutter on the reodering dialog.
Test Plan:
Reorder some columns, save.
{F4959906}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17898
Summary: Ref T12616. This makes "edit" and "reply" work again.
Test Plan:
Used "e" and "r" to edit and reply.
Also used them in bogus ways and got useful UI feedback.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17895
Summary: Ref T12616. This moves most keyboard shortcuts into DiffChangesetList. It breaks some shortcuts that I plan to restore later, noted in T12616 (toggle file, edit inline, reply to inline), since I think ripping them out now and rebuilding them in a little bit will make things much simpler.
Test Plan:
- Used j, k, n, p, J, K shortcuts to navigate a revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17859
Summary:
Ref T12616. This moves "reply" to the new stuff and deletes DifferentialInlineEditor, which no longer does anything.
(This breaks some keyboard shortcuts, but I'll rebase D17859 shortly.)
Test Plan: Replied to inlines; things seemed to work properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17894
Summary:
Ref T12616. This makes creating inlines use the new code.
Creation and editing is now slightly more consistent in how it uses nodes. This will simplify the next change (replies), which I ran into some trouble with in an earlier iteration.
Note that this (and other changes in the series) allow you to create and edit multiple inlines simultaneously. This is mostly a feature, although I expect we'll need to lock it down a little bit. I have some UI ideas to help avoid errors.
Test Plan: Created inlines on a single line; on a range of lines; on the same line; multiple inlines at the same time.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17893
Summary:
Ref T12616. This moves the delete actions to the new, more stateful way of doing things.
These are a little tricky because you can click "Delete" on an inline, but you can also click "Delete" from the preview area at the bottom of the page. If you do, the inline you are deleting may or may not be present on the page.
This has a few bugs -- notably, deleting from the preview without interacting with the on-page inline first won't actually delete the on-page inline yet -- but nothing too serious.
Test Plan: Deleted inlines, undid deletion, deleted from preview.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17890
Summary:
Ref T12616. This updates clicking the "Done" checkbox for the new stuff.
This one is pretty clean since the "Done" checkbox doesn't do too much weird magic.
Test Plan: Clicked the box a few times.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17888
Summary:
Ref T12616. This doesn't pull over everything (some UI feedback didn't make it yet, and you can't cancel + undo cancelling edits yet) but editing comments technically works.
This is a little shaky, but feels less shaky than every other approach I've tried, so I think I'm finally on a reasonable track here.
Test Plan: Edited some inline comments.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17887
Summary:
Ref T12616. Fixes T12153. Currently, when you hide inlines, they hide completely and turn into a little bubble on the previous line.
Instead, collapse them to a single line one-by-one. Narrowly, this fixes T12153.
In the future, I plan to make these changes so this feature makes more sense:
- Introduce global "hide everything" states (T8909) so you can completely hide stuff if you want, and this represents more of a halfway state between "nuke it" and "view it".
- Make the actual rendering better, so it says "epriestley: blah blah..." instead of just "..." -- and looks less dumb.
The real goal here is to introduce `DiffInline` and continue moving stuff from the tangled jungle of a million top-level behaviors to sensible smooth statefulness.
Test Plan:
- Hid and revealed inlines in unified and two-up modes.
- These look pretty junk for now:
{F4948659}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T12153
Differential Revision: https://secure.phabricator.com/D17861
Summary: Ref T12616. This cements the relationship between ChangesetList (parent container) and Changeset (child) and passes translations down so Changeset can use them to translate the text "Loading..."
Test Plan: Viewed loading changes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17846
Summary: Ref T12616. This ends up being a little messy ("one giant function") and maybe I'll clean it up a bit later, but continue consolidating the wild jungle of behaviors into a smaller set of responsible objects.
Test Plan: Clicked all the menu options, saw them work properly. Grepped for removed methods.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17845
Summary:
Ref T12616. Diffusion, only, has a "Show All Context" button which expands the full context on all changes.
I don't remember the exact history on this, but it hasn't existed in Differential for some time and no one has complained. I suspect that the "View Options > Show All Context" on each file may replace it. I can't really come up with good reasons to use it, offhand. If we want to restore it, I think global options after T1591 is promising.
{F4945561}
Test Plan:
- Loaded a commit in Diffusion, no longer saw a button.
- Grepped for relevant sigils.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17843
Summary: Ref T12616. This class is already mostly-reasonable as a representation of an individual changeset, so I plan to just adjust it a little bit.
Test Plan:
- Used `git grep` to search for `ChangesetViewManager`.
- Used `git grep` to search for `changeset-view-manager`.
- Browsed around and interacted with changesets.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17842
Summary: Ref T12616. Put these listeners in DiffChangesetList so the wake/sleep properly for Quicksand.
Test Plan:
- Added some logging.
- With quicksand, moved between diffs.
- Saw "load" and "show more" fire exactly once on each page, with the correct changeset list listener.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17841
Summary:
Ref T12616. Ref T8047. Ref T11093. We currently have several bugs where diff state sticks across Quicksand pages.
Add a new top-level object to handle this, with `sleep()` and `wake()` methods. In `sleep()`, future changes will remove/deacivate all the reticles/editors/etc.
See T12616 for high-level discussion of plans here.
This general idea is likely to become more formal eventually (e.g. for "sheets" or whatever we call them, in T10469) but I think this is probably a reasonable place to draw a line for now.
Test Plan:
- Added some logging to sleep(), wake() and construct().
- Viewed changes in Differential.
- With Quicksand on, browsed around; saw state change logs fire properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T11093, T8047
Differential Revision: https://secure.phabricator.com/D17840
Summary: Also cleans up now-dead code relating to old transactions
Test Plan: Created lots of mocks, replaced their images, added/removed images, changed the sequence, verified expected DB xaction rows, Mock updates, and correct rendering of timeline.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17892
Summary: Various little fixes, mostly moves information from the "Details" section either into the curtain or into the specific watchers or members list based on user viewership. I think this page is both cleaner and more informative.
Test Plan:
Lock, Unlock, Watch, Join, various projects with multiple users.
{F4959101}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17891
Summary: Removal of `PholioMockEditor::applyCustomInternalTransaction()` in D17868 broke creation of new mocks in Pholio. Puts the empty method back until we finish migrating Pholio to modular transactions.
Test Plan: Created some mocks, observed lack of unhandled exception.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17889
Summary: I think this is the correct fix, sets a consistent value for transactions, old and new, for Maniphest point values.
Test Plan:
Edit title, see no point feed story, set points, see point story, set points to same value, see no story, remove points, see remove point story.
{F4958233}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17885
Summary: Fixes T12713. We don't need to show watching and member info on other views other than ApplicationSearch (for now) so add a few methods to restrict the calls.
Test Plan: Visit project search, profile, project home, project home with subprojects
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12713
Differential Revision: https://secure.phabricator.com/D17883
Summary: Slightly nicer, more consistent UI. Also removed "Column History" from dropdowns as this is available on the general board manage page.
Test Plan: Review Board and Column management pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17881
Summary:
Fixes T12710. See that task for discussion. This is pretty ugly/redundant but not broken.
(Feel free to reject this and pursue something else.)
Test Plan:
- For a project with active subprojects/milestones, viewed the project profile and subprojects tabs.
- After patch: they're ugly, but no longer fatal.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12710
Differential Revision: https://secure.phabricator.com/D17882
Summary: Moves "reorder columns" and "change background" up a level, redesigns "manage" page to be a little cleaner.
Test Plan: Change colors, reorder columns, manage page, disable board, re-enable board.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17879
Summary: Fixes T12707 Adds additional information to search results for if user is a member or a watcher of a project. Also removed the icon colors, which I'll find a better way to denote in future.
Test Plan: Join a project, watch a project, view results list in /projects/
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12707
Differential Revision: https://secure.phabricator.com/D17880
Summary:
Ref T12685.
- Better icon/color/label consistency.
- Make "0" a valid response.
- Fix a bug where creating a poll with Quicksand enabled led to bad times (form submitted back to the search screen).
Test Plan: {F4956412}
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17878
Summary:
Ref T12685. When you edit an application's policies but don't make any changes, you currently get stuck on the same page. This isn't how other edit screens work, and I think it's a holdover from eras long ago.
Make it consistent with other applications.
Also, fix a missing `pht()`.
Test Plan:
- Edited an application configuation.
- Clicked "Save" without making changes.
- After patch, was redirected back to detail page like in other applications.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17877
Summary:
Ref T12685. I provided this incorrect (`return new` rather than `throw`) implementation earlier; it can now be replaced with a proper implementation.
This caused application policy edits to spew this into the daemon log:
```
[2017-05-14 15:35:27] EXCEPTION: (Error) Call to undefined method PhutilMethodNotImplementedException::setActor() at [<phabricator>/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php:69]
```
Test Plan:
- Used `bin/worker execute --id <id>` to execute a previously-failing task.
- Saw a feed story publish.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17876
Summary: This brings up "Edit Column" as an action item under the main column dropdown as well as a "Column History" for completeness. Unsure column history is actually useful, but leaving it in anyways. It might be nice to have some sort of dialog version of a history page.
Test Plan: Make a workboard, add a column, edit column name, stay on workboard.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D17874
Summary:
Fixes T12679. Reproduction steps appear to be:
- As a logged-out user, view revision list or commit list.
- Enable bucketing by action required.
- Before patch: `foreach (null as ...)` causes error spew.
- After patch: `foreach (array() as ...)` works great.
Test Plan:
- Reproduced issue by following steps above in Differential (revisions) and Diffusion (audits/commits).
- After patches, no more errors in the log.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12679
Differential Revision: https://secure.phabricator.com/D17872
Summary: Ref T12707. Adds a simple filter for the viewer if logged in.
Test Plan: Watch a project, click on watching list, see project I'm watching.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley
Maniphest Tasks: T12707
Differential Revision: https://secure.phabricator.com/D17873
Summary: Also removes more now-dead code from `PholioTransaction`.
Test Plan: opened and closed a bunch of mocks, edited a bunch of image descriptions
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D17868
Summary: Caught this in the logs, calling an old transaction, update it.
Test Plan: Answer a question, tail log, see no error.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17870
Summary: Trailing logs for errors here, picked up some unset constants in Phame.
Test Plan: New Blog, New Post, tail daemon log for errors. See feed stories.
Reviewers: epriestley, amckinley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17871
Summary: The ports over a similar "profile image" menu item to Projects. It gives us some room to use the project icon in the sidenav along with a larger photo. It also will open up some room in the sub-page headers for us to focus on that page, and not the identity of the project at hand. Expect a few more project related touch up diffs.
Test Plan:
Review new projects menu on a few projects, update the image, see new image. Great for team photos.
{F4951264}
Reviewers: epriestley, amckinley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17869
Summary: Another Franken-transaction. Adds transactions for ImageName and MockName. Adds transaction-level validation for presence of mock name; removed same from edit controller. Removes `PholioTransaction::getRemarkupBodyForFeed()`, which appears to be dead code since that method isn't defined on any other types.
Test Plan: made a bunch of changes to pholio mocks and images and observed expected results
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, chad, epriestley
Differential Revision: https://secure.phabricator.com/D17865
Summary: Updates Legalpad to use EditEngine, paving the way for //transaction comments//. Spooky.
Test Plan:
- New Document
- Require signing, Corp - see fail
- Require signing, Noone - see fail
- Require signing, Ind - get asked to sign
- Edit Document
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17862
Summary: Begins the process of migrating Pholio to Modular Transactions by starting with the mock's description and changing the base class of PholioTransaction. Expect several more of these diffs to quickly follow. Also changes the icon for description changes to `fa-pencil`; previously it was check or ban, depending on the open/closed state. Looks like an accidental switch fallthrough.
Test Plan: made new mocks, edited their descriptions, observed same UI as previously
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D17864
Summary: This function `renderHandleLink` doesn't exist. Update call.
Test Plan: I'm still lost on how to refund a transaction? Existing?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17858
Summary: Ref T12685. Moves Fund to EditEngine commenting.
Test Plan: View an old Fund, leave a comment, add a subscriber.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17857
Summary: Ref T12685, updates fund for edit engine.
Test Plan: Create a Fund, Edit a Fund, wipe out Merchants, check errors for name and missing merchants, back Fund.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17855
Summary: Ref T12685, cleans up various macro issues, remove subscribers, fix feed stories, etc.
Test Plan: Create a new macro, see no subscribers, edit various macros.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17848
Summary:
See D17848. This improves things a little bit in two cases:
Case 1:
- Create a macro.
- Pick a valid file.
- Pick an invalid name.
- Submit form.
- Before patch: your file is lost and you have to pick it again.
- After patch: your file is "held" in the form, you just can't see it in the UI. If you submit again, it keeps the same file. If you pick a new file, it uses that one instead.
Case 2:
- Apply D17848.
- Delete the `if ($value) {` thing that I'm weirded out about (see inline).
- Edit a macro.
- Don't pick a new file.
- Before patch: error, can't null the image PHID.
- Afer patch: not picking a new file means "keep the same file", but you can't tell from the UI.
Basically, the behaviors are good now, they just aren't very clear from the UI since "the field has an existing/just-submitted value" and "the field is empty" look the same. I think this is still a net win and we can fix up the UI later.
Test Plan: See workflows above.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17853
