[Patch] The `helppage` field should transform the Keywords field into a link to an arbitrarily defined wiki page

I have actually made this enhancement. It doesn't specifically handle the case if the keywords = field is left blank, as I actually use it. however I will look at that modification also. It is using trac 11.5 with the patch in #4201 attached on 12/04/08

It is literally 5 characters different: on line 73 of keywordsuggest.py:

-                link = tag.a(href='%s' % helppage, target='blank')
+                link = tag.a(href='wiki/%s' % helppage, target='blank')

Then, in my trac.ini file, my helppage link is:

[keywordsuggest]
helppage = tags
keywords = tag1,tag2

this is line 73 of the merged patch to use the tags in keywordsuggest patch in #4201 (I used the whole file replacement 5.7kb option)

I don't think it's enough of a change to make a patch file at this point, but if I address the other issue, I will.

Question is, does this change belong with the tags patch in #4201, or with the main plugin code? I did it this way, so I could, instead if so desired, use a global keyword help page, since I am set up in a multi-environment trac configuration, and they all use the same basic keyword model. The downside, I suppose is that if you do want a wiki page, you have to use wiki/keywords or similiar, not a terrible burden

Replying to yoheeb@gmail.com:

Then, in my trac.ini file, my helppage link is:
[keywordsuggest]
helppage = tags
keywords = tag1,tag2

CORRECTION: sorry, I made a typo:
the helpage entry is:

• /ParentEnv/ProjectPage/tags

since I am multi-env and don't have all my paths set up. The above worked in a scenario where I had some funky apache stuff going on. Sorry about that, it should be:

[keywordsuggest]
helppage = /ParentEnv/ProjectPage/tags
keywords = tag1,tag2

Replying to yoheeb@gmail.com: I am not sure why the diff in the original looks like that (I just pasted) but I think it should be reversed (just get rid of wiki/). Either that or I need more coffee!:

-                link = tag.a(href='wiki/%s' % helppage, target='blank')
+                link = tag.a(href='%s' % helppage, target='blank')

I'll go ahead and make this change pending feedback on #8156.

I have a (IMHO) better version of the patch, which works even when Trac is not at the root of the virtualhost. It uses Trac's own resource-to-link converter. I'll attach it to this ticket.

Replying to abompard:

I have a (IMHO) better version of the patch, which works even when Trac is not at the root of the virtualhost. It uses Trac's own resource-to-link converter. I'll attach it to this ticket.

It looks like your patch wouldn't allow, e.g. tags to be specified for the help page, since it is not in the wiki realm. If you take a look at the patch I'm about the commit and have any suggestions for improvement, I'll follow-up.

(In [10936]) Fixes #5907: Version 0.4.1: A regression in the 0.4 version of the plugin prevented the helppage link from displaying when helppage.newwindow was False.

The helppage link can now be specified as a resource in any realm, not just a wiki page.

Replying to rjollos:

The helppage link can now be specified as a resource in any realm, not just a wiki page.

True. This has actually been started in [10431] by removing code to always add the mandatory 'wiki/' prefix. I suggest to treat this as a regression bug on it's own, because tickets claiming this are foreseeable, right?

Still I like more flexibility and existing code rules over just wild ideas. So consider the following patch for improving the situation.

some improvements over current, backwards-incompatible state of handling helppage option

Replying to hasienda:

True. This has actually been started in [10431] by removing code to always add the mandatory 'wiki/' prefix. I suggest to treat this as a regression bug on it's own, because tickets claiming this are foreseeable, right? Still I like more flexibility and existing code rules over just wild ideas. So consider the following patch for improving the situation.

Agreed. Thank you for your attention to backward compatibility and for adding additional functionality.

I'd like to extract the code from your patch into a private method __get_helppage_link to allow for unit testing in the future, and to keep the main filter_steam method concise and readable. This is the programming approach I'd take in general, but given your much stronger knowledge of Python and Trac, I wanted to check with you before making this change.

Replying to rjollos:

Agreed. Thank you for your attention to backward compatibility and for adding additional functionality.

You're welcome. Glad you like it.

I'd like to extract the code from your patch into a private method __get_helppage_link ... but ... I wanted to check with you before making this change.

Sure, valid move, even more after my proposed extensions. But let me go ahead and attach the current patch as another preliminary development state:

• changed order of processing to exclude any (explicitly or assumed) wiki resource from being checked - rationale: I'd like to be able to create the configured wiki page, if not existent.
• idea salvaged from abompard's patch: possibility to use an anchor (wiki-only) - rationale: Special care must be taken to prevent it from getting urlencoded inside the Href object.
• interpreting whole string as wiki page ID is an intentional fallback, if resource validation fails, because
  1. the resource realm is unknown or
  2. the resource with the given ID doesn't exist in that realm

