Edgewall Software
Modify

Opened 12 years ago

Closed 9 years ago

Last modified 9 years ago

#10470 closed defect (fixed)

Automatic preview of a diff pasted in comment textarea using WikiProcessor looks slightly different before/after clicking 'preview' button.

Reported by: repeat@… Owned by: Jun Omae
Priority: high Milestone: 1.0.3
Component: rendering Version: 0.12.2
Severity: normal Keywords: context hints
Cc: Jun Omae, osimons, leho@…, olemis+trac@… Branch:
Release Notes:

Additional stylesheets are loaded during the automatic preview of wiki and ticket editing.

API Changes:

Add chrome_info_script to trac.web.chrome.

Internal Changes:

Description

If a user inputs a diff file in the comment textarea using WikiProcessor, a simple diff preview will be given automatically, as in first-preview.png (which use Trac's diff as example).

After clicking 'preview' button, a colorful diff preview will be shown instead, as in second-preview.png.

It will be nice if the automatic preview of a diff in WikiProcessor looks the same as clicking the preview button.

Attachments (4)

first-preview.png (36.6 KB ) - added by repeat@… 12 years ago.
second-preview.png (30.1 KB ) - added by repeat@… 12 years ago.
t10470-load-stylesheet-on-auto-preview-r10876.diff (1.9 KB ) - added by Jun Omae 12 years ago.
t10470-load-stylesheet-on-auto-preview-0.13dev-r10883.diff (8.2 KB ) - added by Jun Omae 12 years ago.
For 0.13dev r10883

Download all attachments as: .zip

Change History (26)

by repeat@…, 12 years ago

Attachment: first-preview.png added

by repeat@…, 12 years ago

Attachment: second-preview.png added

comment:1 by Remy Blank, 12 years ago

Milestone: next-major-0.1X

Yes, that's a known issue. The diff WikiProcessor adds a stylesheet, but the latter isn't set during the automatic preview. We should somehow transmit the list of stylesheets together with the content, and update it in the browser.

comment:2 by Jun Omae, 12 years ago

We can load the stylesheets with $.loadStyleSheet() on the automatic preview. The following patch adds a script tag to the response of /wiki_render. I tested with Firefox 8, Chrome 16 and IE 7. Thoughts?

t10470-load-stylesheet-on-auto-preview-r10876.diff.

comment:3 by Remy Blank, 12 years ago

I haven't tested the patch yet, but it looks quite elegant. I wouldn't have thought of sending the stylesheets using an embedded <script> tag.

comment:4 by Jun Omae, 12 years ago

Cc: Jun Omae added

t10470-load-stylesheet-on-auto-preview-0.13dev-r10883.diff for 0.13dev: Adds load_stylesheet argument to format_to_html. If True is specified, the method generates html with <script>jQuery.loadStyleSheet(...)</script> to load the additional stylesheets.

comment:5 by Remy Blank, 12 years ago

Note that the rendering hint in the context is passed to the formatter, so you don't need to pass it explicitly to the formatting functions.

But I'm not sure you want to make a difference between the initial rendering and the "preview" rendering. Your implementation works for the wiki page preview, but won't work for ticket previews. Can't this be made unconditional?

comment:6 by osimons, 12 years ago

By hooking into a fundamental rendering mechanism and adding <script>...</script> tags, you also make the assumption that all rendering will be destined for web page output inside the standard Trac layout. So unless jQuery (++) are already loaded correctly, any call inserted will of course fail.

To me this just runs a risk of entangling format_to_html() call with a specific purpose. This method should be output agnostic and not make assumptions about use - even though it is though a suggested keyword argument. If anything it should be handled by interacting with the context so maybe that one day will have methods that can customize output for specific purposes.

I've never really liked the structure of the context and how we bury hints inside it in a totally non-obvious manner, and call to get_hint() in arbitrary places to be used by the code as it pleases. It just makes things very complicated, and entangles the code with needs of the Trac project only. Plugins obviously have no way of interacting with this structure. To me, any supported 'hint' should be provided by a component extension point, and likewise the handling of the given hint for a given output context should also be extendible by others. The load_stylesheet hint could then be handled differently when rendering for email, Trac layout web, or even the HTML I call to generate for various purposes through the RPC interface. At that stage we could even drop the vague 'hint' term for a more formal 'feature' structure…

One example for illustration is what to expect if you want to use format_to_html() to render HTML email notification content. It is still HTML, but just not the same. Perhaps then you'd want a completely different strategy to adding content - like for instance inlining the stylesheet instead of linking to it, or maybe ensuring that it exists as content attached to the message and references pointing to cid:<content_id> instead of a server URL.

Yes, I know that for an isolated page render I could hack something that mostly works. But it would be going against the APIs instead of working with it. I'd like for the context to handle this automatically (for instance by owning the add_*() functions and mapping them as supported features), or explicitly by low-level code calling to the context to handle some specific feature (ignoring it if it isn't supported by this context). Then the "full-page rendering context", and the "auto-preview stub context" would be wired slightly differently leading to different handling of the added stylesheet.

