Page MenuHomePhabricator

Add DiffusionBranchListView for browsing branches
ClosedPublic

Authored by chad on Jun 10 2017, 4:25 AM.

Details

Summary

Adds a new DiffusionBranchListView which replaces the BranchTable when browsing all branches in Diffusion. Has all the same capabilities, but is easier to read, adds a Compare button, and plays nicely on mobile. It does take up more space, but I think that's generally OK here since we expect our branches to not be heaping piles of intern revert branches.

Test Plan

Follow a few repositories with branches, like Phabricator and KDE's Krita. View layouts on mobile, tablet, desktop. Try out new compare button.

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

chad created this revision.Jun 10 2017, 4:25 AM
chad edited the test plan for this revision. (Show Details)Jun 10 2017, 4:26 AM
chad added a comment.Jun 10 2017, 5:41 PM

blerg, how do I re-parent this commit to be on master and not on arcpatch-D12345? google is not really helping so much

chad updated this revision to Diff 43573.Jun 10 2017, 5:43 PM

mis-branched

avivey added a subscriber: avivey.Jun 10 2017, 5:45 PM

I'd do this:

$ git checkout master
$ git cherry-pick 00a6a0a06927

(Assuming you want that exact commit; Otherwise, find the right hash, or specify a few commits in cherry-pick in the right order)

chad abandoned this revision.Jun 10 2017, 5:47 PM
chad updated this revision to Diff 43574.Jun 10 2017, 5:53 PM

rebase?

epriestley accepted this revision.Jun 12 2017, 3:17 PM

I think I would expect the "Compare" button to pop this dialog instead:

Compare: [branch you just clicked]
     To: [default branch in the repository]

...where both inputs are text entry forms (so you can type a commit hash instead). It is not intuitive to me that the "branches" UI would behave in a way that is stateful with respect to the current branch.

I think the autoclose stuff could be moved into "Manage" and removed from this UI (e.g., show a simple table under ManageBranches of each branch and its autoclose/track status).

I think the "current" marker is not really useful/meaningful, at least today. There is very little UI for statefully switching branches, and this isn't a concept we emphasize. A better marker might be "Default".

Your test plan does not appear to include Mercurial repositories (unless Krita has an hg repo?) but should.

This revision is now accepted and ready to land.Jun 12 2017, 3:17 PM
chad added a comment.Jun 12 2017, 3:55 PM

Is there a way to get the default branch? The code was ($branch->getShortName() == $current_branch) so it gave me the impression it was dependent on the branch you were viewing. Personally I'd think comparing against master is the most common? So one click to that comparison page, then a second click to specify a hash? Or is there a workflow I'm not imagining? I think this was @avivey 's work.

$repository->getDefaultBranch() has the default branch for the repository.

This is usually (but not always) "master". This will also usually (but not always) be the current branch.

Here's a case where the two are different, although it's a little bit made up:

  • You navigate to /rXYZabc, which you've, for example, used git show stable from the CLI to determine is the head of stable.
  • This commit is an ancestor of the branch heads of stable and release-20170103, so the "Branches" field in the UI shows those branches.
  • You click stable, then you click some link to end up on this UI with "stable" selected as the current branch (I believe this is impossible today, and that no link in the product leads to this UI with any branch other than the default branch selected as the current branch).
  • You want to compare stable to master, but there's no button next to stable, and the button next to master compares master to stable (the opposite of what you want).

In the UI I propose, the dialog is default-filled with the branch names. For example, in the workflow above, clicking "Compare" would show this dialog:

Compare: [stable]
     To: [master]

This is one more click than automatically comparing to the default branch (you must click "Compare" in the dialog to accept the defaults). However, it lets you see what "Compare" will do, and change the targets of the operation (e.g., from a branch head to a particular commit hash) if you have a different range in mind.

This is the same UI we currently implement in the browse view today, so if we change it here we should also change it there:

(As a general note, I've intentionally buried the "Compare" view a bit because I think it still needs significant work, although we haven't seen negative feedback about it that I can recall so it may be in better shape than I thought.)

chad added a comment.Jun 12 2017, 4:16 PM

Well if it's UI work, toss it into the stack. I was planning a pass there as well.

The major thing is that the compare view currently shows a list of commits present on A but not B, but does not show the actual diff of source changes.

On GitHub, the UI looks like this:

We implement a similar UI to the "Commits" tab (not selected) but do not have an equivalent feature to the "Files changed" tab (selected).

I suspect this is probably outside the realm of "UI changes", although it's technically a UI change I guess?

chad added a comment.Jun 12 2017, 4:22 PM

haha, well UI changes should be == work I can do.

chad updated this revision to Diff 43590.Jun 13 2017, 2:51 AM
  • rebase onto master
  • update per comments
chad updated this revision to Diff 43595.Jun 13 2017, 6:04 PM
  • re-re-read feedback and make adjustments
This revision was automatically updated to reflect the committed changes.