Page MenuHomePhabricator

Provide ability to hide all inline or done comments
Closed, ResolvedPublic


When looking through an long and tricky diff there can be many comments, most of which are done. Differential has the ability to hide individual comments but it annoying to hide all the done ones. Its a manual process and its easy to hide the non-done ones.

This is a feature request from multiple of our users (to try and give you some context about why I'm submitting it).

Related Objects

Mentioned In
T13455: Make "View Options" in Differential sticky across reloads
T8250: Would like to see all comments and their status in a list
M1476: Inline Comment Browse bar
D17955: Add quest objectives to the minimap
T12733: (2017 Week 20) Inline Comments Errata / Feedback
rP4fd4ec3d275d: Hide inlines one-by-one, instead of in a big group
D17861: Hide inlines one-by-one, instead of in a big group
T12153: 'Show Hidden Comments' bubble does not fit in margin next to line numbers >1000
T12616: Modernize display code for inline comments
T9686: Ability to hide inline comments
Mentioned Here
T10038: Plan the mid-term pathway for unit-test/linter bindings
T1591: Differential "buoyant" header has been temporarily disabled
T4043: Provide a keyboard shortcut to focus the comment textarea in Differential
T4213: Provide custom themes, skinning, or custom CSS
T7838: Link to inline comment opens in new tab, scrolls to inline comment, then scrolls to bottom of page.
T8047: Differential inline reticle can persist across Quicksand navigations
T8288: Hovering over "Edit" dialog for inline comments which are replies to right-hand-side inline comments shows reticle on wrong side of diff
T8420: Hidden inlines don't work properly with anchor-based navigation
T9270: Toggling File Browser on Differential while creating inline comment causes inline/edit focus reticle to offset
T10049: Differential highlight reticle persists after focusing an inline comment, then hiding it
T10668: When replying to a ghost inline comment, it is possible for the reticle to cover the reply UI
T10954: Differential keyboard shortcut inline selection reticle is visually unclear (bad z-index?)
T11093: DifferentialInlineCommentEditor retains state across Quicksand requests

Event Timeline

eadler raised the priority of this task from to Needs Triage.
eadler updated the task description. (Show Details)
eadler added projects: Maniphest, FreeBSD.
eadler added a subscriber: eadler.

This is especially true for long-lived reviews. I've been implementing a 1-wire bus. I thought it was ready when I submitted it to phabricator, but a number of comments have surfaced (in my case about the man pages, but in other long-lived reviews it is about the code). As the code changes and evolves, there's a natural motion of the comments people leave form the actual code it affects. This is doubly true if the comments are addressed. Tripply true when the comments may be short, but suggest rather major surgery (eg, you should move from a polled model to an interrupt driven state machine).

In my case, I'd love to hide all the old comments (at least by default) so that new reviewers aren't distracted by them. I'm not trying to hide anything, but when reviewers can ignore the obsolete comments, they can use the time they spent ignoring them to review my code instead.


It's very difficult to dive into the current state of play in a long lived diff as the code is split into pieces between each comment.

+1 The workflow with comments is currently challenging.

+1 as well. Long diff, lots of comments, comments no longer aligned to their code (because line numbers shift) = unreadable mess.

chad renamed this task from provide a button to "hide all done comments" to Provide ability to hide all inline or done comments.Nov 2 2015, 4:39 PM
angie added a project: Restricted Project.Apr 21 2016, 11:37 PM
angie added a subscriber: angie.
eadler added a project: Restricted Project.May 21 2016, 4:04 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.May 22 2016, 6:41 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.May 23 2016, 6:13 PM

Here are the interactions I can come up with which we'll potentially get wrong by implementing this naively:

  • Anchors, which also have outstanding bugs for hidden comments in T8420.
  • Previous/next links have related outstanding issues in T8420 and might require work on with "hide done", although they won't be visible with "hide all".
  • Previous/next keyboard commands and current keyboard focus state.
  • When you click to create a new inline in "hide all", we shouldn't hide that inline.
    • Show it until you submit it, then fade it away?
    • Toggle you out of "hide all" mode?
    • Just keep it visible and require the user to cycle modes again to actually "hide all"?
    • Maybe we never hide your own unsubmitted drafts? When you submit them, do we vanish them instantly if you're in "hide all"?
  • When you mark an inline "done" in "hide done" mode, we shouldn't instantly vanish that inline.
    • Fade it away?
    • Keep it hidden, requiring a toggle?
  • Maybe weird stuff like replying to an inline, then hiding all inlines?

The similar issues in T8420 should also be resolved concurrently.

I generally don't like completely vanishing information with no UI cue, either. "Hide" should probably really mean "fold into the gutter" (as hide on individual comments does) and/or we should perhaps look at restoring the "buoyant" header and possibly putting state/controls there (T1591).

Some of this is almost certainly adjacent: T4043, T10049, T9270, T7838, T8288, T8047, T10668, T10954, T11093.

This tasks suffers from a lack of clear implementation boundaries and it could end up taking days to clear related work if it cascades into a majority of that list, but the value of the feature is presumably comparatively low given that this is likely "solvable" -- just not in a maintainable/support-friendly way -- with a small bookmarklet or browser extension.

I vaguely wonder if modularizing keyboard commands is worth considering. In theory, we could let you restore behaviors like /-to-focus-search and provide i-to-hackily-toggle-inline-modes through extensions. One issue is that it seems difficult for command extensions to select when they should activate, because neither URL-based or controller instanceof X-based checks seem particularly good. We'd probably have to have every modular keyboard behavior activate on every page and then test if whatever it's looking for is actually present in the DOM, which feels messy. And I don't really see a large set of use cases for this, and don't want to encourage development that treats the DOM as an API (see also T4213), and building this is not likely easier than implementing this task (hiding inlines) properly.

Anyway, I think the ways forward on this are basically:

  • We can prioritize this normally with a 20-hour estimate to account for all the other things I may need to clean up to get a solid version of it built.
  • We can "sort-of" prioritize this with a 2-hour estimate for the actual interaction, but it won't happen until the queue clears or enough of those adjacent tasks resolve elsewhere and we can reasonably schedule and complete all the other work as unbillable.
  • (Or we can just forget about it from a priority viewpoint.)

I think the desired feature is reasonable, this class of interactions is just fragile already and I don't think all pathways that leave us better off than we started have a larger cost than the value of the feature.

(In some theoretical free market version of our scheduling process, the priority queue would be more of an auction and there would be a way to say "this is worth X, independent of the number of hours it takes", and we could then just divide that by implementation cost and work down the queue in value-order. I might still try to experiment with this at some point, but I think it's probably a poor match for the real world and a particularly poor match for enterprise billing departments.)

I'm moving this to "Rugged Terrain", as the best available pathway forward leaves us with dramatically higher implementation cost than the reasonable value of the feature. We can pursue this after tasks with better aligned costs and value are addressed.

eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 4 2016, 9:04 PM

Broadly, the technical stuff here is in fairly good shape now but I still don't truly understand what problem we're solving. From earlier, I dug this up:

I've been implementing a 1-wire bus.

I believe this is the corresponding change:

At time of writing, this change affected 2,493 lines of code, had received 101 inline comments over approximately 23 days (unclear what became of it).

Of the 101 inline comments, about 90 appear to be related to grammar or spelling. 2 were humans catching lint issues (tab, extra blank line). About 5 appear to be actual high-level human discussion of program behavior. The remainder are discussion and notes-to-self (I counted all this by hand, so these numbers may not be exactly accurate or add up).

Phabricator isn't very good software for processing 90 minor spelling and grammar corrections, but it isn't intended to be, and I'm not sure it should be.

As a reviewer, I probably would not leave these inlines. I would either give the author high-level feedback ("run this through a spellchecker -- for example, I caught at least 6 instances of 'resistor' being spelled wrong") or do the edits myself ("code looks good but the grammar in some of the docs is a bit rough, let's split that out into a separate change and I'll counter-diff you with some improvements?"). These approaches require less of my time -- and less of the author's time -- than leaving 90 separate inlines, 6 of which point out misspellings of "resistor".

I don't think any author at any skill level benefits from this sort of granular review, either: even students/interns would be better served by high-level feedback ("use a spellchecker") that points them toward tools they can use to improve.

I currently plan to implement this request literally, without an understanding of a specific problem which it represents a good solution to. I continue to basically believe that all cases where this is important are cases of review going off the rails and exploding in a fiery wreck, and that a much better intervention was available earlier in the pipeline (like: provide higher-level feedback, use lint, or find a better approach to the problem).

If you don't agree with this, it would be helpful if anyone can provide live examples of reviews with large amounts of meaningful discussion and feedback which these solutions would help with. I'm open to understanding this problem better, it just seems like every time I actually see one of these revisions it's someone leaving dozens of whitespace comments (which a linter should handle) or, here, spelling/grammar corrections.

(If everyone in the world except us uses Phabricator to perform dozens of spelling/grammar corrections, that's great, and it's something we can improve -- but this task probably isn't the highest leverage improvement available.)

Do you bucket style guide violations with lint issues? While I obviously can't post internal reviews, I can give you a hand-waving indication that a non-zero percentage of comments are style-related things that aren't caught by linters.

If a linter could theoretically catch it (conventions, space, formatting, etc.), I think a linter should (in a perfect world) be catching it.

From a personal/experience perspective, I think a good style guide should ultimately have this rule, at least in spirit:

If you can't write a linter for it, it can't be part of the style guide.

And then reviewers should decide for themselves if purely stylistic things they have a fondness for are important enough to be worth writing a linter for, or not that important (which means they can't be part of the style guide, and they can't complain about it in review). You can't cover everything with this approach, but my experience at Facebook and in this project is that you can get well into the "good enough" territory and that style feedback will pretty much disappear from the review process and all authors and all reviewers will be much, much happier for it.

Getting there is hard, though. Most third-party linters can't auto-correct and often can't emit individual changes; our first-party AAST stuff is powerful but rough and can't parse most languages; only a handful of languages like Go have tools like gofmt; the whole binding stuff is a mess (T10038); and I think we're probably wandering down a dark path if we bank on ever being able to ship linters that are a good fit for most projects (because of how much diversity there is across projects, even in the same language -- that said, I do think the general AAST-based-linter approach is sufficiently powerful that we could theoretically make inroads here, just no part of it will be very easy.

If this is primarily solving "auto-correcting, style-enforcing lint is hard, and until we can invest in that [which might be "never"] we have to deal with dozens of style comments on every change" I'm completely onboard with framing this as an onramp toward effective lint, and think it's a reasonable stepping stone to reduce the pain of the pre-lint era of code review.

I just want to challenge assumptions like these, which seem to be implicit in some of the scenarios here but which aren't very consistent with my experience:

  • changes are routinely long-lived (weeks);
  • changes are routinely large (thousands of lines);
  • changes routinely receive large amount of feedback (dozens or hundreds of inlines).

In all cases I've directly observed where one or more of these is true, it looks to me like there's some bigger root problem and this isn't really addressing it. Some of these root problems are really hard to address, like "we're paying engineers $300K/year to collaborate on correcting whitespace errors, which a computer could do faster and more accurately instead and which everyone hates doing and basically feels bad and a little resentful about", but they're the problems I'd ultimately like our sights to be set on.

For me it would be a great enhancement if there would be a shortcut which temporarily toggles this option:
SettingsPersonal SettingsAPPLICATIONSDiff PreferencesShow Older Inlines