Ref T10856. This is pet peeve of mine.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T10856: Small Revision page enhanchments
look at revisions.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- master
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 13124 Build 16789: Run Core Tests Build 16788: arc lint + arc unit
Event Timeline
Comment Actions
What problem does this solve?
It seems like "2 lines added, 2 lines deleted" isn't any better than "4 lines"?
Comment Actions
T10856 had this:
Because removing a 1000 line is nice, but adding 1000 lines is headache
...but I'm still not sure what the actual goal here is?
Email also has [Request, 20 lines] -- should that be [Request, 6 lines added, 14 lines deleted]? Why/why not?
Comment Actions
I think the main goal is "make it easier to estimate how much brain power/attention I need to allocate to review this", so I can decide "should I do it now".
In that vein, deleting lines requires less detailed review than adding them, so splitting the count is useful. That example is not terribly good, but "+2/-18" is much different from "+20/-0".
I pretty much forgot about the email, but I'm pretty sure it should have the detailed information, for the same reason.
Comment Actions
If that's the goal, maybe it would be better to put this information in the email and describe the entire diff?
Showing per-file information doesn't seem like the most useful information for making this judgement call (e.g., if there are 20 files, you potentially have to read through all of them before you see "1,284 lines added" on the last one), and showing it only in the table of contents seems like it's much less accessible than putting it in the email.
I believe we previously had "added" and "deleted" separately a very long time ago, but I merged them because I got several pieces of feedback complaining about moves, copies, near-moves, almost-copies, etc., being represented in a way that users disagreed with. I don't recall any complaints since then. I don't remember if they were separated in the email, UI, both, or elsewhere either. I can't find any discussion about it on this install, so it may have been pre-2011.
Comment Actions
On the table of contents specifically, an element more like the coverage bars in D15287:
...or the CLI output in git show --stat might be be better ways to represent the information:
Neither of those are quite right for this context, but a similar element (absolute size = changed lines, red/green = fraction added/removed) might be a lot quicker to glance at than "281 lines added, 192 lines removed".
We could potentially put a similar element representing the diff size in the UI and the email, with something like this:
- <= 32 lines = 1 bar
- <= 128 lines = 2 bars
- <= 512 lines = 3 bars
- <= 2048 lines = 4 bars
- > 2048 lines = 5 bars
...and then color them by added/removed, so you'd have a consistent way to visually estimate the size of diffs at a glance. This would potentially create some weird incentives (e.g., chopping your 35-line diff down 3 lines to get it out of 2-bar range) but seems like a strong approach to making it very easy to quickly identify the size of changes to me.
That said, I'm not really sure how valuable any of this is. As far as I can recall, everyone who has ever wanted improvements here has been trying to find ways to ignore diffs they don't want to review. If we keep these features "bad" (i.e., very slightly inconvenient to parse visually), does that just mean more diffs get reviewed faster?
Comment Actions
I don't have a very strong case here; I think I get most of my review
requests as "added reviewers", not as "request", so I'm thinking more in
terms of the page.
I'm also happy to just drop it, it's pretty down the list of importance.
Comment Actions
(On the "bars" thing, I just mean this...)
2 lines added, 18 lines removed:
[ ]
300 lines added, 174 lines removed:
[ ]
So you could quickly see "5-bar green = never ever review this" or whatever.
Comment Actions
I'm open to exploring attacks on "make it easier to quickly guess how long a revision will take to review", but this particular approach feels to me like it adds a lot of clutter and only makes guessing a little easier and a little more accurate.
The best approach I can come up to attack this problem with is developing some kind of consistent element, like the "5-star system" above, to show the relative size/nature of a diff at a glance and in a way that's comparable across revisions, so you can quickly guess that a [- ] diff is probably a quick read, and that [---- ] and [+++- ] diffs are both big but the first one is probably a good bit quicker to review. I don't know that this is necessarily the best approach specifically, but I'd like to pursue something in this general vein (a simple visual element with few actual numbers) rather than adding more text which you have to scan through and do math on.
I generally favor erring on the side of "general idea" over "specific numbers" based on experience with users arguing about specific numbers for moved/copied code, when it usually does not really matter too much in practice. This isn't a big issue, but I think being more specific invites some level of nitpicking and confusion without providing any benefit, since I think it never actually matters if a diff is 197 lines of 219 lines, just that it's "about 200 lines".
I'm also not entirely convinced that making this easier is actually valuable/good (one "root problem" it resolves is making it easier to ignore diffs you don't want to review, but that might be a bad thing to make easy), but I'm not really worried about this, since it's already easy for users to find an excuse not to review something.