Merge lp:~zedtux/bzr-gtk/gravatar-urllib2 into lp:bzr-gtk/gtk2

[Jelmer Vernooij]

Proposing zedtux's gravatar branch.

[Jelmer Vernooij (jelmer)]

Hi Guillaume,

I think this is ready to land, though I have a few more minor remarks.
Please let me know if you're happy for me to just fix them before
landing or if you'd like fix them yourself.

> differences between files attachment (review-diff.txt)
> === added file 'avatar.py'
> --- avatar.py 1970-01-01 00:00:00 +0000
> +++ avatar.py 2011-03-11 21:59:14 +0000
> + # ~~~~~ Properties ~~~~~
> + # username
> + def get_username(self):
> + return self.__username
> + def set_username(self, username):
> + self.__username = username
> + username = property(get_username, set_username)
> +
> + # Name
> + def get_name(self):
> + return self.__name
> + def set_name(self, name):
> + self.__name = name
> + name = property(get_name, set_name)
> +
> + # Email
> + def get_email(self):
> + return self.__email
> + def set_email(self, email):
> + self.__email = email
> + email = property(get_email, set_email)
> +
> + # Image
> + def get_image(self):
> + return self.__image
> + def set_image(self, image):
> + self.__image = image
> + image = property(get_image, set_image)
Please avoid boilerplate code like this. Just let external users
set .image, .email, etc directory. They can always be changed to be
properties later, if necessary.

> + def is_identical(self, avatar):
> + """
> + Return True if attributes of the given avatar
> + match to current object attributes otherwise return False
> + """
> + return self.username == avatar.username and \
> + self.name == avatar.name and \
> + self.email == avatar.email
It might make sense to rename this to __eq__, so that you can just use
"==" to compare avatars.

> === added file 'avatarbox.py'
> --- avatarbox.py 1970-01-01 00:00:00 +0000
> +++ avatarbox.py 2011-03-11 21:59:14 +0000

> + # ~~~~~ Properties ~~~~~
> + # Avatar
> + def get_avatar(self):
> + return self.__current_avatar
> + def set_avatar(self, avatar):
> + self.__current_avatar = avatar
> + avatar = property(get_avatar, set_avatar)
^ See above.

> + # ~~~~~ Public methods ~~~~~

> + def and_avatar_email(self, email):
> + """
> + Select the avatar from avatars collection
> + in order to apply future actions
> + """
> + self.avatar = None
> + if not email is "":
> + self.avatar = self.__avatars[email] if email in self.__avatars else None
> + return self
Please avoid "if ... else" in one line as it doesn't work in Python 2.4,
which we bzr-gtk still supports.

> === added file 'avatardownloaderworker.py'
> --- avatardownloaderworker.py 1970-01-01 00:00:00 +0000
> +++ avatardownloaderworker.py 2011-03-11 21:59:14 +0000
> @@ -0,0 +1,72 @@
> +# Copyright (C) 2011 by Guillaume Hain (zedtux) <email address hidden>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This...

[Guillaume Hain (zedtux)]

Hello Jelmer,

Thank you for your merge proposal.

> Please avoid boilerplate code like this. Just let external users
> set .image, .email, etc directory. They can always be changed to be
> properties later, if necessary.

Okay, agree. I'm not a python expert, and you are learning me things about it. I did it in that way, because I was thinking that was the right way. Now you've told me that, it make sense to me to write it without property.

Could you explain me when it make sense to use property please?

> It might make sense to rename this to __eq__, so that you can just use
> "==" to compare avatars.

Same as above. I didn't learned it before. Thanks.

> Please avoid "if ... else" in one line as it doesn't work in Python 2.4,
> which we bzr-gtk still supports.

Again, thank you for your knowledge sharing.

> The urllib3 bit probably is no longer valid :)

Completely true.

> You'd probably want "super(AvatarProviderGravatar, self).__init__()" but
> that does the same thing as "AvatarProvider.__init__(self)".

