Edgewall Software
Modify

Opened 8 years ago

Closed 7 years ago

Last modified 5 years ago

#11129 closed enhancement (fixed)

Handle CSS as text/css type

Reported by: chris.nelson.1022@… Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.2
Component: general Version:
Severity: normal Keywords:
Cc: ethan.jucovy@…, Christian Boos, olemis+trac@… Branch:
Release Notes:
API Changes:

The rendering method may be specified by the request handler as a fourth parameter in its response. The method is passed to post_process_request when the parameter is in the implementation's signature, and can be modified by the IRequestFilter. When not explicitly specified, the rendering method is still inferred from the content type when rendering the template.

Internal Changes:

Description

I have a plugin that implements IRequestHandler and returns CSS from process_request like:

    def process_request(self, req):
	myColors = [
            ('a', '#3a3'),
            ('b', '#fff'),
            ('c', '#ada'),
            ('d', '#888')
	]
...
	return 'myplugin.css', {'myColors': myColors}, 'text/css'

but the CSS doesn't get the correct mime type without patching chrome.py as attached.

Attachments (3)

chrome.diff (432 bytes ) - added by Chris.Nelson@… 8 years ago.
Patch to fix mime type of style sheets
t11129-lightertheme.patch (10.8 KB ) - added by Ryan J Ollos 7 years ago.
Patch for th:LighterTheme
t11129-lightertheme-2.patch (10.8 KB ) - added by Ryan J Ollos 7 years ago.

Download all attachments as: .zip

Change History (29)

by Chris.Nelson@…, 8 years ago

Attachment: chrome.diff added

Patch to fix mime type of style sheets

comment:1 by ethan.jucovy@…, 8 years ago

Cc: ethan.jucovy@… added

This would be helpful for me too — I also have a plugin that renders CSS from a template using a request handler, which doesn't work properly without this patch.

in reply to:  1 ; comment:2 by Ryan J Ollos, 7 years ago

This seems harmless enough. Any objections?

Replying to ethan.jucovy@…:

This would be helpful for me too — I also have a plugin that renders CSS from a template using a request handler, which doesn't work properly without this patch.

Which plugin? I'd just like to take a look at an example use-case.

in reply to:  2 comment:3 by ethan.jucovy@…, 7 years ago

Replying to rjollos:

Which plugin? I'd just like to take a look at an example use-case.

th:LighterTheme, which currently has a configuration option for the primary font-family. My workaround is to render the template to a string with mimetype text/plain, and then to send the string with content-type text/css using req.send.

comment:4 by Ryan J Ollos, 7 years ago

Milestone: next-dev-1.1.x1.1.2
Owner: set to Ryan J Ollos
Status: newassigned

Thanks. I will use your plugin to do some testing before committing the change.

by Ryan J Ollos, 7 years ago

Attachment: t11129-lightertheme.patch added

Patch for th:LighterTheme

comment:5 by Ryan J Ollos, 7 years ago

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

I've attached a patch for the th:LighterTheme that seems to work well after the change to Trac.

Committed to trunk in [12418].

comment:6 by Jun Omae, 7 years ago

If text/css would be handled, I think we should handle text/javascript.

comment:7 by Ryan J Ollos, 7 years ago

Does this patch look okay?

  • trac/web/chrome.py

    diff --git a/trac/web/chrome.py b/trac/web/chrome.py
    index dc9eaf3..854314f 100644
    a b class Chrome(Component):  
    981981        if content_type is None:
    982982            content_type = 'text/html'
    983983        method = {'text/html': 'xhtml',
    984984                  'text/css':  'text',
     985                  'text/javascript': 'text',
    985986                  'text/plain': 'text'}.get(content_type, 'xml')
    986987
    987988        if method == "xhtml":

comment:8 by Jun Omae, 7 years ago

Sorry. My suggestion breaks backward-compatibility. Some plugins expect html parsing for text/javascript. Please ignore it. (I've searched with grep -rE "add_script\\(req, *[\"']/" ./trac-hacks.)

Also, the following plugin expects html parsing for text/css.

Last edited 7 years ago by Jun Omae (previous) (diff)

comment:9 by Ryan J Ollos, 7 years ago

The change is on trunk so API changes seem reasonable to make. It seems like th:DynamicFieldsPlugin should be using add_script_data going forward anyway. Which leads me to the thought that since we the add_script_data mechanism, maybe we don't need to handle text/javascript?

