Modify

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#9739 closed enhancement (fixed)

Two components shown on the WebAdmin panel

Reported by: rjollos Owned by: rjollos
Priority: normal Component: NoteBoxMacro
Severity: normal Keywords: cleanup
Cc: hasienda, osimons Trac Release: 0.11

Description (last modified by rjollos)

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.

Attachments (3)

WebAdminPanel.png (26.4 KB) - added by rjollos 3 years ago.
NoteBoxFilterClassEnabled.png (25.7 KB) - added by rjollos 3 years ago.
NoteBoxClassEnabled.png (12.3 KB) - added by rjollos 3 years ago.

Download all attachments as: .zip

Change History (17)

Changed 3 years ago by rjollos

Changed 3 years ago by rjollos

Changed 3 years ago by rjollos

comment:1 Changed 3 years ago by rjollos

  • Description modified (diff)
  • Owner changed from gruenebe to rjollos
  • Status changed from new to assigned

comment:2 Changed 3 years ago by rjollos

  • Resolution set to fixed
  • Status changed from assigned to 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: Changed 3 years ago by hasienda

  • Keywords cleanup added

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.

comment:4 in reply to: ↑ 3 Changed 3 years ago by rjollos

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.

comment:5 follow-up: Changed 3 years ago by rjollos

  • Cc osimons added

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?

comment:6 in reply to: ↑ 5 ; follow-up: Changed 3 years ago by hasienda

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.

comment:7 Changed 3 years ago by rjollos

(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: Changed 3 years ago by rjollos

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.

comment:9 Changed 3 years ago by rjollos

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

comment:10 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by 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 ...

comment:11 Changed 3 years ago by rjollos

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

comment:12 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by hasienda

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.

comment:13 in reply to: ↑ 12 Changed 3 years ago by rjollos

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!

comment:14 Changed 3 years ago by rjollos

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.

Add Comment

Modify Ticket

Action
as closed The owner will remain rjollos.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.