Modify

Opened 3 years ago

Closed 8 months ago

#9636 closed enhancement (fixed)

[Patch] Exclude PageTemplates from tag-extraction

Reported by: itamarost Owned by: hasienda
Priority: normal Component: TagsPlugin
Severity: normal Keywords: template
Cc: AdrianFritz, luizfernando Trac Release: 0.11

Description

Ticket #3753 brought a long-awaited feature - propagation of tags from PageTemplates to the created page.

Unfortunately, the side effect of the selected solution is that TagsPlugin treats the pages under PageTemplates as any other tagged wiki resource, which means they are included in the tags cloud as well as in results of ListTagged macro.

At least for me, this is undesired, since the templates are "meta-content", and not actual content that I want to be treated the same.

My request is to add an option to exclude everything under PageTemplates from tags extraction.

Attachments (2)

tagsplugin-9636-itamaro-v1.patch (1.2 KB) - added by itamarost 3 years ago.
patch against tagsplugin trunk
tagsplugin-9636-itamaro-v2.patch (1.4 KB) - added by itamarost 3 years ago.
attempting to fix the tag-replace issue with previous patch

Download all attachments as: .zip

Change History (19)

comment:1 follow-up: Changed 3 years ago by hasienda

  • Keywords template added

Good catch, just what I noticed recently too.

+1 for fixing this. Suggestions, patch? :-)

Changed 3 years ago by itamarost

patch against tagsplugin trunk

comment:2 in reply to: ↑ 1 Changed 3 years ago by itamarost

  • Summary changed from Exclude PageTemplates from tag-extraction to [Patch] Exclude PageTemplates from tag-extraction

Replying to hasienda:

Good catch, just what I noticed recently too.

+1 for fixing this. Suggestions, patch? :-)

You ask so nicely, so I could not resist :-)

Patch attached, maybe somewhat dirty, as I add in api a specific test for the wiki realm, but it works for me :-)

On the way - the patch contains a fix for a possible issue my IDE warned me about (resource not defined in the context of the PermissionError exception).

comment:3 follow-up: Changed 3 years ago by hasienda

  • Status changed from new to assigned

While your fix for the undefined variable resource seems perfectly valid, I wonder how to alter the suggested change for the main issue.

Intercepting get_tagged_resources seems fine only on first look. But it would mask wiki templates for the admin tag replace feature too, and I seriously doubt that's desirable or one of your explicit intentions.

Changed 3 years ago by itamarost

attempting to fix the tag-replace issue with previous patch

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

Replying to hasienda:

Intercepting get_tagged_resources seems fine only on first look. But it would mask wiki templates for the admin tag replace feature too, and I seriously doubt that's desirable or one of your explicit intentions.

You're absolutely right. A second version of the patch tries to resolve this (quite a hack I guess, but couldn't think of a better approach).

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

Very well. That should work indeed.

Reason, why I don't apply this right-away is, that after starting to work on #4503 I've never let performance go out of sight. I guess that not retrieving data is better than filtering it out later, so I tend towards implementing a more sophisticated SQL statement inside get_tagged_resources that could be fed by a new optional filter argument. Just finalizing and testing this right now ...

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

Great. Good thing you're keeping an eye on performance - not quite my strong side :-)

That said, I'm not completely sure "more sophisticated SQL" to prevent data retrieval would be better performance-wise compared to dead-simple SQL with simple filtering out.

Using more sophisticated SQL just passes on the bottleneck the the SQL library, so it's not explicit.

Also I generally prefer readability and maintainability over performance when the effect is not considerable.

But, as I said - I don't know much about these things. Just my 2c.

comment:7 Changed 3 years ago by hasienda

(In [11105]) TagsPlugin: Fix undefined resource in PermissionError exception context, refs #9636.

This has been reported and corrected in KISS-style by Itamar Ostricher.

comment:8 Changed 2 years ago by AdrianFritz

Just to point a work-around:

  1. add another Tag like "PageTemplate" and
  2. then exclude it when using [[ListTagged(TagOfInterest1 TagOfInterest2 -PageTemplate)]] macro.

comment:9 follow-ups: Changed 2 years ago by fommil

  • Trac Release changed from 0.11 to 1.0

But hold on, if you add a tag to the PageTemplate then it will be propagated to new pages and therefore those pages will be excluded.

comment:10 in reply to: ↑ 9 Changed 2 years ago by hasienda

  • Trac Release changed from 1.0 to 0.11

