Paths

Table of Contentst

Differential D25862

Communicate max dimensions of profile images before upload
AcceptedPublic
Actions

Authored by aklapper on Sat, Jan 18, 11:48.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15984: Communicate maximum dimensions of avatar images

Summary

Trying to set a large image as a project profile image, Phorge displays the "it is a mystery" placeholder image without errors or explanation.
Thus communicate the maximum file dimensions for transforming thumbnails, like Phorge already does for supported file format types.

Closes T15984

Test Plan

Go to http://phorge.localhost/project/picture/1/ and http://phorge.localhost/people/picture/1/ and set an image with 4096×4096px and an image with 4097×4097px and observe resulting image.
Apply this patch, then go to http://phorge.localhost/project/picture/1/ and http://phorge.localhost/people/picture/1/ and see the additional "Maximum image dimensions: 4096×4096 pixels." in the "Upload New Picture" section, set an image with 4096×4096px and an image with 4097×4097px.
Also test on http://phorge.localhost/phame/post/header/1/ and http://phorge.localhost/phame/blog/header/1/ but realize that these codepaths do not transform larger images, thus no problem. I remain clueless how to trigger PhameBlogProfilePictureController, DiffusionRepositoryProfilePictureController with similar code.

Diff Detail

Repository

rP Phorge

Branch

T15984 (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1674
Build 1674: arc lint + arc unit

Event Timeline

aklapper created this revision.Sat, Jan 18, 11:48

Owners added a reviewer: O1: Blessed Committers.Sat, Jan 18, 11:48

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptSat, Jan 18, 11:48

Harbormaster completed remote builds in B1673: Diff 2606.Sat, Jan 18, 11:48

aklapper requested review of this revision.Sat, Jan 18, 11:48

mb_chr(215) because arc lint dislikes funky ×
const MAX_TRANSFORM_* because arc lint does not like private const, so maybe PhabricatorFileImageTransform::getMaxTransformDimensions() is overengineering as I could also access the two consts directly instead
Cannot private const int because that requires PHP 8.3 (type safety haha)

Cigaryno added projects: People, Projects.Sat, Jan 18, 11:54

@Cigaryno: Could you please not add project tags to patches? You seem to be the only person doing this lately, and it creates notifications that others need to read. Thanks.

@aklapper just to ask, is adding tags to revs pointless or this isn't the right revision for adding tags to?

@Cigaryno: What is your motivation behind and which actual problem are you trying to solve by this? For example, there are now 4 out of ~25000 patches tagged with Projects if I was about to search for patches tagged with Projects. I can imagine tags to be partially useful on Maniphest tasks and I fail to see the advantage for Differential patches so far.

In D25862#23134, @aklapper wrote:

@Cigaryno: What is your motivation behind and which actual problem are you trying to solve by this? For example, there are now 4 out of ~25000 patches tagged with Projects if I was about to search for patches tagged with Projects. I can imagine tags to be partially useful on Maniphest tasks and I fail to see the advantage for Differential patches so far.

This is not problem solving as now I don't think anyone would search for revs with specific pages. So I think you are right with this as what I was doing was just adding unnecessary extra to the feed.

Still untested but seems nice thanks <3

src/applications/files/transform/PhabricatorFileImageTransform.php
337–338	I would personally prefer this line to be based on `$this->getMaxTransformDimensions()`, so people are encouraged in just improving that method / extend the class, without any need to consolidate a potentially new method with the old constants. After that, maybe we can drop the constants, to leave the values just in `getMaxTransformDimensions()`, ready for an override from a children class. (Const-based inheritance is tricky, method-based inheritance is easier - not just in Phorge)
373–375	In any case (with constants or not) maybe better non-static (suggestion by avivey) so it's easier to extend the class.

avivey subscribed.Tue, Jan 21, 08:33

avivey added inline comments.

src/applications/files/transform/PhabricatorFileImageTransform.php
337–338	I can see a future where users ask for this number to be configurable. In the old days we would just T8227 it (and maybe make the const bigger), but we're not as strict about those things now, so 🤷‍♂️.

Do not use consts but keep the two pixel values defined in getMaxTransformDimensions(), make function non-static

Harbormaster completed remote builds in B1674: Diff 2607.Tue, Jan 21, 19:01

Ouch the non-static method raised an error. I'm investigating a possible fix to keep this non-static approach in the next 20 minutes

src/applications/files/transform/PhabricatorFileImageTransform.php
338	Please everybody ignore my next comment: Uh! It exists `array_product(array)` - throw a dice. If >= 6, let's adopt it. lol

Hoping to be useful I will amend to fix 2 new errors caused by my suggestion.

(As usual, we can rollback if needed 🌈 )

fix PhabricatorPeopleProfilePictureController and PhabricatorProjectEditPictureController,
thanks to generic "transform" stuff from PhabricatorFileTransform

Harbormaster completed remote builds in B1675: Diff 2608.Wed, Jan 22, 07:33

It works on my computer \o/ thanks

In a next future we might want to completely refuse to upload a profile picture that will not be visible since it would not be cropped. So users don't see that "It's a mistery" thing.

Also, in a next next future we would probably (also?) want to show the recommended size, rather than just our hard limit. It's $xform->getDimensions() - in case. So designers can write their blog posts, like

How to optimize your images for your Phorge audience, in 10 steps ✨

lol

But this new information is a big step for human beings, as-is

This revision is now accepted and ready to land.Wed, Jan 22, 07:42