Opened 5 years ago

Closed 5 years ago

# Two components shown on the WebAdmin panel

Reported by: Owned by: Ryan J Ollos Ryan J Ollos normal NoteBoxMacro normal cleanup Steffen Hoffmann, Odd Simon Simonsen 0.11

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.

### comment:1 Changed 5 years ago by Ryan J Ollos

Description: modified (diff) changed from gruenebe to Ryan J Ollos new → assigned

### comment:2 Changed 5 years ago by Ryan J Ollos

Resolution: → fixed assigned → closed

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

### comment:3 in reply to:  description ; follow-up:  4 Changed 5 years ago by Steffen Hoffmann

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

### comment:4 in reply to:  3 Changed 5 years ago by Ryan J Ollos

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

### comment:5 follow-up:  6 Changed 5 years ago by 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">
</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?

### comment:6 in reply to:  5 ; follow-up:  8 Changed 5 years ago by Steffen Hoffmann

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.

### comment:7 Changed 5 years ago by Ryan J Ollos

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

### comment:8 in reply to:  6 ; follow-up:  10 Changed 5 years ago by Ryan J Ollos

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.

### comment:9 Changed 5 years ago by Ryan J Ollos

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

### comment:10 in reply to:  8 ; follow-up:  12 Changed 5 years ago by Ryan J Ollos

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

### comment:11 Changed 5 years ago by Ryan J Ollos

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

### comment:12 in reply to:  10 ; follow-up:  13 Changed 5 years ago by Steffen Hoffmann

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.

### comment:13 in reply to:  12 Changed 5 years ago by Ryan J Ollos

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!

### comment:14 Changed 5 years ago by 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.

### Modify Ticket

Change Properties
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.