Theming overhaul
| Project: | Activity Stream |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | won't fix |
Jump to:
I was working on theming a site that's using activitystream.module and noticed that the theming for the module needed a little work. Mostly, I wanted to get as much logic out of the theme layer and into the actual processing as possible. I also wanted to be able to get rid of as much of the extra markup output by the module as possible. So here's what I did.
First, I removed all the logic from theme_activitystrea_item, and all the equivalent functions in the sub-modules. I also noticed that the module was calling node_load() for every single item in your stream, despite the fact that the only information it ever used about the node was it's title, and uid. So I removed the call to node_load, and adjusted the query that loads your stream to include the user id.
I also modified it so that theme('activisytream_name') is only called once for each USERNAME. I felt it was overkill to call the function for every time the username was being printed since it's exactly the same each time.
Before making this change an activitystream page which was displaying 30 items required 207 queries. The same page, after these changes were applied only makes 40 queries. A pretty significant performance improvement.
I figure, if a theme developer really needs more information from the $node object they can load it in their theme function. There's no reason to do so in the module if we're never using it.
Second, the html output seemed a little bloated.
<div class="activitystream">
<ul>
<li><span class="activitystream-item">ICON <span>USERNAME <a>LINK</a> <span class="activitystream-created">DATE</span></span> <a class="permalink">PERMALINK</a></span></li>
</ul>
</div>I removed as many spans as possible, and got rid of the "activitystream-item" class and replaced them in favor of a per module class. So that you can theme each item depending on which module it comes from. The new output looks like this.
<div class="activitystream">
<ul class="activitystream-items">
<li class="acvititystream-MODULE">ICON USERNAME <a>LINK</a> <span class="acvitiystream-created">DATE</span> <a class="permalink">PERMALINK</a></li>
</ul>
</div>One notable difference is the activitystream_feed.module which will add a class of acivitystream-X to each item. Where X is the domain portion of the feed url with the www. replaced, and all other "." replaced with an "_". For example. acivitystream-facebook_com
I think using this method provides a number of benefits. It cuts back on the amount of HTML that it output, yet achieves the same effect. It allows theming of activitystream items on a per item type basis without having to change the theme functions at all. It makes far fewer calls to the database which is never a bad thing. And finally it removes as much logic as possible from the theme_ functions so that theme developers don't have to worry about it when overriding these functions.
The attached patch also addresses issues from these threads.
#292692: Date headers no longer displayed
Let me know if you have any questions or comments. I think these are really important improvements to the module and am willing to make changes as necessary in order to get this committed.
Patch applies against 6.x-1.x-dev
| Attachment | Size |
|---|---|
| activitystream-theme_refactor.patch | 16.05 KB |
#1
eojthebrave, thanks so much for working to improve the module. Much appreciated.
The goal behind how much of Activity Stream is written is to allow people to easily modify and extend it without hacking the module itself. That's why the different stream sources are plugins and why there's so many theme functions. So any improvements should keep these goals in mind.
The purpose behind the extra HTML in there is to ensure that every individual element is addressable by CSS and the DOM. Without those extra spans, that's not possible. And your change doesn't actually reduce the amount of HTML -- it's 3 bytes longer.
The reduction of queries is a good thing -- I'll review the patch.
And anything that remove business logic from the theming is in theory a good thing, too. It's important that while we do that that we still allow the end user lots of hooks to change things and still allow the module developer lots of opportunities to hook into the module. At first glance, your theming improvements look good, but I wanted to let you know what frame of mind I'm in when I review the patches.
#2
I'd like to avoid changing the theme api. Activity Stream is already out there in the wild, and a change to the theme api will break people's sites.
Your method of only running user_load once doesn't work if you have multiple users per site. You load up $name if it's unset and then just loop over all the items in the stream. For a multi-user site, that will mean that /stream shows all items as being from the same user.
The idea of eliminating a repetitive user_load is an important one, however. So what I've done is added a new function activitystream_user_load() that wraps user_load and caches the output for each uid. That way, we only call user_load if it hasn't already been called for that uid.
#3
Not wanting to change the theme API makes sense to me. How about if we just add the $node->title attribute to the $action argument passed to each theme_ function, that way developers can still have access to the $node->title without having to load the whole node object for each $action.
Good thinking on the activitystream_user_load(). I realized I only have one user, and one activity stream on my current dev site so I never would have noticed that problem.
With regards to the HTML, I do still think my method is cleaner, and would still allow each item to be addressed by either CSS or the DOM individually as long as whatever is returned from the theme_ativitystream_name function contains something to uniquely identify the name element.
Also, somewhat un-related, but I noticed while I was playing around with this the other night that the nid column in the {activitystream} table doesn't have an INDEX on it. And it probably should since we're using it to JOIN the {node} table pretty frequently. I added one, and it speeds up the pager, and count queries pretty dramatically.
#4
We'd still be missing the $node->created. I suppose we could add that to $action too, but at some point, we might as well just load the whole node.
#5
There's no good reason to *not* add a key on nid, but I'm not sure it will have a performance impact.
explain SELECT n.title, n.nid, s.module, s.link, s.data, n.created FROM node n INNER JOIN activitystream s on n.nid=s.nid WHERE n.status = 1 ORDER BY n.created DESC\G*************************** 1. row ***************************
id: 1
select_type: SIMPLE
table: s
type: ALL
possible_keys: nid
key: NULL
key_len: NULL
ref: NULL
rows: 92
Extra: Using temporary; Using filesort
*************************** 2. row ***************************
id: 1
select_type: SIMPLE
table: n
type: ref
possible_keys: PRIMARY,status,node_status_type,nid
key: PRIMARY
key_len: 4
ref: d5test.s.nid
rows: 1
Extra: Using where
Notice that the query is doing a full table scan on activitystream even with the key on nid. Now it's entirely possible that I'm seeing something goofy because I don't have much data in the table. MySQL tends to optimize differently on large tables.
I've added the key to the upgrade routine on 5.x, but I'm not sure it will make a difference.
#6
It didn't make a huge difference in my limited testing either, but figured it was worth it. I basically turned on the query log in devel.module, and watched it spit out that pager query as "taking a long time" a couple times in a row. Then added an index and the notice went away. So it at least helped a little bit. Reduced average query time from about 9ms to about 2ms in my tests.
I still think that trying to get away from calling node_load() for every item is a good idea. It's a lot of extra overhead for something that isn't being used at all. Calling node_load() can get really expensive, especially when people start enabling more and more modules that hook the node_load() operation to their site. It can easily get up to 8-10 extra queries just for calling node_load().
Totally understand not wanting to make the changes in the 5.x version since it's already out in the wild. But the 6.x version doesn't even have a stable release yet. I don't think it's to un-fair to ask people using a -dev version of a module to make changes. It is to be expected.
#7