Page MenuHomePhabricator

Show status of linked revisions for tasks
Closed, ResolvedPublic

Tokens
"Evil Spooky Haunted Tree" token, awarded by joshuaspence."Piece of Eight" token, awarded by avivey."Like" token, awarded by johnny-bit.
Assigned To
None
Authored By
fooishbar, Jan 28 2015

Description

While you can link Differential revisions to Maniphest tasks, Maniphest only displays a dumb list of the tasks. Showing their status would make the tasks much more useful at a glance.

My very particular usecase is perhaps out of your target audience, but let me attach the result first, as I think it's desirable:

The reason I really want this, is because I'm hoping to be able to use Differential to manage reviews of patchsets, in the style of most low-level Linux projects. Projects such as the kernel, Wayland (which I want this for), X.Org, GNOME, etc, each use Git with local mutability but remote immutability, to produce patchsets. That is, whilst developing, we break each patch into small, digestible, changes which are easily and independently reviewable, with judicious git rebase -i. These patchsets are meant to be considered as a whole, but for review purposes, each patch is considered individually.

This is probably a good example, where the combined changeset would've been much too large to comprehend in itself, and also where many of the separate changes required entirely separate reviewers: http://lists.x.org/archives/xorg-devel/2011-June/thread.html#22868 - hopefully it goes without saying that we'd love to move this off mailing lists and on to better tooling.

Differential's immutable mode falls down here, because it still presents the result of the entire patchset, not the individual patch for review. I don't expect Differential to grow support for this separate mode, since it seems to run contrary to all your usecases and would present a lot of UI work, but this tiny change would at least allow using Maniphest tasks almost as JIRA-style epics, to track the review status of each patchset and co-ordinate landing those patches. ('arc land' is also useless to us in a number of cases, because the merge process often involves multiple stages of merging through sub-trees, before it lands in the master repository.)

I do have a patch which implements this, however it's pretty horrendously ugly, directly bashing the TaskDetailView controller. I suspect a more reasonable approach would involve the PHID's renderLink method. I haven't attached it as your contributor guidelines make it incredibly clear that I shouldn't at this stage.

Thanks for considering this. Let me know if you consider it useful and we can work out how best to land it.

Event Timeline

fooishbar raised the priority of this task from to Needs Triage.
fooishbar updated the task description. (Show Details)
fooishbar added a project: Maniphest.
fooishbar added a subscriber: fooishbar.
fooishbar updated the task description. (Show Details)Jan 28 2015, 8:39 PM
johnny-bit added a subscriber: johnny-bit.

@chad Ha, no. It's a pretty terrible hack to manually construct the tags directly from within buildPropertyView; pulling the revision object and then building up each of colour/icon/etc with a bunch of manual switch statements. Lots of copy-and-paste. If using PhabricatorStatusUI on the revisions is the way to go, I'll see how that works tomorrow and if there's anything I need to do with the revision class itself to implement that?

epriestley renamed this task from Show status of linked code reviews for tasks to Show status of linked revisions for tasks.Apr 29 2015, 8:51 PM
epriestley added a project: Differential.
chad added a comment.Apr 29 2015, 9:00 PM

Anyone is welcome to contribute this, it would be a nice addition. Easiest UI wise to use https://secure.phabricator.com/uiexample/view/PhabricatorStatusUIExample/.

Thanks @couture! Sorry mine vanished: it took quite some time to get the CLA signed and I lost all time after that.

No worries.

It might not be what folks are looking for, colour-wise. If there's some consensus about what the colours should be for each status, I could update the diff. I just made a best guess from looking at other places in the code.

Sorry, this is a lot harder to build than it seems like it might be, and probably not appropriate for a contributor. In particular:

  • Revision state is not currently present on the handle, and needs to be loaded.
  • Loading revision state on handles unconditionally is too expensive.
  • Caching this state so it's cheap enough to load unconditionally is complicated.
  • After more work in connection with T1279 (T8080 is one example, T8173 has some discussion) revision state may change (and, particularly, become more viewer-dependent).
  • Handles are currently independent of viewers except for visibility, and keeping them this way is desirable to possibly enable more sophisticated handle caching in the future (see, e.g., T7707 for related work in reducing the cost of handles).
  • Handles are application-agnostic and this implies introducing a new class of handle state which has limited usefulness and is orthogonal to existing state (after D12832, "status" and "availability").

Untangling this is very complicated.

couture added a comment.EditedMay 14 2015, 1:50 PM

Fair enough. I can appreciate all the problems with the submitted diff and why the problem space is more complicated than it appears -- this was more of a proof of concept for the team I work with than something that should be consumed by a wider user base.

I suspected my having read the code-base for a couple hours to drum up a PoC is not going to yield a desired, and more importantly sane, approach.

Thanks for taking a look.

angie added a project: Restricted Project.Jul 14 2015, 5:59 PM