Timeline RSS entries truncated

[cmlenz]

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.

[cboos]

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?

[ebray]

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

[cboos]

Dear "anonymous", please stop.

By the way, any feedback on comment:1?

[osimons]

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.

[cboos]

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

[cboos]

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

[cboos]

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.

[cboos]

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

[cboos]

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.

[osimons]

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?

[osimons]

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

[cboos]

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

[cboos]

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

[cboos]

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.

[osimons]

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.

[cboos]

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.