Summary:
I've considered that user may have set editor but not checked out Phabricator repositories.
But stack trace is useful mainly for developers.
Test Plan:
Click on path in Unhandled Exception.
Repeat with disabled editor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2107
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2103
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2091
Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.
Test Plan:
arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2085
Summary:
- Differential, Maniphest and Diffusion use slightly different styles for the object detail panels.
- Instead, use the same styles and CSS.
- Add object actions to Diffusion, including "Flag".
Test Plan: Looked at revisions, tasks and commit. Flagged and unflagged commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2062
Summary:
I think this feature is probably good, but Differential is also really starting to get a lot of stuff which is not the diff in it. Not sure how best to deal with that.
The mixed table styles are also pretty ugly.
So I guess this is more feedback / proof-of-concept, I think I want to try to improve it somehow before I land it.
Test Plan: Looked at some diffs, some had an awkward, ugly list of diffs affecting the same files.
Reviewers: bill, aran, btrahan
Reviewed By: aran
CC: aran, epriestley
Maniphest Tasks: T829
Differential Revision: https://secure.phabricator.com/D2027
Summary:
- When an inline comment preview corresponds to an inline comment on the page, link to it. Just punt in the tough case where the inline is on some other page.
- In "haunted" mode, "z" now toggles through three modes: normal, comment area only, and comment + previews.
Test Plan:
- Viewed visible and not-visible inline comment previews, clicked "View" links.
- Tapped "z" a bunch to toggle haunt modes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T517, T214
Differential Revision: https://secure.phabricator.com/D2041
Summary: See T955. We jump to an awkard place right now; jump above the comment instead.
Test Plan: Clicked inline comment anchor links.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T955
Differential Revision: https://secure.phabricator.com/D2029
Summary: This header allows recipients to distinguish between CCs generated by Herald and CCs generated by humans.
Test Plan: Created a Herald rule to add a bunch of CC's to every revision. Created a revision. Added some CCs manually. Verified that only manual CCs appeared in the "Explicit" header.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T808
Differential Revision: https://secure.phabricator.com/D2018
Summary: Apparently I spent like a good month copy/pasting slightly different versions of this logic all over the codebase.
Test Plan: Selected "View Options -> Browse in Diffusion" for a chagneset, got a URI with a branch name in it under Git.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1993
Summary:
Depends on D1921. Depends on D1899.
Add the "View Options" dropdown menu to Diffusion, with options like "show standalone", "show raw file", "show all", etc.
Test Plan: Viewed commits in Differential and Diffusion.
Reviewers: davidreuss, nh, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1932
Summary:
- Affects the "Inline Comments" summary table which appears in comments that have attached inlines in the discussion threads in Differential.
- Prepares for inclusion in Diffusion.
- No application changes (minor CSS), just factors code better.
- Simplify/separate CSS.
Test Plan: Looked at on-diff and off-diff comment summaries in Differential, display looked correct.
Reviewers: davidreuss, nh, btrahan
Reviewed By: davidreuss
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1928
Summary: Works in Safari, Firefox, Chrome.
Test Plan: Copied some text, threw up a little in my mouth.
Reviewers: aran, tuomaspelkonen, tomo, rstout, btrahan
Reviewed By: aran
CC: aran, epriestley, ddfisher
Maniphest Tasks: T145, T995
Differential Revision: https://secure.phabricator.com/D244
Summary:
It is currently not possible to select source code covered by reticle when creating comment.
This diff hides reticle on mouseout from reply area.
Test Plan:
Hover inline comment, verify that reticle is displayed.
Reply, verify that reticle is displayed when mouseover reply, hidden otherwise.
Repeat for create.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1851
Summary: In very large diffs.
Test Plan:
Display very large diff.
Display normal diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1918
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
Summary: No big surprises here, delted the unused "DarkConsole" class.
Test Plan: Ran 'testEverythingImplemented' to verify I wasn't finalizing anything we extend.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1876
Summary:
- Show a tip in the margin about what coverage colors mean.
- Highlight the line when mousing over coverage.
- Randomly change the colors to different colors.
- Fix a bug with "show more" that I introduced with the other coverage diff (oops!)
Test Plan: Moused over coverage things.
Reviewers: tuomaspelkonen, btrahan
Reviewed By: tuomaspelkonen
CC: aran, epriestley
Maniphest Tasks: T965
Differential Revision: https://secure.phabricator.com/D1874
Summary:
We used to have "ignore all", but it became "ignore most". It does not ignore in-line whitespace changes or whitespace changes in language where whitespace is marked semantic.
Add an explicit "ignore all" option.
Test Plan: Used "ignore all" to view a change with heuristically-detected semantic whitespace, the change was ignored.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T970
Differential Revision: https://secure.phabricator.com/D1865
Summary:
Caption Submit is quite confusing because it doesn't actually make the data visible to other users.
Especially when comment confirm button is also called Submit in serious mode.
Test Plan: Add inline comment.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1796
Summary:
This does two things:
- don't fill '-' for columns where there are data from previous week
- completele hide columns if there are now new data so that noobs don't have a
55 years of history
Test Plan:
View my commits stats.
View my requested changes stats.
View epriestley FB stats.
Reviewers: vii, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1750
Summary:
Some text editors support opening multiple files at once.
I've used space as paths separator which may be compatible with some other
editors (I didn't tried any other though).
Note: This approach is incompatible with spaces in paths.
I am fine with changing it to anything else to support such paths or more
editors.
Probably the cleanest solution (yet still incompatible with most editors) would
be to use something like ##editor://open/?file=A&line=1&file=B&line=2## but it
would require also changing the way how it's configured and I think it's not
worth it.
BTW, I've used a hacky bookmarklet for this feature before.
Deleted or added paths may not exist in users filesystem but we don't know which
so the button tries to open everything.
Test Plan:
Click Edit All.
Delete Editor Link in settings, verify that the button is missing.
View diff without revision, verify that the button is missing.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1741
Summary:
Some people find the current message stating "This diff has Lint/Unit Test
Problems" confusing if the unit tests or lint was skipped. This revision
clarifies those messages.
Test Plan:
Started to accept a revision with skipped lint and unit tests, and saw the new
message.
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1738
Summary:
The current approach of using a modal overlay dialog to create/edit inline
comments is pretty silly. Use an inline textarea instead.
This element isn't perfect and we have some mild modalness issues, but I think
it's better than the silly thing we've got going on right now. We can keep
poking it as people break it.
Test Plan:
- Created comments; submitted and undid them in empty and nonempty states.
Used undo for nonempty states + cancel.
- Edited comments; saved and canceled them. Used undo for changed state.
- Replied to comments; yada yada as above.
- Deleted comments.
- Did various modal trickery where I clicked "Reply" on something else with a
dialog already up, this very mildly glitches but I think it's not a big issue.
Reviewers: vrana, btrahan, Makinde, nh
Reviewed By: vrana
CC: aran, epriestley
Maniphest Tasks: T431
Differential Revision: https://secure.phabricator.com/D1716
Summary:
I want to add comments to commits, and they should obviously share code with the
nearly-identical comments in Maniphest and Differential. Unify code/style as
much as possible.
This program made possible by a generous grant from D1513.
Test Plan:
- Looked at a bunch of different Differential and Maniphest comments; they
appeared to render identically to how they looked before.
- Tested some edge cases like anchors and "show details" on description edits
in Maniphest.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1686
Summary:
When a comments add reviewers or CCs, we just dump that sort of nastily into the
body. Put it in the header like Maniphest instead.
Also, record the diff associated with "update" actions and link to it (T871).
Test Plan: {F8546} {F8547}
Reviewers: btrahan, davidreuss
Reviewed By: davidreuss
CC: aran, epriestley
Maniphest Tasks: T871
Differential Revision: https://secure.phabricator.com/D1659
Summary:
Show some statistics, like number of revisions, number of
revisions per week, lines per revision, etc. for phrivolous amusement.
Test Plan:
- Went to /differential/stats/revisions/
Numbers seem right
- Clicked 'Accepted'
Again
- Changed to another user with long history
Load time was not too long though delay noticeable
- Clicked 'Requested changes to'
User was preserved, looks good
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1643
Summary:
Build the revision list table out of custom fields instead of hard-coding it, so
installs can add all sorts of zany things to it.
NOTE: You may need to implement sortFieldsForRevisionList() if you have a custom
DifferentialFieldSelector, or some fields might show up out of order.
This implementation will preserve the expected behavior:
public function sortFieldsForRevisionList(array $fields) {
$default = new DifferentialDefaultFieldSelector();
return $default->sortFieldsForRevisionList($fields);
}
Test Plan:
- Loaded differential revision list, identical to old list.
- Profiled page to verify the cost increase isn't significant (it's quite
small).
Reviewers: jungejason, btrahan
Reviewed By: btrahan
CC: aran, btrahan, davidreuss, epriestley
Maniphest Tasks: T773, T729
Differential Revision: https://secure.phabricator.com/D1388
Summary: It makes perfect sense to add more reviewers while requesting review.
Test Plan:
Request review. Verify that Add Reviewers field shows and works.
Add some reviewer. Verify that comment preview works.
Submit. Verify that reviewers are saved and displayed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1473
Summary:
Escaped $id is compared with non-escaped $max_id.
Escaped $id is escaped again in phutil_render_tag().
Note: $id is numeric :-).
Test Plan: Display diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1580
Summary:
This code was just all kinds of wrong, but got all the common cases anyone cares
about correct.
- In edit-inline-comments.js, if isOnRight() is true, use data.right, not
data.left (derp).
- Set data.left correctly, not to the same value as data.right (derp derp).
- Set "isNewFile" based on $is_new, not $on_right (derp derp derp).
Test Plan:
- Added JS debugging code to print "OLD" vs "NEW" and "LEFT" vs "RIGHT".
Clicked the left and right sides of diff-vs-base and diff-vs-diff diffs,
verified output was accurate in all cases.
- Added comments to the left-display-side of a diff-of-diffs, saved them, they
showed up where I put them.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T543
Differential Revision: https://secure.phabricator.com/D1567
Summary: We were not correctly updating $diff as we iterated through the loop.
Test Plan: Viewed a revision several diffs that had differing base revision.
Reviewers: fratrik, btrahan
Reviewed By: fratrik
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1523
Summary:
This diff restructures the DOM and alters some CSS within differential.
Original goal was to unify these codepaths more fully into a base class or
classes, but they have quite a bit of custom code such that didn't feel too
compelling in practice. It also felt related to feed stories as I thought
about the more general version(s) of this code...
Also deleted some CSS from maniphest that wasn't doing anything.
Test Plan:
looked at a differential diff and liked what I saw. spent a bunch
of time trying out different types of comments and etc.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T803
Differential Revision: https://secure.phabricator.com/D1513
Summary:
- Expose existing 'committed' filter.
- Add an 'accepted' filter.
- Fix a fatal where $repository may not be defined (for diffs not linked to a
repository).
Test Plan: Ran accepted / committed queries. Viewed a previously fataling diff.
Reviewers: btrahan, vrana, Makinde
Reviewed By: Makinde
CC: Koolvin, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1490
Summary:
It is possible to open a file in editor by registering a custom URI scheme
(pseudo-protocol). Some editors register it by default.
Having links to open the file in external editor is productivity booster
although it is a little bit harder to set up.
There are several other tools using file_link_format configuration directive
(XDebug, Symfony) to bind to this protocol.
I've added the example with editor: protocol which can be used as a proxy to
actual editor (used by Nette Framework:
http://wiki.nette.org/en/howto-editor-link).
Test Plan:
Configure Editor Link in User Preferences.
Register URI scheme in OS.
Open a file in Diffusion. Click on the Edit button.
Open a revision in Differential. Click on the Edit button.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1422
Summary:
Links from lint errors for large diffs don't work.
This diff adds TODO for it because I am not sure how to do it.
Move of changeset links rendering to a separate method would be still useful.
Test Plan:
Display ToC of large diff, verify link.
Repeat for small diff.
Reviewers: tuomaspelkonen, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1476
Test Plan: Display revision containing comments with no content but with inline
comments.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1474
Test Plan:
Display revision with different lint and unit results.
Hover over the stars.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1475
