[PATCH] Pagination and better(?) layout

Pagination support and layoutchanges to discussion plugin

Thanks for the patch, I just wanted to implement pagination today, so I'll investigate the patch and apply it instead.

I implemented pagination for topics with r6051 partially using your code as a guideline but I rather used LIMIT and OFFSET SQL directives. Pagination for messages will follow. I'm just not sure how to paginate messages in tree view. Do you have any idea? Pagination for flat views is straightforward.

humm, limit and offset are probably better but I only have 400 topics so it seemed to much work for too little benefit :)

As for pagination of messages I don't know, the threaded view will probably be "strange" paginated, but I have recently switched to using the flat one (as it is simpler to work) and that would I believe benefit from pagination. But again pagination for the tree view I think will look "odd" so I would probably not go there..

BTW, a few questions:

• Is there a reason for selecting the "body" of all messages?
• Why is the "last seen" implemented as a loop of SQL statements (seems like a lot of waste to me)?
• Any thoughts on the default sort I added ? (sort by last reply/last post seems to make the most sense to me, so I added it as default sort here)
• Any plans on making a bunch of configuration options for things like default sort and such?
• Any plans on cleaning up the topic list view? (I think the current one is too wide and renders poorly as the columns like date keep getting wrapped here which is why I cleaned it up here)

// Michael Medin

As for pagination of messages I don't know, the threaded view will probably be "strange" paginated, but I have recently switched to using the flat one (as it is simpler to work) and that would I believe benefit from pagination. But again pagination for the tree view I think will look "odd" so I would probably not go there..

There are two possibilities: State a weak limit and then iterate over top messages and include all their replies until the limit is reached or state a hard limit and cut threads apart but preserve indentation. First will be more readable but in common cases, where everyone is replying to previous posts, it will include whole thread anyway. Nevertheless, all messages has to be fetched from DB and processed in both cases.

BTW, a few questions: * Is there a reason for selecting the "body" of all messages?

Not in this case but I wanted get_topics() to be general method. I can add with_body = True argument for optimalization.

* Why is the "last seen" implemented as a loop of SQL statements (seems like a lot of waste to me)?

If you mean _get_new_replies_count(), this is just that I was lazy to figure out common SQL query :-) I'll fix this.

* Any thoughts on the default sort I added ? (sort by last reply/last post seems to make the most sense to me, so I added it as default sort here)

* Any plans on making a bunch of configuration options for things like default sort and such?

This should be configurable already (see DiscussionPlugin#Installation for details) but as I checked the code it doesn't seem so which is confusing me.

* Any plans on cleaning up the topic list view? (I think the current one is too wide and renders poorly as the columns like date keep getting wrapped here which is why I cleaned it up here)

Current table view is designed to be consistent with Trac's table views. Anyone who wants different view can change a template accordingly. I quite don't understand your proposed view as I applied it to the trunk (so it may not be applited correctly). Can you post a screenshot how it looks like to you?

Just a quick reply: you can see the layout here and the ideas was to make it a bit like "bb" and other forums with a more compact 2 line rendering.

// MickeM

As for the SQL query optimalization, I found where is a problem that this queries are not joined: To get new messages of the topic there has to be passed different view time for each topic from the user's session. This implies that to join this to a single query I would need to create a temporary table with (topic_id, view_time) pairs. I think that construction of this table is not worth the time saved by a single query, especially when paginated.

Wouldn't it be possible to do one select to get all the topics and on select for the "times" and then another one for the times?

if you do a (and I am just sketching from the top of my head here):

select topic_id,max(date) from messages group by topic_id 

wouldn't you get the information you need ? (and then loop the lists and update the data or some such ?)

// MickeM

If I understand your point, this would just return "lastreply" column (which is already performed in the main query). We are talking about "newreplies" column of messages count which needs query that is joint with table of "topic_id", "lastseen" pairs which are unique for each user and stored in the session (and thus has to be constructed for each request as temporary table).

ahh..

I thought last seen was a time for when I last visited the forum but then I understand...

Did you (BTW) see the forum design I used?

// MickeM

Pagination of messages, default sorting configuration options and with_body optimalization are in SVN.

Yes, I saw it. It seems quite fine but there are three minor things I dislike about it: the header - two rows and "Author" column where there are no authors below, by; Author >> 03/... notation seems little odd to me (i. e. why the semicolon?) and that topic ID's that are not prefixed with #. Although #<id> is a syntax for tickets, I'm using it troughout the plugin for discussion ID's too until I find other one.

Anyway, I would still keep such layout changes to custom templates and keep the current "boring" table view as default. Theoreticaly I can make both of them available via another trac.ini option.

Ahh, the "semicolon" i missed, should not be there, the # might be a good idea. I think a "template parameter" would be nice at least that would allow "others" (read me) to use a "custom" skin.

I don't know if trac has some form of template/skinning system one could use?

As for the "second header row" was just to facilitate "sorting" so ideas there would be nice, my initial idea was to make the sorting columns like so "Subject (ID)" | ... but again that was to much work :)

Look upon my "deisng" more as an idea then a finnished ting, my point was a more compact syntax is (to me) easier to read... :)

// MickeM

I think there could be header columns: ID, Subject, Author, Replies, Last Reply, New Replies and then collspan: 3, 1, 1, 1 for data columns.

I think there could be a similar line with "views" for topics as it is for messages and appropriate trac.ini configuration option. User then could choose between "Classic View" and "Compact View" and this choice would be stored in his session.

Trac interface can be modified using Gensi templating (see http://trac.edgewall.org/wiki/0.11/TracInterfaceCustomization#SiteAppearance). I can add include of a template, let's say discussion_site.html but such massive changes won't be easy to write.

All right, changeset r6060 contains two ways of displaying topics. If the "compact" view suits your needs, please, close this ticket.

Ok, I'm closing it myself.