Some engineers on our team are reluctant to "request changes" in Differential because the red "X" seems too harsh if the suggested changes are minor. Leaving feedback as mere comments doesn't give us the same workflow, though – the change still shows on the reviewer's dashboard as "waiting for review".
Any chance we could get an icon that is fuzzy and cheerful and represents change?
FYI, since you specifically mention upstream – we're using Phacility, so we
can't use a fork, either.
We use to represent the states of each reviewer in Differential. These icons make it fairly clear the status of each, even if you've never used the product before, we also use these same icon patterns in other areas of the product. As for colors we have standard patterns we prefer to adhere to, red (stop, important), yellow (warning), white (neutral), blue (notice), green (success). We also use other colors as consistently as we can as well like purple (closed) and sky (highlight). A consistent design pattern means people understand the UI quicker and better.
While there may be other icons to choose from, they'll all need to perform the same base function, which is to explain that changes are needed before this revision will be accepted. My general assumption here is that change an icon or color will not resolve anything with your engineer's workflow, since they mean the same thing inside the product. That is, this code needs updating as it will break something (reject).
Most of these issues I think can be resolved with Dashboards (ie, people can build whatever they want to track in Differential), T418 (maybe make reviewer status part of the "card"), and through corporate culture (discussing best code review practices). For me, if a reviewer doesn't reject, but I need to follow up, I just "plan changes" to keep it up higher in my queue. I also don't tend to work on more than a handful of diffs at a time, so knowing the state of what's out there is easier (for me). If you have more specifics as to why it's difficult to track your diffs, we can look and provide feedback on workflows.
@epriestley I think once mentioned the concept of queues and moving diffs back and forth. Not sure if he's still thinking about things like that, but resolving someone worried about 'Requesting Changes' would likely be better solved by re-thinking how we pass reviews back and forth from top down.
I was planning to try make "comment" work somewhat like a "soft reject" at some point, but this was before T1279. After T1279, I haven't seen much feedback to the effect that this is important (I think essentially all feedback since T1279 -- other than this case -- is related to cases like the one discussed in T8080 -- roughly, revisions appear in a bucket based on the overall revision state right now, but viewers expect them to appear in a bucket based on their personal reviewer state).
To the degree that "reject" is a problem because it's too harsh, I don't think it can be remedied by making the icon softer. This was more of a problem before T1279 (when we introduced icons) and previous softening of the language prior to 2010 (from "Reject" to "Request Changes") didn't seem to have much effect.
If we do pursue something here, I still favor making "comment" work like "soft reject" as the first / most promising approach, although this is substantially more complicated after T1279 when we have to consider the state of all the reviewers. I haven't though through the cases and I'm not sure an implementation is available which could work as well as it once could have, even in theory. My second-favorite approach is something like removing the "Comment" action for reviewers entirely, or making it non-default, but I suspect many users would find this frustrating and hate it.
Ultimately, I think incentives are fairly well-aligned here even when reviewers don't "reject". The revision author needs to work slightly harder to keep track of their in-flight code, but as long as "Comment" is a possible action they'll always have to do this, and it's generally desirable for the author to bear the overhead here -- they're ultimately responsible for the change.
Overall, my current thinking is that I'll address more clear-cut bucketing cases which arose after T1279 (for example, T8080) first, and then consider making comments push things between buckets (and possibly introduce new buckets for this, like an "Unreplied Comments" bucket between "Blocking Others" and "Waiting on Others").
Tentatively, the bucketing rules might change to:
I think this is probably reasonable overall, but there are some challenges:
This is likely to happen roughly as: fixing T7707, collecting cases like T8080, applying the new viewer-revision-state-based bucketing rules (only -- no unreplied comments bucket yet) to resolve these cases if none of them are problematic, letting that sit for a while to be confident that moving bucketing rules to be more-asymmetric is really a net win and not unduly confusing, then considering adding the "Unreplied Comments" bucket.