Page MenuHomePhabricator

Migrate DiffusionBlameController to use repo identities
ClosedPublic

Authored by amckinley on Aug 14 2018, 11:46 PM.
Tags
None
Referenced Files
F19509134: D19587.diff
Sat, Jan 10, 3:42 PM
F18918859: D19587.id47105.diff
Nov 9 2025, 11:18 AM
F18799112: D19587.id.diff
Oct 17 2025, 11:42 AM
F18705567: D19587.id46818.diff
Sep 28 2025, 11:58 AM
F18697838: D19587.id.diff
Sep 27 2025, 10:28 AM
F18688679: D19587.diff
Sep 26 2025, 5:00 PM
F18658830: D19587.diff
Sep 23 2025, 8:40 AM
F18509748: D19587.id.diff
Sep 5 2025, 3:35 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 20646
Build 28058: Run Core Tests
Build 28057: arc lint + arc unit

Event Timeline

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

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
117–119

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
130

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.