softdevteam / mattermost-github-integration

GitHub integration for Mattermost
Other
79 stars 50 forks source link

[Bug] User avatar resizing #6

Closed forelabs closed 7 years ago

forelabs commented 8 years ago

Hello,

Github has a bug on their own image generation, they do not resize svg files. That's why using

avatar = self.data['sender']['avatar_url'] + "&s=18"

won't work for them. See: https://avatars2.githubusercontent.com/u/4415695?v=3&s=18 avatar with 18 px width

So be aware by using the avatar, when your team has a user with default avatar or an uploaded svg. But because Mattermost is not able to allow resizing as well (for now - I hope this will follow), I do not have a solution for this.

ptersilie commented 8 years ago

I noticed this too and was desperately looking for a solution. You are right, it seems to be a GitHub bug, so as long as that is not fixed, and Mattermost doesn't allow image resizing in markdown, it looks like the only solution is to deactivate avatars. I thought at first I could just alter the Mattermost stylesheet for resizing, but that would change all markdown images.

I will add an option to the settings to deactivate avatars for users who have many collaborators who don't have an avatar.

ptersilie commented 8 years ago

Added an option to the settings to deactivate avatars in notifications: 41d981b. I also send a bug report to Github, so hopefully this will be fixed soon.

ptersilie commented 8 years ago

Github's reply:

Thanks for your feedback! I can see how resizing the default avatars through the URL parameters would be useful. I can't promise we'll do it, but I have shared your feedback with our team for them to consider.

vrenjith commented 8 years ago

Thanks for the update @ptersilie

mharrend commented 8 years ago

Just as a proposal: Maybe the following code could be used to implement a check if the size of the avatar is larger than some threshold, so that too big avatars are dropped while the others are kept?

The only drawback is that further dependencies are introduced.

Taken from http://effbot.org/zone/pil-image-size.htm

import urllib
import ImageFile

def getsizes(uri):
    # get file size *and* image size (None if not known)
    file = urllib.urlopen(uri)
    size = file.headers.get("content-length")
    if size: size = int(size)
    p = ImageFile.Parser()
    while 1:
        data = file.read(1024)
        if not data:
            break
        p.feed(data)
        if p.image:
            return size, p.image.size
            break
    file.close()
    return size, None

print getsizes("http://www.pythonware.com/images/small-yoyo.gif")
# (10965, (179, 188))
ptersilie commented 8 years ago

I thought about this but in the end didn't want to add more dependencies to the plugin, especially since the next logical step from there would be to downsize (and cache) images that are too big, which might introduce more dependencies. I was kind of hoping that Github would fix their API or Mattermost would introduce image scaling. Unfortunately neither has happened so far.

However, I'm more than happy to accept PRs on this, given that the dependencies are optional i.e. if the ImageFile module is not installed, the code is just ignored and doesn't fail. We don't want to break peoples setup the next time they pull.

ptersilie commented 7 years ago

Fixed in #13