Here is another point you are learning me.
Could you give me more details about this please?

Finally, about files: I will do it as you want. For me, a class per file is OK, but if you prefer the other way, I don't care.
How do you do to organize your files?
When do you decide when create a new file or not?

As I'm not at home for this week-end, I will patch my code during next week. Sorry about that.
I'm really happy to contribute to your project. And I hope we will have more collaboration together in the future! :)

Cheers,

[Jelmer Vernooij (jelmer)]

On Sat, 2011-03-12 at 11:26 +0000, zedtux wrote:
> > Please avoid boilerplate code like this. Just let external users >
> set .image, .email, etc directory. They can always be changed to be >
> properties later, if necessary. Okay, agree. I'm not a python expert,
> and you are learning me things about it. I did it in that way, because
> I was thinking that was the right way. Now you've told me that, it make
> sense to me to write it without property.
No problem.

> Could you explain me when it make sense to use property please?
If there are side-effects of setting this member variable or if you're
not storing the property in this class but elsewhere you generally want
a property So you could have a "committer_email" property that was
implemented as:

def get_committer_email(self):
   return self.committer.email

committer_email = property(get_committer_email)

Though I'm not sure how useful having a property would be in that
example, as opposed to just having the user look at .committer.email.

The nice thing is that if something is a member variable and you'd like
to change it to a property later, you can do that without having to
change the code that uses it.

> > You'd probably want "super(AvatarProviderGravatar, self).__init__()" but
> > that does the same thing as "AvatarProvider.__init__(self)".
> Here is another point you are learning me.
> Could you give me more details about this please?
super(AvatarProviderGravatar, self) returns the base instance - so for
AvatarProverGravatar that would be a AvatarProvider object. When calling
__init__() on it there's no need to provide self as it's an instance
object.

In the second case you're taking the __init__ method from AvatarProvider
and calling that on the current object.

Both things have the same result, except when you have multiple base
classes in which case you're better off using the second mechanism for
clarity. I'm not exactly sure which base class super() will return in
case of multiple inheritance, I suspect it'll return the first base
class.

> Finally, about files: I will do it as you want. For me, a class per file is OK, but if you prefer the other way, I don't care.
> How do you do to organize your files?
> When do you decide when create a new file or not?
Generally we stick related things in the same file, or in the same
package and try to keep them readable but not too long. Having too many
files usually makes it hard to find what you're looking for, for example
if you're trying to figure out what file to import something from.
Python also has a bit of overhead per file that's loaded, which we're
already suffering from in Bazaar.

I realize that's a bit handwavy, I guess roughly anywhere under 2000
lines would be ok.

> As I'm not at home for this week-end, I will patch my code during next week. Sorry about that.
> I'm really happy to contribute to your project. And I hope we will have more collaboration together in the future! :)
No worries, looking forward to the followup patch :)

Cheers,

Jelmer

[lp:~zedtux/bzr-gtk/gravatar-urllib2]

721. By Guillaume Hain on 2011-03-14

Patched code following Jelmer's recommandations

[Guillaume Hain (zedtux)]

Alright, I've patched my code.
The only thing I didn't is the AvatarProvider class move into the other file. When I've start to copy my code to move it, I was looking at the same time to your comments, and I saw that you're requesting me to move this class that should be inherited by all other future providers (If one day someone want to implement more providers) in the file of one provider, but it make no sense to me.

This master class actually only add the common attribute size, but it also could implement more attributes/method in the future if needed.

So, we have two possibles:
 - If you don't think this class is necessary, I can delete it.
 - Otherwise, it should stay in its file.

Tell me what is for you the best way.
After that point, you should merge my code ;)

[Jelmer Vernooij (jelmer)]

