Modify

Opened 5 years ago

Closed 5 years ago

#5378 closed enhancement (fixed)

[PATCH] Pagination and better(?) layout

Reported by: mickem Owned by: Blackhex
Priority: normal Component: DiscussionPlugin
Severity: normal Keywords: pagination layout
Cc: Trac Release: 0.11

Description

Since I wanted pagination (a pretty good idea given how this plugin abuses the database) I added it.

This patch in addition to adding pagination also changes the layout (since I though the "normal" layout is pretty unpractical. This looks more like "bb" then the old one.

Michael Medin

Attachments (1)

tracdiscussion.diff (11.3 KB) - added by mickem 5 years ago.
Pagination support and layoutchanges to discussion plugin

Download all attachments as: .zip

Change History (15)

Changed 5 years ago by mickem

Pagination support and layoutchanges to discussion plugin

comment:1 Changed 5 years ago by Blackhex

  • Status changed from new to assigned

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

comment:2 Changed 5 years ago by Blackhex

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.

comment:3 follow-up: Changed 5 years ago by mickem

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

comment:4 in reply to: ↑ 3 Changed 5 years ago by Blackhex

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?

comment:5 Changed 5 years ago by anonymous

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

comment:6 Changed 5 years ago by anonymous

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.

comment:7 Changed 5 years ago by mickem

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

comment:8 Changed 5 years ago by Blackhex

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).

comment:9 Changed 5 years ago by anonymous

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

comment:10 Changed 5 years ago by anonymous

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.

comment:11 Changed 5 years ago by anonymous

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

comment:12 Changed 5 years ago by anonymous

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.

comment:13 Changed 5 years ago by anonymous

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

comment:14 Changed 5 years ago by Blackhex

  • Resolution set to fixed
  • Status changed from assigned to closed

Ok, I'm closing it myself.

Add Comment

Modify Ticket

Action
as closed .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from Blackhex. Next status will be 'closed'.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.