Merge lp:~huwshimi/launchpad/avatars-everywhere-712894 into lp:launchpad

[Huw Wilkins]

Usernames now have the user's avatar next to them instead of the generic user icon.

This branch swaps out the icon checking code for avatar checking code. The tail then returns the username with the avatar instead of the generic user icon.

Screenshot of the change: https://launchpadlibrarian.net/84606031/avatars_screenshot.png

To test the following feature rule needs to be set:

tales.avatars.enabled default 1 on

[Robert Collins (lifeless)]

I haven't triple-checked this, but there is a decent chance this will cause widespread timeouts due to lazy evaluation of the logo (vs the icon).

getPrecachedPersonsFromIDs is the primary eager loading function to update, changing from need_icon to need_logo, and so on down the call chain from there.

(Reason being, that loading even the icons for 50 people can take a significant chunk of time, and some pages like blueprint reports load the icons for hundreds of people).

I'm going to mark this needs fixing, because I fully expect this problem to crop up. However, if you want to land it under a feature flag, so that widespread testing can be done, that would be fine with me.

I haven't done a detailed review, sorry.

review: Needs Fixing

[Huw Wilkins (huwshimi)]

Here's a screenshot of the branch: https://launchpadlibrarian.net/84606031/avatars_screenshot.png

[Huw Wilkins (huwshimi)]

Thanks Rob, I'll add the feature flag. I was kind of hoping I would get away with the change as custom_logo_url and custom_icon_url are very similar, but I don't understand the underlying code. I'll push some changes.

[Robert Collins (lifeless)]

On Mon, Nov 7, 2011 at 6:42 PM, Huw Wilkins <email address hidden> wrote:
> Thanks Rob, I'll add the feature flag. I was kind of hoping I would get away with the change as custom_logo_url and custom_icon_url are very similar, but I don't understand the underlying code. I'll push some changes.

So here is what happens:
Our Person object has a link to a LibraryFileAlias for each of the icon sizes.
The link has to be traversed to render a URL to the librarian for the
icon/logo/branding.
So currently, where we have had performance issues and setup eager
loading, we traverse the *icon* link for all the Person objects at
once, which means one pretty efficient DB query. The results for this
then sit in the storm cache until the template renders them.

Your change makes the template rendering look at 'logo' not 'icon',
and that won't be in the storm cache, so a page with (say) 100
subscribers, will suddenly do 100 extra DB queries.

-Rob

[Huw Wilkins (huwshimi)]

Thanks a lot for the explanation Rob. I've added the feature flag.

To test you would need to set the feature rule:

tales.avatars.enabled default 1 on

[Robert Collins (lifeless)]

This is pretty close to what I meant, yes.

119 columns.extend(valid_stuff["tables"])
120 decorators.extend(valid_stuff["decorators"])
121 - if need_icon:
122 - IconAlias = ClassAlias(LibraryFileAlias, "LibraryFileAlias")
123 - origin.append(LeftJoin(IconAlias, Person.icon == IconAlias.id))
124 - columns.append(IconAlias)
125 + if need_logo:
126 + LogoAlias = ClassAlias(LibraryFileAlias, "LibraryFileAlias")
127 + origin.append(LeftJoin(LogoAlias, Person.logo == LogoAlias.id))
128 + columns.append(LogoAlias)
129 if len(columns) == 1:
130 column = columns[0]
131 # Return a simple ResultSet
132

This unconditionally switches everything over to using logo rather than icon - it needs to be feature guarded as well.

I suggest the following approach:
 - add need_logo as a separate parameter
 - leave need_icon in place
 - grep for places passing need_icon in and feature flag them to pass need_icon or need_logo appropriately (or to not change if they are not affected by your patch).

This gives us feature flag safety in this code layer.

Or, you can change need_icon to need_person_graphic and use feature flags to decide which thing to add to the query - but I think this would be a little uglier and is less flexible, if we want different things in different places.

[Huw Wilkins (huwshimi)]

Thanks for your patience Robert. I've restored the need_icon parameter and changed calls with need_icon to need_logo under the feature flag.

[Huw Wilkins (huwshimi)]

I also fixed the XSS hole wgrant mentioned.

[Curtis Hovey (sinzui)]

Hi huw.

