[PATCH] TitleIndex fixes for hierarchy format

[Michel Jouvin <jouvin@…>]

I saw the new capabilities of TitleIndex macro in r8995 and decided to merge some related features I added a while ago in TOCMacro. I really like the new hierarchy/group format but in our site we use quite extensively the ability of TOCMacro to display the first level heading in addition to the page name. The attached patch adds the same capability to TitleIndex, if show_title option is specified. Default is backward compatible (show_title=false).

[Michel Jouvin <jouvin@…>]

Patch based on wiki/macros.py r8995

[Michel Jouvin <jouvin@…>]

A page demonstrating the new feature can be seen at ​https://trac.lal.in2p3.fr/QWG/wiki/Doc/TitleIndex.

[Ryan Ollos <ryano@…>]

I like the feature and would use it on my Trac installation after I move to 0.12. Thanks!

[Michel Jouvin <jouvin@…>]

BTW, I found a bug in the base implementation (unmodified) of TitleIndex in r8995 with format=hierarchy. The way the group information is built, there is no way to track the page name associated with a hierarchy level. This generates buggy anchors (as hierarchy format always generates anchors for hierarchy level) linking to unexisting pages for all the levels except the first one. I attached a second patch fixing this problem (using a tuple for 3 elements rather than 2 for tracking group information). With this patch, the hierarchy level name is displayed as an anchor only if there is a page associated with this name (its perfectly valid not to have a page matching a hierarchy level, even though TracNav-like hierarchy management tends to enforce it).

[Michel Jouvin <jouvin@…>]

Patch fixing bug described in #comment2

[Michel Jouvin <jouvin@…>]

I should have mentionned that the URL mentionned below (​https://trac.lal.in2p3.fr/QWG/wiki/Doc/TitleIndex) illustrates the new behaviour. Look for LCG2, scripts.

[Michel Jouvin <jouvin@…>]

Sorry for these incomplete comments… I also forgot to mention that I intentionnally kept the current behaviour with format=group where the group name is never an anchor, even if there is an existing page matching the name of the group. In fact the same as with hierarchy could be done (just copy/paste the corresponding lines) but I am not completely sure I understood the intent of group format and with the test I made, I had the feeling it was rather confusing at the end… If the proposed patch is accepted, I let the original authors decide what should be done for group…

[Christian Boos]

Patches look good, I'll test and integrate them. The def write(self, *args): pass is a bit of a hack and shouldn't be needed if the OutlineFormatter do the same thing with out as the other formatters.

The intent of the group format was to be able to introduce a hierarchical structure even in the absence of "/" in page names, by grouping pages sharing the same prefixes. For example, the default set of wiki pages will be structured in "Trac" and "Wiki" groups. If there are nevertheless pages containing "/", then they will also be organized in the same hierarchy (possibly merging things like BlahOne with Blah/Two).

The intent of the hierarchy format is to use only the "/" separator to determine the structure of the hierarchy.

[Michel Jouvin <jouvin@…>]

I let you do the cleanup with the write method, this hack is coming from TOCMacro and looking at the OutlineFormatter it was not clear what else I could do but I was unhappy to do it this way…

group/hierarchy difference as you described it is what I understood, great! I let you decide if the group format could benefit from the algorithm used in hierarchy to decide if an anchor must be generated.

[Christian Boos]

I think I'll postpone the extraction of the title (in a way compatible with #2717 and probably by extending the wiki data model so that the title is stored as a property).

But I'll try to get the bugfix for the issue raised comment:3.

[Christian Boos]

An updated patch with just the bugfix would be wonderful ;-)

[Michel Jouvin <jouvin@…>]

Replying to cboos:

An updated patch with just the bugfix would be wonderful ;-)

I'm afraid it is difficult as the bug was discovered after the initial patch and the fix partially relies on the patch providing the new feature. It's certainly possible to rewrite the fix without the new feature but this basically means reimplement it when the the new feature is backward compatible (thus harmless) and may interest many people I guess…

Michel

[mark.m.mcmahon@…]

For 0.12 should we just update the code so that format=hierarchy works similar to format=group. Where the 'parent' items aren't links. The only difference between group and hierarchy would be that group has the path in each link while hierarchy has just the page name.

Please find two patches:

• MinimalHierarchyChange.patch - just disables linking of 'parent' items
• FullHierarchyChange.patch - disables linking of 'parent' items only if the page doesn't exist.

I recommend the minimal patch and not full patch, as these pages are out of the structure being generated anyway!

[mark.m.mcmahon@…]

minimal patch to macros.py to disable links in hierarchy

[mark.m.mcmahon@…]

patch to macros.py to disable links incorrect in hierarchy format

[Michel Jouvin <jouvin@…>]