> Alright, I've patched my code.
> The only thing I didn't is the AvatarProvider class move into the other file.
> When I've start to copy my code to move it, I was looking at the same time to
> your comments, and I saw that you're requesting me to move this class that
> should be inherited by all other future providers (If one day someone want to
> implement more providers) in the file of one provider, but it make no sense to
> me.
>
> This master class actually only add the common attribute size, but it also
> could implement more attributes/method in the future if needed.
>
> So, we have two possibles:
> - If you don't think this class is necessary, I can delete it.
> - Otherwise, it should stay in its file.
I don't see why it would no longer be necessary if it's in the same file - it defines the interface that providers have to implement, that seems like a perfectly reasonable thing to have, even if it's in the same file.

As part of this, I think it would be useful to define a dummy get_base_url() in AvatarProvider that just raises NotImplementedError.

If you look at other parts of the bzr code you'll see this is a common pattern - a base class that provides a bunch of methods that all raise NotImplementedError and are then overridden by the subclasses. This makes it easy to see what to implement when you create a subclass and it gives a sane error message if a subclass forgot to provide a particular method.

Cheers,

Jelmer

[lp:~zedtux/bzr-gtk/gravatar-urllib2]

722. By Guillaume Hain on 2011-03-14

Renamed avatarprovider.py to avatarproviders.py
Moved class AvatarProviderGravatar into the avatarproviders.py file
Added get_base_url method into the AvatarProvider class raising NotImplementedError exception

[Guillaume Hain (zedtux)]

> As part of this, I think it would be useful to define a dummy get_base_url() in AvatarProvider that just raises NotImplementedError.

Done.

Finally, I've renamed file avatarprovider.py to avatarproviders.py, and then moved the AvatarProviderGravatar class into it.
It clear like this for me.

If it's the same for you, you can merge it, it's fine for me.
Otherwise, I'm open to your suggestions! :-)

[Jelmer Vernooij (jelmer)]

I think it's fine to merge now, but it still adds 5 files, all very small. Would you mind if I brought it down to 1 or 2 ?

[lp:~zedtux/bzr-gtk/gravatar-urllib2]

723. By Guillaume Hain on 2011-03-15

Moved Avatar and AvatarBox classes into avatarsbox file

724. By Guillaume Hain on 2011-03-15

Moved AvatarDownloaderWorker class into avatarsbox.py file

[Guillaume Hain (zedtux)]

Alright, it's done. :)

[Jelmer Vernooij (jelmer)]

On Tue, 2011-03-15 at 16:52 +0000, zedtux wrote:
> Alright, it's done. :)
Merged, thanks very much!

Cheers,

Jelmer

