Blocked by T4214: Allow installs to change or supplement the "Phabricator" icon?

Yeah, I asked the team to get this going so I could change it on Phacility. ;)

+ @btrahan

Added the 1x and 2x logos, not sure where to drop them or how they look in practice. haha.

Sounds like we're all kind of weakly in favor of not branding instances.

How does one generate the sprite map? I'm finding things to be hard to figure out... e.g. the CeleritySpriteGenerator class feels like its important, but I have no idea how it gets invoked?

10:35:04 ~/Dropbox/code/phalanx/src (T7165)
~> grep -r CeleritySpriteGenerator *
__phutil_library_map__.php:    'CeleritySpriteGenerator' => 'applications/celerity/CeleritySpriteGenerator.php',
applications/celerity/CeleritySpriteGenerator.php:final class CeleritySpriteGenerator {

The idea with this particular path was to simply change the file being served without getting into the DOM and / or changing how the plumbing works too heavily. e.g. maybe there's a "--custom-header-image-path" flag to the sprite generation process.

A completely different implementation path would change PhabricatorMainMenuView->renderPhabricatorLogo() to (probably) just use a different class if / when a custom image exists and load that instead, as opposed to trying to make the existing DOM classes load some different image via the sprite map generation process.

For posterity, here's some other implementation advice from another thread, which was particularly aimed at trying to make this general for other installations and not just admin.phacility.com:

I thiiiink the ideal implementation of T7165 would end up just storing a URI in the config (so we don't have to do loads / permission checks on every page), although we could also store something else (file PHID) and just cache the URI (but we don't have a very-cheap, invalidatable cache right now). But storing a URI is also kind of garbage because a bunch of other options could affect it and we can't dirty it.

I thought about putting a file on disk and inlining it with a data URI in the preprocessor, but it's hard for us to dirty that too and I don't want users to have to regenerate the map.

The UI should also hypothetically give users an upload control, not just a "put the PHID of a file into this text box", which is a fair bit of work to integrate into Config.

So I guess maybe:

Put a PHID in a text box for now (some day: nice upload control and resizing).
We'll load it and cache the URL in the immutable cache (for now: tell users to restart the server if their image is screwed up in the instructions on the config option; some day: maybe we'll have a fast dirtyable cache).

I guess I'm obviously talking about making users invalidate the map with the celerity approach. This didn't strike me as too bad for those users customizing their instances / I figured we'd be able to kick off an offline thing with a progress bar eventually anyway, removing the usability issue in time.

scripts/celerity/generate_sprites.php

I think regenerating the map is really bad in the long run. Particularly, it means the sprite map and resource map will be dirty in the working copy, so git pull will fail the next time we update them and we'll get a bunch of instances that can't upgrade. Another issue is that Phacility users could not customize their instance headers, because they all share a single map. And they need optipng installed. And the daemons need write access to the source code itself if we're going to do this in the background.

I expect the implementation to look something like:

if (PhabricatorEnv::getEnvConfig('ui.custom-header')) {
  $uri = cache_magic();
  $override_style = 'background-image: url('.$uri.')';
} else {
  $override_style = null;
}
 
return phutil_tag(
  'a',
  array(
    'style' => $override_style,
  ),
  // blah blah etc);

I haven't actually looked at the code, so I don't know how practical that is, but I think that's a lot simpler and cleaner in the long run than trying to rebuild sprite maps.

I guess you'd probably need to override background-image-position-x/y and such too if it's currently using a sprite.

Did a bunch of work on this and I just realized that I'll need to add some code to when we render the header to add a full on style section. I think this is necessary to support retina or whatever you want to call the 1x vs 2x versions. :/

@chad - just want to confirm two things -

• You just want to replace the text logo (and not also the eye part of the logo?) The zip file on this task is more ambitious than what was checked into rSERVICES so as of my current-sandbox-code you can replace the eye too. :D Anyway, might as well ask pre-diff...
• Image dimensions should be enforced, and they should be enforced as the dimensions of what was checked into rSERVICES? Seems worth it to me to enforce some dimensions so things look okay-ish and otherwise I've currently coded things to enforce the exact dimensions of the rSERVICES images.
  • I've done the leg-work to make this config ...well, configurable, so in theory folks could specify pixel offsets and stuff too. I wouldn't want to make this overly complex though and would prefer a more simple "the image should be this big" sort of rule.

The zip here is void. What I checked into rSERVICES is what's been properly tested.

The image I checked in is the same size as the standard logo.

We should be able to use one image and just background-size-x/y it to the right dimensions. On retina it will have 2x resolution properly, on normal displays it will be "too big", but that's fine (they won't be much bigger, and they'll get cached).

I've done the leg-work to make this config ...well, configurable, so in theory folks could specify pixel offsets and stuff too. I wouldn't want to make this overly complex though and would prefer a more simple "the image should be this big" sort of rule.

I don't think we should have this, just one PHID. We should set the correct size in the "style" attribute and tell users what size to use. If they use the wrong size, too bad, they get a squished image. Eventually we'll write a nice upload control and resize whatever junk they upload fro them.

(In the Phacility case, the 1x is about 2KB and the 2x is about 3KB. Clearly not worth special casing to save 1KB, once, on non-retina displays.)

Okay, no worries about making it simpler. Its not really that fancy anyway; I just made this config a "custom" type, wrote the pertinent class, and have reasonable error-checking logic in there for my guess at the spec. Good to know the retina stuff doesn't really matter; I thought I had to emulate the CSS properties as they exist now for e.g. whatever Apple magic happens when you switch from portrait to landscape mode.