GNOME Bugzilla – Bug 671180
Better behaviour when profile image URL is invalid
Last modified: 2015-09-26 15:17:14 UTC
Created attachment 208837 [details] [review] D-L cosmetic changes Hi, Devs. I've made some changes on a local instance of Damned Lies just to correct an issue when the URL the D-L user provides for his/her image profile is not found. I.e: When the user joined the team, typed the URL for his profile image and for some reason the URL is not actually valid. Currently, when the internet browser doesn't find users image, it simply shows the "alt" attribute of the "img" tag. Instead, I wrote some lines of code that shows the nobody.png or nobody-16.png (when listing users Actions), with a message on "title" attribute when he/she is logged.
Here are some comments. You can disagree of course. Just explain your motivations if you think your approach is the right one: * I don't think it's worth creating a new templatetags file. I know stats_extras.py might be a bit overloaded, but I suggest adding the check_for_image filter in this existing file anyway. * About width/height forced to 100px, what about max-width/max-weight instead? * I suspect something is missing in your patch, as valid_image is referenced in templates, but I don't see where it is defined. Now I wonder if it wouldn't be better to just write a filter which takes person.image and output directly the <img> tag altogether. What do you think?
(In reply to comment #1) > Here are some comments. You can disagree of course. Just explain your > motivations if you think your approach is the right one: > No, problem. > * I don't think it's worth creating a new templatetags file. I know > stats_extras.py might be a bit overloaded, but I suggest adding the > check_for_image filter in this existing file anyway. > Well, in fact, I must confess that I created the new templatetag file because I didn't read stats_extras.py. +1 for you. > * About width/height forced to 100px, what about max-width/max-weight instead? > OK! Suggestion approved. +1 again. > * I suspect something is missing in your patch, as valid_image is referenced in > templates, but I don't see where it is defined. > This time, I understood that valid_image is used just used to hold the value returned from check_for_image. Am I wrong? > Now I wonder if it wouldn't be better to just write a filter which takes > person.image and output directly the <img> tag altogether. What do you think? Agreed! This would make the template code a little bit easyer to understand. Thanks!
(In reply to comment #2) > > * I suspect something is missing in your patch, as valid_image is referenced in > > templates, but I don't see where it is defined. > > > > This time, I understood that valid_image is used just used to hold the value > returned from check_for_image. Am I wrong? You are wrong here. Template filters do not assign values to variables. You should have written: {{ person.image|check_for_image }} with your filter receiving only one argument which is person.image content. I'm looking forward to your next patch :-)
Created attachment 209130 [details] [review] D-L cosmetic changes #2
(In reply to comment #3) > You are wrong here. Template filters do not assign values to variables. You > should have written: {{ person.image|check_for_image }} with your filter > receiving only one argument which is person.image content. Thanks for the explanation. After reading this, I skimmed through the Django documentation and understood a little bit about filters. But one thing that left me bored, was the impossibility of passing more than one parameter to the filter, which in this case, would help me/us a lot. So I decided to quickly correct the issue with the images using only nodody.png while I study a little more on how to use nobody.png and nobody-16.png without creating another filter just to make it work inside vertimus_detail.html. > > I'm looking forward to your next patch :-) And here it is!
Review of attachment 209130 [details] [review]: I don't think it's worth keeping both nobody and nobody-16, it's fine for me if you use the same image and just set the size to 14x14 through css for the vertimus_detail.html page. ::: stats/templatetags/stats_extras.py @@ +3,3 @@ from django import template from django.conf import settings +from django.contrib.auth.decorators import login_required Probably a leftover from a previous tentative... @@ +186,3 @@ + if is_authenticated: + result = '<img src="%simg/nobody.png" alt="" title="%s"/>' % (settings.MEDIA_URL, img_title) + return mark_safe(result) I think that it is unnecessary complicated to use is_authenticated. Just set the title to 'The image is invalid or was not found' for all cases.
Henrique, are you still interested to get your patch in? :) If you address Claude's last concerns in comment #6 maybe everything would be already fine to get the patch accepted and upstream ;)
Henrique seems to have disappeared. Gil or Claude, can you fix the patch and get it in?
Using URLValidator in the templates seems to be a bad idea to me both in term of performances and security. Django used to have a verify_url option on URLFied that has been removed for this reason. I'm trying to see if there is a way to do it client-side. I.e. let the browser do the request (which it'll do anyway) and putting our own fallback image if it fails.
Sure, this is the way to go.
Created attachment 312205 [details] [review] Add a javascript fallback for invalid avatar url
Review of attachment 312205 [details] [review]: Thanks!