=== added file 'avatarproviders.py'
--- avatarproviders.py	1970-01-01 00:00:00 +0000
+++ avatarproviders.py	2011-03-15 16:52:28 +0000
@@ -0,0 +1,110 @@
+# Copyright (C) 2011 by Guillaume Hain (zedtux) <[e-mail]>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# 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, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+
+import Queue
+import urllib
+import urllib2
+import hashlib
+import threading
+
+class AvatarProvider(object):
+    """
+    Master class for Avatar providers.
+    
+    All AvatarProviderXxxx classes should inherite from this one
+    and override at least get_base_url.
+    """
+    def __init__(self, size=80):
+        """ Constructor """
+        self.size = size
+    
+    def get_base_url(self):
+        """
+        Override this methode in your provider class in order to return
+        base url of your provider.
+        """
+        raise NotImplementedError("You must implement the get_base_url method.")
+
+
+class AvatarDownloaderWorker(threading.Thread):
+    """
+    Threaded worker to retrieve avatar from a provider.
+    
+    It create a persitante connection to the provider in order
+    to get avatars quickly through the same socket (urllib3).
+    """
+    
+    def __init__(self, provider_method):
+        """
+        Constructor
+        
+        provider_method: Provider method that return fields
+                         to send with the request.
+        """
+        threading.Thread.__init__(self)
+        self.__stop = threading.Event()
+        self.__queue = Queue.Queue()
+        
+        self.__provider_method = provider_method
+        self.__end_thread = False
+    
+    def stop(self):
+        """ Stop this worker """
+        self.__end_thread = True
+        self.__stop.set()
+    
+    def set_callback_method(self, method):
+        """ Fire the given callback method when treatment is finished """
+        self.__callback_method = method
+    
+    def queue(self, id_field):
+        """
+        Put in Queue the id_field to treat in the thread.
+        This id_field is for example with Gravatar the email address.
+        """
+        self.__queue.put(id_field)
+    
+    def run(self):
+        """ Worker core code. """
+        while not self.__end_thread:
+            id_field = self.__queue.get()
+            # Call provider method to get fields to pass in the request
+            url = self.__provider_method(id_field)
+            # Execute the request
+            response = urllib2.urlopen(url)
+            # Fire the callback method
+            if not self.__callback_method is None:
+                self.__callback_method(response, id_field)
+            self.__queue.task_done()
+
+
+class AvatarProviderGravatar(AvatarProvider):
+    """ Gravatar provider """
+    
+    def __init__(self):
+        """ Constructor """
+        super(AvatarProviderGravatar, self).__init__()
+    
+    def get_base_url(self):
+        return "http://www.gravatar.com/avatar.php?"
+    
+    def gravatar_id_for_email(self, email):
+        """ Return a converted email address to a gravatar_id """
+        return self.get_base_url() + \
+                urllib.urlencode({
+                    'gravatar_id':hashlib.md5(email.lower()).hexdigest(),
+                    'size':str(self.size)
+                })

