Page MenuHomePhabricator

Plans: Form Steering Committee For "+/-" Line Indicators
Open, NormalPublic

Assigned To
Authored By
Apr 17 2018, 11:45 PM
Referenced Files
F5546176: Screen Shot 2018-04-26 at 3.44.21 PM.png
Apr 26 2018, 10:48 PM
"Piece of Eight" token, awarded by scp."The World Burns" token, awarded by richardvanvelzen."Hungry Hippo" token, awarded by eadler."Dat Boi" token, awarded by avivey.


In 2018 Week 15 (Mid April), I shipped a new [+++- ] element to provide a quick visual indicator of revision sizes. The original request for this was PHI230. I dragged my heels for a long time on this and made various claims like these:

On the value of the feature:

(PHI230) I'm also not sure this really serves any use case except "make it easier to ignore large revisions I don't want to review".
(D16322) 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.
(T376) I think the root problem here is that people are worried that other people are ignoring their diffs because of a large linecount, possibly because they themselves ignore diffs with a large linecount. I think this is basically a social problem. Line count should be a curio, not something people actually pay real attention to or make decisions based on. If anything, we should simply remove it.

On the behavior:

(PHI230) Also if anyone complains that the line counts are off because moved lines should/should not be counted and/or blank lines should/should not count/should only count on a log scale I am deleting the feature forever.
(T376) There are too many types of changes (like generated files, copies, deletes, indentation changes) which make the line count larger than it "should" be. Many of these don't have clear-cut "right" ways to count line changes (people will continue to complain that certain types of changes were or weren't counted), and some are technically difficult or prohibitively expensive to count.

This has now shipped, and we've collected some feedback:

  • (PHI569) The element was generally "very well received".
  • (PHI579) The element was generally received with "meh".
  • (PHI587) The colors are too bright and the element stands out too much.
  • (PHI569) The element is too hard to see and does not stand out enough, particularly for colorblind users.
  • (PHI579) Red and green are good color choices.
  • (PHI587) Red and green are poor color choices, and should be shades of blue.
  • (PHI579) Grey is a poor color choice, and should be orange.
  • (PHI579) "I hate the colors. ... i hate it"
  • (PHI579) The meaning of the element is completely unclear.
  • (Z2413) The element is confusing, but you can understand what it means if you hover over it.
  • (Z2413) The element is fairly clear and the rules driving it make reasonable sense.
  • (PHI230) Generated lines should be excluded from line counts.
  • (PHI230) Revision size information should be readily available on the dashboard.
  • (PHI579) Revision size information should be hidden by default and require hovering over a tooltip.
  • (PHI579) Revision size information should be far away from the left side of the UI.
  • (PHI579) This feels like a WIP feature which is sufficiently broken/harmful that it should be turned off until it is more stable.

Broadly, all incarnations of this feature have invited an enormous amount of bikeshedding for the last decade. I think this feature is second only to button text (T10000) as a magnet for bikeshedding.

I think part of the core problem with this feature is that it isn't really solving a very concrete or well-defined problem and the information is barely of any importance, so all decisions about it are mostly aesthetic decisions.

The feedback is so wide-ranging and contradictory that no version of this element will satisfy everyone, so I currently plan to continue changing it at random as the mood strikes me.

At least as of time of writing, a piece of feedback I haven't received is "this should just be a number, the visual element is worse than a number", so I consider this change a resounding success overall. (But I currently have one unread followup on one of the support issues which probably says "maybe this should just be a number".)

These are likely future changes:

  • With "Red/Green" colorblind mode enabled, it is extremely likely that the element will stop using red/green colors in the future.
  • The use of red/grey/green in default color mode may also change.
  • The tooltip may get a subjective hint about the color, e.g. "Smaller Change" or "Larger Change".
  • I generally think that the element stands out too much relative to the importance of the information it conveys today. It is likely that future versions of the element will be less prominent than the version that exists today. I may decrease prominence mostly by changing the colors.
  • It is not currently trivial to exclude generated changes from line counts. If this becomes easy in the future (PHI64, T784 are possible avenues) it is likely that future versions of this element will not count generated lines.
  • It is not currently trivial to exclude moved lines from line counts. If this becomes easy in the future, it is likely that future versions of this element will not count moved lines.
  • It is not currently trivial to exclude blank lines from line counts. I think it's unlikely that this will change, but perhaps.
  • (In all "what counts as a line" cases, I feel emboldened to try to make this element reflect "review difficulty" rather than "exact change size" because the move away from actual line numbers to a visual representation seems to have been successful.)
  • The current line counts in the table of contents may become more visual, to echo this element.

