This change removes the maximum dimensions and file size text appears in the description for uploading user profile images (avatars). Drupal appears to handle the resizing of images automatically now so text is not needed.

Files: 
CommentFileSizeAuthor
#42 drupal.user_picture_text_27234_42.patch2.06 KBrfay
PASSED: [[SimpleTest]]: [MySQL] 22,138 pass(es).
[ View ]
#37 drupal.user_picture_text_27234_37.patch2.12 KBrfay
PASSED: [[SimpleTest]]: [MySQL] 22,024 pass(es).
[ View ]
#34 drupal.user_picture_text_27234_33.patch1.51 KBrfay
PASSED: [[SimpleTest]]: [MySQL] 22,016 pass(es).
[ View ]
#31 27234_help_text_picture_size.patch1.57 KBmaartenvg
Failed: Failed to apply patch.
[ View ]
#23 user_27234.patch1.73 KBdrewish
Unable to apply patch user_27234.patch
[ View ]
#20 user_35.patch3.12 KBSteve Dondley
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_35.patch.
[ View ]
#10 user_26.patch1.38 KBSteve Dondley
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_26.patch.
[ View ]
#7 user_24.patch1.3 KBSteve Dondley
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_24.patch.
[ View ]
#1 user_23.patch1.06 KBSteve Dondley
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_23.patch.
[ View ]
user_22.patch1.06 KBSteve Dondley
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_22.patch.
[ View ]

Comments

Steve Dondley’s picture

StatusFileSize
new1.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_23.patch.
[ View ]

Bug in above patch (missing paren) fixed.

matt westgate’s picture

The maximum width and height dimensions still apply when there is no image library installed (see user_validate_picture()). Maybe we could detect if an image library is present and inform the user their picture will automatically be resized to dimension x and y, otherwise the text is displayed as it presently is.

Dries’s picture

I didn't even know we could run without an image library installed. Is that a valid scenario, or more of a left-over from the Drupal 4.5 era? If possible, I'd prefer having one way of doing things, rather than having a new and old way of doing things.

Bèr Kessels’s picture

I am all for a image-toolkit-only system. But I am a bit concerned that that will rule out a lot of people who have no access to the server/php set up.

So, would it be worth it, for sake of usability and ease-of use to demand a toolkit for thumbs, and leave those who have no server softwar efor hendling images without avatars?

Steven’s picture

-1 on this.

For one thing, it still makes sense to inform people what the maximum avatar size is. Don't underestimate how many avatars are custom made for a particular site.

Secondly, it is important to know the maximum size for animated avatars... if they go over the maximum size, they are resized and come out as static images. Without this info you would need trial and error to figure it out.

Steve Dondley’s picture

The big problem with displaying the dimensions and maximum size is that if people have an image that exceeds those limits, they will not bother to upload the image because they think they will have to open their image editing software to do that. Those instructions serve as an image upload deterrent, not a help at all.

Steve Dondley’s picture

StatusFileSize
new1.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_24.patch.
[ View ]

Here's a compromise patch to address Steven's objection. I don't know the best way to detect if an image library is installed. Someone else will have to write that or at least save me time and give me a hint on how to do it.

matt westgate’s picture

You can use image_get_toolkit() to check if an image library is installed. Here's some sample code taken from user.module.

<?php
if (image_get_toolkit()) { // Imaging library exists
 
image_scale($file->filepath, $file->filepath, $maxwidth, $maxheight);
}
?>

Once the profile help text is based on the existence of an image library, this patch would be good to go.

matt westgate’s picture

You can use image_get_toolkit() to check if an image library is installed. Here's some sample code taken from user.module.

<?php
if (image_get_toolkit()) { // Imaging library exists
 
image_scale($file->filepath, $file->filepath, $maxwidth, $maxheight);
}
?>

Once the profile help text is based on the existence of an image library, this patch would be good to go.

Steve Dondley’s picture

StatusFileSize
new1.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_26.patch.
[ View ]

Thanks for the hand, Mathias. Here's a new patch.

matt westgate’s picture

+1 for this patch and the additional help text it provides.

Bèr Kessels’s picture

The patch applies and works.

But I find the text: Your image will be automatically resized to fit the maximum dimension of 100x100 and maximum file size of 200 k
strange. how can something be resized under the maximum file size of 200 k ?

Bèr Kessels’s picture

On second thought: I (personally) dislike giving this kind of information. These are exactly the texts that scare people away, or at least make them not even read a text.

But I will not object against this patch on that basis, since I can change it, using locales. I will change it to "If your image is too big, it will be resized". For me, that is more then enought information. And for Joe Average it s too.

killes@www.drop.org’s picture

I agree with Ber, there is no need to scare or bore end users with this kind of info.

Dries’s picture

Here is what I want you to do: go for a image toolkit only approach -- that is what we introduced toolkits for. If I'm not mistaken, there are several toolkits available in contrib so people should be able to find a working toolkit.

This means, you'll have to simplify/test user_validate_picture() and the surrounding code. While testing, I'd modify image_get_toolkit() to return NULL to 'simulate' the absense of a toolkit.

Can you investigate that?

Steve Dondley’s picture

Dries, who are you addressing here?

Dries’s picture

nysus: mainly you but also the other developers who want to see this fixed.

Steve Dondley’s picture

Yeah, I'll take a look into it when I get a chance.

Dries’s picture

Status:Needs review» Needs work
Steve Dondley’s picture

