Summary:
Ref T13161.
- Don't show ">>" when the line indentation changed but the text also changed, this is just "the line changed".
- The indicator seems a little cleaner if we just reuse the existing "bright" colors, which already have colorblind colors anyway.
Test Plan: Got slightly better rendering for some diffs locally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20195
Summary:
Ref T12822. Ref T2495. This is the good version of D20193.
Currently, we display various nonprintable characters (ZWS, nonbreaking space, various control characters) as themselves, so they're generally invisible.
In T12822, one user reports that all their engineers frequently type ZWS characters into source somehow? I don't really believe this (??), and this should be fixed in lint.
That said, the only real reason not to show these weird characters in a special way was that it would break copy/paste: if we render ZWS as "🐑", and a user copy-pastes the line including the ZWS, they'll get a sheep.
At least, they would have, until D20191. Now that this whole thing is end-to-end Javascript magic, we can copy whatever we want.
In particular, we can render any character `X` as `<span data-copy-text="Y">X</span>`, and then copy "Y" instead of "X" when the user copies the node. Limitations:
- If users select only "X", they'll get "X" on their clipboard. This seems fine. If you're selecting our ZWS marker *only*, you probably want to copy it?
- If "X" is more than one character long, users will get the full "Y" if they select any part of "X". At least here, this only matters when "X" is several spaces and "Y" is a tab. This also seems fine.
- We have to be kind of careful because this approach involves editing an HTML blob directly. However, we already do that elsewhere and this isn't really too hard to get right.
With those tools in hand:
- Replace "\t" (raw text / what gets copied) with the number of spaces to the next tab stop for display.
- Replace ZWS and NBSP (raw text) with a special marker for display.
- Replace control characters 0x00-0x19 and 0x7F, except for "\t", "\r", and "\n", with the special unicode "control character pictures" reserved for this purpose.
Test Plan:
- Generated and viewed a file like this one:
{F6220422}
- Copied text out of it, got authentic raw original source text instead of displayed text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822, T2495
Differential Revision: https://secure.phabricator.com/D20194
Summary:
Depends on D20185. Ref T13161. Fixes T6791.
See some discusison in T13161. I want to move to a world where:
- whitespace changes are always shown, so users writing YAML and Python are happy without adjusting settings;
- the visual impact of indentation-only whitespace changes is significanlty reduced, so indentation changes are easy to read and users writing Javascript or other flavors of Javascript are happy.
D20181 needs a little more work, but generally tackles these visual changes and lets us always show whitespace changes, but show them in a very low-impact way when they're relatively unimportant.
However, a second aspect to this is how the diff is "aligned". If this file:
```
A
```
..is changed to this file:
```
X
A
Y
Z
```
...diff tools will generally produce this diff:
```
+ X
A
+ Y
+ Z
```
This is good, and easy to read, and what humans expect, and it will "align" in two-up like this:
```
1 X
1 A 2 A
3 Y
4 Z
```
However, if the new file looks like this instead:
```
X
A'
Y
Z
```
...we get a diff like this:
```
- A
+ X
+ A'
+ Y
+ Z
```
This one aligns like this:
```
1 A
1 X
2 A'
3 Y
4 Z
```
This is correct if `A` and `A'` are totally different lines. However, if `A'` is pretty much the same as `A` and it just had a whitespace change, human viewers would prefer this alignment:
```
1 X
1 A 2 A'
3 Y
4 Z
```
Note that `A` and `A'` are different, but we've aligned them on the same line. `diff`, `git diff`, etc., won't do this automatically, and a `.diff` doesn't have a way to say "these lines are more or less the same even though they're different", although some other visual diff tools will do this.
Although `diff` can't do this for us, we can do it ourselves, and already have the code to do it, because we already nearly did this in the changes removed in D20185: in "Ignore All" or "Ignore Most" mode, we pretty much did this already.
This mostly just restores a bit of the code from D20185, with some adjustments/simplifications. Here's how it works:
- Rebuild the text of the old and new files from the diff we got out of `arc`, `git diff`, etc.
- Normalize the files (for example, by removing whitespace from each line).
- Diff the normalized files to produce a second diff.
- Parse that diff.
- Take the "alignment" from the normalized diff (whitespace removed) and the actual text from the original diff (whitespace preserved) to build a new diff with the correct text, but also better diff alignment.
Originally, we normalized with `diff -bw`. I've replaced that with `preg_replace()` here mostly just so that we have more control over things. I believe the two behaviors are pretty much identical, but this way lets us see more of the pipeline and possibly add more behaviors in the future to improve diff quality (e.g., normalize case? normalize text encoding?).
Test Plan:
{F6217133}
(Also, fix a unit test.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T6791
Differential Revision: https://secure.phabricator.com/D20187
Summary:
Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely.
I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction.
Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T3498
Differential Revision: https://secure.phabricator.com/D20185
Summary:
Ref T13161. See PHI723. Our whitespace handling is based on whitespace flags like `diff -bw`, mostly just for historical reasons: long ago, the easiest way to minimize the visual impact of indentation changes was to literally use `diff -bw`.
However, this approach is very coarse and has a lot of problems, like detecting `"ab" -> "a b"` as "only a whitespace change" even though this is always semantic. It also causes problems in YAML, Python, etc. Over time, we've added a lot of stuff to mitigate the downsides to this approach.
We also no longer get any benefits from this approach being simple: we need faithful diffs as the authoritative source, and have to completely rebuild the diff to `diff -bw` it. In the UI, we have a "whitespace mode" flag. We have the "whitespace matters" configuration.
I think ReviewBoard generally has a better approach to indent depth changes than we do (see T13161) where it detects them and renders them in a minimal way with low visual impact. This is ultimately what we want: reduce visual clutter for depth-only changes, but preserve whitespace changes in strings, etc.
Move toward detecting and rendering indent depth changes. Followup work:
- These should get colorblind colors and the design can probably use a little more tweaking.
- The OneUp mode is okay, but could be improved.
- Whitespace mode can now be removed completely.
- I'm trying to handle tabs correctly, but since we currently mangle them into spaces today, it's hard to be sure I actually got it right.
Test Plan: {F6214084}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20181
Summary:
Ref T13249. Ref T11738. See PHI985. Currently, we have a crude heuristic for guessing what line in a source file provides the best context.
We get it wrong in a lot of cases, sometimes selecting very silly lines like "{". Although we can't always pick the same line a human would pick, we //can// pile on heuristics until this is less frequently completely wrong and perhaps eventually get it to work fairly well most of the time.
Pull the logic for this into a separate standalone class and make it testable to prepare for adding heuristics.
Test Plan: Ran unit tests, browsed various files in the web UI and saw as-good-or-better context selection.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249, T11738
Differential Revision: https://secure.phabricator.com/D20171
Summary: Depends on D19837. Ref T13216. See PHI985. There's an off-by-one error here between how inline comments store "length" and how context rendering treats "length". We need to add 1 to the length, but currently do it a little too early. Do it slightly later so that inlines on the final line of a file render properly.
Test Plan: Left an inline on the final line of a new file, saw it render properly in HTML mail.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19838
Summary:
Ref T13137. The "analyze/cache data about changesets" step is becoming more involved. We recently added detection for generated code to support "Ignore generated changes" in Owners, and I now plan to hash the new file content so we can hide changes which have no effect.
Before adding this new hashing step, pull the "detect copied code" and "detect generated code" stuff out and move them to a separate `ChangesetEngine`. Then support doing a changeset rebuild directly with `bin/differential rebuild-changesets`.
This simplifies things a bit and makes testing easier since you don't need to keep creating new revisions to re-run copy/generated/hash logic.
Test Plan: Ran `bin/differential rebuild-changesets --revision Dxxx`, saw changesets rebuild. See also next change.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19452
Summary:
Ref T13130. See PHI251. Currently, changesets are marked as "generated" (i.e., the file contains generated code and does not normally need to be reviewed) at display time.
An install would like support for having Owners rules ignore generated files. Additionally, future changes anticipate making "generated" and some other similar behaviors more flexible and more general.
To support these, move toward a world where:
- Changesets have "attributes": today, generated. In the future, perhaps: third-party, highlight-as, encoding, enormous-text-file, etc.
- Attributes are either "trusted" (usually: the server assigned the attribute) or "untrusted" (usually: the client assigned the attribute). For attributes like "highlight-as", this isn't relevant, but I'd like to provide tools so that you can't make `arc` mark every file as "generated" and sneak past review rules in the future.
Here, the `differential.generated-paths` config can mark a file as "generated" with a trusted attribute. The `@generated`-in-content rule can mark a file as "generated" with an untrusted attribute.
Putting these attributes on changesets at creation time instead of display time will let Owners interact with changesets cheaply: it won't have to render an entire changeset just to figure out if it's generated or not.
Test Plan:
- Created a revision touching several files, some generated and some not.
- Saw the generated files get marked properly with attribute metadata in the database, and show/fold as "Generated" in the UI.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19425
Summary: Depends on D19056. Fixes T8475. Ref T13054. Merges "ModernHunk" back into "Hunk".
Test Plan: Grepped for `modernhunk`. Reviewed revisions. Created a new revision. Used `bin/differential migrate-hunk` to migrate hunks between storage formats and back.
Maniphest Tasks: T13054, T8475
Differential Revision: https://secure.phabricator.com/D19057
Summary:
Depends on D18740. Prepares `arc` to receive a `--draft` flag by letting us switch to "differential.revision.edit" instead of "differential.createrevision".
To "differential.revision.edit", we need a transaction list, but we can't automatically construct this list from a field map. Return the transaction list alongside the field map.
The next change uses this list (if available) to switch us to the modern API method.
Test Plan: Ran `arc diff` on the experiemntal branch with followup changes, got a new revision.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18741
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary: Adds a very basic list of all inline comments, threaded, and their status. Kept this a little simpler than the mock, mostly because sorting here feels a little strange given threads would be all over the place. Not sure sorted is needed in practice anyways. I'd probably lean towards just adding a JS checkbox to hide certain rows if needed in the future.
Test Plan:
Test various commenting structures:
- Leave Comment
- Update Diff
- Leave new comment
- Reply to comment
- Reply to comment as revision author
- Mark items as done
- Update diff again
{F4996915}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18112
Summary: Fixes T10312. If your first line is "Reviewers: xyz", it's a title, not a "Reviewers" field.
Test Plan: Added unit test.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10312
Differential Revision: https://secure.phabricator.com/D17122
Summary:
Fixes T8360. We will now parse revisions out of "Differential Revision: X" followed by other ad-hoc fields which we do not recognize. Previously, these fields would be treated as part of the value.
(In the general case, other fields may line wrap so we can't assume that fields are only one line long. However, we can make that assumption safely for this field.)
Also maybe fix whatever was going on in T9965 although that didn't really have a reproduction case.
Test Plan: Added unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8360
Differential Revision: https://secure.phabricator.com/D17121
Summary: See D17058.
Test Plan: Ran `arc diff`, which parsed fields as a side effect.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17112
Summary: Ref T11114. Missed this while converting.
Test Plan: Tried to create a revision with no test plan. Before: fatal; after: helpful message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17061
Summary:
Ref T11114. See that task for some discussion.
Overall, Differential custom fields ended up with too many responsibilities. Later work in EditEngine provides a more promising model for achieving modularity with smaller, more consistent components.
In particular, we have some custom fields like `DifferentialGitSVNIDField` and `DifferentialConflictsField` which serve //only// to support the field parser.
This starts pulling commit message responsibilities out of the core list of custom fields and into simpler dedicated parsers.
Test Plan: Created and edited revisions from the CLI. Added a bit of test coverage.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17058
Summary:
Ref T11114. I want to move this step away from custom fields. To start with, isolate all the parsing in one class with a clearer API boundary.
Next, I'll make this class use new field objects to perform parsing, without CustomField interactions.
Test Plan: Created and edited revisions from the CLI, using valid and invalid commit messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17055
Summary:
Ref T11085. To recreate the issue:
- From the web UI, click "Edit Revision".
- Write something like this as your "Summary" (i.e., put another field marker, like "Test Plan:", into the summary):
> This is a test of the
> Test Plan: field to see
> if it works.
- Save changes.
Later, when the summary is amended into a commit message, the parser will see two "Test Plan:" fields and fail to parse the message.
Instead, prevent users from making this edit.
Test Plan: {F1917640}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11085
Differential Revision: https://secure.phabricator.com/D16846
Summary:
Ref T4103. This doesn't get everything, but takes care of most of the easy stuff.
The tricky-ish bit here is that I need to move timezones, pronouns and translations to proper settings. I expect to pursue that next.
Test Plan:
- Grepped for `loadPreferences` to identify callsites.
- Changed start-of-week setting, loaded Calendar, saw correct start.
- Visited welcome page, read "Adjust Settings" point.
- Loaded Conpherence -- I changed behavior here slightly (switching threads drops the title glyph) but it wasn't consistent to start with and this seems like a good thing to push to the next version of Conpherence.
- Enabled Filetree, toggled in Differential.
- Disabled Filetree, no longer visible in Differential.
- Changed "Unified Diffs" preference to "Small Screens" vs "Always".
- Toggled filetree in Diffusion.
- Edited a task, saw sensible projects in policy dropdown.
- Viewed user profile, uncollapsed/collapsed side nav, reloaded page, sticky'd.
- Toggled "monospaced textareas", used a comment box, got appropriate fonts.
- Toggled durable column.
- Disabled title glyphs.
- Changed monospaced font to 18px/36px impact.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16004
Summary:
Ref T10769. The user availability cache write shouldn't happen in read-only mode, nor should the Differential parse cache write.
(We might want to turn off the availbility feature completely since it's potentially expensive if we can't cache it, but I think we're OK for now.)
Test Plan:
In read-only mode:
- Browsed as a user with an out-of-date availability cache.
- Loaded an older revision without cached parse data.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10769
Differential Revision: https://secure.phabricator.com/D15988
Summary: Fixes T9790. This uses a simple renderer, like the inline context renderer, that emphasizes getting a quick glance at small changes and working reasonably on mobile devices.
Test Plan:
- Set `inline` setting to `9999`.
- Created a diff.
- Saw it render reasonably in HTML mail.
- Also tested text mail to make sure I didn't break that.
{F1310137, size=full}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9790
Differential Revision: https://secure.phabricator.com/D15901
Summary:
Ref T10694. This mostly prevents us from having a degenerate case if someone leaves a 200-line inline.
- For one-line inlines, show 1 line of context above and below (3 lines total).
- For 3+ line inlines, show just the inline.
- For 7+ line inlines, show only the first part.
Test Plan: Made a bunch of weird long/short/different-sized comments, saw reasonble-appearing context in text and HTML mail output.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15853
Summary:
Ref T10694. Ref T9790. When generating inline diff context, highlight it and then mangle the highlighted output into `style="..."` so it works in HTML.
Also try to tighten up some spacing/formatting stuff.
Test Plan:
Got some output in this vein:
{F1259937}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9790, T10694
Differential Revision: https://secure.phabricator.com/D15852
Summary:
Ref T10563. This isn't a complete fix, but should make viewing complex inline threads a little more manageable.
This just tries to put stuff in thread order instead of in pure chronological order. We can likely improve the display treatment -- this is a pretty minimal approach, but should improve clarity.
Test Plan:
T10563 has a "before" shot. Here's the "after":
{F1169018}
This makes it a bit easier to follow the conversations.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10563
Differential Revision: https://secure.phabricator.com/D15459
Summary:
Fixes T9268. Currently, we try to match any string like "a2f313f1" as a commit/revision, so short hashes will get picked up.
However, we don't require a word boundary or terminal after the match, so for input like "aaa...aaaaz" the engine can get stuck trying to split the string into sub-matches.
That is, in the original case, the input "aaaz" had valid matches against `[rA-Z0-9a-f]+` up to "z" of:
aaa
aa a
a aa
a a a
All of these will fail once it hits "z", but it has to try them all. This complexity is explosive with longer strings.
Instead, require a word boundary or EOL after the match, so this is the only valid match:
aaa
Then the engine sees the "z", says "nope, no match" and doesn't have to backtrack across all possible combinations.
Test Plan: Added a failing unit test, applied patch, clean test.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9268
Differential Revision: https://secure.phabricator.com/D13997
Summary: Return `$this` from setter methods for consistency. I started writing a [[https://secure.phabricator.com/differential/diff/32506/ | linter rule]] to detect this, but I don't think it is trivial to do this properly.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13422
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary: These format strings use `%d` instead of `%s`.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12996
Summary:
Ref T7447. Fixes T7600. This likely needs significant adjustment, but implements content-aware comment porting for line changes.
Specifically, this moves lines around to adjust their position considering added and removed lines between the diffs and across rebases.
It does not try to do any actual content (line against line) matching.
Test Plan:
- Unit tests.
- Poking around in the web UI seems to generate mostly reasonable-ish results?
- This may be a huge step backward in some cases that I just haven't hit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, epriestley
Maniphest Tasks: T7600, T7447
Differential Revision: https://secure.phabricator.com/D12741
Summary: Ref T7776. This could get better, but I think I got most of the big stuff. It's ~4x faster now.
Test Plan:
Before:
{F393338}
After:
{F393339}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7776
Differential Revision: https://secure.phabricator.com/D12730
Summary:
Ref T7447. Ref T7870. See T7870 for a detailed description of this issue.
NOTE: Replying to these inlines from the UI still does the wrong thing, because we use the database line number, not the UI line number.
Test Plan:
Before:
{F377732}
After:
{F377733}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7447, T7870
Differential Revision: https://secure.phabricator.com/D12491
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:
Ref T1266. We won't detect a move/copy if fewer than 3 lines are changed.
However, you may move a block like:
Complicated Line A
Trivial Line B
Complicated Line C
...where "Trivial Line B" is something like a curly brace. If you move this block somewhere that happened to previously have a similar trivial curly brace line, we won't be able to find 3 contiguous added lines in order to detect the copy/move.
Instead, consider both changed and unchanged lines when trying to find contiguous blocks. This allows us to detect across gaps where lines were not actually changed.
This new algorithm may be too liberal (for example, we may end up incorrectly identifying moved/copied code before or after changed lines, not just between changed lines), but we can keep an eye on it and tweak it. The algorithm is better factored and better covered, now.
Test Plan:
- Added a unit test for this case.
- Spot-checked a handful of diffs and generally saw behavior that made sense and looked better than before.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1266
Differential Revision: https://secure.phabricator.com/D12146
Summary:
Ref T1266. This doesn't change any behaviors, but some of this code has a lot of really complicated conditionals and I tried to break that up a bit.
Also, reexpress this stuff in terms of the "structured" parser in D12144.
Test Plan: Unit tests still pass. They aren't hugely comprehensive but did reliably fail when I screwed stuff up.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1266
Differential Revision: https://secure.phabricator.com/D12145
Summary:
Ref T5644. See some discussion in D8040.
When a file is very large (more than 64KB of text), don't activate syntax highlighting by default. This should prevent us from wasting resources running `pygmentize` on enormous files.
Users who want the file highlighted can still select "Highlight As...".
The tricky part of this diff is separating the headers into "changeset" headers and "undershield" (rendering) headers. Specifically, a file might have these headers/shields:
- "This file is newly added."
- "This file is generated. Show Changes"
- "Highlighting is disabled for this large file."
In this case, I want the user to see "added" and "generated" when they load the page, and only see "highlighting disabled" after they click "Show Changes". So there are several categories:
- "Changeset" headers, which discuss the changeset as a whole (binary file, image file, moved, added, deleted, etc.)
- "Property" headers, which describe metadata changes (not relevant here).
- "Shields", which hide files from view by default.
- "Undershield" headers, which provide rendering information that is only relevant if there is no shield on the file.
Test Plan:
- Viewed a diff with the library map, clicked "show changes", got a "highlighting disabled" header back with highlighting disabled.
- Enabled highlighting explicitly (this currently restores the shield, which it probably shouldn't, but that feels out of scope for this change). The deshielded file is highlighted per the user's request.
- Loaded context on normal files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T5644
Differential Revision: https://secure.phabricator.com/D12132
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 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:
Ref T2009. Currently, we do not persist view parameters when making context rendering requests.
The big one is the renderer (1up vs 2up). This makes context on unified diffs come in with too many columns.
However, it impacts other parameters too. For example, at HEAD, if you change highlighting to "rainbow" and then load more context, the context uses the original highlighter instead of the rainbow highlighter.
This moves context loads into ChangesetViewManager, which maintains view parameters and can provide them correctly.
- This removes "ref"; it is no longer required, as the ChangesetViewManager tracks it.
- This removes URI management from `behavior-show-more`; it is no longer required, since the ChangesetViewManager knows how to render.
- This removes "whitespace" since this is handled properly by the view manager.
Test Plan:
- Used "Show Top" / "Show All" / "Show Bottom" in 1-up and 2-up views.
- Changed file highlighting to rainbow, loaded stuff, saw rainbow stick.
- Used "Show Entire File" in 1-up and 2-up views.
- Saw loading chrome.
- No loading chrome normally.
- Made inlines, verified `copyRows()` code runs.
- Poked around Diffusion -- it is missing some parameter handling, but works OK.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11977
Summary:
See D11468 and D11465. Fixes T5163. Fixes T4105. This makes it practical to test shields, unshielding, moves, etc.
This fixes the issue in D11468, where line maps from whitespace-ignored hunks could have fewer lines than line maps from whitespace-respected hunks, causing a warning.
This encodes the behavior which D11465 changed, making it the canon behavior. Specifically, we do **not** show a shield. I think this is correct. It seems misleading to show "the contents of this file were not changed", because they were changed in both the sense that the file was completely removed, and also changed in the sense that the content itself was (or may have been) changed at the destination. Instead, we just show nothing.
Test Plan:
- Added test coverage.
- Ran tests.
- Used `arc diff --raw --browse` to verify that web behavior was consistent with CLI/test behavior.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4105, T5163
Differential Revision: https://secure.phabricator.com/D11970
Summary: Rename `DifferentialChangesetParser::WHITESPACE_IGNORE_FORCE` to `DifferentialChangesetParser::WHITESPACE_IGNORE_ALL` to better reflect reality.
Test Plan: Viewed a diff with various settings for the "Whitespace changes" option.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11730
Summary: Rename `DifferentialChangesetParser::WHITESPACE_IGNORE_ALL` to `DifferentialChangesetParser::WHITESPACE_IGNORE_MOST`.
Test Plan: Browsed a diff with a few different settings for "Whitespace changes".
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11715
Summary:
Ref T7094. We should do a policy query on the files IMO because there exists a scenario where the file gets locked down directly. This requires being a bit more disciplined about setting user, which in turn requires deciding whether or not to show edit / reply links as a separate piece of logic, not conditional on user presence.
This is not the best code but I don't think it gets worse with this and is just some other nuance in any larger cleanup we take on someday.
Test Plan: looked at a revision and noted inline comments rendered correctly with reply / edit actions. looked at a diff standalone and noted no reply / edit actions as expected. looked at a "details" link on a transaction and it rendered correctly. looked at a diff in phriction of page edits and it looked good. grepped around and verified the remaining callsite in diffusion already has the setUser call.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11579
Summary: Fixes T6608, though I'll also clean up the comment for PhutilStringTruncator in another diff. If I understand correctly, before T1191, MySQL column length was by character count and post T1191 its by byte count. Ergo, most of these changes are going from codepoint -> bytes. See test plan for complete list of what was and was not done.
Test Plan:
Thought very carefully about each callsite and made changes as appropos. "Display" means the string is clearly used for display-only purposes and correctly uses "glyph" already.
grep -rn PhutilUTF8StringTruncator *
applications/calendar/query/PhabricatorCalendarEventSearchEngine.php:217: ->addAttribute(id(new PhutilUTF8StringTruncator()) -- display
applications/chatlog/controller/PhabricatorChatLogChannelLogController.php:111: $author = id(new PhutilUTF8StringTruncator()) -- display
applications/conduit/method/ConduitConnectConduitAPIMethod.php:62: $client_description = id(new PhutilUTF8StringTruncator()) -- was codepoint, changed to bytes
applications/conpherence/view/ConpherenceFileWidgetView.php:22: ->setFileName(id(new PhutilUTF8StringTruncator()) -- display
applications/differential/controller/DifferentialDiffViewController.php:65: id(new PhutilUTF8StringTruncator()) -- display
applications/differential/event/DifferentialHovercardEventListener.php:69: id(new PhutilUTF8StringTruncator()) -- display
applications/differential/parser/DifferentialCommitMessageParser.php:144: $short = id(new PhutilUTF8StringTruncator()) -- was glyphs, made to bytes
applications/differential/view/DifferentialLocalCommitsView.php:80: $summary = id(new PhutilUTF8StringTruncator()) -- display
applications/diffusion/controller/DiffusionBrowseFileController.php:686: id(new PhutilUTF8StringTruncator()) -- display
applications/feed/story/PhabricatorFeedStory.php:392: $text = id(new PhutilUTF8StringTruncator()) -- display, unless people are saving the results of renderSummary() somewhere...
applications/harbormaster/storage/build/HarbormasterBuild.php:216: $log_source = id(new PhutilUTF8StringTruncator()) -- was codepoints now bytes
applications/herald/storage/transcript/HeraldObjectTranscript.php:55: // NOTE: PhutilUTF8StringTruncator has huge runtime for giant strings. -- not applicable
applications/maniphest/export/ManiphestExcelDefaultFormat.php:107: id(new PhutilUTF8StringTruncator()) -- bytes
applications/metamta/storage/PhabricatorMetaMTAMail.php:587: $body = id(new PhutilUTF8StringTruncator()) -- bytes
applications/people/event/PhabricatorPeopleHovercardEventListener.php:62: id(new PhutilUTF8StringTruncator()) -- display
applications/phame/conduit/PhameCreatePostConduitAPIMethod.php:93: id(new PhutilUTF8StringTruncator()) -- was codepoints, now bytes
applications/pholio/storage/PholioTransaction.php:300: id(new PhutilUTF8StringTruncator()) -- display
applications/phortune/provider/PhortuneBalancedPaymentProvider.php:147: $charge_as = id(new PhutilUTF8StringTruncator()) -- bytes
applications/ponder/storage/PonderAnswerTransaction.php:86: id(new PhutilUTF8StringTruncator()) -- display
applications/ponder/storage/PonderQuestionTransaction.php:267: id(new PhutilUTF8StringTruncator()) -- display
applications/ponder/storage/PonderQuestionTransaction.php:276: id(new PhutilUTF8StringTruncator()) -- display
applications/repository/storage/PhabricatorRepositoryCommitData.php:43: $summary = id(new PhutilUTF8StringTruncator()) -- was codepoints, now bytes
applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php:20: $data->setAuthorName(id(new PhutilUTF8StringTruncator()) -- was codepoints, now bytes
applications/slowvote/query/PhabricatorSlowvoteSearchEngine.php:158: $item->addAttribute(id(new PhutilUTF8StringTruncator()) -- display
infrastructure/daemon/workers/query/PhabricatorWorkerLeaseQuery.php:317: $host = id(new PhutilUTF8StringTruncator()) -- bytes
view/form/control/AphrontFormPolicyControl.php:61: $policy_short_name = id(new PhutilUTF8StringTruncator()) -- glyphs, probably display only
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6608
Differential Revision: https://secure.phabricator.com/D11219
