Page MenuHomePhabricator

Track a "Done" state on inline comments
ClosedPublic

Authored by epriestley on Mar 10 2015, 1:48 AM.

Details

Summary

Ref T1460. This just barely works, but throwing it up in case any of it sounds mechanically crazy before we build integrations/UI/etc.

Specifically, these are the behaviors:

  • You can mark your own draft comments as "done" before you submit them. The intent is to let reviewers mark their stuff advisory/minor/not-important before they submit it, to hint to authors that they don't expect the feedback to necessarily be addressed (maybe it's a joke, maybe it's just discussion, maybe it's "consider..").
  • You can mark others' published comments as "done" if you're the revision/commit author. The intent is to keep this lightweight by not requiring an audit trail of who marked what done when. If anyone could mark anything done, we'd have to have some way to show who marked stuff.
  • When you mark stuff done (or unmark it), it goes into a "draft" state, where you see the change but others don't see it yet. The intent is twofold:
    • Be consistent with how inlines work.
    • Allow us to publish a "epriestley updated this revision + epriestley marked 15 inlines as done" story later if we want. This seems more useful than publishing 15 "epriestley marked one thing as done" stories.
  • The actual bit where done-ness publishes isn't implemented.
  • UI is bare bones.
  • No integration with the rest of the UI yet.
Test Plan

Clicked some checkboxes.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley updated this revision to Diff 28964.Mar 10 2015, 1:48 AM
epriestley retitled this revision from to Track a "Done" state on inline comments.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: btrahan, chad.

Here's the current state of affairs:

epriestley updated this object.Mar 10 2015, 1:50 AM
chad edited edge metadata.Mar 10 2015, 2:02 AM

Marking a [draft] as done seems like a bug or process-heavy if I didn't know its purpose. Mostly, it makes me feel like Phabricator expects every comment to be marked as done, so I can choose to pre-mark it. I'm assuming the "done" feature is really an optional workflow to help people manage large amounts of little feedback.

I'm thinking we maaaaybe put this much process on it?

$ arc land

This revision has 3 unaddressed inline comments with no replies:

something.js:9 (epriestley) Nice cleanup!
something.css:14-16: (epriestley) Dank meme!
something.php:13 (btrahan) This is a gaping security hole.

Do you want to land it anyway? [y/N] y
...

Does that feel too heavy to you?

I think it seems fairly reasonable to me, and like it would occasionally catch a mistake where I miss an inline, and not get in the way too much in normal use. But I'm not sure.

If we're comfortable going that far, I think pre-checking the draft inlines feels OK since we sort of get it for free, and it keeps the UI looking the same / having the same elements all the time.

chad added a comment.Mar 10 2015, 2:19 AM

Yeah I thought this was an optional feature, not a workflow change. I'll... have to think about that.