Sorry to complicate yet another ticket… :-)

Version 0, edited 12 years ago by osimons (next)

comment:7 by Remy Blank, 12 years ago

All valid comments. TBH, I don't see how you would replace the current context hints with extension points. There's some pretty fundamental differences, for example contexts are local to a given code path, and can be nested, whereas extension points are global to the environment. So I would be very interested in understanding that point.

in reply to:  7 ; comment:8 by osimons, 12 years ago

Cc: osimons added

Replying to rblank:

All valid comments. TBH, I don't see how you would replace the current context hints with extension points. There's some pretty fundamental differences, for example contexts are local to a given code path, and can be nested, whereas extension points are global to the environment. So I would be very interested in understanding that point.

Sorry, got no real details… But I'll try… :-)

  1. Extension points for hints would be primarily to make them visible and part of a registry of some sort, instead of a below-the-surface implementation. So maybe the stylesheet example would provide handle_stylesheet as the implementation method for to implement in various contexts.
  1. Which implementation to use would be up to the rendering context to decide. We could then create template contexts for HtmlWebPageContext, HtmlWebStubContext, HtmlEmailContext where each would implement the handle_stylesheets method in the manner most suitable for each. It could decide to override it with a custom implementation, or just call default providers to handle it - or raise an error if code calls on a feature that isn't present in current installation/version (or pass it silently if that makes more sense).
  1. Plugins would be able to construct and implement other template contexts, and there is no reason why we shouldn't make it possible to override the template context selected for a given task. That means a plugin could provide a specific alternative implementation for a given situation. Perhaps we could make the generation of the initial context a pluggable system too, so that contexts are created by calling something like (pseudo):
    context = ContextManager(env).make_context('html_web_page', **kwargs)
    
    kwargs could be the regulars like perm, href and so on, but alternatively also context if we want to nest a html_web_stub inside a html_web_page.

I see your point about global/local issue, and agree: We could not make the context a component as we don't want to share structures with all that entails. So the API would need to stay at a higher level and just provide lookup and template features for generating short-lived rendering context structures.

Like current add_stylesheet tied into Chrome would really just be needed when rendering a full page, while just calling context.handle_stylesheet('my_stylesheet') would also mean that the caller (perhaps from deep inside some markup handler) would not need to worry about req and what that looks like - like knowing there would need to exist some req.chrome dict for storing stylesheets. This non-obvious structure is one of the reasons why it is so hard to things like #2625 properly, and also ties in with the ongoing discussion at #10125 that also execute outside regular request context. For #2625 I have HTML email working for my own needs, but apart from a few predictable fields I don't dare to render HTML markup for something like ticket comments as user content can call on any wiki processor that may in turn make any assumption about req directly or indirectly that I would never be able to predict in my mock request object.

I'll be the first to admit that the formatter/rendering/context code is an area where I've spent little time. I've always found it non-obvious and hard to understand, so I'm quite sure there are essential aspects that I've missed. However, particularly with the RPC plugin I'm probably the one that most often bump my head against issues and assumptions made by buried conditionals deep inside code that I cannot influence. I just want formatting and contexts to be less entangled. I don't know exactly how… :-)

in reply to:  8 comment:9 by Christian Boos, 12 years ago

Component: ticket systemrendering
Keywords: context hints added
Priority: normalhigh

Replying to osimons:

(snip) For #2625 I have HTML email working for my own needs, but apart from a few predictable fields I don't dare to render HTML markup for something like ticket comments as user content can call on any wiki processor that may in turn make any assumption about req directly or indirectly that I would never be able to predict in my mock request object.

The main idea behind the RenderingContext was precisely to get progressively rid of the (web) Request as the central part of the rendering, as you're not always rendering HTML and even for HTML, not always for a browser. The rendering context "knows" how things should be rendered, and you can focus on providing the content. Now, that the API could be improved and that the hints could evolve to something more structured with pluggable behavior, that's certainly possible and desirable. Material for post-1.0 ;-)

(snip) However, particularly with the RPC plugin I'm probably the one that most often bump my head against issues and assumptions made by buried conditionals deep inside code that I cannot influence.

Another wish for post-1.0, maybe it will be a good idea to integrate the TH:XmlRpcPlugin into core, as this will provide a few opportunities to refactor the code, as often discussed.