I personally don't really understand why you want a patch to disable an interesting feature rather than fixing it. My patch in fact was backward compatible AFAIK and adding new features that may be of interest for others… I understand that the proposed implementation for #2717 will make the things easier… but this ticket is already 4 years old! My implementation provides an acceptable workaround until we can take advantage of the title being of formatter property. This will simplify the code but should not lead to backward compatibility issues.

But this is up to you to decide…

Michel

[mark.m.mcmahon@…]

Hi Michel,

I found these defect because while in hierarchy mode some of the links were wrong. I didn't care too much about the titles either way at that point.

A request was made to provide a patch that fixes the links only (without the title changes) so that's what I tried to do :)

[Christian Boos]

I have some patches there, need to finish this…

[Adrian Fritz <adrian@…>]

I think #9402 is a duplicate of this one.

[Thijs Triemstra]

Replying to Adrian Fritz <adrian@…>:

I think #9402 is a duplicate of this one.

Closed it, the ticket contained a test case.

[stuart.p.rossiter@…]

This seems the correct ticket for this (let me know if you'd prefer a new one). The TitleIndex macro in 0.12 has a lot of issues, especially with hierarchical structures. Hopefully this might also help partially clarify the problems that need fixing. (I don't know what Michel's patch fixes.)

See my attachment of a test page showing the issues. Basically:

a) Level N pages are included as a header AND incorrectly repeated in the list of sub-pages b) Using a trailing / in the page prefix to attempt to get round this results in broken formatting of page names with the / omitted

Hope this is of use. IMO, format=hierarchy indexing is broken without this fixed.

[Stuart Rossiter <stuart.p.rossiter@…>]

Screenshot of illustration of TitleIndex bugs

[Stuart Rossiter <stuart.p.rossiter@…>]

Oops, original attachment was incorrect. Reattached here​. (No deletion rights for the incorrect attachment; an admin can feel free to clean up.)

[Peter Suter]

Fix hierarchy format issues

[Peter Suter]

The above patch attempts to fix the issues with the hierarchy format. I guess it's similar to what's described in comment:3 in some ways.

The issue b) from comment:20 remains for the group format. I think it's because node = (key + subkey, subnodes) # FIXME can't cope with the rstrip('/') from split_pages_group.

[Peter Suter]

Allows folding groups

[Peter Suter]

The above patch adds another small feature to the TicketIndex macro in format=group mode: Adding another argument foldable makes all group headers foldable.

(Edit: slightly better would be else None instead of else '')

[Peter Suter]

I continued working on this in ​my clone, updating and adding a few more unit tests, and also fixing issue b) from comment:20.

[Christian Boos]

I tested the branch (at 4c96a6d80979/psuter), and it works great, especially for groups!

I've only detected a problem with format=hierarchy and the min parameter. First, the min is not correctly adjusted, it's off by one (e.g. min=6 should mean "there should be at least 6 sub-pages to form a group", otherwise the group is flattened; but actually 5 is enough). Second, when the group is not formed (< min or < min + 1 currently), the prefix is not added, which means the information shown for the page names is misleading.

I added a use case to your TITLEINDEX5_MACRO_TEST_CASES, i.e. with the additional pages:

• TracDev
• TracDev/ApiChanges
• TracDev/ApiChanges/0.10
• TracDev/ApiChanges/0.11
• TracDev/ApiChanges/0.12
• TracDev/ApiChanges/0.12/Missing/Exists

the call [[TitleIndex(format=hierarchy,min=6]] gives:

• TracDev
  • ApiChanges
  • 0.10
  • 0.11
  • 0.12
  • Exists
• WikiStart

and I think it should produce instead:

• TracDev
  • ApiChanges
  • ApiChanges/0.10
  • ApiChanges/0.11
  • ApiChanges/0.12
  • ApiChanges/0.12/Missing/Exists
• WikiStart

… but it's already much better than the current result which I don't even dare showing ;-)

Note that if the first problem is fixed, it should be min=5 here, i.e. at least 5 entries to form a group - here we have 4 pages below ApiChanges and I don't want them to form a group (but if currently I specify 5, I still have a group).

I just pushed the "final" test case that should cover both issues in 4dbf1db2b133/cboos.

[Peter Suter]

Replying to cboos:

I tested the branch (at 4c96a6d80979/psuter), and it works great, especially for groups!

Thanks for testing!

I've only detected a problem with format=hierarchy and the min parameter.

Ah, right. That combination didn't quite make sense to me.

First, the min is not correctly adjusted, it's off by one (e.g. min=6 should mean "there should be at least 6 sub-pages to form a group", otherwise the group is flattened; but actually 5 is enough).

I assumed it would be preferable when changing between format=group and format=hierarchy that no groups would (dis)appear. But switching back should be as easy as removing that + 1 again, IIRC. Edit: Should we reduce the default and minimum for min to 1 in this case?

Second, when the group is not formed (< min or < min + 1 currently), the prefix is not added, which means the information shown for the page names is misleading.

Right, that needs to be fixed.

