[PATCH] Various fixes and enhancements

ough... I'm unable to upload the patch (rejected by spam filter)...

will get back to it once I find some place to upload and share.

patch against trunk at r10506

Successfully attached!

Referred tickets: #9060, #9061, #9062, #9063, #9064

Includes existing patches from: #8638,#3754

Wow, thanks. That's a bunch of improvements, really a master ticket.

I'll review this and certainly apply as a series of changes again. Nevertheless it's good to have code for all the single patches rebased on current trunk.

Thanks for breaking it into discrete issues. And I do especially appreciate your non-anonymous contributions. :-)

Nice - the plugin deserves som TLC :-) Looks good to me, and the wiki rename methods will of course also be compatible with 0.11 as the method will just exist but never be called.

One minor nit: Interface class declarations in Trac (and plugins) are always defined without adding self to the template methods. It has no real functional meaning, other than established practice to specify the essence of the API without implementation details + prevent that someone by accident makes an instance of the Interface class and tries to call it (if so it should fail).

Sorry for the anonymity... My only internet-access at the moment is the Black Hat WiFi? (security conference, so be sure the network is heavily monitored), and the t-h site is configured with basic authentication (password in cleartext), so I rather *not* login... :-)

Also sorry about all the self args. It bothered my coding style...

(In [10621]) TagsPlugin: Control default link destination in tags cloud, refs #3754 and #9059.

Known behavior to always link to an existing wiki page with name equal to the tag is still default, but configurable by new wiki_page_link boolean option.

(In [10622]) TagsPlugin: Cleanup of imports and other stuff, refs #9059.

Hide TagTemplateProvider providing the ITemplateProvider for being an abstract class, and remove mentioning of ClearSilver? templates. Old author email added to display something at Trac 0.11 plugin admin page.

(In [10624]) TagsPlugin: Adapt REGEXP for 1st title extraction to new Trac 0.12 wiki syntax, refs #9059 and #9062.

Report and fix have been contributed by Itamar Ostricher, thank you.

(In [10627]) TagsPlugin: Improve permission definition for more intuitive permission settings, refs #9058 and #9059.

Add a third permission TAGS_ADMIN for access to future 'tags' admin panel.

(In [10629]) TagsPlugin: Add deduplication capability to tags query, refs #9059 and #9060.

This has been tested to fix both, the tags query command line exposed in 'tags' main page as well as ListTagged() macro calls with 2 or more repetitions of the same resource realm.

Thanks - again to Itamar Ostricher - for test/report and proposed fix.

(In [10632]) TagsPlugin: Implement IWikiChangeListener interface for Trac 0.12, refs #8638 and #9059.

This work is based on code contributed/tested by Itamar Ostricher and sika.

Had a hard time myself to understand the consequences of wiki rename in Trac 0.12 i.e. for attachments, but now I favor this approach, even regarding naming conventions.

So I preferred reparent_resource_tags over rename_resource because the former is what we actually do while the latter has already been done, when IWikiChangeListener method wiki_page_renamed is triggered. Other details regarding the latest patch by Itamar have been discussed in #9059 before.

(In [10636]) TagsPlugin: A spurious self argument slipped in into changeset [10632], refs #8638 and #9059.

As outlined by Odd (osimons) in comment 4 of #9059 the usual first argument self is omitted from Interface definitions intentionally; cite:

Interface class declarations in Trac (and plugins) are always defined without adding self to the template methods. It has no real functional meaning, other than established practice to specify the essence of the API without implementation details + prevent that someone by accident makes an instance of the Interface class and tries to call it (if so it should fail).

(In [10637]) TagsPlugin: Refactor author retrieval from tag change request, refs #9059 and #9061.

Code threw an AttributeError: username exception when attempting to change tags on tickets. Couldn't find another occurance of req.username in any other plugin nor in Trac core itself.

Turns out this flaw has been specific to the 0.6 release introduced by the first development code in changeset [2953] back in 2008 - wow. I stumbled upon this when attempting to implement tag mass replacement - a lucky catch. Lucky again for finding get_reporter_id, that has been avaiable since earliest days - 2004, and it works very well.