comment:10 by Christian Boos, 12 years ago

Milestone: next-major-releasesnext-dev-1.1.x

One comment about the patch t10470-load-stylesheet-on-auto-preview-0.13dev-r10883.diff itself: I haven't studied it in details yet, but I don't think that each wiki_to_html should have to care about adding a load_stylesheet parameter. We should find a way which makes it implicit.

Also moving to 1.1.x for the points discussed in comment:9 and above.

comment:11 by lkraav <leho@…>, 12 years ago

Cc: leho@… added

comment:12 by olemis+trac@…, 11 years ago

Cc: olemis+trac@… added

comment:13 by Jun Omae, 9 years ago

New proposed changes in [98eebd9c/jomae.git] (jomae.git@t10470_1.0) and [acd4654e/jomae.git] (jomae.git@t10470_0.12). I've created these branches on Jan 2014 and rebased now.

The changes added jQuery.loadStyleSheet(...) to content sending from trac/wiki/web_api.py and trac/ticket/web_ui.py using filter_stream.

For 0.12.x users, using th:TracFewFixesPlugin is able to fix it.

in reply to:  13 comment:14 by Jun Omae, 9 years ago

Replying to jomae:

New proposed changes in [98eebd9c/jomae.git] (jomae.git@t10470_1.0) and [acd4654e/jomae.git] (jomae.git@t10470_0.12). I've created these branches on Jan 2014 and rebased now.

The changes added jQuery.loadStyleSheet(...) to content sending from trac/wiki/web_api.py and trac/ticket/web_ui.py using filter_stream.

Updated jomae.git@t10470_1.0. filter_stream isn't used in the changes.

comment:15 by Jun Omae, 9 years ago

Milestone: next-dev-1.1.x1.0.3
Owner: set to Jun Omae
Status: newassigned

I'm retrying to fix it, jomae.git@t10470_1.0. After the changes, XHR response of auto-preview includes script elements with jQuery.loadStylesheet(). Could someone please review the changes?

in reply to:  15 comment:16 by Christian Boos, 9 years ago

Reviewing:

  • in source:jomae.git/trac/wiki/web_api.py@aad5c02d, there's a new from genshi.builder import tag which doesn't seem to be used
  • chrome_info_script could return '' (simplifying the calls where it's followed by the or ''); otherwise if there's a real need to have None we could also simplify by leaving out the else: None

comment:17 by Jun Omae, 9 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Thanks for the reviewing. Committed based on the review in [13328] and merged to trunk in [13329].

comment:18 by Ryan J Ollos, 9 years ago

Release Notes: modified (diff)

comment:19 by lkraav <leho@…>, 9 years ago

I had to revert this 28b914a or [13329] in one of my production branches, where site.html doesn't load trac.css at all because it's using its own CSS on a blank canvas.

Now trac.css gets shoved back in during preview and ruins everything. How do I filter this out?

in reply to:  19 ; comment:20 by Jun Omae, 9 years ago

Now trac.css gets shoved back in during preview and ruins everything. How do I filter this out?

Currently, It is not able to filter contents of the auto-preview by site.html. Similar to #11199.

Instead, I think that you can use the following small plugin in your case. If the plugin is installed to your $TRACENV/plugins, it sends empty contents when trac.css is requested.

# -*- coding: utf-8 -*-

from trac.core import Component, implements
from trac.web.api import IRequestFilter
from trac.web.chrome import Chrome

class AvoidTracCss(Component):

    implements(IRequestFilter)

    def pre_process_request(self, req, handler):
        if isinstance(handler, Chrome) and \
                req.args['prefix'] == 'common' and \
                req.args['filename'] == 'css/trac.css':
            return self
        return handler

    def post_process_request(self, req, template, data, content_type):
        return template, data, content_type

    def process_request(self, req):
        return req.send('', 'text/css')

in reply to:  20 ; comment:21 by lkraav <leho@…>, 9 years ago

Replying to jomae:

Instead, I think that you can use the following small plugin in your case. If the plugin is installed to your $TRACENV/plugins, it sends empty contents when trac.css is requested.

Thanks @jomae, but the plugin as written doesn't kill off the injection. Haven't had time to debug specifics yet. debug.log shows the plugin does get loaded, but I think it's missing something.

in reply to:  21 comment:22 by Jun Omae, 9 years ago

Replying to lkraav <leho@…>:

Thanks @jomae, but the plugin as written doesn't kill off the injection. Haven't had time to debug specifics yet. debug.log shows the plugin does get loaded, but I think it's missing something.

Confirm empty response for trac.css using like this and try force-reload.

wget -S -O - http://your.trac.site/trac/chrome/common/css/trac.css

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Jun Omae 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.