Edgewall Software
Modify

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#7316 closed defect (fixed)

Timeline RSS entries truncated

Reported by: Christopher Lenz Owned by: Christian Boos
Priority: highest Milestone: 0.11.2
Component: timeline Version: 0.11-stable
Severity: critical Keywords:
Cc: ironfroggy@…, osimons Branch:
Release Notes:
API Changes:

Description

In all previous versions of Trac, you could have the entries in the HTML timeline be truncated so that the timeline remains compact and readable, but still get the full entry text in the RSS feed. That behavior makes a lot of sense because you normally consume RSS feeds by reading one item a time, so you can afford to have the full text displayed. The new behavior requires one to visit the corresponding page to read the full text for the vast majority of check-ins, tickets, etc.

It should be possible to configure the timeline to show truncated entries in the HTML version and non-truncated entries in the RSS version. And this should be the default, as it has been in the past.

Personally, I think this is a fairly serious bug that severely limits the usefulness of the timeline RSS feed in 0.11.

Attachments (2)

7316-timeline-render-refact-r7595.diff (13.7 KB ) - added by Christian Boos 11 years ago.
add an extra abbreviated optional parameter to render_timeline_event, so that the timeline.rss can explicitly force full content
7316-timeline-render-hints-r7595.diff (15.5 KB ) - added by Christian Boos 11 years ago.
Use rendering hints in the context for telling the ITimelineEventProviders in which way they should format their wiki texts

Download all attachments as: .zip

Change History (27)

comment:1 by Christian Boos, 11 years ago

Each ITimelineEventProvider .render('description', context) method should be able to figure out that we're generating content for a RSS field. In previous versions, the trick was to check for req.args['format'] == 'rss'.

Would you be ok with a .format property added to the Context or even better yet, a .mime_type?

comment:3 by anonymous, 11 years ago

Status: assignednew

comment:4 by anonymous, 11 years ago

Status: newassigned

comment:5 by anonymous, 11 years ago

Type: defecttask

comment:6 by ironfroggy@…, 11 years ago

Cc: ironfroggy@… added

comment:7 by anonymous, 11 years ago

Owner: changed from Christian Boos to anonymous
Status: assignednew

comment:8 by ebray, 11 years ago

Owner: changed from anonymous to Christian Boos

This ticket seems to be a popular target for random butchering.

comment:9 by anonymous, 11 years ago

Resolution: fixed
Status: newclosed

comment:10 by Noah Kantrowitz, 11 years ago

Resolution: fixed
Status: closedreopened

comment:11 by anonymous, 11 years ago

Resolution: fixed
Status: reopenedclosed

comment:12 by anonymous, 11 years ago

Resolution: fixed
Status: closedreopened

comment:13 by anonymous, 11 years ago

Resolution: fixed
Status: reopenedclosed

comment:14 by Christian Boos, 11 years ago

Resolution: fixed
Status: closedreopened

Dear "anonymous", please stop.

By the way, any feedback on comment:1?

comment:15 by osimons, 11 years ago

I agree that feeds should really be available in full, but then some might not want that leading perhaps to a setting for 'short' or 'full' content at some stage - or even a personal preference for that matter. I think the mime-type is a less flexible option than 'format' that we also use in many similar situations.

comment:16 by Christian Boos, 11 years ago

Well, the main issue here is how do we get the ITimelineProvider to know that they're rendering in full or not. In both timeline.html and timeline.rss we have things like ${event.render('description', context)}. So either we extend render(field,ctxt) to add an extra format argument (care should be taken about API compatibility then), or we stuff that into the context, as a kind of rendering hint (verbose vs. compact?).

The notion of "which mimetype am I rendering" in a Context would be useful for other things, but I agree that special casing the full/compact mode on that would not be really better than what we did in the past (and not extensible, if we consider, say, Atom support).

by Christian Boos, 11 years ago

add an extra abbreviated optional parameter to render_timeline_event, so that the timeline.rss can explicitly force full content

comment:17 by Christian Boos, 11 years ago

Keywords: review added
Milestone: 0.11.30.11.2

Please try out the above patch, which works fine for me.

What could be tested is the backward compatibility support for other 0.10 or 0.11 ITimelineEventProviders.

Also, while working on the above patch, I noticed that there's an optimization waiting to be done in the event data preparation that could probably have a quite noticeable performance impact, so there's another patch forthcoming.

comment:18 by Christian Boos, 11 years ago

Hm, should be arity(...) == 4 in the patch, for backward compatibility.

in reply to:  17 comment:19 by Christian Boos, 11 years ago

Following-up to comment:17