(In [10639]) TagsPlugin: Add tag mass replacement functionality, refs #9058, #9059 and #9061.

This is a modified version of code originally contributed by Itamar Ostricher. I felt like improving usability by replacing the input field for old tag(s) with a select box containing all currently defined tags, where even multiple selection and tag mass deletion is possible now.

Thank you for your truely inspiring work, Itamar.

(In [10661]) TagsPlugin: Provide configurable default list of realms to exclude from tags query, refs #9059 and #9063.

Administrators may configure tag realms to require explicite selection for every new tags query by adding them to a new ListOption 'exclude_realms' or 'listtagged_exclude_realms' for ListTagged macro calls respectively. The original suggestion has been contributed by Itamar Ostricher, thank you.

These changes include an additional workaround to get the expected result, even if you deselect all available realms. Otherwise this would have neutralized your setting for these options.

The remaining changes as referred to in #9064 are non-trivial again. I'm still struggling to get them right. And there are some more issues that got my attention along the way; will keep you updated as I progress.

Hope the results of my "patch-picking" do satisfy the extra waiting time. Thanks for your patience anyway.

As denoted in #9064 the final commit for this patch set is getting near.

Any more thoughts for next steps towards tags-0.7? I'll try to keep at least the current pace and go through some more tickets assigned to this plugin that I feel the need to resolve before declaring next stable release. Advice/guidance on this is always welcome. Just comment existing tickets appropriately, please.

Probably one of the most pressing, but undocumented issues is backwards-compatibility down to Trac 0.11, that has been lost for trunk due to my earlier i18n work IIRC.

Replying to hasienda:

Probably one of the most pressing, but undocumented issues is backwards-compatibility down to Trac 0.11, that has been lost for trunk due to my earlier i18n work IIRC.

That is not ideal. Ideally the same code should work for both 0.11 and 0.12 as there is no branch alternative either. Making various compat & noop tweaks should not be so hard - it is what I try to do for all my plugins.

A start would be to wire up unit tests for the plugin. It really is somewhat overdue... I can just commit this if you'd like.

Replying to osimons:

Replying to hasienda:

[...] backwards-compatibility down to Trac 0.11, that has been lost for trunk due to my earlier i18n work IIRC.

That is not ideal. Ideally the same code should work for both 0.11 and 0.12 as there is no branch alternative either. Making various compat & noop tweaks should not be so hard - it is what I try to do for all my plugins.

I know. But at the time of first implementation I didn't know better and there was no 'best practice' to follow. And I had no own 0.11 test install to check on my own. All this changed and I've already found ways around the various issues for AccountManagerPlugin before. So this is just checking for possible similar glitches and fixing them by applying similar code changes (at least in theory). I'm not afraid of this. Can do that, as my time permits.

A start would be to wire up unit tests for the plugin. It really is somewhat overdue... I can just commit this if you'd like.

Took me some time to understand the strong points of a solid unittest framework with good coverage. Yes, feel free to go ahead with #9194, please. I'll certainly watch your steps to learn more for myself. There are many more plugins that lack any systematic code testing.

(In [10680]) TagsPlugin: Provide default sets of two new options for both, ListTagged and /tags handler, refs #9059 and #9064.

The TagQuery result presentation is now configurable. Supported values for the format keyword argument are:

• oldlist - list in known ListTagged style
• compact - list with results shortend to just the description
• table - table view similar to TicketQuery with 'table' option

and for 'table' view columns can be selected with the cols argument from a

frozenset(['realm', 'id', 'description', 'tags'])

separating multiple values by pipe character ('|').

While these are great new features I wanted myself before, it would not have happend by now without the contribution from Itamar Ostricher. Thanks a bunch.

(In [10681]) TagsPlugin: Fix system-message-like banner insert, refs #9059.

First, cleanup superfluous part of a REGEXP. But it turned out, that adding the message while already rendering doesn't work, not even for wiki pages. So I've changed the whole concept for message insertion.

(In [10682]) TagsPlugin: Rework TicketTagProvider, refs #9059.

