Two components shown on the WebAdmin panel

[Ryan J Ollos]

The WebAdmin panel shows two components:

There doesn't seem to be any reason to have two components. If only NoteBox is enabled, this is the result:

If only NoteBoxFilter is enabled, this is the result:

I'm going to just go ahead and combine these two classes. Someone please correct me if there is a reason to revert the changes I'm making.

[Ryan J Ollos]

(In [11212]) Fixes #9739: NoteBoxPlugin is now a single class. All imports have been moved to the top of the module.

[Steffen Hoffmann]

Replying to rjollos:

... There doesn't seem to be any reason to have two components. ...

I'm going to just go ahead and combine these two classes. Someone please correct me if there is a reason to revert the changes I'm making.

+1 from my side, for moving the imports. Sure, no point in using more than just the single class. Good catch.

[Ryan J Ollos]

Replying to hasienda:

+1 from my side, for moving the imports. Sure, no point in using more than just the single class. Good catch.

Thanks for the feedback! I was mostly sure this was the right things to do, but it's nice to have second opinions and be more certain.

[Ryan J Ollos]

This ticket got me thinking in more detail about how the stylesheet is added to the page. In contrast to the NoteBoxPlugin, the FootNoteMacro does not implement the IRequestFilter methods. The FootNoteMacro adds a script and stylesheet in expand_macro. On a page in which the FootNoteMacro is used, I see the following:

<script type="text/javascript">
    jQuery.loadStyleSheet("/chrome/footnote/footnote.css", "text/css");
</script>
<script type="text/javascript" src="/chrome/footnote/footnote.js"></script>

On a page that does not use the FootNoteMacro, there are no references to FootNoteMacro files in the page source.

The NoteBoxPlugin implements the IRequestFilter methods and the stylesheet is added in post_process_request. There is no realm matching done in post_process_request so I see the stylesheet loaded for every realm. The page source will contain:

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

This includes realms that don't have editable wiki markup, and for which the stylesheet almost certainly does not need to be loaded, such as the browser realm.

In this case, the style sheet is a small amount of data, so I would think that it is unlikely to result in any measurable performance difference, but as a general good practice it seems like it is probably better to add the stylesheet in expand_macro and not implement IRequestFilter methods for the NoteBoxPlugin. The alternative, of adding realm filtering, seems impractical because we can't know all the realms that might want to make use of the macro.

Well, those are my thoughts on the issue, but I'm just getting started with Trac development, so I think there is probably more to it. I'd appreciate more experienced feedback on the issue, so I've cc'ed the two best source I know on Trac-Hacks, with hope of grabbing a couple minutes of your time. Any thoughts on adding the scripts and stylesheets in expand_macro vs post_process_request?

[Steffen Hoffmann]

Replying to rjollos:

This ticket got me thinking in more detail about how the stylesheet is added to the page. In contrast to the NoteBoxPlugin, the FootNoteMacro does not implement the IRequestFilter methods. The FootNoteMacro adds a script and stylesheet in expand_macro.

Yes, I'm right now doing the same to TracFormsPlugin too.

... On a page that does not use the FootNoteMacro, there are no references to FootNoteMacro files in the page source.

... The alternative, of adding realm filtering, seems impractical because we can't know all the realms that might want to make use of the macro.

Within WikiMacros the expand_macro way is definitely the way to go exactly for the reason you mentioned. Have the added scripts only where they are required, that is where the macro is found and expanded. In contrast to Simon I'm not the experienced developer you kindly make me look like, but I feel rather determined about this.

[Ryan J Ollos]

(In [11215]) Refs #9739: Add the stylesheet in expand_macros and don't implement the IRequestFilter interface.

[Ryan J Ollos]

Replying to hasienda:

Yes, I'm right now doing the same to TracFormsPlugin too.

Thanks for the feedback. I'll have to track your work on TracFormsPlugin more closely.

Another style question ...

Shouldn't this macro inherit from WikiMacroBase rather than Component? I think that eliminates the need to implement get_macros and get_macro_description.

[Ryan J Ollos]

(In [11217]) Refs #9739: (Refactoring) Inherit from WikiMacroBase rather than Component.

[Ryan J Ollos]

Replying to rjollos:

Another style question ...

Shouldn't this macro inherit from WikiMacroBase rather than Component? I think that eliminates the need to implement get_macros and get_macro_description.

Made this change in [11217]. Hope to hear feedback on this ...

[Ryan J Ollos]

(In [11219]) Refs #9739: Minor refactoring and cleanup.

[Steffen Hoffmann]

Replying to rjollos:

Replying to rjollos:

Another style question ...

Shouldn't this macro inherit from WikiMacroBase rather than Component? I think that eliminates the need to implement get_macros and get_macro_description.

Made this change in [11217]. Hope to hear feedback on this ...

Yes, this is what abstract base classes are for. Good catch. And another +1 for replacing the license text with a reference to the full license. This is not something you have to care for very often.

Look ​what you've done: Much straightened code, in fact a whole new plugin with respect to the source.

[Ryan J Ollos]

Replying to hasienda:

Look ​what you've done: Much straightened code, in fact a whole new plugin with respect to the source.

Yeah, I was surprised by how much simplification in code that resulted. It just goes to show the power of the Trac framework. With proper use, a macro can be created with just a few lines of code.

Thanks for your ongoing review, help and feedback!

[Ryan J Ollos]

Btw, next on my hit list is TracPlantUmlPlugin, which is really just a macro. I see some possibilities for minor enhancement there, and code cleanup similar to what was done for this plugin.