Would like to see all comments and their status in a list
Open, Needs TriagePublic

Description

Currently I have a very hard time seeing what comments I've marked as done and those which I haven't addressed. If I could see a list of all comments with and without checkmarks that would be great. Finding them inline in a code review is hard if the code review is large or touches many files.

Related Objects

elsigh created this task.May 18 2015, 10:43 PM
elsigh updated the task description. (Show Details)
elsigh raised the priority of this task from to Needs Triage.
elsigh added a project: Support.
elsigh added a subscriber: elsigh.
ohler added a subscriber: ohler.May 18 2015, 10:48 PM
chad edited projects, added Differential; removed Support.May 19 2015, 3:49 AM
eadler added a project: Restricted Project.Jul 7 2016, 5:14 PM
epriestley added a subscriber: epriestley.

My tentative plan for this is a separate view where all inlines are shown in threads, sorted by done-ness and activity.

eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 7 2016, 10:58 PM
jayvdb added a subscriber: jayvdb.Jul 7 2016, 11:42 PM

Here's approximately where I'm headed with this:

markwei added a subscriber: markwei.Aug 3 2016, 6:58 PM

That looks really useful. With such a view, perhaps the existence of inline comments that are Not Done can be a blocker for arc land.

venky added a subscriber: venky.Aug 9 2016, 5:20 PM
scode added a subscriber: scode.Aug 11 2016, 7:12 PM
bizrad6 added a subscriber: bizrad6.Sep 6 2016, 6:09 PM
sohamr added a subscriber: sohamr.Oct 12 2016, 6:39 PM
mtsgrd added a subscriber: mtsgrd.Apr 4 2017, 11:16 AM
chad added a comment.EditedJun 8 2017, 6:24 PM

Alrighty, took at pass at this I think isn't too much work, and is probably 98% of what people want. Specifically tried to look at the following scenarios:

  • As an author, see if I'd replied or marked as done all comments.
  • Be able to see full comment history across updates.
  • Provide sorting for Diff, Status, Reviewer
  • As a reviewer have all my comments been addressed by the author.
  • Clicking on comment text takes me to that diff so I can comment to reply or mark done. Pro points for using J769 to manage the complexity of modern browsers.

I went with no interaction here (marking done, replying). These are probably something we can look at down the road, but I think that little efficiency comes at a pretty high cost code/maintenance wise based on @epriestley's mock above. By keeping this in Revision Contents, I think we summarize for those who want to see this pretty well.

I suspect that isn't what very many users want (I'd guess like ~10%, not ~98%?) -- or, at least, doesn't really solve whatever problems they're running into. For example, I think the "90 spelling corrections" diff in T8909 wouldn't benefit much from this (it doesn't help you go through 90 spelling corrections or 90 lint style fixes), and it's the opposite of the request in T5654, which sounds very similar to this to me. But maybe T5654 is really just "our users like GitHub, so make it look similar to GitHub so it's more familiar", I'm not really sure.

As mocked it's also slightly tricky to build because it needs to update as you make edits to inlines, although that's much less work than my mock above. It also looks like I have to implement a client-side sortable table, which is probably PHUIXTableView. Still less work than my mock, but dramatically more work than a hypothetical mock that didn't require JS. Actually, I guess it's a bit trickier than that because the dataset in the table, not just the table, needs to update as you make edits, but the client-side sort order of the table needs to remain the same.

I'm a little divided on knowingly shipping "bad" features when we're paid to do it (not that this mock is necessarily "bad", exactly, but I think (?) it's probably not something we'd ship on our own, or if we did our v1 would probably support richer interactions). On the one hand, we get paid, and it incentivizes our customers to actually describe their problems instead of just asking for specific solutions. On the other hand, it kind of makes the product look under-considered and hurts other users a bit: if our "we did exactly what you asked us to" gambit is successful and we eventually iterate the feature into something good, innocent users get caught in the crossfire while the UI changes and evolves. It's also an inefficient use of our time (e.g., if I build PHUIXTableView, we ship this, and then that prompts feedback which really motivates the other mock, we build it "for nothing").

chad added a comment.EditedJun 8 2017, 8:48 PM

I do think this is something I would have used in the past when faced with ensuring I've addressed all feedback for multiple designers. So in some part, I feel I understand the core problem users are looking for. I don't think we can fix abuses like the 90 comments on spelling errors, so that's not something I feel I need to consider in this design. I just want to make sure I haven't missed anything as an author, or that as a reviewer, all my comments have been satisfactorily addressed. There could be something to explore in that second directive, but we haven't seen a lot of issues in that area, so I'm not really concerned if this is the best solution for that. Wanting a view of only inline comments and the ability to reply isn't something I want, as I feel Differential already does that.

