Modify

Opened 4 years ago

Last modified 3 years ago

#7790 assigned enhancement

Update BreadCrumbsNavPlugin to support Trac 0.12

Reported by: hansfn@… Owned by: hasienda
Priority: normal Component: BreadCrumbsNavPlugin
Severity: normal Keywords: compatibility i18n
Cc: rjollos, penmark Trac Release: 0.12

Description

Please apply the patch from penmark found on the bottom of http://trac-hacks.org/wiki/BreadCrumbsNavPlugin

Attachments (2)

breadcrumbs.diff (8.8 KB) - added by hasienda 3 years ago.
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
breadcrumbs.2.diff (13.5 KB) - added by penmark 3 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by hasienda

  • Cc rjollos added; anonymous removed
  • Keywords compatibility i18n added
  • Owner changed from ixokai to hasienda
  • Status changed from new to assigned

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.

comment:2 Changed 3 years ago by hasienda

  • Type changed from defect to enhancement

After all this is a suggested improvement, right?

comment:3 Changed 3 years ago by hasienda

(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.

Changed 3 years ago by hasienda

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

comment:4 Changed 3 years ago by hasienda

  • Cc penmark added

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.

comment:5 Changed 3 years ago by hasienda

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.

comment:6 Changed 3 years ago by hasienda

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.

comment:7 Changed 3 years ago by hasienda

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.

comment:8 follow-up: Changed 3 years ago by 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.

Changed 3 years ago by penmark

comment:9 Changed 3 years ago by penmark

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

comment:10 in reply to: ↑ 8 Changed 3 years ago by hasienda

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.

comment:11 Changed 3 years ago by hasienda

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.

comment:12 Changed 3 years ago by hasienda

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.

comment:13 Changed 3 years ago by hasienda

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.

Add Comment

Modify Ticket

Action
as assigned The owner will remain hasienda.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.