Another issue is that /lightertheme/theme.css will not be treated as a cacheble resource (th:#7617, th:#9764), so maybe there is a better solution?

Version 0, edited 7 years ago by Ryan J Ollos (next)

comment:10 by Remy Blank, 7 years ago

Instead of adding more and more items to the method dict, how about allowing process_request() to specify the method as an additional return value? This could even be done in a backward-compatible way.

comment:11 by Ryan J Ollos, 7 years ago

Resolution: fixed
Status: closedreopened

comment:12 by Ryan J Ollos, 7 years ago

Status: reopenedassigned

in reply to:  10 ; comment:13 by Ryan J Ollos, 7 years ago

Replying to rblank:

Instead of adding more and more items to the method dict, how about allowing process_request() to specify the method as an additional return value? This could even be done in a backward-compatible way.

Proposed changes can be found in log:rjollos.git:t11129. Should method be passed to post_process_request? The changes seem to get much more complex in that case.

in reply to:  13 ; comment:14 by Remy Blank, 7 years ago

Cc: Christian Boos added

Replying to rjollos:

Proposed changes can be found in log:rjollos.git:t11129. Should method be passed to post_process_request? The changes seem to get much more complex in that case.

Looks nice. Yes, I think the method should be passed to post_process_request. This can be done relatively easily in a backward-compatible way by using the arity() function that we have in trac.util. Basically, add the method argument to the interface and document it, and before calling it, check whether it takes 3 or 4 arguments. We already have a few instances of this pattern (e.g. in trac/wiki/formatter.py).

But before committing this, since it's a pretty major API change (returning the method from process_request(), not the issue with post_process_request()), I would like Christian to chime in. I have been away from Trac development for too long, and he may have had other plans for process_request().

in reply to:  14 ; comment:15 by Olemis Lang <olemis+trac@…>, 7 years ago

Replying to rblank:

Replying to rjollos:

Proposed changes can be found in log:rjollos.git:t11129. Should method be passed to post_process_request? The changes seem to get much more complex in that case.

Looks nice. Yes, I think the method should be passed to post_process_request. This can be done relatively easily in a backward-compatible way by using the arity() function that we have in trac.util. Basically, add the method argument to the interface and document it, and before calling it, check whether it takes 3 or 4 arguments. We already have a few instances of this pattern (e.g. in trac/wiki/formatter.py).

+1 , function arity() should be invoked to determine whether post_process_request supports new method arg . Nevertheless I'm wondering whether the condition should be if len(resp) > 3 … a subtle difference , maybe not important at all .

[…]

in reply to:  15 comment:16 by Olemis Lang <olemis+trac@…>, 7 years ago

Replying to Olemis Lang <olemis+trac@…>:

Replying to rblank:

Replying to rjollos:

Proposed changes can be found in log:rjollos.git:t11129. Should method be passed to post_process_request? The changes seem to get much more complex in that case.

Looks nice. Yes, I think the method should be passed to post_process_request. This can be done relatively easily in a backward-compatible way by using the arity() function that we have in trac.util. Basically, add the method argument to the interface and document it, and before calling it, check whether it takes 3 or 4 arguments. We already have a few instances of this pattern (e.g. in trac/wiki/formatter.py).

+1 , function arity() should be invoked to determine whether post_process_request supports new method arg . Nevertheless I'm wondering whether the condition should be if len(resp) > 3 … a subtle difference , maybe not important at all .

[…]

Indeed I think I'd rather prefer

                    try:
                        method = resp[3] 
                    except IndexError:
                        method = None 

comment:17 by Olemis Lang <olemis+trac@…>, 7 years ago

Cc: olemis+trac@… added

in reply to:  14 ; comment:18 by Christian Boos, 7 years ago

Replying to rblank:

… (cboos) may have had other plans for process_request().

Well, I had various ideas for refactoring the request dispatching part, some discussed in #8509, others in VcRefactoring/Controller… but nothing there which should prevent making incremental enhancements to the current API! So please go ahead.

comment:19 by Ryan J Ollos, 7 years ago

[12418] reverse-merged in [12433]. I'll continue working on the log:rjollos.git:t11129 changes this week.

in reply to:  18 comment:20 by Olemis Lang <olemis+trac@…>, 7 years ago

Replying to cboos:

Replying to rblank:

… (cboos) may have had other plans for process_request().

Well, I had various ideas for refactoring the request dispatching part, some discussed in #8509, others in VcRefactoring/Controller

afaict, this approach looks very nice . Please see comment:24:ticket:8509

but nothing there which should prevent making incremental enhancements to the current API! So please go ahead.

IMO , @rjollos patches are looking good . Other refinements to the API will represent major upgrade steps towards future adoption e.g. by plugins .

comment:21 by Ryan J Ollos, 7 years ago

Latest proposed changes can be found in log:rjollos.git:t11129.2. I plan to add some tests.

comment:22 by Ryan J Ollos, 7 years ago

The latest changes can be found in log:rjollos.git:t11129.3:

  • Unit tests added.
  • Refactored to localize most of the changes to RequestDispatcher._post_process_request.

Revised patch for th:LighterTheme: t11129-lightertheme-2.patch.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

comment:23 by Ryan J Ollos, 7 years ago

The file move doesn't seem to have been captured in the patch, though it's shown by git status:

$ git status
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#       renamed:    lightertheme/templates/lightertheme/theme.css.tmpl -> lightertheme/templates/theme.css.tmpl
#
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#       modified:   lightertheme/__init__.py
#       modified:   setup.py

by Ryan J Ollos, 7 years ago

Attachment: t11129-lightertheme-2.patch added

comment:24 by Ryan J Ollos, 7 years ago

The patch is corrected now. The issue was that I had a mix of staged (moved files) and unstaged (modified files) changes, so even the -M option didn't result in the moved file being included in the patch. The --cached option would result in the moved file being included, but not the modified files.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

comment:25 by Ryan J Ollos, 7 years ago

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

Changes committed to trunk in [12527].

comment:26 by Ryan J Ollos, 5 years ago

Reporter: changed from Chris.Nelson@… to chris.nelson.1022@…

Modify Ticket

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