Replying to fommil:

But hold on, if you add a tag to the PageTemplate then it will be propagated to new pages and therefore those pages will be excluded.

Indeed. Still I don't see a reason, why we shouldn't implement this in a way, that is usable for Trac-0.11 onwards, not just 1.0, right?

comment:11 in reply to: ↑ 6 Changed 2 years ago by hasienda

Replying to anonymous:

That said, I'm not completely sure "more sophisticated SQL" to prevent data retrieval would be better performance-wise compared to dead-simple SQL with simple filtering out. Using more sophisticated SQL just passes on the bottleneck the the SQL library, so it's not explicit.

I think I've found a valid answer testing with 30.000 auto-generated tickets. On my development system it took around 100-300 ms to pull the keywords from all tickets, but more than 3 seconds to check access permission on each one. Well, maybe a good share of the time has been constructing the Trac Resource objects for the check, but it remains questionable to me, if there is such a thing like "simple filtering". Only by using rather heavy recursion in the SQL statement I could imagine to hit db performance to the point, where post processing in Python could gain speed.

Also I generally prefer readability and maintainability over performance when the effect is not considerable.

The aforementioned Test environment takes 15 s or longer to render an all-tagged-resources list of these 30.000 tickets. It still takes about 6 s for each tag cloud render pass. Scaling this up to 500.000+ tickets know to sit in some other, real-world Trac applications, there is definitely a big point for performance. I've learned this and will not sleep relaxed before we've gained at least ~ 10 times of the current speed.

comment:12 Changed 2 years ago by hasienda

(In [12390]) TagsPlugin: Don't use tags on wiki templates for anything else than copying tags, refs #3753 and #9636.

Once again this has been reported by Itamar Ostricher. Thank you very much for nudging me to fix it.

comment:13 follow-up: Changed 2 years ago by AdrianFritz

I deduced from digging into source code, tags in wiki pages located under specific "sud-dir" will not be "tagged" anymore. Example:

  • from source def get_tagged_resources(req, tags=None, filter=None)
    • setting under [PageTemplates] section in trac.ini file filter = PageTemplates

Am I correct?

comment:14 in reply to: ↑ 9 Changed 2 years ago by AdrianFritz

Replying to fommil:

But hold on, if you add a tag to the PageTemplate then it will be propagated to new pages and therefore those pages will be excluded.

Yes, it propagates PageTemplates too. But still very handy (in our use case at least ).

comment:15 in reply to: ↑ 13 Changed 2 years ago by hasienda

Replying to AdrianFritz:

I deduced from digging into source code, tags in wiki pages located under specific "sud-dir" will not be "tagged" anymore. Example:

  • from source def get_tagged_resources(req, tags=None, filter=None)
    • setting under [PageTemplates] section in trac.ini file filter = PageTemplates

Am I correct?

Yes, tags on any page below PageTemplates (technically correct: sharing common path 'PageTemplates/') are excluded from 'all-realm' or 'all-Trac' tag lists, but a per-resource query still shows them, as does the display of the wiki page view or a corresponding field in wiki page editor.

comment:16 in reply to: ↑ 9 Changed 2 years ago by hasienda

Replying to fommil:

But hold on, if you add a tag to the PageTemplate then it will be propagated to new pages and therefore those pages will be excluded.

Yes, so I felt urged to implement the better approach: The new (implicit) default configuration keeps tag lists working and in-depended of such meta-tag hacks, that require user action to not exclude new pages from tag lists derived from the respective template as well.

comment:17 Changed 8 months ago by hasienda

  • Resolution set to fixed
  • Status changed from assigned to closed

In 13815:

TagsPlugin: Completing preparation for v0.7 release.

Availability of that code as stable, tagged release
closes #2429, #3359, #3610, #3624, #3677, #3754, #3864, #3947, #3983, #4078, #4277, #4503, #4799, #5523, #7787, #7857, #8638, #9057, #9058, #9059, #9060, #9061, #9062, #9063, #9149, #9210, #9521, #9630, #9636, #10032, #10416, #10636, #11096, #11147, #11152, #11274, #11302, #11658 and #11659.

Additionally there are some issues and enhancement requests showing progress,
but known to require more work to resolve them satisfactorily, specifically
refs #2804, #4200, #8747 and #9064.

Thanks to all contributors and followers, that enabled and encouraged a good
portion of this development work.

Add Comment

Modify Ticket

Action
as closed The owner will remain hasienda.
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.