I think there is a disconnect between your change and what a user can do. We rejected scaling images because early tests showed that they scaled badly. So I expect users will lots of ugly logos every where. When the user visits <https://launchpad.net/~/+branding>, the page does not explain that the logo will be used as an icon to make links to the person. Actually, this is not entirely true. When Lp had maps, we had icons for users to show in the map. So many users will be confused if they see the size, and know they had set something different for that size.

Team's do have icons set, your branch ignores them, so the instructions on a team +branding page are not just wrong, they are contradictory.

See https://bugs.launchpad.net/launchpad/+bugs?field.tag=branding for a short list of bugs about what is wrong with branding, including the fact that I have have a link called "branding" on my profile page--I assume I have an agent if I have a brand.

[Huw Wilkins (huwshimi)]

Hi Curtis,

I realise the problem of the size issues. Really there is an abnormal distiction between sizes in Launchpad compared with how everyone else handles avatars. I guess this is because we don't have any image resizing infrastructure. Once we do have the infrastructure I hope to replace the different size uploads with one image. It'll also mean we can resize these images properly for the small avatars instead of doing browser resizing like this branch has to do.

For this branch I've ignored all of that because we have to start somewhere.

=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css	2011-11-02 21:19:13 +0000
+++ lib/canonical/launchpad/icing/style.css	2011-11-08 02:25:27 +0000
@@ -203,6 +203,15 @@
     padding-bottom: 0.5em;
     padding-left: 18px;
 }
+a.bg-image.has-avatar {
+    padding-left: 0;
+    }
+.avatar {
+    width: 18px;
+    height: 18px;
+    margin-right: 5px;
+    vertical-align: top;
+    }
 
 /* == Application-specific styles == */
 

=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2011-10-28 15:23:20 +0000
+++ lib/lp/app/browser/tales.py	2011-11-08 02:25:27 +0000
@@ -70,6 +70,7 @@
 from canonical.launchpad.webapp.menu import (
     get_current_view,
     get_facet,
+    structured,
     )
 from canonical.launchpad.webapp.publisher import (
     get_current_browser_request,
@@ -831,6 +832,25 @@
         #XXX: this should go away as soon as all image:icon where replaced
         return None
 
+    def custom_logo_url(self):
+        """Return the URL for this object's logo."""
+        context = self._context
+        if not IHasLogo.providedBy(context):
+            context = nearest(context, IHasLogo)
+        if context is None:
+            # we use the Launchpad logo for anything which is in no way
+            # related to a Pillar (for example, a buildfarm)
+            url = '/@@/launchpad-logo'
+        elif context.logo is not None:
+            url = context.logo.getURL()
+        else:
+            url = self.default_logo_resource(context)
+            if url is None:
+                # We want to indicate that there is no logo for this
+                # object.
+                return None
+        return url
+
     def logo(self):
         """Return the appropriate <img> tag for this object's logo.
 
@@ -1201,15 +1221,25 @@
     def _makeLink(self, view_name, rootsite, text):
         person = self._context
         url = self.url(view_name, rootsite)
-        custom_icon = ObjectImageDisplayAPI(person).custom_icon_url()
+        if getFeatureFlag('tales.avatars.enabled'):
+            custom_icon = ObjectImageDisplayAPI(person).custom_logo_url()
+        else:
+            custom_icon = ObjectImageDisplayAPI(person).custom_icon_url()
         if custom_icon is None:
             css_class = ObjectImageDisplayAPI(person).sprite_css()
             return (u'<a href="%s" class="%s">%s</a>') % (
                 url, css_class, cgi.escape(text))
         else:
-            return (u'<a href="%s" class="bg-image" '
-                     'style="background-image: url(%s)">%s</a>') % (
-                url, custom_icon, cgi.escape(text))
+            if getFeatureFlag('tales.avatars.enabled'):
+                return structured(u'<a href="%(url)s"'
+                    'class="bg-image has-avatar">'
+                    '<img src="%(icon_url)s" alt="Avatar for %(link_text)s"'
+                    'class="avatar" />%(link_text)s</a>',
+                    url=url, icon_url=custom_icon, link_text=text).escapedtext
+            else:
+                return (u'<a href="%s" class="bg-image" '
+                         'style="background-image: url(%s)">%s</a>') % (
+                    url, custom_icon, cgi.escape(text))
 
     def link(self, view_name, rootsite='mainsite'):
         """See `ObjectFormatterAPI`.

=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py	2011-11-04 15:23:35 +0000
+++ lib/lp/code/browser/branchlisting.py	2011-11-08 02:25:27 +0000
@@ -454,9 +454,14 @@
 
         # Cache display information for authors of branches' respective
         # last revisions.
-        getUtility(IPersonSet).getPrecachedPersonsFromIDs(
-            [revision.revision_author.personID for revision in revisions],
-            need_icon=True)
+        if getFeatureFlag('tales.avatars.enabled'):
+            getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+                [revision.revision_author.personID for revision in revisions],
+                need_logo=True)
+        else:
+            getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+                [revision.revision_author.personID for revision in revisions],
+                need_icon=True)
 
         # Return a dict keyed on branch id.
         return dict(

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-10-27 03:02:10 +0000
+++ lib/lp/registry/model/person.py	2011-11-08 02:25:27 +0000
@@ -4276,7 +4276,8 @@
     def getPrecachedPersonsFromIDs(
         self, person_ids, need_karma=False, need_ubuntu_coc=False,
         need_location=False, need_archive=False,
-        need_preferred_email=False, need_validity=False, need_icon=False):
+        need_preferred_email=False, need_validity=False, 
+        need_icon=False, need_logo=False):
         """See `IPersonSet`."""
         origin = [Person]
         conditions = [
@@ -4286,13 +4287,14 @@
             need_karma=need_karma, need_ubuntu_coc=need_ubuntu_coc,
             need_location=need_location, need_archive=need_archive,
             need_preferred_email=need_preferred_email,
-            need_validity=need_validity, need_icon=need_icon)
+            need_validity=need_validity,
+            need_icon=need_icon, need_logo=need_logo)
 
     def _getPrecachedPersons(
         self, origin, conditions, store=None,
         need_karma=False, need_ubuntu_coc=False,
         need_location=False, need_archive=False, need_preferred_email=False,
-        need_validity=False, need_icon=False):
+        need_validity=False, need_icon=False, need_logo=False):
         """Lookup all members of the team with optional precaching.
 
         :param store: Provide ability to specify the store.
@@ -4309,6 +4311,8 @@
         :param need_validity: The is_valid attribute will be cached.
         :param need_icon: Cache the persons' icons so that their URLs can
             be generated without further reference to the database.
+        :param need_logo: Cache the persons' logos so that their URLs can
+            be generated without further reference to the database.
         """
         if store is None:
             store = IStore(Person)
