Merge lp:~huwshimi/launchpad/avatars-everywhere-712894 into lp:launchpad
- avatars-everywhere-712894
- Merge into devel
Status: | Work in progress |
---|---|
Proposed branch: | lp:~huwshimi/launchpad/avatars-everywhere-712894 |
Merge into: | lp:launchpad |
Diff against target: |
202 lines (+73/-12) 6 files modified
lib/canonical/launchpad/icing/style.css (+9/-0) lib/lp/app/browser/tales.py (+34/-4) lib/lp/code/browser/branchlisting.py (+8/-3) lib/lp/registry/model/person.py (+11/-3) lib/lp/registry/model/product.py (+7/-2) lib/lp/services/features/flags.py (+4/-0) |
To merge this branch: | bzr merge lp:~huwshimi/launchpad/avatars-everywhere-712894 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Robert Collins (community) | Needs Fixing | ||
Review via email: mp+81430@code.launchpad.net |
Commit message
Usernames now have the user's avatar next to them instead of the generic user icon.
Description of the change
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:/
To test the following feature rule needs to be set:
tales.avatars.
Huw Wilkins (huwshimi) wrote : | # |
Here's a screenshot of the branch: https:/
Huw Wilkins (huwshimi) 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.
Robert Collins (lifeless) wrote : | # |
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) wrote : | # |
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.
Robert Collins (lifeless) wrote : | # |
This is pretty close to what I meant, yes.
119 columns.
120 decorators.
121 - if need_icon:
122 - IconAlias = ClassAlias(
123 - origin.
124 - columns.
125 + if need_logo:
126 + LogoAlias = ClassAlias(
127 + origin.
128 + columns.
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) wrote : | # |
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) wrote : | # |
I also fixed the XSS hole wgrant mentioned.
Curtis Hovey (sinzui) wrote : | # |
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:/
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:/
Huw Wilkins (huwshimi) wrote : | # |
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.
Preview Diff
1 | === modified file 'lib/canonical/launchpad/icing/style.css' | |||
2 | --- lib/canonical/launchpad/icing/style.css 2011-11-02 21:19:13 +0000 | |||
3 | +++ lib/canonical/launchpad/icing/style.css 2011-11-08 02:25:27 +0000 | |||
4 | @@ -203,6 +203,15 @@ | |||
5 | 203 | padding-bottom: 0.5em; | 203 | padding-bottom: 0.5em; |
6 | 204 | padding-left: 18px; | 204 | padding-left: 18px; |
7 | 205 | } | 205 | } |
8 | 206 | a.bg-image.has-avatar { | ||
9 | 207 | padding-left: 0; | ||
10 | 208 | } | ||
11 | 209 | .avatar { | ||
12 | 210 | width: 18px; | ||
13 | 211 | height: 18px; | ||
14 | 212 | margin-right: 5px; | ||
15 | 213 | vertical-align: top; | ||
16 | 214 | } | ||
17 | 206 | 215 | ||
18 | 207 | /* == Application-specific styles == */ | 216 | /* == Application-specific styles == */ |
19 | 208 | 217 | ||
20 | 209 | 218 | ||
21 | === modified file 'lib/lp/app/browser/tales.py' | |||
22 | --- lib/lp/app/browser/tales.py 2011-10-28 15:23:20 +0000 | |||
23 | +++ lib/lp/app/browser/tales.py 2011-11-08 02:25:27 +0000 | |||
24 | @@ -70,6 +70,7 @@ | |||
25 | 70 | from canonical.launchpad.webapp.menu import ( | 70 | from canonical.launchpad.webapp.menu import ( |
26 | 71 | get_current_view, | 71 | get_current_view, |
27 | 72 | get_facet, | 72 | get_facet, |
28 | 73 | structured, | ||
29 | 73 | ) | 74 | ) |
30 | 74 | from canonical.launchpad.webapp.publisher import ( | 75 | from canonical.launchpad.webapp.publisher import ( |
31 | 75 | get_current_browser_request, | 76 | get_current_browser_request, |
32 | @@ -831,6 +832,25 @@ | |||
33 | 831 | #XXX: this should go away as soon as all image:icon where replaced | 832 | #XXX: this should go away as soon as all image:icon where replaced |
34 | 832 | return None | 833 | return None |
35 | 833 | 834 | ||
36 | 835 | def custom_logo_url(self): | ||
37 | 836 | """Return the URL for this object's logo.""" | ||
38 | 837 | context = self._context | ||
39 | 838 | if not IHasLogo.providedBy(context): | ||
40 | 839 | context = nearest(context, IHasLogo) | ||
41 | 840 | if context is None: | ||
42 | 841 | # we use the Launchpad logo for anything which is in no way | ||
43 | 842 | # related to a Pillar (for example, a buildfarm) | ||
44 | 843 | url = '/@@/launchpad-logo' | ||
45 | 844 | elif context.logo is not None: | ||
46 | 845 | url = context.logo.getURL() | ||
47 | 846 | else: | ||
48 | 847 | url = self.default_logo_resource(context) | ||
49 | 848 | if url is None: | ||
50 | 849 | # We want to indicate that there is no logo for this | ||
51 | 850 | # object. | ||
52 | 851 | return None | ||
53 | 852 | return url | ||
54 | 853 | |||
55 | 834 | def logo(self): | 854 | def logo(self): |
56 | 835 | """Return the appropriate <img> tag for this object's logo. | 855 | """Return the appropriate <img> tag for this object's logo. |
57 | 836 | 856 | ||
58 | @@ -1201,15 +1221,25 @@ | |||
59 | 1201 | def _makeLink(self, view_name, rootsite, text): | 1221 | def _makeLink(self, view_name, rootsite, text): |
60 | 1202 | person = self._context | 1222 | person = self._context |
61 | 1203 | url = self.url(view_name, rootsite) | 1223 | url = self.url(view_name, rootsite) |
63 | 1204 | custom_icon = ObjectImageDisplayAPI(person).custom_icon_url() | 1224 | if getFeatureFlag('tales.avatars.enabled'): |
64 | 1225 | custom_icon = ObjectImageDisplayAPI(person).custom_logo_url() | ||
65 | 1226 | else: | ||
66 | 1227 | custom_icon = ObjectImageDisplayAPI(person).custom_icon_url() | ||
67 | 1205 | if custom_icon is None: | 1228 | if custom_icon is None: |
68 | 1206 | css_class = ObjectImageDisplayAPI(person).sprite_css() | 1229 | css_class = ObjectImageDisplayAPI(person).sprite_css() |
69 | 1207 | return (u'<a href="%s" class="%s">%s</a>') % ( | 1230 | return (u'<a href="%s" class="%s">%s</a>') % ( |
70 | 1208 | url, css_class, cgi.escape(text)) | 1231 | url, css_class, cgi.escape(text)) |
71 | 1209 | else: | 1232 | else: |
75 | 1210 | return (u'<a href="%s" class="bg-image" ' | 1233 | if getFeatureFlag('tales.avatars.enabled'): |
76 | 1211 | 'style="background-image: url(%s)">%s</a>') % ( | 1234 | return structured(u'<a href="%(url)s"' |
77 | 1212 | url, custom_icon, cgi.escape(text)) | 1235 | 'class="bg-image has-avatar">' |
78 | 1236 | '<img src="%(icon_url)s" alt="Avatar for %(link_text)s"' | ||
79 | 1237 | 'class="avatar" />%(link_text)s</a>', | ||
80 | 1238 | url=url, icon_url=custom_icon, link_text=text).escapedtext | ||
81 | 1239 | else: | ||
82 | 1240 | return (u'<a href="%s" class="bg-image" ' | ||
83 | 1241 | 'style="background-image: url(%s)">%s</a>') % ( | ||
84 | 1242 | url, custom_icon, cgi.escape(text)) | ||
85 | 1213 | 1243 | ||
86 | 1214 | def link(self, view_name, rootsite='mainsite'): | 1244 | def link(self, view_name, rootsite='mainsite'): |
87 | 1215 | """See `ObjectFormatterAPI`. | 1245 | """See `ObjectFormatterAPI`. |
88 | 1216 | 1246 | ||
89 | === modified file 'lib/lp/code/browser/branchlisting.py' | |||
90 | --- lib/lp/code/browser/branchlisting.py 2011-11-04 15:23:35 +0000 | |||
91 | +++ lib/lp/code/browser/branchlisting.py 2011-11-08 02:25:27 +0000 | |||
92 | @@ -454,9 +454,14 @@ | |||
93 | 454 | 454 | ||
94 | 455 | # Cache display information for authors of branches' respective | 455 | # Cache display information for authors of branches' respective |
95 | 456 | # last revisions. | 456 | # last revisions. |
99 | 457 | getUtility(IPersonSet).getPrecachedPersonsFromIDs( | 457 | if getFeatureFlag('tales.avatars.enabled'): |
100 | 458 | [revision.revision_author.personID for revision in revisions], | 458 | getUtility(IPersonSet).getPrecachedPersonsFromIDs( |
101 | 459 | need_icon=True) | 459 | [revision.revision_author.personID for revision in revisions], |
102 | 460 | need_logo=True) | ||
103 | 461 | else: | ||
104 | 462 | getUtility(IPersonSet).getPrecachedPersonsFromIDs( | ||
105 | 463 | [revision.revision_author.personID for revision in revisions], | ||
106 | 464 | need_icon=True) | ||
107 | 460 | 465 | ||
108 | 461 | # Return a dict keyed on branch id. | 466 | # Return a dict keyed on branch id. |
109 | 462 | return dict( | 467 | return dict( |
110 | 463 | 468 | ||
111 | === modified file 'lib/lp/registry/model/person.py' | |||
112 | --- lib/lp/registry/model/person.py 2011-10-27 03:02:10 +0000 | |||
113 | +++ lib/lp/registry/model/person.py 2011-11-08 02:25:27 +0000 | |||
114 | @@ -4276,7 +4276,8 @@ | |||
115 | 4276 | def getPrecachedPersonsFromIDs( | 4276 | def getPrecachedPersonsFromIDs( |
116 | 4277 | self, person_ids, need_karma=False, need_ubuntu_coc=False, | 4277 | self, person_ids, need_karma=False, need_ubuntu_coc=False, |
117 | 4278 | need_location=False, need_archive=False, | 4278 | need_location=False, need_archive=False, |
119 | 4279 | need_preferred_email=False, need_validity=False, need_icon=False): | 4279 | need_preferred_email=False, need_validity=False, |
120 | 4280 | need_icon=False, need_logo=False): | ||
121 | 4280 | """See `IPersonSet`.""" | 4281 | """See `IPersonSet`.""" |
122 | 4281 | origin = [Person] | 4282 | origin = [Person] |
123 | 4282 | conditions = [ | 4283 | conditions = [ |
124 | @@ -4286,13 +4287,14 @@ | |||
125 | 4286 | need_karma=need_karma, need_ubuntu_coc=need_ubuntu_coc, | 4287 | need_karma=need_karma, need_ubuntu_coc=need_ubuntu_coc, |
126 | 4287 | need_location=need_location, need_archive=need_archive, | 4288 | need_location=need_location, need_archive=need_archive, |
127 | 4288 | need_preferred_email=need_preferred_email, | 4289 | need_preferred_email=need_preferred_email, |
129 | 4289 | need_validity=need_validity, need_icon=need_icon) | 4290 | need_validity=need_validity, |
130 | 4291 | need_icon=need_icon, need_logo=need_logo) | ||
131 | 4290 | 4292 | ||
132 | 4291 | def _getPrecachedPersons( | 4293 | def _getPrecachedPersons( |
133 | 4292 | self, origin, conditions, store=None, | 4294 | self, origin, conditions, store=None, |
134 | 4293 | need_karma=False, need_ubuntu_coc=False, | 4295 | need_karma=False, need_ubuntu_coc=False, |
135 | 4294 | need_location=False, need_archive=False, need_preferred_email=False, | 4296 | need_location=False, need_archive=False, need_preferred_email=False, |
137 | 4295 | need_validity=False, need_icon=False): | 4297 | need_validity=False, need_icon=False, need_logo=False): |
138 | 4296 | """Lookup all members of the team with optional precaching. | 4298 | """Lookup all members of the team with optional precaching. |
139 | 4297 | 4299 | ||
140 | 4298 | :param store: Provide ability to specify the store. | 4300 | :param store: Provide ability to specify the store. |
141 | @@ -4309,6 +4311,8 @@ | |||
142 | 4309 | :param need_validity: The is_valid attribute will be cached. | 4311 | :param need_validity: The is_valid attribute will be cached. |
143 | 4310 | :param need_icon: Cache the persons' icons so that their URLs can | 4312 | :param need_icon: Cache the persons' icons so that their URLs can |
144 | 4311 | be generated without further reference to the database. | 4313 | be generated without further reference to the database. |
145 | 4314 | :param need_logo: Cache the persons' logos so that their URLs can | ||
146 | 4315 | be generated without further reference to the database. | ||
147 | 4312 | """ | 4316 | """ |
148 | 4313 | if store is None: | 4317 | if store is None: |
149 | 4314 | store = IStore(Person) | 4318 | store = IStore(Person) |
150 | @@ -4367,6 +4371,10 @@ | |||
151 | 4367 | IconAlias = ClassAlias(LibraryFileAlias, "LibraryFileAlias") | 4371 | IconAlias = ClassAlias(LibraryFileAlias, "LibraryFileAlias") |
152 | 4368 | origin.append(LeftJoin(IconAlias, Person.icon == IconAlias.id)) | 4372 | origin.append(LeftJoin(IconAlias, Person.icon == IconAlias.id)) |
153 | 4369 | columns.append(IconAlias) | 4373 | columns.append(IconAlias) |
154 | 4374 | if need_logo: | ||
155 | 4375 | LogoAlias = ClassAlias(LibraryFileAlias, "LibraryFileAlias") | ||
156 | 4376 | origin.append(LeftJoin(LogoAlias, Person.logo == LogoAlias.id)) | ||
157 | 4377 | columns.append(LogoAlias) | ||
158 | 4370 | if len(columns) == 1: | 4378 | if len(columns) == 1: |
159 | 4371 | column = columns[0] | 4379 | column = columns[0] |
160 | 4372 | # Return a simple ResultSet | 4380 | # Return a simple ResultSet |
161 | 4373 | 4381 | ||
162 | === modified file 'lib/lp/registry/model/product.py' | |||
163 | --- lib/lp/registry/model/product.py 2011-10-29 18:23:06 +0000 | |||
164 | +++ lib/lp/registry/model/product.py 2011-11-08 02:25:27 +0000 | |||
165 | @@ -168,6 +168,7 @@ | |||
166 | 168 | from lp.registry.model.productseries import ProductSeries | 168 | from lp.registry.model.productseries import ProductSeries |
167 | 169 | from lp.registry.model.series import ACTIVE_STATUSES | 169 | from lp.registry.model.series import ACTIVE_STATUSES |
168 | 170 | from lp.registry.model.sourcepackagename import SourcePackageName | 170 | from lp.registry.model.sourcepackagename import SourcePackageName |
169 | 171 | from lp.services.features import getFeatureFlag | ||
170 | 171 | from lp.services.database import bulk | 172 | from lp.services.database import bulk |
171 | 172 | from lp.services.propertycache import ( | 173 | from lp.services.propertycache import ( |
172 | 173 | cachedproperty, | 174 | cachedproperty, |
173 | @@ -1378,8 +1379,12 @@ | |||
174 | 1378 | def do_eager_load(rows): | 1379 | def do_eager_load(rows): |
175 | 1379 | owner_ids = set(map(operator.attrgetter('_ownerID'), rows)) | 1380 | owner_ids = set(map(operator.attrgetter('_ownerID'), rows)) |
176 | 1380 | # +detailed-listing renders the person with team branding. | 1381 | # +detailed-listing renders the person with team branding. |
179 | 1381 | list(getUtility(IPersonSet).getPrecachedPersonsFromIDs( | 1382 | if getFeatureFlag('tales.avatars.enabled'): |
180 | 1382 | owner_ids, need_validity=True, need_icon=True)) | 1383 | list(getUtility(IPersonSet).getPrecachedPersonsFromIDs( |
181 | 1384 | owner_ids, need_validity=True, need_logo=True)) | ||
182 | 1385 | else: | ||
183 | 1386 | list(getUtility(IPersonSet).getPrecachedPersonsFromIDs( | ||
184 | 1387 | owner_ids, need_validity=True, need_icon=True)) | ||
185 | 1383 | 1388 | ||
186 | 1384 | return DecoratedResultSet(result, pre_iter_hook=do_eager_load) | 1389 | return DecoratedResultSet(result, pre_iter_hook=do_eager_load) |
187 | 1385 | 1390 | ||
188 | 1386 | 1391 | ||
189 | === modified file 'lib/lp/services/features/flags.py' | |||
190 | --- lib/lp/services/features/flags.py 2011-11-03 13:58:07 +0000 | |||
191 | +++ lib/lp/services/features/flags.py 2011-11-08 02:25:27 +0000 | |||
192 | @@ -225,6 +225,10 @@ | |||
193 | 225 | '', | 225 | '', |
194 | 226 | '', | 226 | '', |
195 | 227 | ''), | 227 | ''), |
196 | 228 | ('tales.avatars.enabled', | ||
197 | 229 | 'boolean', | ||
198 | 230 | 'Replaces generic user icons with avatars next to usernames.', | ||
199 | 231 | ''), | ||
200 | 228 | ]) | 232 | ]) |
201 | 229 | 233 | ||
202 | 230 | # The set of all flag names that are documented. | 234 | # The set of all flag names that are documented. |
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).
getPrecachedPer sonsFromIDs 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.