Event Timeline

epriestley created this task.

I feel your instincts are correct, this information isn't useful in a list form and shouldn't be included. You could make the argument to include it in a hovercard, or bring it into the header on the diff page, and I think those are both fine. Outside of that this seems mostly geared towards avoiding code review, a practice I don't think is well aligned with the overall product goals.

I think there's possibly at least some argument that this element could serve noble goals:

  • encourage quick review of small revisions;
  • encourage authors to break apart larger revisions.

My intent was to build an element that at least tries to do these things, i.e. be the best possible version of this information that can be presented. But I'm not sure it succeeds.

Historically, I think every time I try to remove/reduce line counts we get a fair number of requests to return/increase them. This was a number of years ago, but I believe I removed them from email subject lines entirely a long time ago (at Facebook? From blame, it was before T2222, at least, but I don't remember if it was pre-2011 or post-2011) and got a mountain of pushback.

It's not entirely clear to me why this invites as much attention as it does. I don't use line counts personally and don't really understand the motivation other than "ignore large revisions", but it's a little hard for me to believe that everyone requesting this feature is really just motivated by naked laziness. GitHub doesn't have this feature and the isaacs/github request for it (#307) barely seems to have any interest. I'm not exactly sure what to make of this.

Instead of fighting line sizes, we could embrace them. Examples of changes which would embrace line sizes (not necessarily good features) might include: warnings in arc about large changes taking longer to review (at least one install has added a custom warning like this); UI warnings that flag large changes as cursed (the [+++- ] element going orange for big changes is a small step in this direction); rejecting these changes outright in arc; collecting statistics on size-vs-time-to-review (T12177) and showing them to the user ("This change is larger than 80% of changes, and will typically take 3.5x longer to review than a small change. Are you SURE you want to continue with your BIG BAD CHANGE?"); adding a button that says "[ This change is too big and I don't want to review it. ]" which automatically rejects a revision with a form comment, etc.

Most of these features aren't great and aren't things I'd imagine adding, but I do think overlarge changes are legitimately bad too, and I can sort of imagine a world where we try to make this fight between author laziness (not wanting to separate changes into smaller cohesive steps) and reviewer laziness (not wanting to review large changes) happen in the open instead of in secret.

You could maybe leave the element off unless it is a larger change. I think my concern here is seeing it on every revision becomes something extra I have to mentally process, but if the intent is to either help show small changes, or large changes, it could just be visible in those edge cases. In those cases at least, the element provides context as to why it's shown at all vs. all the time?

Alternatively, you could use the battery indicator icon and just a tooltip. That gives some indication of how much work the revision potentially is without overbearing the rest of the info. Visually to me, this is really busy and if I actually worked or had lots of diffs, would take me a bit to process mentally.

Just to make sure we're talking about the same thing, that feedback is true even of the version at HEAD (blue/grey/orange) vs the version that originally shipped (green/grey/red)?

Screen Shot 2018-04-26 at 3.44.21 PM.png (273×165 px, 20 KB)

I tried to mute it fairly aggressively in the recolor and, at least for me, the grey version pretty much just fades into the background now. I notice the orange and blue but they feel like a hint, and the actual "+-" thing isn't distracting to me unless I actually look at it. It sounds like it's way higher impact than that for you?

Yeah, I'm looking at here, assuming it's ahead of last release.

A different approach may be to swap it with "Author"? At least, the information feels secondary to me, like date, and not the level of importance the UI is currently assigning it. I think some of my initial reaction is that interacting with it seems mandatory on the left side. This would also give, if memory serves, more UI space for Author since the right side is a fixed width.