Static resources shouldn't be loaded for every page

[Ryan J Ollos]

The static resources of this macro are loaded for every page, even on pages that the macro is not used. For example, on a ticket page (with no instances of the macro), I see the following:

<link rel="stylesheet" href="/chrome/tracjsgantt/jsgantt.css" type="text/css" />
<link rel="stylesheet" href="/chrome/tracjsgantt/tracjsgantt.css" type="text/css" />

</script><script type="text/javascript" src="/chrome/tracjsgantt/jsgantt.js">

Also, there are two components that must be enabled for this plugin, but I can't see any circumstance in which only one component could be enabled without enabling the other.

My suggestions are:

• Don't implement IRequestFilter. Add the stylesheets and scripts in expand_macro.
• Inherit from WikiMacroBase rather than Component.

Dose that make sense, or am I missing something? I can try this out and provide a patch very shortly if it makes sense to you. I did the exact same cleanup for the NoteBoxPlugin recently in #9739, and had some discussion with hasienda about these issues.

[anonymous]

Replying to rjollos:

The static resources of this macro are loaded for every page, even on pages that the macro is not used. For example, on a ticket page (with no instances of the macro), I see the following: ...

Yes, only loading when necessary makes a lot of sense.

Also, there are two components that must be enabled for this plugin, but I can't see any circumstance in which only one component could be enabled without enabling the other.

I have *7* components on my system! (I've added some stuff and refactored some more.)

My suggestions are:

• Don't implement IRequestFilter. Add the stylesheets and scripts in expand_macro.

I think I understand the latter, I'm less clear on the effect of the former.

• Inherit from WikiMacroBase rather than Component.

That's greek to me.

Dose that make sense, or am I missing something? I can try this out and provide a patch very shortly if it makes sense to you. I did the exact same cleanup for the NoteBoxPlugin recently in #9739, and had some discussion with hasienda about these issues.

I'd welcome a patch. I've done major surgery lately (I haven't pushed to TH because I want to run it a while in our production so I'm sure of it) but will use your patch as a basis to revise my code; surely easier than figuring it out for myself.

[Ryan J Ollos]

Thanks for the quick response. I'll have a patch for you shortly and followup with more details.

[Ryan J Ollos]

Do you have any interest in making this backward compatible all the way to 0.11.0? hasienda brought this good practice to my attention, and I've been trying to do that lately, but I don't have any extremely strong opinions on the issue. The argument that all users on 0.11.x should be running 0.11.7 also seems reasonable, though I know this can be challenging in some environments and for users that depend on a packaged distribution (e.g. an RPM).

​comment:16:ticket:5907 has the relevant info. We'd just need to add the javascript_quote function, as in [11200]. If you'd rather not go that route, I think you should add install_requires = ['Trac >= 0.11.3'] to setup.py.

See also:

• ​http://trac.edgewall.org/ticket/7912#comment:3
• ​http://trac.edgewall.org/browser/tags/trac-0.11.2/trac/util/text.py
• ​http://trac.edgewall.org/browser/tags/trac-0.11.3/trac/util/text.py

[Ryan J Ollos]

I have a patch that allows install with 0.11.0. It is nearly the same as [11200]. I should probably create a different ticket if you have an interest in it.

[Ryan J Ollos]

Here is a patch which does part of what I wanted to accomplish. It:

• Removes the TracJSGanttSupport class and moves all its methods to the TracJSGanttChart class.
• Moves add_stylesheet from post_process_request to expand_macro.

I wasn't able to move add_script into expand_macro and eliminate the need to implement IRequestInterface. When a script is added in expand_macro, it is apparently added using a jQuery call near the end of the page, rather than at the top of the page, which seems to be the case when you call add_script in post_process_request. The result is, the script is called before it is added, which results in an error.

To see what I'm talking about, after you apply the patch look for the following near the end of the page:

<script type="text/javascript">
    jQuery.loadStyleSheet("/tracdev/chrome/tracjsgantt/jsgantt.css", "text/css");
    jQuery.loadStyleSheet("/tracdev/chrome/tracjsgantt/tracjsgantt.css", "text/css");
</script>

I'm not sure how to solve this issue, but I think it can be done. I also think this patch is a step in the right direction since the stylesheets no longer get loaded on every page. There is a related comment in pre_process_request:

# I think we should look for a TracJSGantt on the page and set 
# a flag for the post_process_request handler if found

This shouldn't be necessary if we can figure out how to move add_script to expand_macro, since you won't need to implement these IRequestHandler methods anymore.

Have you looked at using some of the genshi.builder elements to construct the elements that are added to the page?

from genshi.builder import tag

tags = []
tags.append(tag.div(class_='gantt', style='position:relative', id='GanttChartDIV_%s' %(self.GanttID,)))
tags.append(tag.script(chart, type='text/javascript'))

[Ryan J Ollos]

Replying to anonymous:

My suggestions are:

• Don't implement IRequestFilter. Add the stylesheets and scripts in expand_macro.

I think I understand the latter, I'm less clear on the effect of the former.

I sort of covered this in my follow-on comments, but just to be explicit and state things as I understand them. The only work you are doing in post_process_request is to add the stylesheets and scripts, so if you can move these two functions, you no longer have to implement these two methods, and by extension you no longer have to implement the interface. This is a good thing, as I understand it, because the methods get called for every request.

Here is a reference: ​t:docs:/trac-trunk/html/api/trac_web_api.html#trac.web.api.IRequestFilter.

[Ryan J Ollos]

Replying to rjollos:

I wasn't able to move add_script into expand_macro and eliminate the need to implement IRequestInterface. When a script is added in expand_macro, it is apparently added using a jQuery call near the end of the page, rather than at the top of the page, which seems to be the case when you call add_script in post_process_request. The result is, the script is called before it is added, which results in an error.

I've been thinking about this in the course of my work on #7617, which you may find interesting to track. I'm thinking that the thing to do here is something along the lines of:

• add an id to the div element that you add to the page, and use a jQuery selector to add the javascript code.
• move the javascript code out of the python class.
• use add_script_data to pass variables to the javascript code.

... or something like that. I'm just learning here, so don't put too much weight on this. After you release the next version that incorporates all these patches, I'm happy to do some experimentation and see if we can get this to work. It would be nice for maintainability if you didn't have to build up that javascript by concatenating strings in the py file.

[Chris Nelson]

Replying to rjollos:

... I've been thinking about this in the course of my work on #7617, which you may find interesting to track. I'm thinking that the thing to do here is something along the lines of:

• add an id to the div element that you add to the page, and use a jQuery selector to add the javascript code.
• move the javascript code out of the python class.
• use add_script_data to pass variables to the javascript code.

... or something like that. I'm just learning here, so don't put too much weight on this. After you release the next version that incorporates all these patches, I'm happy to do some experimentation and see if we can get this to work. It would be nice for maintainability if you didn't have to build up that javascript by concatenating strings in the py file.

Maybe I'm not thinking about it right but adding another technology (jQuery) doesn't make sense to me. Aren't you having problems with jQuery version in other plugins you're updating?

[Ryan J Ollos]

Replying to ChrisNelson:

Maybe I'm not thinking about it right but adding another technology (jQuery) doesn't make sense to me. Aren't you having problems with jQuery version in other plugins you're updating?

I've had issues with jQuery UI because different plugins inject bits of the jQuery UI libraries into pages, and they conflict. jQuery itself is provided by Trac and injected into every page. You wouldn't have to add any jQuery libraries yourself.

The distinction between jQuery and jQuery UI took me a little while to sort out.