Sorry if I misspoke about shipping something bad. I think what I'm recommending is simply let's make a reasonably best attempt at this knowing it may not be perfect in anticipation that better use cases / scenarios will arise after community feedback. I've noticed it's hard sometimes to build a product you're not going to use, so sending something out (for me at least) and getting feedback / iteration sometimes is the best path forward there. Soleio used to say "photoshop lies" and I think that's probably accurate here. It's hard to design the usefulness without some amount of seeing it.

I'm, going to propose that we do this table on a second page instead of inline. That removes any JS needs I think? I think this view should also add a checkbox for "hide done" which is pretty simple to do. This all might be 100% something I can build myself.

To step back slightly, one problem we know users have, which is motivating at least some of this request, is "90 spelling corrections". T8909 has a specific example of that, and T8909#224085 alludes to a flavor of this ("90 style corrections") elsewhere. I don't think this is a problem that users "should" have, but I suspect it's responsible for between, like, 50% and 85% of this request (maybe I'm way off, but I'd bet dollars to donuts that it's more than 2%), since it's the case where managing inlines is most overwhelming. And human reviewers are somehow often unable to resist making 90 inlines about style/spelling corrections, even when everyone is worse off for it.

The workflow to deal with these is something like:

  1. For each inline which you haven't checked off yet:
  2. Open the file to the line the inline identifies.
  3. Correct the spelling of the word, or add or remove the missing whitespace.
  4. Save the file.
  5. Check off the inline.
  6. Repeat 90 times (!!)

It's no surprise that users find this frustrating and feel like Differential isn't helping them succeed in it today. This workflow is mind-numbing and repetitive and slow and clumsy.

I think the "sortable table" UI above doesn't help with this very much because it makes step (2) quite difficult (almost more difficult than the current UI): it does not show the filename, line number, or change context. You'd have to click into a new tab to complete the step, since you want to save your place in the original table. This is slow (at least, without additional work) since we have to load the entire revision in the new tab, and users generally don't seem to naturally gravitate toward "Open in New Tab" as a workflow step very much anyway, and they'll lose their place in the table if they click the link in the same tab.

The mock with context above is a little better about this, although you still have to scroll through the document comment-by-comment, copy/paste filenames, and context switch to and from your IDE.

To take things a step further, we could write arc inlines D123 which would open files to the right line in your editor one by one, then you hit "n" in the console to go to the next one once you've made the fix (or whatever). You still have to context switch back to the CLI, but you'd no longer need to copy/paste filenames or line numbers. This has been suggested in the past, too.

A step further, we could write an IDE plugin which just pulled inlines into your IDE directly (someone wrote a prototype of this, at least, at Facebook). Or, equivalently, let you write replacement text directly in Differential, then provide a way to arc take-updates-from-web (this has also been suggested in the past). The idea with this one is that your reviewer would make 90 spelling corrections, instead of telling you to make them, and then you'd just pull down the diff with an "accept all the random patches reviewers suggested" command.

A step further is lint (so these aren't problems in the first place) and something I'm broadly going to describe as "better review culture / discipline", where reviewers say "there are a lot of spelling errors, you should spellcheck this before sending it for review" or "please format this like the style guide describes", rather than leaving 90 inlines in the first place. I think this is the actual solution to this problem, and a chunk of the reason that we don't really have this problem ourselves (of course, there are other reasons we don't experience it, too).

We could just ignore this problem, and build something for the other 15% or 50% or 98% of the use case. Or we could try to tackle this problem, but I think no solution in the web UI will ever be good, since there's a real limit to how painless we can possibly ever make step (2) above from the web UI, and the only way to break through that limit is to not treat this as a web UI problem.

Yeah, this is relatively simple if it's on a separate page, and can be built in 20 minutes if there's no sorting or "Hide Done".

You could sort "Done" to the bottom, perhaps, to "solve" both problems.

Let me see if I can dig up my hack-branch above and hand you a skeleton for it.

Mock above rebased on master and apparently working:

https://secure.phabricator.com/differential/diff/43560/

chad added a comment.Jun 8 2017, 8:58 PM

Alrighty, I'll take a look at it. 💯

lvital added a subscriber: lvital.Jun 9 2017, 5:08 PM

This has landed for anyone who wants to provide feedback on the v0 version.

Thanks! We've deployed this and are soliciting select feedback. Will get back to you shortly.

girtsf added a subscriber: girtsf.Fri, Jul 6, 4:40 PM