Gwibber

Merge lp:~robru/gwibber/avatars into lp:~barry/gwibber/py3

avatars
Merge into py3

Proposed by Robert Bruce Park on 2012-09-10

Status:	Merged
Merged at revision:	1424
Proposed branch:	lp:~robru/gwibber/avatars
Merge into:	lp:~barry/gwibber/py3
Diff against target:	199 lines (+113/-35) 3 files modified gwibber/gwibber/tests/test_avatars.py (+56/-0) gwibber/gwibber/utils/avatar.py (+52/-0) gwibber/microblog/util/resources.py (+5/-35)
To merge this branch:	bzr merge lp:~robru/gwibber/avatars
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Barry Warsaw		2012-09-10	Pending
Review via email: mp+123634@code.launchpad.net

Description of the change

New Avatar class handles the downloading and caching of any arbitrary avatar URL online.

Thanks to GdkPixbuf, I was able to slash out a lot of code related to file IO and resizing/saving of images. Also all images are now converted to PNG before being stored in the cache. I didn't profile the code but the 75% reduction in file IO should see some good performance enhancements.

Revision history for this message

Barry Warsaw (barry) wrote on 2012-09-10:

Download full text (8.5 KiB)

On Sep 10, 2012, at 08:09 PM, Robert Bruce Park wrote:

>New Avatar class handles the downloading and caching of any arbitrary avatar
>URL online.

Thanks again for another fine refactoring and code deletion branch.
Eventually this sweater will look lean and mean! :)

>Thanks to GdkPixbuf, I was able to slash out a lot of code related to file IO
>and resizing/saving of images. Also all images are now converted to PNG
>before being stored in the cache. I didn't profile the code but the 75%
>reduction in file IO should see some good performance enhancements.

Of course, right now, we're much happier about the 200% improvement in code
readability. We can worry about performance later. :)

I'll clean up a few of the little style nits when I check things in, but let's
look at the code substantively.

=== added file 'gwibber/gwibber/tests/test_avatars.py'
--- gwibber/gwibber/tests/test_avatars.py 1970-01-01 00:00:00 +0000
+++ gwibber/gwibber/tests/test_avatars.py 2012-09-10 20:08:21 +0000
> @@ -0,0 +1,56 @@
> +# Copyright (C) 2012 Canonical Ltd
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License version 2 as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +"""Test the Avatar cacher."""
> +
> +__all__ = [
> + 'TestAvatars',
> + ]
> +
> +
> +import unittest
> +
> +from gwibber.utils.avatar import Avatar
> +from gwibber.testing.mocks import FakeData
> +
> +from gi.repository import GdkPixbuf
> +
> +try:
> + # Python 3.3
> + from unittest import mock
> +except ImportError:
> + import mock
> +
> +
> +class TestAvatars(unittest.TestCase):
> + """Test Avatar logic."""
> +
> + def test_hashing(self):
> + path = Avatar.get_path('fake_url')
> +
> + self.assertEqual(
> + '.cache/gwibber/avatars/4f37e5dc9d38391db1728048344c3ab5ff8cecb2',
> + path[-63:])

There are a few problems with this test, which I am happy to fix so that you
can continue with the more important work of working on the dispatcher.

The first is that because this test uses the user's actual cache, it's success
can be affected by previous runs, or even the operational environment of the
user. There are a couple of ways to fix this, but what I like best is to use
a temporary directory to store the cache for the test run. This can be
accomplished by mocking the base path to the cache, setting up the tempdir in
a setUp() and removing it in a tearDown().

Probably also a better way to test the expected path is to do an
os.path.split() and check just the components you care about.

> +
> + @mock.patch('gwibber.utils.download.urlopen',
> + FakeData('gwibber.tests.data', 'ubuntu.png'))
> + def test_cache_fill(self):
> + p...

On Sep 10, 2012, at 08:09 PM, Robert Bruce Park wrote:

>New Avatar class handles the downloading and caching of any arbitrary avatar
>URL online.

Thanks again for another fine refactoring and code deletion branch.
Eventually this sweater will look lean and mean! :)

Of course, right now, we're much happier about the 200% improvement in code
readability.  We can worry about performance later. :)

I'll clean up a few of the little style nits when I check things in, but let's
look at the code substantively.