=== added file 'avatarsbox.py'
--- avatarsbox.py	1970-01-01 00:00:00 +0000
+++ avatarsbox.py	2011-03-15 16:52:28 +0000
@@ -0,0 +1,249 @@
+# Copyright (C) 2011 by Guillaume Hain (zedtux) <[e-mail]>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# 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, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+
+import gtk
+
+from bzrlib.plugins.gtk import _i18n
+from bzrlib.config import parse_username
+from bzrlib.plugins.gtk.avatarproviders import AvatarProviderGravatar, AvatarDownloaderWorker
+
+
+class Avatar(gtk.Box):
+    """ Author or committer avatar """
+    
+    def __init__(self, apparent_username):
+        """ Constructor """
+        gtk.Box.__init__(self)
+        
+        self.apparent_username = apparent_username
+        self.username, self.email = parse_username(apparent_username)
+        self.image = None
+    
+    def __eq__(self, other):
+        """
+        Return True if attributes of the given avatar
+        match to current object attributes otherwise return False
+        """
+        return self.apparent_username == other.apparent_username and \
+               self.name == other.name and \
+               self.email == other.email
+    
+    # ~~~~~ Public methods ~~~~~
+    def show_spinner(self):
+        """
+        Replace the current content of the Avatar with a gtk.Spinner
+        if an email address has been parsed. If not, show an gtk.Label with
+        the translatable 'No email' text.
+        """
+        if not self.email is "":
+            spinner = gtk.Spinner()
+            spinner.start()
+            self.pack_start(spinner, False)
+            spinner.set_tooltip_text(_i18n("Retrieving avatar for %s...") % self.email)
+            spinner.set_size_request(20, 20)
+            spinner.show()
+        else:
+            no_email = gtk.Label(_i18n("No email"))
+            self.pack_start(no_email)
+            self.set_tooltip_text(self.apparent_username)
+            no_email.show()
+    
+    def show_image(self):
+        """ Replace the current content of the Avatar with the gtk.Image """
+        if not self.email is "" and self.image:
+            self.remove(self.get_children()[0])
+            self.pack_start(self.image)
+            self.image.set_tooltip_text(self.apparent_username)
+            self.image.show()
+
+
+class AvatarBox(gtk.HBox):
+    """ Improved gtk.HBox """
+    
+    def __init__(self, homogeneous=False, spacing=0):
+        """ Constructor """
+        gtk.HBox.__init__(self, homogeneous, spacing)
+        self.__avatars = {}
+        self.avatar = None
+        self.__displaying = None
+    
+    
+    # ~~~~~ Public methods ~~~~~
+    def reset_view(self):
+        """ Remove current avatars from the gtk box """
+        for child in self.get_children():
+            self.remove(child)
+        self.__displaying = None
+    
+    def have_avatar(self, avatar):
+        """
+        Return True if this box has registered given avatar,
+        otherwise return False
+        """
+        return avatar.email in self.__avatars
+    
+    def showing(self, avatar):
+        """
+        Return True if the displaying avatar is the same
+        as the given one otherwise return False
+        """
+        return self.__displaying and self.__displaying == avatar
+    
+    def append_avatars_with(self, avatar):
+        """
+        Append avatars collection with the given one if not already registed
+        otherwise render it back.
+        When an avatar is added this method True, otherwise, if the avatar
+        was in the collection, return False.
+        """
+        if not avatar.email is "" and not avatar.email in self.__avatars:
+            self.__avatars[avatar.email] = avatar
+            self._new_cell_for_avatar(avatar)
+            return True
+        else:
+            self.and_avatar_email(avatar.email).come_back_to_gui()
+        return False
+    
+    def and_avatar_email(self, email):
+        """
+        Select the avatar from avatars collection
+        in order to apply future actions
+        """
+        self.avatar = None
+        if not email is "" and email in self.__avatars:
+            self.avatar = self.__avatars[email]
+        else:
+            self.avatar = None
+        return self
+    
+    def update_avatar_image_from_pixbuf_loader(self, loader):
+        """ Replace the gtk.Spinner with the given loader """
+        if self.avatar:
+            self.avatar.image = gtk.Image()
+            self.avatar.image.set_from_pixbuf(loader.get_pixbuf())
+            self.avatar.show_image()
+            self.__displaying = self.avatar
+    
+    def come_back_to_gui(self):
+        """ Render back avatar in the GUI """
+        if self.avatar:
+            self.pack_start(self.avatar)
+            self.__displaying = self.avatar
+        else:
+            self._show_no_email()
+    
+    
+    # ~~~~~ Private methods ~~~~~~
+    def _new_cell_for_avatar(self, avatar):
+        """ Create a new cell in this box with a gtk.Spinner """
+        avatar.show_spinner()
+        self.pack_start(avatar)
+        avatar.show()
+        self.__displaying = avatar
+    
+    def _show_no_email(self):
+        """ Show a gtk.Label with test 'No email' """
+        no_email = gtk.Label(_i18n("No email"))
+        self.pack_start(no_email)
+        no_email.show()
+
+
+class AvatarsBox(gtk.HBox):
+    """ GTK container for authors and committers avatars """
+    
+    def __init__(self):
+        """ Constructor """
+        gtk.HBox.__init__(self, False, 10)
+        
+        self.__committer_box = None
+        self.__authors_box = None
+        self._init_gui()
+        
+        # If more later you want to implement more avatar providers, to it like this:
+        # Create a new class named AvatarProvider + provider_name that inherit from
+        # the AvatarProvider class.
+        # Implement a method that return url to use in the request.
+        # For example, with Gravatar, the method return the complete url
+        # with MD5 hash of the email address and put the value in a gravatar_id field.
+        # Then create a new worker (manage them in a python dictionnary).
+        provider = AvatarProviderGravatar()
+        self.__worker = AvatarDownloaderWorker(
+            provider.gravatar_id_for_email
+        )
+        # This callback method should be fired bt all workers when a request is done.
+        self.__worker.set_callback_method(self._update_avatar_from_response)
+        self.__worker.start()
+    
+    
+    # ~~~~~ Public methods ~~~~~
+    def add(self, username, role):
+        """
+        Add the given username in the right role box and add in the worker queue.
+        Here again: If you want to implement more providers, you should add the
+        avatar request in all workers queue.
+        """
+        avatar = Avatar(username)
+        if role is "author" and not self._role_box_for("committer").showing(avatar) or role is "committer":
+            if self._role_box_for(role).append_avatars_with(avatar):
+                self.__worker.queue(avatar.email)
+    
+    def merge(self, usernames, role):
+        """ Add avatars from a list """
+        for username in usernames:
+            self.add(username, role)
+    
+    def reset(self):
+        """
+        Request a reset view for all boxes in order to show only avatars
+        of the selected line in the revision view screen.
+        """
+        [self._role_box_for(role).reset_view() for role in ["committer", "author"]]
+    
+    
+    # ~~~~~ Private methods ~~~~~
+    def _init_gui(self):
+        """ Create boxes where avatars will be displayed """
+        # 2 gtk.HBox: One for the committer and one for authors
+        # Committer
+        self.__committer_box = AvatarBox()
+        self.__committer_box.set_size_request(80, 80)
+        self.pack_end(self.__committer_box, False)
+        self.__committer_box.show()
+        # Authors
+        self.__authors_box = AvatarBox()
+        self.pack_end(self.__authors_box, False)
+        self.__authors_box.set_spacing(10)
+        self.__authors_box.show()
+    
+    def _update_avatar_from_response(self, response, email):
+        """
+        Callback method fired by avatar worker when finished.
+        
+        response is a urllib2.urlopen() return value.
+        email is used to identify item from self.__avatars.
+        """
+        if not email is "":
+            # Convert downloaded image from provider to gtk.Image
+            loader = gtk.gdk.PixbufLoader()
+            loader.write(response.read())
+            loader.close()
+            
+            for role in ["committer", "author"]:
+                self._role_box_for(role).and_avatar_email(email).update_avatar_image_from_pixbuf_loader(loader)
+    
+    def _role_box_for(self, role):
+        """ Return the gtk.HBox for the given role """
+        return self.__committer_box if role is "committer" else self.__authors_box

