Here's the current state of affairs:

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.

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).

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?

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.

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.

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.

(I'm fine with your plan)

In D12033#118054, @epriestley wrote:

• 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.

(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.)

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.

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.

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.

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.