Page MenuHomePhabricator

Replace "Next Step" Differential advice with comprehensive instructions on "Land Revision"
Open, NormalPublic

Description

I'm removing "Next Step: arc land <whatever>" from the UI. This is purely a UI hint for new users.

In the future, we should make "Land Revision" provide this information (and other options, including the existing flow) instead.

See also T4706.

Event Timeline

@jcarrillo7: there is no command line way to get back a list of reviews (with their identifiers) that are active for the current repo.

Are you aware of arc branch? I think it does exactly this?

I assume he means other peoples reviews that are out

Er, oh, other reviews. Then maybe this feedback should be on T12026, rather than here.

Moving over comment from T12179

Due to D17076 a lot of users on my instance have complained about these useful fields missing. Particularly around patching. It seems that a lot of users find it tedious to go "figure out" how to apply a patch or land a branch.

I know you guys plan to address this in T12027 but hearing the feedback from my users it sounds like they really miss the convenience of having a nice thing they could easily copy and execute on the command line. This seems to be due to two things:

  • The differential UI does not show any easily copiable text of the DXXX identifier of the review
    image.png (86×360 px, 6 KB)
    is not easily highlightable or large enough to be obvious/useful for this
  • there is no command line way to get back a list of reviews (with their identifiers) that are active for the current repo. This could allow someone to lookup the right ID to use easily.

Having this information back is currently the most often requested feature on my instance. A lot of non-power users really got used to the convenience and never learned much about the arc commands. Not sure how to really address this one since this is also the same group of users that prefers UI's instead of command lines.

Would these two items above be worth splitting into a improvement and feature request respectively? Or do you feel that this task will how you want to handle the above?

@epriestley arc branch only lists reviews open for branches you have locally no? My users are looking for all reviews currently open on this repo (mainly so they can arc patch).

arc list will eventually be more flexible (T7715) but today it's only your stuff.

arc <list of diffs I need to review>

I'm also confused about this:

is not easily highlightable or large enough to be obvious/useful for this

This text is larger than the old arc patch text was, isn't it? It's also copyable from the URL bar. And surely text from a hypothetical arc list-stuff-I-am-reviewing isn't any easier to copy/paste than this text?

I'd like to make sure this is really a usability problem, not just a "things changed and I was used to the old way of doing things" problem. It's not clear to me how we made running arc patch any harder by removing the arc patch hint, except for very new users who haven't learned the command yet.

One general improvement we could make is to have arc patch without arguments offer you open, non-authored revisions in the same repository, using the new numerical menu selector from T10895. That might moot this, and I can't immediately imagine any other default behaviors for arc patch with no arguments (other than showing you how to provide arguments, as it currently does, but we could retain this text in an abbreviated form).

This text is larger than the old arc patch text was, isn't it? It's also copyable from the URL bar. And surely text from a hypothetical arc list-stuff-I-am-reviewing isn't any easier to copy/paste than this text?

This complaint I've heard mainly from users who aren't power users. Also a lot of users do not notice this text is in the URL since on Safari the browser hides everything except the domain until you click on it.

The main issue here is that this text is actually hard to highlight (at least on safari). It seems picky when highlighting from right to left and also tends to highlight a bunch of other things.

I'd like to make sure this is really a usability problem, not just a "things changed and I was used to the old way of doing things" problem.

I'm sure this is some of the users.

It's not clear to me how we made running arc patch any harder by removing the arc patch hint, except for very new users who haven't learned the command yet.

Some users really do not like command line. Like really. 😂

Others really like the convenience. And you guys did take away a feature without a replacement.

Just thought I would pass along feedback I've received after running a decently large install (~60 repos, > 100 users)

One general improvement we could make is to have arc patch without arguments offer you open, non-authored revisions in the same repository, using the new numerical menu selector from T10895. That might moot this, and I can't immediately imagine any other default behaviors for arc patch with no arguments (other than showing you how to provide arguments, as it currently does, but we could retain this text in an abbreviated form).

This would solve it for a lot of the users who love the command line and hate the UI 😂 (most of them are in this camp)

Also a lot of users do not notice this text is in the URL since on Safari the browser hides everything except the domain until you click on it.

I can't immediately reproduce this, do I have some kind of weird setting?

Screen Shot 2017-01-31 at 6.41.56 PM.png (834×1 px, 147 KB)

Oh, apparently I do: PreferencesAdvancedShow full website address.

The main issue here is that this text is actually hard to highlight (at least on safari). It seems picky when highlighting from right to left and also tends to highlight a bunch of other things.

We can probably fix this. See also T12043 for arguments supporting changing the (currently inconsistent) behavior in Differential.

Some users really do not like command line. Like really. 😂

Just for my own clarification: these are technical users using Git to review changes to code, who don't enable "Full website address" and don't like the command line?

The selection junk is caused by float: left which apparently fixed some kind of IE thing in 2014 (T5902) so that may be involved to fix.

Some users really do not like command line. Like really. 😂

Just for my own clarification: these are technical users using Git to review changes to code, who don't enable "Full website address" and don't like the command line?

Think old guys who are used to SVN or perforce and new to Git and prefer to use GUI Git clients like Tower because git is "too complicated". Oh and are begrudgingly branching their repos now because they don't like branching.

This is a very small camp on my instance (just one team about 6 users). Most would just want to not leave the keyboard and an arc patch magical selector would be amazing to them. They didn't mind the UI copy paste because normally they were already on that page for the review anyways. When that was gone a lot expressed having a hard time copy pasting the text I mentioned above due to highlighting issues. Only a few thought about it being in the URL. Just not a place you normally think to go copy an substring I guess.

D17283 allows you to double-click the D1234 crumb to select it, since it's no longer a link. I don't know if double-clicks are too complicated or not.

I'd like to fix the selection stuff since it shouldn't need to be as janky as it is and removing float: left pretty much just fixes it, but need to test changes on IE.

I think the arc patch change is a good idea, but it'll be a bit before we get to it.

D17283 allows you to double-click the D1234 crumb to select it, since it's no longer a link. I don't know if double-clicks are too complicated or not.

God I'd hope not. Though naturally you don't think to double click to highlight text. At least I naturally don't.

I'd like to fix the selection stuff since it shouldn't need to be as janky as it is and removing float: left pretty much just fixes it, but need to test changes on IE.

I think the arc patch change is a good idea, but it'll be a bit before we get to it.

Keyboard lovers rejoice!

(arc list will also grow some kind of --reviewer=me --status=open mode in T7715, but I suspect that no one would really care if arc patch just did that by default.)