Update BreadCrumbsNavPlugin to support Trac 0.12

[hansfn@…]

Please apply the patch from penmark found on the bottom of the BreadCrumbsNavPlugin page.

[Steffen Hoffmann]

Thanks for the hint/nudging.

I plan to have a dedicated 0.13 branch for the new decorator based Trac database access code, that would break precious backwards-compatibility on the 0.11 branch.

Other non-intrusive parts of the patch will be applied to trunk development branch soon.

Side-note: Please withstand common laziness to create all-in-one patches, but do cumulative patches in patch stacks for all but the most trivial changes. Doing cherry-picking in such patch files like penmark's tend to be a real pain to the maintainer, thus diminishing the value of the contribution and reputation of the patch author unnecessarily. Still any contribution is appreciated for sure.

[Steffen Hoffmann]

After all this is a suggested improvement, right?

[Steffen Hoffmann]

(In [10892]) BreadCrumbsNavPlugin: Add configurable default list label, refs #7231 and #7790.

This modified version of the change suggested by anonymous reporter of #7231 and addressed in penmark's patch will work better with coming i18n support, with the drawback, that the implicit default value isn't visible within the configuration system. At least tried to address this by extending option doc.

[Steffen Hoffmann]

patch for making breadcrumbsnavplugin use the trac resource system and the 0.12 database API - originally added to wiki:BreadCrumbsNavPlugin by penmark on 01-Sep-2010 17:56:39

[Steffen Hoffmann]

Not sure, if someone cares about this patch right now, anyway I'll comment to help resume later discussion:

One of my personal favorites in this patch(set) is the private method _break().

Use of __slots__ and doing most stuff with resource object rather than strings looks very good and is certainly beyond what I would have done without your contribution.

OTOH I'll retain current rendering of the list, i.e. hiding of the current resource while viewing it, or the way, a custom breadcrumbs list label is implemented.

The patch practically disables the old supported_paths option without comment or obvious reason. One might argue, that the new supported_realms option might replace it, but if so, why had the old option survived at all?

Maybe this is an unwanted inconsistency, but a closer look at both options reveals, that one isn't a duplicate of the other. While the supported_realms is identical, if you stick with default pattern/values of supported_paths, the latter might be used to filter resources beyond the realm. Consider

[breadcrumbs]
supported_paths = /wiki/PublicSpace/

So I tend to retain the old option for both, backwards-compatibility and advanced configuration and use supported_realms as a pre-filter. Hope, you don't mind, if I remove 'pastebin' from the default resource list, because this is most likely, what you use (I know about TracPastePlugin.), but not a standard resource for most Trac applications out there.

[Steffen Hoffmann]

Another note: Thanks for pointing at the obsolete import of parse_args, but import of resource_filename should be moved from the ITemplateProvider method to the top rather then removed from there.

[Steffen Hoffmann]

Two more thoughts:

Since more than six years Trac core IEnvironmentSetupParticipant methods provide the db connection as argument. So a fallback shouldn't be needed for them at all.

Method get_read_db is already depreciated in Trac 0.13dev in favour of db_query.

[Steffen Hoffmann]

I'm reluctant to break resource tracking for anonymous users as suggested by the patch, since I never saw a complaint about that, and this is definitely a feature that is expected in some applications today.

Speak-up please, if this would be really meaningful or is even required in your application, so that I could change my mind and introduce another option to switch it off. But with a growing number of configuration option for this plugin I feel, that feature creep is already just 'round the corner.

[Pontus Enmark]

I like the possibility to hide current page's crumb. I've been working a bit on this since (for use in my application), attaching a diff with my current version.

[Pontus Enmark]

Oh, sorry for not merging the latest version into my diff, I'll fix that.

[Steffen Hoffmann]

Replying to penmark:

I like the possibility to hide current page's crumb. I've been working a bit on this since (for use in my application), attaching a diff with my current version.

Glad to hear from you. Please bear with me, that I won't follow your latest changes instantly. I've already had a hard time to cherry-pick from your 1st diff, while retaining backwards-compatibility down to Trac 0.11 for now.

I'd love to see you adopting the cumulative patch stack approach instead of the big all-in-one diff, if you can afford the time. I require the incremental development approach for traceability of changes. It simply takes too much time (repeatedly!) to see what's going in form your big diff.

Anyway, i.e. "badcrumbs" is a nice idea, because meanwhile I was wondering about how to handle non-validating crumbs too. For crumbs list formatting I've found a less elaborated solution compared to yours with Genshi methods, but I'll look into it too.

I'd like to discuss various issues inside this topic a little more, as you and me see the need or interest to do so. I.e. I've been surprised to not find a dedicated resource manager for reports. Most probably there are even more, that we should take care of. I tend to merge in a workaround for these non-explicit resources as suggested in #4608.

Would be great, if you could review my commits as I push them out. Thanks in advance for taking care.

[Steffen Hoffmann]

Another development note:

This development work uncovered, that the get_resource_url method for the repository-related realms ('file', 'repository', 'source') has been broken in Trac core since it's introduction in ​t:r9125 for Trac 0.12dev, see ​t:#10466. Obviously, for now we can't rely on ​the fix, that has been accepted and applied to Trac trunk for 0.12.3 yesterday by osimons. So this plugin needs a workaround for mapping 'source' to 'browser' realm, and code similar to what has been suggested in #4608 for other cases.

To sum it up, I'm still convinced, that the move towards handling all crumbs as resource objects internally is right in the long run. But I would have postponed it, if I would have foreseen the implications. We have a fixed bug in Trac core on the positive side for this development effort, but still a lot of rather dirty hacking to make it work right now and for existing applications in the near future.

In fact, 0.11 is still rather common use, even for some new applications (see i.e. ​t:wiki:TracUsers). To be honest, I don't believe Trac 0.12.3 or even Trac 0.13 to supersede almost any application sticking to 0.11 right now within the next two years.

[Steffen Hoffmann]

If you look at files in the repository or at wiki page history and browse through several versions, the initial patch would produce multiple crumbs for the same resource (correctly), but get_resource_url would always produce the same link, because it's version-agnostic.

I consider this another bug (in Trac core) and even with only the aforementioned fix for repository resources targeted for Trac 0.12.3 won't fix this for wiki resources. So once more code is needed to (optionally?) clean-up crumbs for multiple versions/revisions of the same resource.

[Steffen Hoffmann]

Investing hours of testing as well as studies of documentation and discussion on ​char-set issues, ​performance, ​portability and security of serialization algorithms in Python I found, that the current cPickle approach is not able to cope with non-ascii chars (plus added hurdles for sensibility to cher-set settings of the Trac db backend), is rather insecure and slower than the alternatives.

Therefor I've decided to drop cPickle in favor of json, with the drawback, that for Python<2.6 this adds another external dependency from simplejson. Bear with me, please. I see no other way to make it happen, and the benefits should outweight the hassle of installing yet another python module.