Opened 13 years ago
Closed 11 years ago
#9059 closed enhancement (fixed)
[PATCH] Various fixes and enhancements
Reported by: | anonymous | Owned by: | Steffen Hoffmann |
---|---|---|---|
Priority: | normal | Component: | TagsPlugin |
Severity: | normal | Keywords: | admin configuration option query presentation i18n |
Cc: | Itamar Oren, Ryan J Ollos, Michael Renzmann | Trac Release: | 0.12 |
Description
One ticket for the patch.
Additional tickets to be created for each specific fix / enhancement.
Attachments (1)
Change History (34)
comment:1 Changed 13 years ago by
Changed 13 years ago by
Attachment: | tagsplugin-misc-enhancements.patch added |
---|
patch against trunk at r10506
comment:2 Changed 13 years ago by
Cc: | Itamar Oren added; anonymous removed |
---|---|
Trac Release: | 0.11 → 0.12 |
comment:3 Changed 13 years ago by
Cc: | Ryan J Ollos Michael Renzmann added |
---|---|
Status: | new → assigned |
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. :-)
comment:4 Changed 13 years ago by
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).
comment:5 Changed 13 years ago by
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...
comment:6 Changed 13 years ago by
(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.
comment:7 Changed 13 years ago by
(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.
comment:8 Changed 13 years ago by
(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.
comment:9 Changed 13 years ago by
(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.
comment:10 Changed 13 years ago by
(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.
comment:11 Changed 13 years ago by
(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.
comment:12 Changed 13 years ago by
(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).
comment:13 Changed 13 years ago by
(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.
comment:14 Changed 13 years ago by
(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.
comment:15 Changed 13 years ago by
(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.
comment:16 Changed 13 years ago by
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.
comment:17 follow-ups: 18 24 Changed 13 years ago by
Keywords: | admin configuration option query presentation i18n added |
---|
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.
comment:18 follow-up: 19 Changed 13 years ago by
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.
comment:19 Changed 13 years ago by
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.
comment:20 Changed 13 years ago by
(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.
comment:21 Changed 13 years ago by
(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.
comment:22 Changed 13 years ago by
(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.
comment:23 Changed 13 years ago by
(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.
comment:24 Changed 13 years ago by
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")
comment:25 Changed 13 years ago by
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...
comment:26 follow-up: 27 Changed 13 years ago by
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.
comment:27 follow-up: 28 Changed 13 years ago by
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?
comment:28 Changed 13 years ago by
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...
comment:29 Changed 13 years ago by
(In [10707]) TagsPlugin: Withdraw ChoiceOption for Trac 0.11 backwards-compatibility, refs #9059.
comment:30 Changed 13 years ago by
(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.
comment:31 Changed 13 years ago by
I'm running r10708 on Trac 0.11.7 and it seems to be all working correctly.
comment:32 Changed 11 years ago by
(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].
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.