… I noticed that there's an optimization waiting to be done in the event data preparation

Ok, so I thought that not creating a new lambda for every event would make a difference, but actually there's no speed benefit at all (it's even a tiny bit slower). The memory savings are not that noticeable either, so I'm dropping the idea.

comment:20 by osimons, 11 years ago

Tested the patch, and it seems to work well for plain Trac. Using arity of 4 works for existing 0.11 providers. I don't have any 0.10 providers left for testing though.

For my non-default providers, I do see that updates are necessary to support the new rendering. Thinking about it, and seeing the amount of similar code needed in each provider, I wonder if this can't be done dfferently when updates are needed anyway. What if we instead changed and checked for optional return type? If basestring or element, we use as-is, but if the return type is a tuple like (context or resource, text, options) we complete the rendering in the timeline module according to whatever format is suggested by current rendering context? Deferring the rendering as a default strategy for providers would simplify code, leave api as-is, and make it easier to change in the future. Would that work?

comment:21 by osimons, 11 years ago

Cc: osimons added

Dear Trac. Please notify me when changes occur :-)

in reply to:  20 comment:22 by Christian Boos, 11 years ago

Replying to osimons:

(snip) If basestring or element, we use as-is, but if the return type is a tuple like (context or resource, text, options) we complete the rendering in the timeline module according to whatever format is suggested by current rendering context?

Interesting idea, that would work for some providers, but others (ticket, changeset) are producing a more complex output anyway.

But I see your point, and instead of simply specifying abbreviated and deducing the formatter flavor and options from this, we could directly go one step further and specify those to the providers. That way, we could imagine having a plain-text timeline, by specifying a 'text' flavor. Note that this was more or less the alternative discussed in comment:16 of having those rendering hints stored in the (rendering) context. Where's the .property dict gone? Ah yes … r6086.

Also, trying a bit harder to factor out the common bits would also eventually leave some room for further enhancements that other providers would get for free, like a saner way to set the preferences (in the .ini, [timeline] abbreviated_message_<provider-key> and eventually later on a per-user basis).

by Christian Boos, 11 years ago

Use rendering hints in the context for telling the ITimelineEventProviders in which way they should format their wiki texts

comment:23 by Christian Boos, 11 years ago

Please have a look at attachment:7316-timeline-render-hints-r7595.diff which implements the approach outlined in comment:22. That patch is an alternative to the other (i.e. it applies cleanly on r7595).

For non-default providers, all what's need to do in order to benefit from the new logic is to replace explicit calls to:

format_to(env, 'html', context, wikitext)
# or the equivalent: format_to_html(env, context, wikitext)

by:

format_to(env, None, context, wikitext)

so that the formatting will be done according to the hints set in the context.

I've not yet added the extended configuration, but the idea would be to support something like:

[timeline]
#
# Spec:
#
# format_<format> = html|oneliner [key=value...]
# format_<format>_<key> = html|oneliner [key=value...]
#
# Example:
#
format_rss = html escape_newlines
format_html = html
format_html_wiki = oneliner shorten_lines=true

… as a way to override and replace the various options we added over time to tweak the timeline.

Also, what about preserve_newlines vs. escape_newlines? I think we should standardize on one (preferably preserve_newlines, better name and less compatibility code to add).

A note about compatibility: the changes done in this patch are less intrusive (no change for the render_timeline_event() signature) on one side, more on the other side (addition of the rendering hints API on trac.mimeview.Context + possibility to specify a sub-context sharing the same resource as the parent's context). However those last changes should be fully backward compatible.

comment:24 by osimons, 11 years ago

Patch tested, and works really well. You found a nice way of deferring renderer. I like.

I've also prepared the conversion for my trachacks:FullBlogPlugin, and that was a simple operation. Due to this change happening in mid-0.11, it needed some 'compat'-switching but that is no big deal.

As for your other notes:

  • Your extended configuration looks useful enough, but I'd hesitate to add this configuration complexity without a stated need from the users. Suggest that after closing this ticket, you create a new ticket for that topic to allow others to voice their opinion and vote on the issue. Schedule it for some future milestone - 0.12/0.13.
  • preserve_newlines rings better in my ears too if newlines is actually the only thing the 'escaping' affects.

comment:25 by Christian Boos, 11 years ago

Resolution: fixed
Status: reopenedclosed

Patch applied in [7608:7610], using preserve_newlines as hint.

In comment:23, I was effectively targeting "some future version", we can create a new enhancement ticket for that when/if the need arise.

comment:26 by Christian Boos, 11 years ago

Keywords: review removed
Type: taskdefect

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.