@@ -4367,6 +4371,10 @@
             IconAlias = ClassAlias(LibraryFileAlias, "LibraryFileAlias")
             origin.append(LeftJoin(IconAlias, Person.icon == IconAlias.id))
             columns.append(IconAlias)
+        if need_logo:
+            LogoAlias = ClassAlias(LibraryFileAlias, "LibraryFileAlias")
+            origin.append(LeftJoin(LogoAlias, Person.logo == LogoAlias.id))
+            columns.append(LogoAlias)
         if len(columns) == 1:
             column = columns[0]
             # Return a simple ResultSet

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2011-10-29 18:23:06 +0000
+++ lib/lp/registry/model/product.py	2011-11-08 02:25:27 +0000
@@ -168,6 +168,7 @@
 from lp.registry.model.productseries import ProductSeries
 from lp.registry.model.series import ACTIVE_STATUSES
 from lp.registry.model.sourcepackagename import SourcePackageName
+from lp.services.features import getFeatureFlag
 from lp.services.database import bulk
 from lp.services.propertycache import (
     cachedproperty,
@@ -1378,8 +1379,12 @@
         def do_eager_load(rows):
             owner_ids = set(map(operator.attrgetter('_ownerID'), rows))
             # +detailed-listing renders the person with team branding.
-            list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
-                owner_ids, need_validity=True, need_icon=True))
+            if getFeatureFlag('tales.avatars.enabled'):
+                list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+                    owner_ids, need_validity=True, need_logo=True))
+            else:
+                list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+                    owner_ids, need_validity=True, need_icon=True))
 
         return DecoratedResultSet(result, pre_iter_hook=do_eager_load)
 

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2011-11-03 13:58:07 +0000
+++ lib/lp/services/features/flags.py	2011-11-08 02:25:27 +0000
@@ -225,6 +225,10 @@
      '',
      '',
      ''),
+     ('tales.avatars.enabled',
+      'boolean',
+      'Replaces generic user icons with avatars next to usernames.',
+      ''),
     ])
 
 # The set of all flag names that are documented.