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:
Split from D1921.
- DifferentialChangesetParser doesn't have this property declared.
- We weren't providing a markup engine, which caused some warnings.
- This would cause failures if comment caches weren't present. Currently, they always will be though unless someone has wiped them explicitly in the DB.
Test Plan: Viewed a diff with inline comments, didn't get any warnings in the log.
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1925
Summary:
If the 'Summary' is not present and not inferred to be empty from a newline after the title with no explicit 'Summary' field, we'll copy all the field values from the revision (including NULL), not overwrite the 'Summary' value from the message (since it's not present) and then write the NULL back to the revision.
Instead, string cast the read from the Revision so we write back empty string in the not-provided case.
Test Plan: Ran "arc diff --create --use-commit-message HEAD" with P336 in HEAD, didn't fail (previously, it failed).
Reviewers: btrahan, 20after4
Reviewed By: 20after4
CC: aran, epriestley
Maniphest Tasks: T1019
Differential Revision: https://secure.phabricator.com/D1956
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: Facebook extends this, just unfinal it for now and we can figure out the right long-term fix at our leisure.
Test Plan: Inspection.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1917
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:
If we can't look up the name for a mailing list, don't put 'Unknown Mailing
List' in the commit message - it will confuse things later down the road.
Surely there is a better way of doing this than checking the name of the
handle for the mailing list.
Test Plan:
called differential.getcommitmessage on a revision that had an invalid mailing
list phid.
Reviewers: epriestley, schrockn, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1910
Summary: I've done the same stupid copy/paste mistake twice in one day :-(.
Test Plan: Display diff with no newline at end of file in new file.
Reviewers: davidreuss, epriestley
Reviewed By: davidreuss
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1911
Summary: Text "No newline at end of file" was sent to highlighter causing error with most languages.
Test Plan: Display diff containing file with no newline at end of file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1903
Summary:
It looks like there is really this text written e.g. at https://secure.phabricator.com/D1896#0a6a1957
I am not sure that it is the only place which needs to be fixed.
Test Plan: Display diff with no newline at end of file in Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1900
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:
Currently, we mark some lines that otherwise count as "unchanged" to be "changed" when they have internal whitespace changes.
However, we've already excluded them from the intra-line diff algorithm. Unset the flag so they can get intra-line diffed.
Test Plan: Viewed a change with internal whitespace changes, internal whitespace changes were intraline diffed.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T970
Differential Revision: https://secure.phabricator.com/D1871
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:
These are all unambiguously unextensible. Issues I hit:
- Maniphest Change/Diff controllers, just consolidated them.
- Some search controllers incorrectly extend from "Search" but should extend from "SearchBase". This has no runtime effects.
- D1836 introduced a closure, which we don't handle correctly (somewhat on purpose; we target PHP 5.2). See T962.
Test Plan: Ran "testEverythingImplemented" unit test to identify classes extending from `final` classes. Resolved issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1843
Summary:
More complicated updates deserves title and explanation.
This diff uses only the first line from comment in diff's description.
It also removes the truncating at 80 chars which looks as an error.
Test Plan:
var_dump(preg_replace('/\n.*/s', '', "abc"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef\nghi"));
var_dump(preg_replace('/\n.*/s', '', "\nabc")); // empty string
Display revision with 255 chars description in update history - it looks OK.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1812
Summary: We currently parse these as directory changes and discard them. Instead, parse them as a new "SUBMODULE" type of change.
Test Plan:
- Reparsed a commit which changes submodules and verified it parses correctly.
- Reparsed a commit which adds submodules and verified it parses correctly.
Reviewers: btrahan, kdeggelman
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1815
Summary:
Currently, we sort all results alphabetically. This isn't ideal. Instead, sort them like this:
- If the viewing user appears in the list, always sort them first. This is common in a lot of contexts and some "Ben Evans" guy is sorting first on secure.phabricator.com and causing me no end of aggravation.
- If the tokens match a "priority" component (e.g., username), sort that before results which do not have a "priority" match.
- Within a group (self, priority, everything else) sort tokens alphabetically.
NOTE: I need to go add setUser() to all the tokenizers to make the "self" rule work, but that's trivial so I figured I'd get this out first.
Test Plan:
https://secure.phabricator.com/file/data/4s2a72l5hhyyqqkq4bnd/PHID-FILE-x2r6ubk7s7dz54kxmtwx/Screen_Shot_2012-03-07_at_9.18.03_AM.png
Previously, "aaaaaepriestley" (first alphabetic match) would sort before "epriestley" (the viewing user). Now, "epriestley" sorts first because that is the viewer.
https://secure.phabricator.com/file/data/rmnxgnafz42f23fsjwui/PHID-FILE-yrnn55jl3ysbntldq3af/Screen_Shot_2012-03-07_at_9.18.09_AM.png
Previously, "aaaagopher" (first alphabetic match) would sort before "banana" (the "priority" match). Now, "banana" sorts first because it priority matches on username.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T946
Differential Revision: https://secure.phabricator.com/D1807
Summary:
- Enforce proper workflow rules.
- Fix a derp-bug with patches.
Test Plan:
- Tried to mark a revision I didn't own.
- Tried to mark a revision already marked committed.
- Tried to mark a revision otherwise not accepted.
- Verified daemon can override workflow rules and mark from arbitrary states.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T948
Differential Revision: https://secure.phabricator.com/D1809
Summary: When a revision is accepted, allow users to manually mark it committed if there's a daemon/tracking problem. This shouldn't happen in most installs but we have less-robust support for Mercurial and some installs may not be fully configured.
Test Plan: Marked a revision committed.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T948
Differential Revision: https://secure.phabricator.com/D1808
Summary:
Adds an optional "Auditors" field (like "Reviewers") to commit messages which gives installs a zero-config method for making audit requests.
This field does not appear on templates unless set, and is mostly ignored (but validated and preserved) by Differential.
It is then parsed by the daemons if present, and audit requests are pushed to valid users.
Test Plan: Made an "Auditors" commit and verified it was retained with "arc amend --show". Pushed it and verified the audit was triggered.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904, T880
Differential Revision: https://secure.phabricator.com/D1793
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:
Filter Revisions button currently resets Status and Order fields.
I've rewritten it to GET form because it doesn't perform any action.
It fixed the problem along the way.
Test Plan:
/differential/filter/revisions/
Status: Open.
Filter Revisions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1771
Summary:
Displaying reviewer who was by coincidence listed first is quite confusing, especially for committed revisions.
This displays the one who really reviewed the revision if available.
This implementation is pretty bad from performance perspective - O(N) queries to retrieve all comments.
The page load still feels quite fast.
Test Plan: /differential/filter/revisions/
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1772
Summary:
We already generate patches, but currently attach them. Allow them to be inlined instead (optionally, up to a certain size).
Also allow selection between unified and git patches.
Test Plan: Set these options in my local config, sent out a diff.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T874
Differential Revision: https://secure.phabricator.com/D1759
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:
- We have a lot of headers now; document them.
- Remove the one random protip from like 3 years ago from all Differential
mail.
Test Plan: generated; read documentation
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T931
Differential Revision: https://secure.phabricator.com/D1748
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:
I want to flag messages which require an immediate action from me in e-mail
client.
It is currently not possible because Author and Reviewers fields are both in
To:.
So the filtering rule cannot recognize if I am the person who should take the
action.
This diff adds these headers:
- X-Differential-Author
- X-Differential-Reviewers
- X-Differential-CCs
Test Plan:
Send comment to the diff.
Verify X-Differential-* headers.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T808
Differential Revision: https://secure.phabricator.com/D1724
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: This control is a very thin shell right now with Maniphest/Differential
code duplication; unify the implemenations better for use in Audit.
Test Plan: Clicked toggle buttons in Differential and Maniphest.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1700
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