With i18n there's no point in hard-coding labels for tagged resource display. It's not sensible to reinvent the wheel too, so describe_tagged_resource method for tickets just calls the corresponding module in Trac core, and we'll get good localization/translations of labels for free.

This is the final part of bundled suggestions contributed by Itamar Ostricher. Thank you for all the invaluable ground work and inspiration.

(In [10683]) TagsPlugin: Start tracking changes more visibly, i.e. to serve summary on releases, refs #9059.

Adding changes since tractags-0.6 to prepare for next release, but regarding earlier plugin versions this is merely a stub. Please correct, if you know better.

Replying to hasienda:

Probably one of the most pressing, but undocumented issues is backwards-compatibility down to Trac 0.11, that has been lost for trunk due to my earlier i18n work IIRC.

I'm running Trac 0.11.7 and r10671 of the TagsPlugin is working well. However, following [10680] I can no longer run the latest version of the TagsPlugin trunk; I get errors about the inability to import ChoiceOption. That class doesn't look familiar to me so I'm assuming it must have been added in 0.12 or later.

2011-09-25 19:06:50,685 Trac[loader] ERROR: Skipping "tractags = tractags":
(can't import "ImportError: cannot import name ChoiceOption")

It causes the test suite to crash for 0.11.x runs too. Perhaps you should make a virtualenv with Trac 0.11.x? No need to configure web server or things like that, just install Babel, Genshi and Trac and you'll have a python to use for running tests...

Thanks for the feedback. Sure, I've not checked this before, because I've been quite sure there would be some compatibility issues.

Step-by-step, first the original patches reworked, now the backwards compatibility. I've already created some virtualenv's for Trac 0.11 and 0.11.8 to check other plugins, so will resolve this soonish. Just felt like everything at once would have been too much for me, sorry.

This specific option has been added late in Trac 0.12 development, near release time. I could create an import from future. Fairly easy to implement as additional code handling the ImportError. But worth the hassle? I think NO, but KISS instead:

The plugin will not be 'degraded' much, if we resort to ListOption instead. ChoiceOption? or not, as discussed in #9064 before, code is currently designed to behave the same as with value 'oldlist' in case of an invalid value, so I can't see how one would mess up anything with this option anyway. We just loose the added functionality of reporting back valid options.

Replying to hasienda:

Thanks for the feedback. Sure, I've not checked this before, because I've been quite sure there would be some compatibility issues.

No problem on my end ... happy to test whatever you push to the trunk. Worst case, I just roll back to a previous version. I wish I could contribute more in terms of attempted patches, but I seem to have no free time lately ... at least until my company gets its product out and I finish up this other contract job.

Is your plan to support the last two versions of Trac? ... i.e. when Trac 0.13 is released, drop support for 0.11?

Replying to rjollos:

No problem on my end ... happy to test whatever you push to the trunk. Worst case, I just roll back to a previous version. [...]

Ok, I'll push something out soon. Feels good to have some faithful (close) followers. :-)

Is your plan to support the last two versions of Trac? ... i.e. when Trac 0.13 is released, drop support for 0.11?

Not yet. As stated at other occasions, the effort to keep compatibility including Trac versions down to 0.11 is reasonable regarding seemingly widespread use. Not forcing Trac upgrade, just to get latest improvements of plugins should be general policy - at least I think that way.

And Trac 0.13 doesn't break 0.11 stuff by now, AFAIK, so I think the next branch will be beyond 0.13, but than for sure at least to no longer ignore @with_transaction & Co. and get rid of the old db handling, more capable context/formatting code, maybe more...

(In [10707]) TagsPlugin: Withdraw ChoiceOption for Trac 0.11 backwards-compatibility, refs #9059.

(In [10708]) TagsPlugin: Provide fallback for non-existing TicketSystem? attribute, refs #9059.

The previous change was just enought to uncover another issue, this time related to a new Trac 0.12 attribute. But seems not hard to fix that as well.

I'm running r10708 on Trac 0.11.7 and it seems to be all working correctly.

(In [13235]) TagsPlugin: Prevent TypeError for ListTagged macros without arguments, refs #9059.

Only [[ListTagged()]] worked before, now [[ListTagged]] does as well. This regression was introduced in [10681].