Summary: Fixes T8428. Adds status to packages, allows setting and application search. I presume though these need checked elsewhere?
Test Plan: New package, edit package, archive package, run search queries.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8428
Differential Revision: https://secure.phabricator.com/D13925
Summary:
Fixes T8004.
- For paths which are part of a package, show the package.
- Highlight paths which are part of a package you (the viewer) have authority over.
Test Plan:
{F725418}
- Viewed owned and unowned chagnes in Diffusion and Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8004
Differential Revision: https://secure.phabricator.com/D13923
Summary:
Fixes T2183. We now use the same rendering element in both places.
Intentional changes:
- Package highlighting is out, coming back to both apps in next diff.
- removed redundant-feeling "Change" link. The information is now shown with a character ("M", "V", etc.) and the page is a click away under "History". Clicking the path also jumps you to substantially similar content. (We could restore it fairly easily, I just think it's probably the least useful thing in the table right now.)
Test Plan: Viewed a bunch of commits in Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2183
Differential Revision: https://secure.phabricator.com/D13910
Summary:
Ref T2183. Introduces a new View which can (in theory) unify the Revision, Diff and Commit table of contents views.
This has the same behavior as before, but accepts slightly more general primitives and parameters and has somewhat cleaner code.
I've made one intentinoal behavior change: removing the "Open All in Editor" button. I suspect this is essentially unused, and is a pain to keep around. We can look at restoring it if anyone notices.
Test Plan: Looked at a bunch of revisions, no changes from before.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2183
Differential Revision: https://secure.phabricator.com/D13908
Summary: Ref T2183. These properties are not used and not useful.
Test Plan: Grepped for callsites and uses. Loaded revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2183
Differential Revision: https://secure.phabricator.com/D13906
Summary: Fixes T9128. I missed this when converting other controllers to look in Harbormaster for lint/unit data.
Test Plan: Copy/pasted a diff successfully.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9128
Differential Revision: https://secure.phabricator.com/D13865
Summary:
Ref T8096. This modernizes the last thing which was reading the old datasource.
Also fix a bug where it didn't work.
Test Plan: {F698405}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13852
Summary: Ref T8096. This information has moved into Harbormaster.
Test Plan:
Pushed some test results in; see right margin:
{F698394}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13850
Summary:
Ref T8096.
Long ago, support for "postponed" lint and unit tests got hacked in. `arc` would publish a bunch of ghost results, and then something else would fill the results in later.
This was always a hack. It is not nearly as powerful or flexible as having a real build system, and is obsolete with Harbormaster, which supports these operations in a more reasonable and straightforward way.
This was used (only? almost only?) at Facebook.
- Remove `differential.finishpostponedlinters`. This only served to update postponed linters.
- Remove lint magic in `differential.setdiffproperty`. This magic only made sense in the context of postponed linters.
- Remove `differential.updateunitresults`. The only made sense for postponed unit tests.
And one minor change: when a diff contains >100 affected files, we hide the content by default, but show content for files with inline comments. Previously, we'd do this for lint inlines, too. I don't tink this is too useful, and it's much simpler to just remove it. We could add it back at some point, but I think large changes often trigger a lot of lint and no one actually cares. The behavior for actual human inlines is retained.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13848
Summary: Ref T8096. These are still reading out of the old diff property, but should read from Harbormaster instead.
Test Plan: {F698346}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8096
Differential Revision: https://secure.phabricator.com/D13847
Summary:
Fixes T8917. Prior to T2618, deleting inlines prompted users, then really deleted the rows.
After T2618, we delete immediately and offer "Undo". However, some interactions with drafts were missed, and we were only clearing the "this revision has a draft" flag on one of the delete pathways (when you delete all the comment text, then save the comment).
Make both the "Delete" action and the "Delete All Comment Text + Save" workflows do the same thing: mark the row as deleted, and clear any relevant drafts.
Test Plan:
- Made an inline comment on a clean revision with no "draft comments" marker in the list view.
- Used "delete" to delete it.
- After applying the patch, verified that no "draft commetns" marker appears in the list view.
- Used Delete and Edit + Remove Text + Save to delete comments in Differential and Diffusion.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8917
Differential Revision: https://secure.phabricator.com/D13665
Summary: Fixes T8095. Still needs UI/UX work (see T8096) but this has all the core features now.
Test Plan: Saw Harbormaster lint/unit data as though it was Differential lint-unit data.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8095
Differential Revision: https://secure.phabricator.com/D13401
Summary:
Fixes T5138. Some of the "revision" properties are really "diff" properties, but we only show the properties for the most recent / current diff.
- Immediately, this makes it hard or impossible to review, e.g., lint/unit results for older diffs.
- Longer-term, these limits will become more problematic with more data on diffs after Harbormaster.
Instead, separate "revision" from "diff" properties.
(In the long term, it might make sense to show more diffs in this panel -- e.g., tabs for the 8 most recent updates or something -- but I went with the simplest approach for now since I don't have a clean way to deal with 100-update revisions offhand.)
Test Plan: {F500480}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: cburroughs, epriestley
Maniphest Tasks: T5138
Differential Revision: https://secure.phabricator.com/D13282
Summary:
Ref T7447. Implements per-viewer comment hiding. Once a comment is obsolete or uninteresting, you can hide it completely.
This is sticky per-user.
My hope is that this will strike a better balance between concerns than some of the other approaches (conservative porting, summarization, hide-all).
Specifically, this adds a new action here:
{F435621}
Clicking it completely collapses the comment into a small icon on the previous line, and saves the comment state as hidden for you:
{F435626}
You can click the icon to reveal all hidden comments below the line.
Test Plan:
- Hid comments.
- Showed comments.
- Created, edited, deleted and submitted comments.
- Used Diffusion comments (hiding is not implemented there yet, but I'd plan to bring it there eventually if it works out in Differential).
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: jparise, yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D13009
Summary:
fixes T8260. Only turn on symbol links if:
- The repository has any configuration about symbols, or
- There actually are symbols in the repository.
Test Plan: Look at revisions and files in various states of configurations and having symbols.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, Korvin, epriestley
Maniphest Tasks: T8260
Differential Revision: https://secure.phabricator.com/D12946
Summary: Converts most all tables to be directly set via `setTable` to an ObjectBox. I think this path is more flexible design wise, as we can change the box based on children, and not just CSS. We also already do this with PropertyList, Forms, ObjectList, and Header. `setCollapsed` is added to ObjectBox to all children objects to bleed to the edges (like diffs).
Test Plan: I did a grep of `appendChild($table)` as well as searches for `PHUIObjectBoxView`, also with manual opening of hundreds of files. I'm sure I missed 5-8 places. If you just appendChild($table) nothing breaks, it just looks a little funny.
Reviewers: epriestley, btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12955
Summary:
Fixes T7977.
- Move Indexed Languages and See Symbols From config to Repository
- Make symbol search skip projects
This also makes the default languages to Everything instead of Nothing.
Test Plan:
- Browse files, click symbols.
- Use quick search to find symbols
- Browse revision, click symbols
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7977
Differential Revision: https://secure.phabricator.com/D12687
Summary: See D12702.
Test Plan: Made something a link and clicked it, seemed to work OK.
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim, epriestley
Differential Revision: https://secure.phabricator.com/D12703
Summary:
Ref T7447. Ref T7870.
When a ghostly inline appears on the page, the timeline isn't currently aware that it's present, so it links elsewhere.
Instead, apply adjustments before rendering the timeline.
Ref T5030. This makes the behaviors in T5030 irrelevant most of the time.
Test Plan:
Before:
{F378106}
After:
- Inlines visible on page are linked directly.
- Inlines which we can't port (e.g., on files not present on current page) are still linked off-page.
- Used "Show Older" to page through and verify consistent rendering.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, epriestley
Maniphest Tasks: T5030, T7870, T7447
Differential Revision: https://secure.phabricator.com/D12497
Summary:
Ref T7447. Ref T7870. When a diff affects more than 100 files, we collapse files by default, then expand files with inlines.
Ghostly inlines currently don't cause files to expand, but reasonably should.
Test Plan:
Before:
{F378065}
After:
{F378066}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, epriestley
Maniphest Tasks: T7447, T7870
Differential Revision: https://secure.phabricator.com/D12495
Summary:
Ref T7447. Ref T7870.
- Forward: When rendering some file "B" which was moved from "A", port ghosts for both "A" and "B" to it.
- Backward: When porting a comment on "X" which was moved from some "Y", allow it to port to a file named "Y" if it can't find a file named "X".
Test Plan:
Before:
{F377809}
After:
{F377806}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7447, T7870
Differential Revision: https://secure.phabricator.com/D12493
Summary:
Ref T7447. This might be overkill, but I want to over-explain things until we have more confidence that this is rarely confusing.
NOTE: I'm playing it a bit fast and loose with `setIsGhost()` (passing a dictionary) because making API changes requires changing the interface and Diffusion, which is a pain. I'll clean this up at the end once the interface is more final. This is well-contained for now.
Test Plan:
- Viewed "base vs 2" in a diff with 3 diffs, saw some "older comments" and some "newer comments".
- Hovered the tags for an explanation of comment spookiness.
{F377703}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12490
Summary:
Ref T7447. After compiling inlines which will appear on the changeset, remove inlines which
Later stages remove these anyway, so it doesn't change anything to keep them around, but we can filter them out here cheaply.
This will also let us drive the Differential timeline view with the same logic a few diffs from now, to improve how it renders inlines. Generalize things a little bit.
Test Plan:
- Made a comment on the left of diff 1.
- Made diff 2.
- Viewed diff 2 vs diff 1.
- Verified old-left comment was filtered out by the new loop.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12488
Summary:
Ref T7447. This ports comments forward and backward in the best case:
- The old comment is on a changeset with the same filename.
- The old and new files are pretty much the same, line-for-line.
This will fail to port a lot of comments around and probably port a lot of comments into goofy places. We can see how bad it is in practice.
Errata:
- Design is me cobbling something together, probably worth tweaking.
- "Old Comment" should, at a minimum, say "Newer Comment" sometimes, or we should come up with some better name for this stuff.
Test Plan: {F377214}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: johnny-bit, yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12484
Summary:
Ref T7447. This class is currently a big mess with a lot of `withWeirdSpecialThingUsedInOnePlace()` type qualifiers.
Try to generalize/normalize it a bit.
Test Plan:
- Viewed inline comments.
- Created a new inline comment.
- Edited an inline comment.
- Marked an inline comment complete.
- Deleted, then undeleted an inline comment.
- Previewed inline comments.
- Viewed drafts as another user, verified they don't show up.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, epriestley
Maniphest Tasks: T7447
Differential Revision: https://secure.phabricator.com/D12483
Summary:
Ref T7803. This is a performance hack, not a real order, and isn't really meaningful or pageable.
After D12158, we constraint his query on `dateModified` anyway, which should generally give the database a relatively small result set to examine.
Test Plan: Browsed Differential and Diffusion. Checked query plan, it didn't look too crazy.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12361
Summary: These arrays looks a little odd, most likely due to the autofix applied by `ArcanistXHPASTLinter::LINT_ARRAY_SEPARATOR`. See D12296 in which I attempt to improve the autocorrection from this linter rule.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12281
Summary: Ref T7689. Ref T4100. This advances the goals of removing `loadViewerHandles()` (only 67 callsites remain!) and letting tokenizers some day take token functions like `viewer()` and `members(differential)`.
Test Plan:
- Sent a new message; used "To".
- I simplified the cancel URI construction slightly because it's moot in all normal cases.
- Edited a thread; used "Add Participants".
- Searched rooms; used "Participants".
- Searched countdowns; used "Authors".
- Created a diff; used "Repository".
- Edited a revision; edited "Projects"; edited "Reveiwers"; edited "Subscribers".
- Searched for revisions; edited "responsible users"; "authors"; "reviwers"; "subscribers"; "repositories".
- Added revision comments; edited "Add Reveiwers"; "Add Subscribers".
- Commented on a commit; edited "Add Auditors"; "Add subscribers".
- Edited a commit; edited "Projects".
- Edited a repository; edited "Projects".
- Searched feed, used "include Users"; "include Proejcts".
- Searched files, used "authors".
- Edited initiative; edited "Projects".
- Searched backers; used "Backers".
- Searched initiatives; used "Owners".
- Edited build plans; edited "Run Command".
- Searched Herald; used "Authors".
- Added signature exemption in Legalpad.
- Searhced legalpad; used "creators"; used "contributors".
- Searched signatures; used "documents"; used "signers".
- Created meme.
- Searched macros; used "Authors".
- Used "Projects" in Maniphest reports.
- Used Maniphest comment actions.
- Edited Maniphest tasks; edited "Assigned To"; edited "CC"; edited "projects".
- Used "parent" in Maniphest task creation workflow.
- Searched for projects; used "assigned to"; "in any projec"; "in all projects"; "not in projects"; "in users' projects"; "authors"; "subscribers".
- Edited Maniphest bug filing domains, used "Default Author".
- Searched for OAuth applications, used "Creators".
- Edited Owners pacakge; edited "Primary Owner"; edited "Owners".
- Searched for Owners packages; used "Owner".
- OMG this UI is OLD
- Edited a paste; edited "Projects".
- Searched for paste; used "Authors".
- Searched user activity log; used "Actors"; used "Users".
- Edited a mock; edited "Projects"; edited "CC".
- Searched for mocks; used "Authors".
- Edited Phortune account; edited "Members".
- Edited Phortune merchant account; edited "Members".
- Searched Phrequent; used "Users".
- Edited Ponder question; sued "projects".
- Searched Ponder; used "Authors"; used "Answered By".
- Added project members.
- Searched for projects; used "Members".
- Edited a Releeph product; edited "Pushers".
- Searched pull requests; searched "Requestors".
- Edited an arcanist project; used "Uses Symbols From".
- Searhced push logs; used "Repositories"; used "Pushers".
- Searched repositories; used "In nay project".
- Used global search; used Authors/owners/Subscribers/In Any Project.
- Edited a slowvote; used "Projects".
- Searched slovotes; used "Authors".
- Created a custom "Users" field; edited and searched for it.
- Made a whole lot of typos in this list. ^^^^^^
Did not test:
- Lint is nontrivial to test locally, I'll test it in production.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100, T7689
Differential Revision: https://secure.phabricator.com/D12224
Summary:
Ref T1460. Overall:
- Pass `objectOwnerPHID` consistently.
- Pass viewer consistently.
- Set the correct draft state for checkboxes on the client.
Test Plan:
- Made inline comments in Differential.
- Made inline comments in Diffusion.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12186
Summary:
This returns the PHID of the current revision owner, or the commit author, if one exists.
NOTE: For drafts, we currently return `null`; I'll fix that in a future change. Should be correct for submitted comments.
Test Plan: Added an inline, nothing seemed broken.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12185
Summary:
Fixes T7602. This is similar to the existing behavior for "changes planned" and "needs revision" revisions.
Also fix the "Update Diff" workflow so it correctly selects closed revisions as attachable.
Test Plan: Updated an abandoned revision, saw it change to "Needs Review".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7602
Differential Revision: https://secure.phabricator.com/D12167
Summary: Fixes T5658. Over a long period of time, some cruft can build up here. Only show revisions which have been updated in the last 30 days.
Test Plan:
- Viewed panel in Differential and Diffusion.
- Changed limit from 30 days to 30 seconds and saw no revisions.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5658
Differential Revision: https://secure.phabricator.com/D12158
Summary:
Ref T1460. This just barely works, but throwing it up in case any of it sounds mechanically crazy before we build integrations/UI/etc.
Specifically, these are the behaviors:
- You can mark your own draft comments as "done" before you submit them. The intent is to let reviewers mark their stuff advisory/minor/not-important before they submit it, to hint to authors that they don't expect the feedback to necessarily be addressed (maybe it's a joke, maybe it's just discussion, maybe it's "consider..").
- You can mark others' published comments as "done" if you're the revision/commit author. The intent is to keep this lightweight by not requiring an audit trail of who marked what done when. If anyone could mark anything done, we'd have to have some way to show who marked stuff.
- When you mark stuff done (or unmark it), it goes into a "draft" state, where you see the change but others don't see it yet. The intent is twofold:
- Be consistent with how inlines work.
- Allow us to publish a "epriestley updated this revision + epriestley marked 15 inlines as done" story later if we want. This seems more useful than publishing 15 "epriestley marked one thing as done" stories.
- The actual bit where done-ness publishes isn't implemented.
- UI is bare bones.
- No integration with the rest of the UI yet.
Test Plan: Clicked some checkboxes.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: paulshen, chasemp, epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12033
Summary:
Fixes T1102. If you don't use `arc`, the web workflow requires some extra needless steps when updating diffs.
Provide a more streamlined "Update Diff" workflow.
Test Plan: {F347750}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1102
Differential Revision: https://secure.phabricator.com/D12131
Summary:
Ref T2009. This is another almost-identical copy of the row scaffolding, which has the same 1up/2up bugs as the 8 other copies of this code.
Turn the "undo" element into an InlineCommentView so we can scaffold it.
Then, scaffold it with the same code as everything else.
Test Plan: Hit "Undo", swapped from 1up to 2up, hit "undo" again, swapped back, tried left/right, everything rendered with proper scaffolding.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12019
Summary:
Ref T1460. Track and store which comments are threaded replies to other comments, vs merely appearing on the same lines.
This doesn't actually write `hasReplies` yet, since that needs to happen when we un-draft comments on submission.
Test Plan: Made inline comments in Differential and Diffusion, including replies. Replies were marked as "Is Reply".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12017
Summary:
Fixes T4452. Ref T2009. There's a hierarchy of changeset rendering power: only low-level calls, use of ChangesetDetailView, then use of ChangesetListView (a list of DetailViews).
Prior to work here, the various changeset rendering controllers got their hands dirty to varying degrees, with some using only the lowest-level rendering pipeline:
- Phriction: no view (lowest level)
- Diffusion: DetailView
- Differential Changeset: DetailView
- Differential Diff: ListView
- Differential Revision: ListView
I brought Phriction up to use DetailView, but want to bring everything all the way up to use ListView. Each composition layer adds more features to diff browsing. In particular, this change enables "Highlight As", switching 1up vs 2up, adding inlines, etc., on the standalone view.
Test Plan:
- Viewed a changeset standalone. Could change highlighting, switch 1up vs 2up, add and edit inlines, etc.
- Viewed a revision; no behavioral changes.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4452, T2009
Differential Revision: https://secure.phabricator.com/D12012
Summary: Renames the method in PHUIObjectBoxView to match the new PHUIInfoView class.
Test Plan: grepped codebase. Went to Calendar and tried a new status.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12005
Summary: Ref T2009. It doesn't make sense to have these as separate behaviors. We require a ChangesetViewManager to track view parameter state.
Test Plan: Interacted with changesets in Phriction, Differential and Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11979
