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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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.