=== added file 'gwibber/gwibber/tests/test_avatars.py'
--- gwibber/gwibber/tests/test_avatars.py	1970-01-01 00:00:00 +0000
+++ gwibber/gwibber/tests/test_avatars.py	2012-09-10 20:08:21 +0000
> @@ -0,0 +1,56 @@
> +# Copyright (C) 2012 Canonical Ltd
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License version 2 as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +"""Test the Avatar cacher."""
> +
> +__all__ = [
> +    'TestAvatars',
> +    ]
> +
> +
> +import unittest
> +
> +from gwibber.utils.avatar import Avatar
> +from gwibber.testing.mocks import FakeData
> +
> +from gi.repository import GdkPixbuf
> +
> +try:
> +    # Python 3.3
> +    from unittest import mock
> +except ImportError:
> +    import mock
> +
> +
> +class TestAvatars(unittest.TestCase):
> +    """Test Avatar logic."""
> +
> +    def test_hashing(self):
> +        path = Avatar.get_path('fake_url')
> +
> +        self.assertEqual(
> +            '.cache/gwibber/avatars/4f37e5dc9d38391db1728048344c3ab5ff8cecb2',
> +            path[-63:])

There are a few problems with this test, which I am happy to fix so that you
can continue with the more important work of working on the dispatcher.

The first is that because this test uses the user's actual cache, it's success
can be affected by previous runs, or even the operational environment of the
user.  There are a couple of ways to fix this, but what I like best is to use
a temporary directory to store the cache for the test run.  This can be
accomplished by mocking the base path to the cache, setting up the tempdir in
a setUp() and removing it in a tearDown().

Probably also a better way to test the expected path is to do an
os.path.split() and check just the components you care about.

> +
> +    @mock.patch('gwibber.utils.download.urlopen',
> +                FakeData('gwibber.tests.data', 'ubuntu.png'))
> +    def test_cache_fill(self):
> +        path = Avatar.get_image('http://example.com')
> +        pixbuf = GdkPixbuf.Pixbuf.new_from_file(path)
> +        self.assertEqual(pixbuf.get_height(), 48)
> +        self.assertEqual(pixbuf.get_width(), 48)
> +

> +        # Confirm that the resulting cache image is actually a PNG.
> +        with open(path, 'rb') as raw:
> +            self.assertEqual(raw.read(8), bytes.fromhex('89504E470D0A1A0A'))

I want to add another test, which is that if the cache is already filled,
another download does not happen.

=== added file 'gwibber/gwibber/utils/avatar.py'
--- gwibber/gwibber/utils/avatar.py	1970-01-01 00:00:00 +0000
+++ gwibber/gwibber/utils/avatar.py	2012-09-10 20:08:21 +0000
> @@ -0,0 +1,52 @@
> +# Copyright (C) 2012 Canonical Ltd
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License version 2 as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +"""Utils for downloading, sizing, and caching of avatar images."""
> +
> +__all__ = [
> +    'Avatar',
> +    ]
> +
> +from gi.repository import Gio, GLib, GdkPixbuf
> +from hashlib import sha1
> +
> +import os
> +import logging
> +log = logging.getLogger('gwibber.avatars')
> +
> +from gwibber.utils.download import Downloader
> +
> +
> +CACHE_DIR = os.path.realpath(os.path.join(
> +    GLib.get_user_cache_dir(), 'gwibber', 'avatars'))

We'll mock this out in the test so that we get the tempdir test isolation.

> +
> +
> +class Avatar:
> +    @staticmethod
> +    def get_path(url):

