Page MenuHomePhabricator

Migrate DiffusionBlameController to use repo identities
ClosedPublic

Authored by amckinley on Aug 14 2018, 11:46 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 6:50 PM
Unknown Object (File)
Wed, Dec 18, 7:09 AM
Unknown Object (File)
Tue, Dec 17, 9:06 PM
Unknown Object (File)
Tue, Dec 10, 4:05 PM
Unknown Object (File)
Wed, Dec 4, 3:29 AM
Unknown Object (File)
Fri, Nov 29, 10:46 PM
Unknown Object (File)
Mon, Nov 25, 4:48 AM
Unknown Object (File)
Nov 21 2024, 9:29 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
arcpatch-D19587
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 20648
Build 28061: Run Core Tests
Build 28060: 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
120–121

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.