Please discuss, if anything is questionable or undesired from your point of view.

I'll create the private method now and attach a third patch for review.

another preliminary development state with some improvements

Replying to hasienda:

Please discuss, if anything is questionable or undesired from your point of view.

Only thing that jumps out initially is that the first 2 if statements might be transformed to an if-elif, dropping the if not link check on the second.

Replying to rjollos:

Only thing that jumps out initially is that the first 2 if statements might be transformed to an if-elif, dropping the if not link check on the second.

That is, unless req.href can return None, but I'm not familiar with a condition in which this can occur.

Replying to rjollos:

Replying to rjollos:

Only thing that jumps out initially is that the first 2 if statements might be transformed to an if-elif, dropping the if not link check on the second.

That is, unless req.href can return None, but I'm not familiar with a condition in which this can occur.

Me too, but I can't map your comment to current code now. Maybe it's just too late, anyway, let me drop in the rework towards a private method.

Patch has been tested on Trac 0.11 and 0.13dev-r10880, so I noticed another issue: recently added javascript_quote from trac.util.text is not available before Trac 0.11.3 . Still like full backwards-compatibility? Suggestion included in the following patch.

more improvements: private method _get_helppage_link and backwards-incompatibility down to Trac 0.11

Forgot: Please make sure to include explicit file encoding ('utf-8') as suggested in latest patch, thanks.

Replying to hasienda:

Patch has been tested on Trac 0.11 and 0.13dev-r10880, so I noticed another issue: recently added javascript_quote from trac.util.text is not available before Trac 0.11.3 . Still like full backwards-compatibility? Suggestion included in the following patch.

Certainly. I'll take the code since your have generously provided it, though I probably wouldn't have gone out of my way to do it myself up until now.

I've been developing on 0.11.7 and testing on 0.12.2, which I had felt gave me sufficient coverage to satisfy most users. I have a dream of setting up automated builds to test installation and perhaps run unit tests against a greater range of versions, but I'm a ways off from that. Until then, I might modify my approach and start developing against 0.11.0, like you are doing, at least for plugins that have been around quite a while such as this one.

A note of caution: 0.13dev has great auto-preview like wiki side-by-side editor in tickets, now not only for comments but all fields.

But this preview interferes with plugin JS code, because it uses redirection internally. So I can't see the drop-down list anymore. Seen functional regression in other plugins as well, i.e. TracTicketTemplatePlugin.

Will investigate and most probably open another ticket dedicated to 0.13 compatibility - no need to do that in 0.11 branch I guess.

Replying to rjollos:

... I have a dream of setting up automated builds to test installation and perhaps run unit tests against a greater range of versions, but I'm a ways off from that.

Me too, once again.

<wild-dream> Super-cool, if this could even become a centralized service a-la ci@trac-hacks.org, to provide independent insight into code quality with a few mouse clicks.</wild-dream>

Replying to hasienda:

Will investigate and most probably open another ticket dedicated to 0.13 compatibility - no need to do that in 0.11 branch I guess.

Yeah, that is definitely appreciated. I'm actually surprised I didn't destroy 0.11 compatibility with the jQuery UI changes made in #8141. After I made those changes, I was thinking this would be the case, and I'd have to rewind some changes on the 0.11 branch and start a trunk version or at least a 0.12 branch. I did break compatibility with the AutocompleteUsersPlugin. I intend to fix this issue in #9599.

This never seems to get high enough on my priority list lately for me to test and commit the patch. Steffen, if convenient for you, feel free to push your patch straight to the repository. You have repository-wide commit access now so this is possible, and I trust your work!

Ok, will take care, but IIRC you did some change in-between, so I'll have to adapt/test/re-validate.

Splitting the latest path in two should do nicely: Add compat-function, and in another step add the flexible option parser code. Thanks for the thumbs-up.

(In [11201]) KeywordSuggestPlugin: Check helppage config option for valid realm ID, refs #5907.

Try Trac's resource system to create appropriate links to any existing resource. We gain backwards-compatibility by continuing to assume a wiki resource, if options value has no leading /.

Replying to hasienda:

Ok, will take care, but IIRC you did some change in-between, so I'll have to adapt/test/re-validate. Splitting the latest path in two should do nicely: Add compat-function, and in another step add the flexible option parser code. Thanks for the thumbs-up.

Done, so we have all that I was speaking about in another comment before. Now it is a very versatile, backwards-compatible option and allows for virtually any possible customization to the 'Keywords' labeled link. Feedback welcome.