StatusFileSize
new3.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_35.patch.
[ View ]

Patch per Dries request.

drumm’s picture

patching file user.module
Hunk #1 FAILED at 241.
Hunk #2 FAILED at 1161.
2 out of 2 hunks FAILED -- saving rejects to file user.module.rej

lilou’s picture

Version:x.y.z» 7.x-dev

This feature request is it still valid ?

drewish’s picture

Status:Needs work» Needs review
StatusFileSize
new1.73 KB
Unable to apply patch user_27234.patch
[ View ]

here's a current version of this.

maartenvg’s picture

Works like a charm, but has a different approach then #20. Was that intentionally (there have of course been some changes since 2005)?

Dave Reid’s picture

Status:Needs review» Needs work

I think the easier solution would be to not allow the site admin to enable user pictures if there is no image toolkit. I'd like to incorporate this issue into #305802: Improve default user picture interface since this issue seems like it's lost it's steam.

drewish’s picture

Status:Needs work» Needs review

Dave Reid, no need to take such a simple help fix off topic. don't let the perfect be the enemy of the good. if i've got a single user site and i want my account to have a picture but don't have gd, it's much simpler for me to just scale one image by hand than to recompile php.

Dave Reid’s picture

Whoops. Sorry drewish! I'll continue in my issue and not bother you again here. :)

Anonymous’s picture

Status:Needs review» Needs work

The last submitted patch failed testing.

maartenvg’s picture

Status:Needs work» Needs review

No longer fails locally, so resetting.

catch’s picture

While we're here, can we split that really long array into multiple lines?

maartenvg’s picture

Assigned:Steve Dondley» Unassigned
StatusFileSize
new1.57 KB
Failed: Failed to apply patch.
[ View ]

Reroll per #30.
And I unassigned Steve, because he hasn't been in this thread since 2005.

Dave Reid’s picture

Issue tags:+Usability, +user pictures

Status:Needs review» Needs work

The last submitted patch failed testing.

rfay’s picture

Status:Needs work» Needs review
StatusFileSize
new1.51 KB
PASSED: [[SimpleTest]]: [MySQL] 22,016 pass(es).
[ View ]

Just stumbled on this. Here's my own wording change.

Do we even need the "max size" limitation in the admin interface any more?

joachim’s picture

Status:Needs review» Needs work
+++ modules/user/user.module
@@ -1115,7 +1115,7 @@ function user_account_form(&$form, &$form_state) {
+    '#description' => t('Your virtual face or picture. You may upload larger pictures, but they will be resized to %dimensions pixels.', array('%dimensions' => variable_get('user_picture_dimensions', '85x85'))) . ' ' . filter_xss_admin(variable_get('user_picture_guidelines', '')),

Larger than what?

Better wording something like:

'Your virtual face or picture. Pictures larger than %dimensions pixels will be scaled to fit.'

Powered by Dreditor.

andypost’s picture

rfay’s picture

Status:Needs work» Needs review
StatusFileSize
new2.12 KB
PASSED: [[SimpleTest]]: [MySQL] 22,024 pass(es).
[ View ]

Here's another round with improved text - thanks, @joachim.

I did some study and debugging, and here's the scoop:

1. If the image library works at all (or even seems to work) then an upload image is downsized (if necesssary) to the provided image dimensions *before* the file size is checked.

2. The file size limit is therefore basically irrelevant or at least misleading, as long as it's properly proportional to the image dimensions. It's misleading for us to even call it the "file upload size" limit, when it's actually the "after resizing image size".

This text attempts to deal with those facts as well as it can. It would not be unreasonable to completely remove the image file size settings in the admin interface, since we do assume that a system with a broken image library is a broken system.

joachim’s picture

Maximum allowed file size for uploaded pictures. On a normal working system this only needs to be large enough for a file of size implied by "Picture upload dimensions" above because larger files will be rescaled to the dimensions above.'

This is kinda scary. How do I know if my system is 'normal working'??
And it's also a lot of verbiage to say the obvious -- of COURSE the dimensions restriction acts too.

Compare with the description text on image field size setting:

Enter a value like "512" (bytes), "80 KB" (kilobytes) or "50 MB" (megabytes) in order to restrict the allowed file size. If left empty the file sizes will be limited only by PHP's maximum post and file upload sizes (current limit 32 MB).
Your virtual face or picture. Pictures larger than @dimensions pixels will be scaled down to @dimensions.'

That's pretty ugly with the dimensions string twice.

joachim’s picture

Would this do?

'Your virtual face or picture. Pictures larger than @dimensions pixels will be scaled down to fit.'

rfay’s picture

We probably need to get this going. It's a tiny patch, shouldn't take that much work to get it RTBC. Doesn't deserve *so* much conversation. @joachim, do you want to roll a version with the text the way you'd like it?

rfay’s picture

StatusFileSize
new2.06 KB
PASSED: [[SimpleTest]]: [MySQL] 22,138 pass(es).
[ View ]

OK, I think this takes people's comments into account.

Let's get this silly thing done. If you want something different, post a patch. Otherwise, let's get it done.

joachim’s picture

Looks good.

andypost’s picture

Status:Needs review» Reviewed & tested by the community

A much clear. Let's RTBC this.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

Status:Fixed» Closed (fixed)
Issue tags:-Usability, -user pictures

Automatically closed -- issue fixed for 2 weeks with no activity.