I added a use case … [[TitleIndex(format=hierarchy,min=6]] … Note that if the first problem is fixed, it should be min=5 here … I just pushed the "final" test case that should cover both issues in 4dbf1db2b133/cboos.

So shouldn't the "final" test case say min=5? I also don't get the maxDiff = None line.

[Peter Suter]

How about ​d720c64f1570-d99913c6fe78?

[Christian Boos]

Replying to psuter:

How about ​d720c64f1570-d99913c6fe78?

Great work, perfect!

I've made a new ticket (#10523) for keeping track of the show_title idea of Michel, but this one can now be closed as a fixed defect. Btw, maybe this could even be rebased on 0.12-stable…

[Christian Boos]

Replying to cboos:

… Btw, maybe this could even be rebased on 0.12-stable…

Wow, hg rebase behaves very awkwardly here… :( (shouldn't it be hg rebase -s 4f1c3db9b2da -d de42c6d08434 --keep?).

Anyway, too many Python 2.5 specific idioms (which is good!). Nevermind people should just upgrade to 0.13dev ;-)

[Christian Boos]

Replying to psuter:

Should we reduce the default and minimum for min to 1 in this case?

Agreed (as you did in the last cset).

I also don't get the maxDiff = None line.

It's a Python 2.7 specific issue.

For a test failure, I'd get the following [truncated] test output (here I triggered a failure on purpose):

======================================================================
FAIL: test (trac.wiki.tests.formatter.WikiTestCase)
Test TitleIndex, hierarchy format with complex hierarchy
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\Trac\repos\trunk\trac\wiki\tests\formatter.py", line 201, in test
    % (msg, self.file, self.line, self.title, formatter.flavor))
AssertionError: u'<p>\n</p><dv class="titleindex"><ul><li><a href="/wiki/TracDev">TracDev</a><ul [truncated]... != u'<p>\n</p><div
class="titleindex"><ul><li><a href="/wiki/TracDev">TracDev</a><u [truncated]...
Diff is 1026 characters long. Set self.maxDiff to None to see it.

c:\Trac\repos\trunk\trac\wiki\tests\macros.py:1: "TitleIndex, hierarchy format with complex hierarchy" (default flavor)

----------------------------------------------------------------------

By adding maxDiff = None the message contains the full diff… but it's far from optimal anyway, as unittest produces its own "diff" which is less nice than our custom "diff". I've committed a better fix in r10904.

[Peter Suter]

Replying to cboos:

Replying to cboos:

… Btw, maybe this could even be rebased on 0.12-stable…

Wow, hg rebase behaves very awkwardly here… :( (shouldn't it be hg rebase -s 4f1c3db9b2da -d de42c6d08434 --keep?).

I'm not sure about the command line details. I think I got it eventually with TortoiseHG…

Anyway, too many Python 2.5 specific idioms (which is good!). Nevermind people should just upgrade to 0.13dev ;-)

…but I'm not really keen on getting Python 2.4 SQLite running. Is there a lot more to it than replacing if then expressions with and or?

[Christian Boos]

Replying to psuter:

Replying to cboos:

Replying to cboos:

… Btw, maybe this could even be rebased on 0.12-stable…

Wow, hg rebase behaves very awkwardly here… :( (shouldn't it be hg rebase -s 4f1c3db9b2da -d de42c6d08434 --keep?).

I'm not sure about the command line details. I think I got it eventually with TortoiseHG…

I wonder then if thg used MQ or pure rebase…

Anyway, too many Python 2.5 specific idioms (which is good!). Nevermind people should just upgrade to 0.13dev ;-)

…but I'm not really keen on getting Python 2.4 SQLite running. Is there a lot more to it than replacing if then expressions with and or?

I just went through the patch set again, and that must be the only thing… so if you feel like rebasing it, I'm OK to test it (should reinstall 2.4.x and test before releasing 0.12.3 anyway).

[Peter Suter]

Replying to cboos:

I just went through the patch set again, and that must be the only thing… so if you feel like rebasing it, I'm OK to test it (should reinstall 2.4.x and test before releasing 0.12.3 anyway).

Thanks. Here's what I got: [c1fcc2ad2ec1/psuter]

[Christian Boos]

Replying to psuter:

Replying to cboos:

I just went through the patch set again, and that must be the only thing… so if you feel like rebasing it, I'm OK to test it (should reinstall 2.4.x and test before releasing 0.12.3 anyway).

Thanks. Here's what I got: [c1fcc2ad2ec1/psuter]

Great! Tested with 2.4.6 on Linux, seems to work fine. Please commit it on 0.12-stable (but keep the if ... else version for trunk!).

[Peter Suter]

Replying to cboos:

I've made a new ticket (#10523) for keeping track of the show_title idea of Michel

Changed summary to reflect this.

[Peter Suter]

Applied in [10908] for 0.12 (merged to trunk in [10909]).