=== modified file 'revisionview.py'
--- revisionview.py	2009-12-09 13:27:53 +0000
+++ revisionview.py	2011-03-15 16:52:28 +0000
@@ -32,6 +32,9 @@
 
 from bzrlib.plugins.gtk import icon_path
 
+from bzrlib.plugins.gtk.avatarsbox import AvatarsBox
+from bzrlib.plugins.gtk.avatar import Avatar
+
 try:
     from bzrlib.plugins.gtk import seahorse
 except ImportError:
@@ -402,13 +405,18 @@
 
     def _set_revision(self, revision):
         if revision is None: return
-
+        
+        self.avatarsbox.reset()
+        
         self._revision = revision
         if revision.committer is not None:
             self.committer.set_text(revision.committer)
+            self.avatarsbox.add(revision.committer, "committer")
         else:
             self.committer.set_text("")
+            self.avatarsbox.hide()
         author = revision.properties.get('author', '')
+        self.avatarsbox.merge(revision.get_apparent_authors(), "author")
         if author != '':
             self.author.set_text(author)
             self.author.show()
@@ -579,10 +587,15 @@
         self.connect_after('notify::revision', self._update_signature)
 
     def _create_headers(self):
+        self.avatarsbox = AvatarsBox()
+        
         self.table = gtk.Table(rows=5, columns=2)
         self.table.set_row_spacings(6)
         self.table.set_col_spacings(6)
         self.table.show()
+        
+        self.avatarsbox.pack_start(self.table)
+        self.avatarsbox.show()
 
         row = 0
 
@@ -673,7 +686,8 @@
 
         self.connect('notify::revision', self._add_tags)
 
-        return self.table
+        self.avatarsbox.show()
+        return self.avatarsbox
     
     def _create_parents(self):
         hbox = gtk.HBox(True, 3)