And I imagine we can maybe clarify the purpose with tooltips, like "Mark as Not Important" (when composing a draft) and "Mark as Addressed" (when you're the author).

Also, the author's comments might need some special consideration (checked by default? Always checked? Just special rules in integrations).

chad added a comment.Mar 10 2015, 2:21 AM

This to me is along the lines of sticky-accept. I want to 'trust but verify' or (sadly) have options to people to choose the more rigid process.

Okay, so you'd favor no warning in arc land and preventing anyone except the revision owner from checking boxes?

chad added a comment.Mar 10 2015, 3:06 AM

Would it be weird to be magical here? If I checked 3/4 comments as done, then give me a warning? If I did 0/4, no warning? This may be unexpected / confusing, but for power users seems like its correct.

btrahan edited edge metadata.Mar 10 2015, 5:11 PM

Rather than marking "Done" at time of inline comment creation, maybe they can mark "needs followup", and that's pre-checked? In theory then this "done" thing only shows up for items that "need followup". Maybe less process for all and process is initiated by feedback giver...

I favor the warning in arc. Seems useful if we're going to have the workflow and pretty minor to hit "y".

I think if 80 percent of the time this feature is useful its worth it, but I worry its useful more like 20 percent (or less) of the time and thus we should not ship it. That said, I favor shipping and cutting / making it config if we get uproarious feedback against.

chad added a comment.Mar 10 2015, 5:39 PM

I'm trying to take a step back with the design. I wonder if (since this is something you both seems to want to do) we should more explicitly call out the "type" of comment (vs. imply with a checkbox).

So maybe a dropdown, let's call it "Type" for sake of argument, with a positive, neutral, and negative UI representation. So something like:

Type: Important
Useful in calling out grave errors or large concerns. This must be resolved. (commonly referred to as "defect" in other platforms).

Type: Feedback
Standard lightweight feedback. "Consider naming this xyz..." or "This color is terrible, what about blindigo"

Type: Praise/Info/FYI
Hey man, this code is like hella awesome.

Done state then could be say required on Important and Feedback, but not Praise/Info.

I anticipate never interacting with this checkbox as a user, and I'd definitely never choose a comment type. If there's anything important I "reject".

The "security" warning probably isn't a great use case, since that should always be a reject. A more realistic use case is when @btrahan reviews like 3-4 of my revisions and I go to land then, and then I get:

$ arc land

This revision has a yada yada:

example.js:23 (btrahan) Typo, "sucess" should be "success".

That would be useful to me because I occasionally miss those if I have a sequence of things to land.

"arc land" also has a bunch of similar warnings already, all of which feel useful on the balance to me:

  • Revision owned by someone else.
  • Revision not accepted.
  • Revision has open dependencies.
  • Harbormaster tests still running.
  • Harbormaster tests failed.

The inlines warning would probably fire more often than these.

We could also add the "arc" warning only for comments on the most recent diff. The case where I miss comments is normally accept + comments with several revisions getting reviewed at once.

Warning external user intrusion.

Just wanted to add a bit of context. So for code review where I'm at now a few of us have a sort of system for this, which is overall poor. But maybe demonstrates a use case. One common case is packaging. We do a lot of it, and I'm constantly learning new things. When I upload a change to debianize something there are places where I get a "This is bad and you should feel bad." This is a comment + needs revision state. But there can be 5 comments and some of them are just "hey did you know xyz is another way to do this?". i.e. there are purely style consideration that are useful commentary. When there are a handful of inline comments and only really one of them is the root of the rejection, it would be pretty cool to be able to see that at a glance. As of now we do a "+" prepend for anything style or commentary, and then no prefix for just general "hey this is an issue".

Could I suggest differentiating between necessary feedback and subjective feedback as a first step? It all seems neat to me, but it also like a lot of moving parts to have each thing have a state independent of the overall state of the diff. In my mind when I put in 3 comments and one is a hard "no" source the next time that diff is updated as ready for review I know it has been addressed or I'm asking why it hasn't. I suppose for me and my history with differential I can see this being overly burdensome and confusing to users, and the last state of the diff itself could be decoupled in a strange way from the feedback and revision loop.

Hopefully that makes any kind of sense.

epriestley planned changes to this revision.Mar 10 2015, 7:42 PM

Let's start with this:

  • No marking drafts. I think this feature might eventually be useful, but it's definitely a little odd at first glance, and it's simpler to start without it.
  • Plan to try a warning for un-replied, un-done, authored-by-others-users inlines on the most recent diff in "arc land" at some point. This is at least partially blocked on T5873 anyway. Before actually implementing this, we can take another look at where this feature stands. I think I'd find this warning useful even if we decided to completely remove "Done", though, so I want to at least try it out.
  • No specific action on the @chasemp use case for now -- you guys are still doing this in Gerrit, right? If/when the issue ripens, we can look at the current state of the world. Allowing draft-checks could be a very gentle step in this direction, but let's see how far we get without them first.

So I'll revise this change by:

  • Removing the ability to mark your own draft comments as "Done" before you submit them.
epriestley updated this revision to Diff 28967.Mar 10 2015, 7:50 PM
epriestley edited edge metadata.
  • Don't allow draft comments to be marked as "Done".
chad added a comment.Mar 10 2015, 7:56 PM

(I'm fine with your plan)

  • No specific action on the @chasemp use case for now -- you guys are still doing this in Gerrit, right? If/when the issue ripens, we can look at the current state of the world. Allowing draft-checks could be a very gentle step in this direction, but let's see how far we get without them first.

Sigh, yes it is true. We are in baby step mode for differential. I was mainly trying to bolster the comments here https://secure.phabricator.com/D12033#118045 as I think some version of that would be useful. Even a "mark as informative" checkbox equivalent. But anyhoo, just wanted to chime in.

Even a "mark as informative" checkbox equivalent.

The original version of this essentially has that (before you submit an inline, you can mark your own inline as "Done", communicating that you don't expect the author to make changes to address it -- basically, instead of "+", you'd click a checkbox), but I agree with @chad that it's kind of surprising as an interaction. I think we'll take another look at it once we get a better feel for this feature.

The original version of this essentially has that (before you submit an inline, you can mark your own inline as "Done", communicating that you don't expect the author to make changes to address it -- basically, instead of "+", you'd click a checkbox), but I agree with @chad that it's kind of surprising as an interaction. I think we'll take another look at it once we get a better feel for this feature.

Doh. TBH I misinterpreted "Track a "Done" state on inline comments" as the code submitee marking a comment as "done". As in: User A submits, User B says "hey this is bad" and "this is neat". User A marks the first comment as "done" to reflect it was addressed in the code. I wasn't thinking the original intention was for the comment submitting user to do nearly exactly what I was suggesting. Makes sense on waiting.

Methinks it is time to eat an ice cream sandwich and take a break for a bit :)

Yeah, the primary use case addressed here is:

  • Senior Engineer Alice makes 300 inline comments on Intern Bob's code.
  • Intern Bob can keep track of them by checking them off as he works through things and fixes them.

If we re-enable the self-draft-check, it might go like this:

  • Senior Engineer Alice makes 300 inline comments on Intern Bob's code.
  • One of them is pointing out something good, so she checks that one herself before submitting feedback.
  • Intern Bob can keep track of the other 299 comments by checking them off as he fixes them.

We don't currently plan to have comment authors ever go through a workflow to explicitly confirm that inlines have been fixed by checking them off.

btrahan accepted this revision.Mar 10 2015, 9:00 PM
btrahan edited edge metadata.

whatcouldgowrong

This revision is now accepted and ready to land.Mar 10 2015, 9:00 PM

(I'm going to hold this for a little bit until I can lay in the transaction update stuff since it's probably too perplexing right now without that, but that shouldn't be very complicated.)

chad added a comment.Mar 11 2015, 12:27 AM

Question, does [Done] only appear on initial comment (and not subsequent replies)?

Currently, it appears on every comment, including the revision author's own comments.

Mostly, that's just the simplest rule. We could remove it from the revision author's comments, although I figured authors might find it useful to be able to leave a comment like "Okay, I will do X here" and then check it off later.

chad added a comment.Mar 11 2015, 1:35 AM

I'm just thinking of the case where 2-3 people have a reply-a-thon on a line of code, where it's a discussion and not a todo list.

chad added a comment.Mar 11 2015, 1:36 AM

It seems like the original line of code in question is all that should get the Done button.

Yeah, I'm not sure what ruleset is best.

In whatever summary view we end up with, we could present "replied" as similar to "marked done". For example, hypothetically, if we just had a "doneness progress bar" (I don't think we should), it could count progress for either an explicit "done" or a reply.

For example, if Senior Engineer Alice says "this should use x()", and then Intern Bob replies "hmm, I can't find x()", and then Senior Engineer Alice says "ah, sorry, in recent Dxxx x() got renamed to y()", it makes sense that Bob would want to check off the last comment (which discusses a specific and correct remedy), not necessarily the first one. The thread could fork into multiple discussions with things to address, too.

I think we should carry "Done" to the ends of the thread, not the root of it, but I'm not sure how heavily it should be carried. It might make sense to have reply actually count fully as marking "done", and to never show a "done" state for author inlines.

chad added a comment.Mar 11 2015, 4:48 PM

I'd still lean towards a lightweight "Done" interface, as in "1 Done" per issue, which is easy to grok. There are several design directions that are impacted by this direction (One Done vs. Many). In fact the multi-comment case is probably what should be designed first, since it's easier to work backwards from that case. For example with [ Reply | Done ] as a single action to a block of replies, informs us to sit the interaction underneath the last comment, instead of attached to each comment.

Another possibility would be to clear the "Done" whenever a new comment comes in.

Anyways, I think defining out the worst case scenario (multiple comments, multiple reviews, etc) better informs the basic cases, so I've probably approached the UI incorrectly. Having someone dig through old reviews just to find the done button seems counter-intuitive. Maybe we even use a subsequent arc diff to ask if they completed all the comments, which then updates the diff for them.

I'm also happy to just meet for coffee and brainstorm a little here. Mostly concerned there are two design paths here depending on what we'd end up shipping.

This revision was automatically updated to reflect the committed changes.