Edgewall Software
Modify

Opened 11 years ago

Closed 8 years ago

Last modified 8 years ago

#7418 closed defect (fixed)

[PATCH] Active navigation items are broken in Clearsilver templates

Reported by: anonymous Owned by: Christian Boos
Priority: low Milestone: 0.12.3
Component: general Version: 0.11-stable
Severity: normal Keywords: clearsilver navigation
Cc: Branch:
Release Notes:
API Changes:

Description

It looks like changes made to move templates away from Clearsilver have introduced a regression where active navigation items are not highlighted properly.

To reproduce, use a plugin that returns a Clearsilver template from its process_request() method. You will see that the "mainnav" navigation bar does not render the active item highlighted.

The bug is that the CSS class "active" is not assigned to the list element in the navigation bar.

The fix is to change trac/web/chrome.py:

class Chrome(component)
...
    def populate_hdf(self, req):
    ...
        for category, items in req.chrome['nav'].items():
+           req.hdf['chrome.nav.%s' % category] = items
-           for item in items:
-               prefix = 'chrome.nav.%s.%s' % (category, item['name'])
-               req.hdf[prefix] = item['label']

And then change trac/templates/header.cs:

<?cs def:nav(items) ?><?cs
 if:len(items) ?><ul><?cs
  set:idx = 0 ?><?cs
  set:max = len(items) - 1 ?><?cs
  each:item = items ?><?cs
   set:first = idx == 0 ?><?cs
   set:last = idx == max ?><li<?cs
   if:first || last || item.active ?> class="<?cs
    if:item.active ?>active<?cs /if ?><?cs
    if:item.active && (first || last) ?> <?cs /if ?><?cs
    if:first ?>first<?cs /if ?><?cs
    if:(item.active || first) && last ?> <?cs /if ?><?cs
    if:last ?>last<?cs /if ?>"<?cs
+  /if ?>><?cs var:item.label ?></li><?cs
-  /if ?>><?cs var:item?></li><?cs
   set:idx = idx + 1 ?><?cs
  /each ?></ul><?cs
 /if ?><?cs
/def ?>

Attachments (1)

active_nativigation_fix.diff (1.1 KB ) - added by anonymous 11 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 by anonymous, 11 years ago

Milestone: 0.11.2

comment:2 by Christian Boos, 11 years ago

Keywords: clearsilver navigation added
Milestone: 0.11.20.11.3
Priority: normallow

Thanks for reminding us how ugly the ClearSilver templates were, sort of mitigates all the trouble we had with Genshi :P)

If anyone can make a clean patch (see TracDev/SubmittingPatches) and test the patch, we could eventually apply it.

by anonymous, 11 years ago

comment:3 by anonymous, 11 years ago

Summary: Active navigation items are broken in Clearsilver templates[PATCH] Active navigation items are broken in Clearsilver templates

Added a patch to fix this bug.

comment:4 by anonymous, 9 years ago

I tried to post this comment onto the SeaChange/WhatDevelopersWant page, but "Akismat" thinks it is spam.

Comment: Or look at #7418. A regression was introduced as part of the change in template systems, a patch was suggested as part of the bug report. While very "minor", it affected anyone using Clearsilver in plugins and could easily be applied and tested. That was almost 2 years ago! The only comment from a core developer is "provide a patch", which was almost immediately provided, even though the bug report had an inline diff provided for the fix. And now it is still waiting to be applied and released…

comment:5 by Christian Boos, 9 years ago

Milestone: next-minor-0.12.x
Resolution: wontfix
Status: newclosed

We almost never apply patches without testing them ourselves first.

At the time the patch was submitted, I no longer had a working setup with ClearSilver, nobody else from the team cared about it either. In retrospect, applying the patch anyway would probably have been very low risk, and worth doing it anyway. Maybe other factors were in play (a release was getting near, other more important fixes on the way, I can't remember).

Now it's certainly no longer worth applying it, as the next "change" we'll do in this area will be to remove the ClearSilver support completely.

comment:6 by Christian Boos, 9 years ago

As I wanted to add a few things to our patch guidelines, I found out that it was already written there: TracDev/SubmittingPatches#Whatisagoodpatch.

In particular, a patch is more like a prerequisite, not a sufficient condition.

comment:7 by FilipeCorreia, 8 years ago

I've just reproduced this on an instance of trac 0.12.2rc1, and solved it by applying the patch. So, I'd say this patch is still worth applying.

comment:8 by Christian Boos, 8 years ago

Milestone: 0.12.3
Resolution: wontfix
Status: closedreopened

Thanks for testing!

Note that ClearSilver support has indeed be removed now in newer versions of Trac, but as 0.12.x will therefore be the last version to support ClearSilver and that we got both a patch and testing feedback, let's finally apply this.

comment:9 by FilipeCorreia, 8 years ago

Great! Thanks.

comment:10 by Christian Boos, 8 years ago

Resolution: fixed
Status: reopenedclosed

Applied in r10568.

comment:11 by Christian Boos, 8 years ago

Owner: set to Christian Boos

As we have an anonymous patch contributor here, I'll take the "blame" for the ticket…

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.