Allow inline comments to be individually hidden
ClosedPublic
Actions

Authored by epriestley on May 25 2015, 6:58 PM.

Details

Reviewers

btrahan
chad

Maniphest Tasks

T7447: Bring inline comments forward across revision updates

Commits

Restricted Diffusion Commit
rPe9f4a84a892c: Allow inline comments to be individually hidden

Summary

Ref T7447. Implements per-viewer comment hiding. Once a comment is obsolete or uninteresting, you can hide it completely.

This is sticky per-user.

My hope is that this will strike a better balance between concerns than some of the other approaches (conservative porting, summarization, hide-all).

Specifically, this adds a new action here:

Clicking it completely collapses the comment into a small icon on the previous line, and saves the comment state as hidden for you:

You can click the icon to reveal all hidden comments below the line.

Test Plan

Hid comments.
Showed comments.
Created, edited, deleted and submitted comments.
Used Diffusion comments (hiding is not implemented there yet, but I'd plan to bring it there eventually if it works out in Differential).

Diff Detail

Repository

rP Phabricator

Branch

hidecomment1

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 6314
Build 6336: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley updated this revision to Diff 31369.May 25 2015, 6:58 PM

epriestley retitled this revision from to Allow inline comments to be individually hidden.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added reviewers: btrahan, chad.

epriestley added a task: T7447: Bring inline comments forward across revision updates.

Herald added subscribers: epriestley, yelirekim. · View Herald TranscriptMay 25 2015, 6:58 PM

In theory, this lets us enjoy the advantages of liberal comment porting (like easy verification of small changes by reviewers) while mitigating the disadvantages (by letting users dismiss uninteresting or obsolete comments).

It removes hidden comments from the source listing completely, so it might be a good enough step toward "hide all comments" that we don't really need that. It dodges the technical difficulty of summarizing comments. It also has proper UI, rather than being tucked away in a keyboard shortcut, which is desirable.

It uses explicit user action to decide when comments are obsolete/uninteresting, so it should avoid some of the pitfalls we ran into with autohide/autocollapse.

It might be pushing too much manual work on the user, but the interaction is pretty simple and the perspectives on T7447 are so diverse that I suspect the are very few good rules for making these distinctions automatically.

jparise added a subscriber: jparise.May 26 2015, 5:09 PM

Good stuff. The part of this interaction as is I am unsure of is how you reveal all the hidden comments at once. I'd think maybe folks would just want to reveal them one at a time? I'd say ship it and that's my best guess at a possible minor iteration here.

src/applications/differential/query/DifferentialInlineCommentQuery.php
60	use $need here just in case someone later tries to use the same query object twice with true / false values specified

This revision is now accepted and ready to land.May 27 2015, 4:55 PM

Yeah, we might want a "show everything" / "hide everything" still, either as part of this or as a separate layer on top. This felt plausible as-is to me locally, but I'm not sure if/how users will actually use it.

I think there might be some issues with handling anchors, too, but I can look at that in a followup.