Of course, this could also be a module global function.  It's a tough call,
with arguments on either side, but since the function is probably not needed
outside this module (tests don't count), I'd personally make it a non-public
module global.

In a previous merge proposal we talked a little about public vs. non-public
names, and you're right that Python doesn't make any strict distinction
between the two.  That's not why it's useful to start non-public names with an
underscore.

The entire reason is as a useful, Pythonic, recognizable clue for future
readers and developers.   Think about it this way: you have to port some
unfamiliar code base (oh, I don't know, take a guess :) and you're trying to
decide whether some functionality must be preserved or not.  You don't really
know if there are any third party clients out there of the code, so you don't
know whether a change of yours will break something or not.

Even in a language like Python, marking non-public names with a single leading
underscore tells clients "You can use this if you must, but the risk is
yours.  If I change it in a future version, and it breaks your code, tough
luck for you."  When you use public methods (e.g. no leading underscore),
you're implicitly agreeing not to break the API.

This way, clients need only look at your code to answer the question: can I
safely use this API or not?

At least, that's the way I always think about these types of things.

> +        if not os.path.isdir(CACHE_DIR):
> +            os.makedirs(CACHE_DIR)
> +        return os.path.join(CACHE_DIR, sha1(url.encode('utf-8')).hexdigest())

There's one problem with this protocol.  The file is saved in a hash of the
url used to download it, but without regard to the contents of the file.

Imagine that an image changes but it is still available under the original
url.  How would gwibber-service ever notice the new version of the image?

I'll put an XXX in the code when I merge it, but there are a couple of
considerations to give.  First, the hash could be calculated from the file
contents, which you do get from the Downloader.  Second, there probably needs
to be an API for cache eviction, e.g. after some amount of time, any existing
cache entry would time out, and you'd re-download the image anyway.  Or, you
can provide an API for explicitly clearing the cache.

The extra network access will be offset by ensuring that the image won't be
out of date too often.

> +
> +    @staticmethod
> +    def get_image(url):
> +        local_path = Avatar.get_path(url)
> +        if not os.path.exists(local_path) or os.stat(local_path).st_size == 0:

This is a typical "look before you leap" style, which we usually try to avoid
in Python, in favor of an "ask forgiveness" type style.  Or in other words,
just do the os.state() and catch the exception that might occur if the file is
missing.

> +            log.debug('Avatar {} empty or missing, download triggered...', url)
> +            image_data = Downloader(url).get_bytes()
> +            input_stream = Gio.MemoryInputStream.new_from_data(image_data, None)
> +            pixbuf = GdkPixbuf.Pixbuf.new_from_stream_at_scale(
> +                input_stream, 48, 48, True, None)
> +            pixbuf.savev(local_path, 'png', [], [])
> +        return local_path

=== modified file 'gwibber/microblog/util/resources.py'
--- gwibber/microblog/util/resources.py	2012-08-21 22:53:27 +0000
+++ gwibber/microblog/util/resources.py	2012-09-10 20:08:21 +0000

Yay for code deletion!

Revision history for this message

Robert Bruce Park (robru) wrote on 2012-09-10:

Download full text (7.7 KiB)

On 12-09-10 03:55 PM, Barry Warsaw wrote:
> Thanks again for another fine refactoring and code deletion branch.
> Eventually this sweater will look lean and mean! :)

Thanks for reviewing so promptly. If it wasn't for your timely advice I
wouldn't be able to move so swiftly ;-)

>> + self.assertEqual(
>> + '.cache/gwibber/avatars/4f37e5dc9d38391db1728048344c3ab5ff8cecb2',
>> + path[-63:])
>
> There are a few problems with this test, which I am happy to fix so that you
> can continue with the more important work of working on the dispatcher.
>
> The first is that because this test uses the user's actual cache, it's success
> can be affected by previous runs, or even the operational environment of the
> user. There are a couple of ways to fix this, but what I like best is to use
> a temporary directory to store the cache for the test run. This can be
> accomplished by mocking the base path to the cache, setting up the tempdir in
> a setUp() and removing it in a tearDown().

Surely this critique refers to the *following* code block and not to
what is immediately quoted above? The get_path test specifically avoids
testing the specific user's home dir, and doesn't do anything but hash a
string.

> Probably also a better way to test the expected path is to do an
> os.path.split() and check just the components you care about.

That would be cleaner, yeah.

>> + # Confirm that the resulting cache image is actually a PNG.
>> + with open(path, 'rb') as raw:
>> + self.assertEqual(raw.read(8), bytes.fromhex('89504E470D0A1A0A'))
>
> I want to add another test, which is that if the cache is already filled,
> another download does not happen.

Yes, and I see you already mocked out the users actual cache dir,
allowing the test to avoid stale results from previous test runs, which
is a problem that hadn't occurred to me until after submitting the mp.

>> +class Avatar:
>> + @staticmethod
>> + def get_path(url):
>
> Of course, this could also be a module global function. It's a tough call,
> with arguments on either side, but since the function is probably not needed
> outside this module (tests don't count), I'd personally make it a non-public
> module global.

I suspected you might have concerns about a class that contains only two
staticmethods. There is still a del_avatar method left to port (was
going to leave it for later since it didn't seem immediately critical),
and I was thinking that Avatar class could grow into a more
general-purpose 'AvatarManager' kind of thing, but for now I just wanted
it to be a class for the sake of being an easy little bundle for
importing (eg, import Avatar and you get both methods easily).

On 12-09-10 03:55 PM, Barry Warsaw wrote:
> Thanks again for another fine refactoring and code deletion branch.
> Eventually this sweater will look lean and mean! :)

Thanks for reviewing so promptly. If it wasn't for your timely advice I 
wouldn't be able to move so swiftly ;-)

>> +        self.assertEqual(
>> +            '.cache/gwibber/avatars/4f37e5dc9d38391db1728048344c3ab5ff8cecb2',
>> +            path[-63:])
>
> There are a few problems with this test, which I am happy to fix so that you
> can continue with the more important work of working on the dispatcher.
>
> The first is that because this test uses the user's actual cache, it's success
> can be affected by previous runs, or even the operational environment of the
> user.  There are a couple of ways to fix this, but what I like best is to use
> a temporary directory to store the cache for the test run.  This can be
> accomplished by mocking the base path to the cache, setting up the tempdir in
> a setUp() and removing it in a tearDown().

Surely this critique refers to the *following* code block and not to 
what is immediately quoted above? The get_path test specifically avoids 
testing the specific user's home dir, and doesn't do anything but hash a 
string.

> Probably also a better way to test the expected path is to do an
> os.path.split() and check just the components you care about.

That would be cleaner, yeah.

>> +        # Confirm that the resulting cache image is actually a PNG.
>> +        with open(path, 'rb') as raw:
>> +            self.assertEqual(raw.read(8), bytes.fromhex('89504E470D0A1A0A'))
>
> I want to add another test, which is that if the cache is already filled,
> another download does not happen.

Yes, and I see you already mocked out the users actual cache dir, 
allowing the test to avoid stale results from previous test runs, which 
is a problem that hadn't occurred to me until after submitting the mp.

>> +class Avatar:
>> +    @staticmethod
>> +    def get_path(url):
>
> Of course, this could also be a module global function.  It's a tough call,
> with arguments on either side, but since the function is probably not needed
> outside this module (tests don't count), I'd personally make it a non-public
> module global.

I suspected you might have concerns about a class that contains only two 
staticmethods. There is still a del_avatar method left to port (was 
going to leave it for later since it didn't seem immediately critical), 
and I was thinking that Avatar class could grow into a more 
general-purpose 'AvatarManager' kind of thing, but for now I just wanted 
it to be a class for the sake of being an easy little bundle for 
importing (eg, import Avatar and you get both methods easily).

> The entire reason is as a useful, Pythonic, recognizable clue for future
> readers and developers.   Think about it this way: you have to port some
> unfamiliar code base (oh, I don't know, take a guess :) and you're trying to
> decide whether some functionality must be preserved or not.  You don't really
> know if there are any third party clients out there of the code, so you don't
> know whether a change of yours will break something or not.
>
> Even in a language like Python, marking non-public names with a single leading
> underscore tells clients "You can use this if you must, but the risk is
> yours.  If I change it in a future version, and it breaks your code, tough
> luck for you."  When you use public methods (e.g. no leading underscore),
> you're implicitly agreeing not to break the API.

Hmmmm, that point is slowly starting to sink in. I'll fully appreciate 
it one day soon I'm sure ;-)

>> +        if not os.path.isdir(CACHE_DIR):
>> +            os.makedirs(CACHE_DIR)
>> +        return os.path.join(CACHE_DIR, sha1(url.encode('utf-8')).hexdigest())
>
> There's one problem with this protocol.  The file is saved in a hash of the
> url used to download it, but without regard to the contents of the file.
>
> Imagine that an image changes but it is still available under the original
> url.  How would gwibber-service ever notice the new version of the image?

Well, it wouldn't, but this is a direct port of the previous behavior, 
so this bug you've identified was there from before. That is, unless 
there's perhaps something somewhere else in the codebase I haven't found 
that expires or deletes cache files. I did a grep -R and all I came up 
with was one spot where it deletes cache files that fail to load, which 
makes me shudder to think that there's some *other* code somewhere else 
that manages to write invalid image data into the cache files... 
(hopefully gdkpixbuf smooths over most of that mess for us)

Like I said, later on we can expand Avatar into AvatarManager and 
perhaps it can just do a quick scan of the cache dir and delete any 
files that are older than a month or something (some arbitrarily large 
time value such that it doesn't add a lot of overhead for downloading 
new avatars, but does make it possible for people to change their avatars).

Keep in mind, this would only be a bug for protocols that use a 
canonical URL for the user's avatar image. I'm not actually aware of any 
like this; if you look at the actual avatar URLs, you find that most of 
them contain some kind of hash of the image data, so that when somebody 
picks a new avatar, they get a new avatar URL, and thus the problem 
stops being "people can't change their avatar image ever" but reduces 
simply to "cache fills up with stale avatars, eventually filling the 
user's disk at some point after the heat death of the universe."

There is a log.debug statement there that triggers whenever it downloads 
a new URL, so perhaps we could monitor those logs and see what kinds of 
URLs go through it. That would necessarily come after integration 
testing, though.

> I'll put an XXX in the code when I merge it, but there are a couple of
> considerations to give.  First, the hash could be calculated from the file
> contents, which you do get from the Downloader.

This could never work because the whole point of this cache is to stop 
from having to re-download the avatar. If you have to download the 
avatar in order to hash it in order to check that it's already in the 
cache, then you've already lost.

> Second, there probably needs
> to be an API for cache eviction, e.g. after some amount of time, any existing
> cache entry would time out, and you'd re-download the image anyway.  Or, you
> can provide an API for explicitly clearing the cache.

Yes, this I definitely agree with, we'll need some cache expiry logic at 
some point.

>> +    @staticmethod
>> +    def get_image(url):
>> +        local_path = Avatar.get_path(url)
>> +        if not os.path.exists(local_path) or os.stat(local_path).st_size == 0:
>
> This is a typical "look before you leap" style, which we usually try to avoid
> in Python, in favor of an "ask forgiveness" type style.  Or in other words,
> just do the os.state() and catch the exception that might occur if the file is
> missing.

Darn, I had actually written it as try/except the first time around, but 
then I changed it. I see you changed it back, but did it better. The 
reason I didn't like it that way originally was that I wasn't quite 
familiar enough with that errno stuff to be confident that I wasn't 
catching too general of an exception, and also it felt a bit contorted 
the way I was attempting to conflate the case of the missing file with 
empty file.

I like the way you have it quite a bit though, the except: size = 0 
embodies the perfect semantics.

> === modified file 'gwibber/microblog/util/resources.py'
> --- gwibber/microblog/util/resources.py	2012-08-21 22:53:27 +0000
> +++ gwibber/microblog/util/resources.py	2012-09-10 20:08:21 +0000
>
> Yay for code deletion!

Easily my favorite part! ;-)

Revision history for this message

Barry Warsaw (barry) wrote on 2012-09-10:

Download full text (7.8 KiB)

On Sep 10, 2012, at 10:01 PM, Robert Bruce Park wrote:

>>> + self.assertEqual(
>>> + '.cache/gwibber/avatars/4f37e5dc9d38391db1728048344c3ab5ff8cecb2',
>>> + path[-63:])
>>
>> There are a few problems with this test, which I am happy to fix so that
>> you can continue with the more important work of working on the dispatcher.
>>
>> The first is that because this test uses the user's actual cache, it's
>> success can be affected by previous runs, or even the operational
>> environment of the user. There are a couple of ways to fix this, but what
>> I like best is to use a temporary directory to store the cache for the test
>> run. This can be accomplished by mocking the base path to the cache,
>> setting up the tempdir in a setUp() and removing it in a tearDown().
>
>Surely this critique refers to the *following* code block and not to
>what is immediately quoted above? The get_path test specifically avoids
>testing the specific user's home dir, and doesn't do anything but hash a
>string.

Ah, but it has the side-effect of creating a directory path in ~/.cache so it
does pollute the user's environment.

>>> +class Avatar:
>>> + @staticmethod
>>> + def get_path(url):
>>
>> Of course, this could also be a module global function. It's a tough call,
>> with arguments on either side, but since the function is probably not needed
>> outside this module (tests don't count), I'd personally make it a non-public
>> module global.
>
>I suspected you might have concerns about a class that contains only two
>staticmethods. There is still a del_avatar method left to port (was
>going to leave it for later since it didn't seem immediately critical),
>and I was thinking that Avatar class could grow into a more
>general-purpose 'AvatarManager' kind of thing, but for now I just wanted
>it to be a class for the sake of being an easy little bundle for
>importing (eg, import Avatar and you get both methods easily).

Yep, no problem.

>> The entire reason is as a useful, Pythonic, recognizable clue for future
>> readers and developers. Think about it this way: you have to port some
>> unfamiliar code base (oh, I don't know, take a guess :) and you're trying
>> to decide whether some functionality must be preserved or not. You don't
>> really know if there are any third party clients out there of the code, so
>> you don't know whether a change of yours will break something or not.
>>
>> Even in a language like Python, marking non-public names with a single
>> leading underscore tells clients "You can use this if you must, but the
>> risk is yours. If I change it in a future version, and it breaks your
>> code, tough luck for you." When you use public methods (e.g. no leading
>> underscore), you're implicitly agreeing not to break the API.
>
>Hmmmm, that point is slowly starting to sink in. I'll fully appreciate
>it one day soon I'm sure ;-)

Just keep repeating the Zen of Python every night before bed.

`import this`

Contemplate the 20th Zen until you achieve Pythonista (or Pythoneer)
enlightenment.

>>> + if not os.path.isdir(CACHE_DIR):
>>> + os.makedirs(CACHE_DIR)
>>> + return os.path.jo...

On Sep 10, 2012, at 10:01 PM, Robert Bruce Park wrote:

>>> +        self.assertEqual(
>>> +            '.cache/gwibber/avatars/4f37e5dc9d38391db1728048344c3ab5ff8cecb2',
>>> +            path[-63:])
>>
>> There are a few problems with this test, which I am happy to fix so that
>> you can continue with the more important work of working on the dispatcher.
>>
>> The first is that because this test uses the user's actual cache, it's
>> success can be affected by previous runs, or even the operational
>> environment of the user.  There are a couple of ways to fix this, but what
>> I like best is to use a temporary directory to store the cache for the test
>> run.  This can be accomplished by mocking the base path to the cache,
>> setting up the tempdir in a setUp() and removing it in a tearDown().
>
>Surely this critique refers to the *following* code block and not to 
>what is immediately quoted above? The get_path test specifically avoids 
>testing the specific user's home dir, and doesn't do anything but hash a 
>string.

Ah, but it has the side-effect of creating a directory path in ~/.cache so it
does pollute the user's environment.

>>> +class Avatar:
>>> +    @staticmethod
>>> +    def get_path(url):
>>
>> Of course, this could also be a module global function.  It's a tough call,
>> with arguments on either side, but since the function is probably not needed
>> outside this module (tests don't count), I'd personally make it a non-public
>> module global.
>
>I suspected you might have concerns about a class that contains only two 
>staticmethods. There is still a del_avatar method left to port (was 
>going to leave it for later since it didn't seem immediately critical), 
>and I was thinking that Avatar class could grow into a more 
>general-purpose 'AvatarManager' kind of thing, but for now I just wanted 
>it to be a class for the sake of being an easy little bundle for 
>importing (eg, import Avatar and you get both methods easily).

Yep, no problem.

>> The entire reason is as a useful, Pythonic, recognizable clue for future
>> readers and developers.  Think about it this way: you have to port some
>> unfamiliar code base (oh, I don't know, take a guess :) and you're trying
>> to decide whether some functionality must be preserved or not.  You don't
>> really know if there are any third party clients out there of the code, so
>> you don't know whether a change of yours will break something or not.
>>
>> Even in a language like Python, marking non-public names with a single
>> leading underscore tells clients "You can use this if you must, but the
>> risk is yours.  If I change it in a future version, and it breaks your
>> code, tough luck for you."  When you use public methods (e.g. no leading
>> underscore), you're implicitly agreeing not to break the API.
>
>Hmmmm, that point is slowly starting to sink in. I'll fully appreciate 
>it one day soon I'm sure ;-)

Just keep repeating the Zen of Python every night before bed.

`import this`

Contemplate the 20th Zen until you achieve Pythonista (or Pythoneer)
enlightenment.

>>> +        if not os.path.isdir(CACHE_DIR):
>>> +            os.makedirs(CACHE_DIR)
>>> +        return os.path.join(CACHE_DIR, sha1(url.encode('utf-8')).hexdigest())
>>
>> There's one problem with this protocol.  The file is saved in a hash of the
>> url used to download it, but without regard to the contents of the file.
>>
>> Imagine that an image changes but it is still available under the original
>> url.  How would gwibber-service ever notice the new version of the image?
>
>Well, it wouldn't, but this is a direct port of the previous behavior, 
>so this bug you've identified was there from before. That is, unless 
>there's perhaps something somewhere else in the codebase I haven't found 
>that expires or deletes cache files. I did a grep -R and all I came up 
>with was one spot where it deletes cache files that fail to load, which 
>makes me shudder to think that there's some *other* code somewhere else 
>that manages to write invalid image data into the cache files... 
>(hopefully gdkpixbuf smooths over most of that mess for us)

Naw, I'm sorry about giving the wrong impression.  I wasn't necessarily saying
it was a bug in your code, I was just blathering on about observations I was
making.  I've added an XXX to avatars.py for future consideration.

>Like I said, later on we can expand Avatar into AvatarManager and 
>perhaps it can just do a quick scan of the cache dir and delete any 
>files that are older than a month or something (some arbitrarily large 
>time value such that it doesn't add a lot of overhead for downloading 
>new avatars, but does make it possible for people to change their avatars).
>
>Keep in mind, this would only be a bug for protocols that use a 
>canonical URL for the user's avatar image. I'm not actually aware of any 
>like this; if you look at the actual avatar URLs, you find that most of 
>them contain some kind of hash of the image data, so that when somebody 
>picks a new avatar, they get a new avatar URL, and thus the problem 
>stops being "people can't change their avatar image ever" but reduces 
>simply to "cache fills up with stale avatars, eventually filling the 
>user's disk at some point after the heat death of the universe."
>
>There is a log.debug statement there that triggers whenever it downloads 
>a new URL, so perhaps we could monitor those logs and see what kinds of 
>URLs go through it. That would necessarily come after integration 
>testing, though.
>
>> I'll put an XXX in the code when I merge it, but there are a couple of
>> considerations to give.  First, the hash could be calculated from the file
>> contents, which you do get from the Downloader.
>
>This could never work because the whole point of this cache is to stop 
>from having to re-download the avatar. If you have to download the 
>avatar in order to hash it in order to check that it's already in the 
>cache, then you've already lost.

Right.  Ultimately, you might want to use HTTP metadata to do this.  But as
you say, let's leave this for another day.

>> Second, there probably needs to be an API for cache eviction, e.g. after
>> some amount of time, any existing cache entry would time out, and you'd
>> re-download the image anyway.  Or, you can provide an API for explicitly
>> clearing the cache.
>
>Yes, this I definitely agree with, we'll need some cache expiry logic at 
>some point.
>
>>> +    @staticmethod
>>> +    def get_image(url):
>>> +        local_path = Avatar.get_path(url)
>>> +        if not os.path.exists(local_path) or os.stat(local_path).st_size == 0:
>>
>> This is a typical "look before you leap" style, which we usually try to
>> avoid in Python, in favor of an "ask forgiveness" type style.  Or in other
>> words, just do the os.state() and catch the exception that might occur if
>> the file is missing.
>
>Darn, I had actually written it as try/except the first time around, but 
>then I changed it. I see you changed it back, but did it better. The 
>reason I didn't like it that way originally was that I wasn't quite 
>familiar enough with that errno stuff to be confident that I wasn't 
>catching too general of an exception, and also it felt a bit contorted 
>the way I was attempting to conflate the case of the missing file with 
>empty file.

It's going to be even cooler in Python 3.3:

http://www.python.org/dev/peps/pep-3151/

then, errno won't even be needed and the code will look like this:

try:
    size = os.state(local_path).st_size
except FileNowFoundError:
    size = 0

>I like the way you have it quite a bit though, the except: size = 0 
>embodies the perfect semantics.
>
>> === modified file 'gwibber/microblog/util/resources.py'
>> --- gwibber/microblog/util/resources.py	2012-08-21 22:53:27 +0000
>> +++ gwibber/microblog/util/resources.py	2012-09-10 20:08:21 +0000
>>
>> Yay for code deletion!
>
>Easily my favorite part! ;-)

*Always* :)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Barry Warsaw

Robert Bruce Park

 === added file 'gwibber/gwibber/tests/data/ubuntu.png'
 Binary files gwibber/gwibber/tests/data/ubuntu.png	1970-01-01 00:00:00 +0000 and gwibber/gwibber/tests/data/ubuntu.png	2012-09-10 20:08:21 +0000 differ
 === added file 'gwibber/gwibber/tests/test_avatars.py'
 --- gwibber/gwibber/tests/test_avatars.py	1970-01-01 00:00:00 +0000
 +++ gwibber/gwibber/tests/test_avatars.py	2012-09-10 20:08:21 +0000
@@ -0,0 +1,56 @@
++# Copyright (C) 2012 Canonical Ltd
++#
++# This program is free software: you can redistribute it and/or modify
++# it under the terms of the GNU General Public License version 2 as
++# published by the Free Software Foundation.
++#
++# This program is distributed in the hope that it will be useful,
++# but WITHOUT ANY WARRANTY; without even the implied warranty of
++# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++# GNU General Public License for more details.
++#
++# You should have received a copy of the GNU General Public License
++# along with this program.  If not, see <http://www.gnu.org/licenses/>.
++
++"""Test the Avatar cacher."""
++
++__all__ = [
++    'TestAvatars',
++    ]
++
++
++import unittest
++
++from gwibber.utils.avatar import Avatar
++from gwibber.testing.mocks import FakeData
++
++from gi.repository import GdkPixbuf
++
++try:
++    # Python 3.3
++    from unittest import mock
++except ImportError:
++    import mock
++
++
++class TestAvatars(unittest.TestCase):
++    """Test Avatar logic."""
++
++    def test_hashing(self):
++        path = Avatar.get_path('fake_url')
++
++        self.assertEqual(
++            '.cache/gwibber/avatars/4f37e5dc9d38391db1728048344c3ab5ff8cecb2',
++            path[-63:])
++
++    @mock.patch('gwibber.utils.download.urlopen',
++                FakeData('gwibber.tests.data', 'ubuntu.png'))
++    def test_cache_fill(self):
++        path = Avatar.get_image('http://example.com')
++        pixbuf = GdkPixbuf.Pixbuf.new_from_file(path)
++        self.assertEqual(pixbuf.get_height(), 48)
++        self.assertEqual(pixbuf.get_width(), 48)
++
++        # Confirm that the resulting cache image is actually a PNG.
++        with open(path, 'rb') as raw:
++            self.assertEqual(raw.read(8), bytes.fromhex('89504E470D0A1A0A'))
 === added file 'gwibber/gwibber/utils/avatar.py'
 --- gwibber/gwibber/utils/avatar.py	1970-01-01 00:00:00 +0000
 +++ gwibber/gwibber/utils/avatar.py	2012-09-10 20:08:21 +0000
@@ -0,0 +1,52 @@
++# Copyright (C) 2012 Canonical Ltd
++#
++# This program is free software: you can redistribute it and/or modify
++# it under the terms of the GNU General Public License version 2 as
++# published by the Free Software Foundation.
++#
++# This program is distributed in the hope that it will be useful,
++# but WITHOUT ANY WARRANTY; without even the implied warranty of
++# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++# GNU General Public License for more details.
++#
++# You should have received a copy of the GNU General Public License
++# along with this program.  If not, see <http://www.gnu.org/licenses/>.
++
++"""Utils for downloading, sizing, and caching of avatar images."""
++
++__all__ = [
++    'Avatar',
++    ]
++
++from gi.repository import Gio, GLib, GdkPixbuf
++from hashlib import sha1
++
++import os
++import logging
++log = logging.getLogger('gwibber.avatars')
++
++from gwibber.utils.download import Downloader
++
++
++CACHE_DIR = os.path.realpath(os.path.join(
++    GLib.get_user_cache_dir(), 'gwibber', 'avatars'))
++
++
++class Avatar:
++    @staticmethod
++    def get_path(url):
++        if not os.path.isdir(CACHE_DIR):
++            os.makedirs(CACHE_DIR)
++        return os.path.join(CACHE_DIR, sha1(url.encode('utf-8')).hexdigest())
++
++    @staticmethod
++    def get_image(url):
++        local_path = Avatar.get_path(url)
++        if not os.path.exists(local_path) or os.stat(local_path).st_size == 0:
++            log.debug('Avatar {} empty or missing, download triggered...', url)
++            image_data = Downloader(url).get_bytes()
++            input_stream = Gio.MemoryInputStream.new_from_data(image_data, None)
++            pixbuf = GdkPixbuf.Pixbuf.new_from_stream_at_scale(
++                input_stream, 48, 48, True, None)
++            pixbuf.savev(local_path, 'png', [], [])
++        return local_path
 === modified file 'gwibber/microblog/util/resources.py'
 --- gwibber/microblog/util/resources.py	2012-08-21 22:53:27 +0000
 +++ gwibber/microblog/util/resources.py	2012-09-10 20:08:21 +0000
@@ -17,7 +17,7 @@
  import logging
  logger = logging.getLogger("Resources")
--# Try to import * from custom, install custom.py to include packaging
++# Try to import * from custom, install custom.py to include packaging
  # customizations like distro API keys, etc
  try:
    from custom import *
@@ -32,8 +32,8 @@
  THEME_NAME = "default"
  # Minimum theme version, this is a serial to ensure themes are compatible
--# with current version of the client.  This serial is set in the theme
--# dir in a file named theme.version
++# with current version of the client.  This serial is set in the theme
++# dir in a file named theme.version
  THEME_MIN_VERSION = 2
  try:
@@ -77,30 +77,12 @@
          if os.path.isdir(os.path.join(p, d)):
            if d not in PLUGINS:
              PLUGINS.append(d)
--  return [PLUGINS,PLUGINS_DIRS]
++  return [PLUGINS,PLUGINS_DIRS]
  def get_twitter_keys():
    # Distros should register their own keys and not rely on the defaults
    return TWITTER_OAUTH_KEY, TWITTER_OAUTH_SECRET
--def get_avatar_path(url):
--  avatar_cache_dir = realpath(join(CACHE_BASE_DIR, "gwibber", "avatars"))
--  if not isdir(avatar_cache_dir):
--    makedirs(avatar_cache_dir)
--  avatar_cache_image = join(avatar_cache_dir, sha1(url).hexdigest())
--
--  if not exists(avatar_cache_image) or len(open(avatar_cache_image, "r").read()) < 1:
--    logger.debug("Downloading avatar %s", url)
--    f = file(avatar_cache_image, "w")
--    data = network.Download(url)
--    f.write(data.get_string())
--    f.close()
--    img_resize(avatar_cache_image, 48)
--
--  if len(open(avatar_cache_image, "r").read()) > 0:
--    return avatar_cache_image
--  return None
--
  def del_avatar(avatar):
    if exists(avatar):
      try:
@@ -108,22 +90,10 @@
      except:
        logger.error("Failed to remove avatar from cache: %s", avatar)
--def img_resize(img_path, size):
--  try:
--    image = Image.open(img_path)
--    x, y = image.size
--    if x != size or y != size:
--      # need to upsample limited palette images before resizing
--      if image.mode == 'P': image = image.convert('RGBA')
--      image.resize((size, size), Image.ANTIALIAS).save(img_path, format="jpeg")
--  except Exception as e:
--    from traceback import format_exc
--    logger.error("Image resizing failed:\n%s", format_exc())
--
  def get_desktop_file():
    p = os.path.join(LAUNCH_DIR, "gwibber.desktop")
    if os.path.exists(p): return p
--
++
    for base in DATA_BASE_DIRS:
      p = os.path.join(base, "applications", "gwibber.desktop")
      if os.path.exists(p): return p

Gwibber

Merge lp:~robru/gwibber/avatars into lp:~barry/gwibber/py3

Commit message

Description of the change

Preview Diff

Subscribers