Edgewall Software
Modify

Opened 12 years ago

Closed 10 years ago

#8925 closed enhancement (fixed)

[PATCH] TitleIndex fixes for hierarchy format

Reported by: Michel Jouvin <jouvin@…> Owned by: Peter Suter
Priority: normal Milestone: 0.12.3
Component: wiki system Version: 0.12dev
Severity: normal Keywords: titleindex
Cc: mark.m.mcmahon@…, Thijs Triemstra Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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

Attachments (7)

macros.py.patch (4.8 KB ) - added by Michel Jouvin <jouvin@…> 12 years ago.
Patch based on wiki/macros.py r8995
macros.py.patch.2 (6.2 KB ) - added by Michel Jouvin <jouvin@…> 12 years ago.
Patch fixing bug described in #comment2
MinimalHierarchyChange.patch (624 bytes ) - added by mark.m.mcmahon@… 12 years ago.
minimal patch to macros.py to disable links in hierarchy
FullHierarchyChange.patch (1.9 KB ) - added by mark.m.mcmahon@… 12 years ago.
patch to macros.py to disable links incorrect in hierarchy format
tracBug1.2.png (35.3 KB ) - added by Stuart Rossiter <stuart.p.rossiter@…> 10 years ago.
Screenshot of illustration of TitleIndex bugs
tree_hierarchy.patch (5.1 KB ) - added by Peter Suter 10 years ago.
Fix hierarchy format issues
foldable_group.patch (1.5 KB ) - added by Peter Suter 10 years ago.
Allows folding groups

Download all attachments as: .zip

Change History (43)

by Michel Jouvin <jouvin@…>, 12 years ago

Attachment: macros.py.patch added

Patch based on wiki/macros.py r8995

comment:1 by Michel Jouvin <jouvin@…>, 12 years ago

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

comment:2 by Ryan Ollos <ryano@…>, 12 years ago

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

comment:3 by Michel Jouvin <jouvin@…>, 12 years ago

Summary: TitleIndex enhancement: add option 'show_title' to display first level headingTitleIndex fixes + enhancement (option 'show_title' to display first level heading)

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

by Michel Jouvin <jouvin@…>, 12 years ago

Attachment: macros.py.patch.2 added

Patch fixing bug described in #comment2

comment:4 by Michel Jouvin <jouvin@…>, 12 years ago

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.

comment:5 by Michel Jouvin <jouvin@…>, 12 years ago

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

comment:6 by Michel Jouvin <jouvin@…>, 12 years ago

Summary: TitleIndex fixes + enhancement (option 'show_title' to display first level heading)[PATCH] TitleIndex fixes + enhancement (option 'show_title' to display first level heading)

comment:7 by Christian Boos, 12 years ago

Keywords: titleindex added
Owner: set to Christian Boos
Version: 0.12dev

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.

comment:8 by Michel Jouvin <jouvin@…>, 12 years ago

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.

comment:9 by Christian Boos, 12 years ago

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.

comment:10 by Christian Boos, 12 years ago

Keywords: needpatch added
Milestone: 0.12next-minor-0.12.x

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

in reply to:  10 comment:11 by Michel Jouvin <jouvin@…>, 12 years ago

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

comment:12 by Christian Boos, 12 years ago

Milestone: next-minor-0.12.xnext-major-0.1X

comment:13 by mark.m.mcmahon@…, 12 years ago

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!

by mark.m.mcmahon@…, 12 years ago

minimal patch to macros.py to disable links in hierarchy

by mark.m.mcmahon@…, 12 years ago

Attachment: FullHierarchyChange.patch added

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

comment:14 by mark.m.mcmahon@…, 12 years ago

Cc: mark.m.mcmahon@… added

comment:15 by Michel Jouvin <jouvin@…>, 12 years ago

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

comment:16 by mark.m.mcmahon@…, 12 years ago

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 :)

comment:17 by Christian Boos, 11 years ago

Keywords: needpatch removed

I have some patches there, need to finish this…

comment:18 by Adrian Fritz <adrian@…>, 11 years ago

I think #9402 is a duplicate of this one.

in reply to:  18 comment:19 by Thijs Triemstra, 11 years ago

Cc: Thijs Triemstra added

Replying to Adrian Fritz <adrian@…>:

I think #9402 is a duplicate of this one.

Closed it, the ticket contained a test case.

comment:20 by stuart.p.rossiter@…, 10 years ago

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.

by Stuart Rossiter <stuart.p.rossiter@…>, 10 years ago

Attachment: tracBug1.2.png added

Screenshot of illustration of TitleIndex bugs

comment:21 by Stuart Rossiter <stuart.p.rossiter@…>, 10 years ago

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

by Peter Suter, 10 years ago

Attachment: tree_hierarchy.patch added

Fix hierarchy format issues

comment:22 by Peter Suter, 10 years ago

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.

by Peter Suter, 10 years ago

Attachment: foldable_group.patch added

Allows folding groups

comment:23 by Peter Suter, 10 years ago

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 '')

Last edited 10 years ago by Peter Suter (previous) (diff)

comment:24 by Peter Suter, 10 years ago

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

comment:25 by Christian Boos, 10 years ago

Owner: changed from Christian Boos to Peter Suter

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:

and I think it should produce instead:

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

in reply to:  25 ; comment:26 by Peter Suter, 10 years ago

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.

Last edited 10 years ago by Peter Suter (previous) (diff)

comment:27 by Peter Suter, 10 years ago

in reply to:  27 ; comment:28 by Christian Boos, 10 years ago

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…

in reply to:  28 ; comment:29 by Christian Boos, 10 years ago

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 ;-)

in reply to:  26 comment:30 by Christian Boos, 10 years ago

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.

in reply to:  29 ; comment:31 by Peter Suter, 10 years ago

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?

in reply to:  31 ; comment:32 by Christian Boos, 10 years ago

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

in reply to:  32 ; comment:33 by Peter Suter, 10 years ago

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]

in reply to:  33 comment:34 by Christian Boos, 10 years ago

Milestone: next-major-0.1X0.12.3

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!).

in reply to:  28 comment:35 by Peter Suter, 10 years ago

Summary: [PATCH] TitleIndex fixes + enhancement (option 'show_title' to display first level heading)[PATCH] TitleIndex fixes for hierarchy format

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.

comment:36 by Peter Suter, 10 years ago

Resolution: fixed
Status: newclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Peter Suter.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Peter Suter 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.