Handle CSS as text/css type

[chris.nelson.1022@…]

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.

[Chris.Nelson@…]

Patch to fix mime type of style sheets

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

[Ryan J Ollos]

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.

[ethan.jucovy@…]

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.

[Ryan J Ollos]

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

[Ryan J Ollos]

Patch for ​th:LighterTheme

[Ryan J Ollos]

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

Committed to trunk in [12418].

[Jun Omae]

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

[Ryan J Ollos]

Does this patch look okay?

trac/web/chrome.py

@@ -981 +981 @@
         if content_type is None:
             content_type = 'text/html'
         method = {'text/html': 'xhtml',
                   'text/css':  'text',
+                  'text/javascript': 'text',
                   'text/plain': 'text'}.get(content_type, 'xml')
 
         if method == "xhtml":

[Jun Omae]

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

• ​th:source:dynamicfieldsplugin/0.12/dynfields/web_ui.py#L59
• ​th:source:dynamicfieldsplugin/0.12/dynfields/templates/dynfields.html

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

• ​th:source:sumstatsplugin/0.12/sumstats/web_ui.py#L176
• ​th:source:sumstatsplugin/0.12/sumstats/templates/sumstats.css

[Ryan J Ollos]

The change is on trunk so API changes seem reasonable to make. It seems like ​th:DynamicFieldsPlugin should be using add_script_data for Trac ≥ 0.12 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?

[Remy Blank]

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.

[Ryan J Ollos]

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.

[Remy Blank]

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

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

[…]

[Olemis Lang <olemis+trac@…>]

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 

[Christian Boos]

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.

[Ryan J Ollos]

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

[Olemis Lang <olemis+trac@…>]

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 .

[Ryan J Ollos]

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

[Ryan J Ollos]

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

[Ryan J Ollos]

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

Changes committed to trunk in [12527].