Page MenuHomePhabricator

Migrate DiffusionBlameController to use repo identities
ClosedPublic

Authored by amckinley on Aug 14 2018, 11:46 PM.
Tags
None
Referenced Files
F13236081: D19587.diff
Tue, May 21, 7:57 AM
F13225846: D19587.id46847.diff
Sun, May 19, 5:45 PM
F13216158: D19587.id.diff
Fri, May 17, 10:42 PM
F13200712: D19587.id47105.diff
Tue, May 14, 2:56 AM
F13197448: D19587.id46818.diff
Mon, May 13, 12:11 AM
F13192602: D19587.id.diff
Sun, May 12, 8:58 AM
F13191855: D19587.id46849.diff
Sun, May 12, 1:28 AM
F13186326: D19587.diff
Sat, May 11, 3:34 AM
Subscribers

Details

Summary

Now on the blame page, identities get avatar.png and there are little tooltips that show a few characters of the committer identity string.

Also add a default icon for repo identities.

Test Plan

Loaded some blame pages for files touched by users with and without repo identities attached.

Diff Detail

Repository
rP Phabricator
Branch
commit-cleanup3
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 20618
Build 28018: Run Core Tests
Build 28017: arc lint + arc unit

Event Timeline

src/applications/diffusion/controller/DiffusionBlameController.php
134

Does something like this look OK?

if (strlen($author_src)) {
  $author_style = ...;
  $author_icon = null;
} else {
  $author_style = null;
  $author_icon = id(new PHUIIconView())->setIcon('fa-something-something');
}

Then $author_link = javelin_tag(..., ..., $author_icon) down below. Basically, plug in a little identity icon instead of an actual background image if we have an Identity instead of a real user. This might make the CSS go craaaaaazy, though.

Alternatively, we could change if (!$author_phid) to if (!$author_phid || !$image_uri), which would "fix" this without a regression, but it would be nice to actually render something useful here instead of just giving up.

Failing this, you can use PhabricatorFile::loadBuiltin(...) and drop some static image file in resources/builtin/ to get a real image file, something like:

$identity_file = PhabricatorFile::loadBuiltin($viewer, 'identity-default-profile.png');
$identity_src = $identity_file->getViewURI();
src/applications/diffusion/controller/DiffusionBlameController.php
121–123

We could probably drop this bit of code now -- it should be exceptionally rare now, since we'll almost always have an Identity even if we don't have a real user. (Before, it was a reasonable fallback if you committed with a weird name but we found a revision and could kind of guess things.)

Use avatar.png as a default image for identities.

amckinley marked an inline comment as done.

Fix a problem with truncated tooltips by setting size to auto.

src/applications/diffusion/controller/DiffusionBlameController.php
134

I was leaning towards a change in PhabricatorRepositoryIdentityPHIDType to solve this everywhere we might want to render identity objects. Let me know if this is a Bad Idea for some reason.

Ah, yeah, this is even simpler.

This revision is now accepted and ready to land.Sep 4 2018, 3:32 PM
This revision was automatically updated to reflect the committed changes.