#4771 TOC macro doesn't add headers of included pages [ss246]

unreleased
open
nobody
General
nobody
2015-02-22
2012-08-23
Chris Tsai
No

[forge:site-support:#246]

On the page above there is a table of contents just underneath the Versions Heading, and then each of the headings after that are included from seperate pages per version. I would expect that the table of contents to display the headings from the included pages as well as the headings from the current page. This does not happen.

It may just be me not quite understanding it, however it would be nice to have

I've put together a page demonstrating this here: https://sourceforge.net/p/strawhat/wiki/top/

Discussion

1 2 > >> (Page 1 of 2)
  • Chris Tsai - 2012-09-26
    • labels: support --> support, p3
     
  • Anonymous - 2014-07-01

    Originally by: ligc

    HI,

    We are running into the same issue with the TOC supporting included pages, see https://sourceforge.net/p/forge/site-support/8032/.

    This is very important for us, we are using included pages everywhere, without the TOC for these included pages, our documentation seems quite messed up.

    Any idea on when this bug will fixed? Thank you.

     
  • Dave Brondsema

    Dave Brondsema - 2014-08-15
    • labels: support, p3 --> support, p3, 42cc
    • status: open --> in-progress
    • assigned_to: Igor Bondarenko
     
  • Dave Brondsema

    Dave Brondsema - 2014-08-22
    • Milestone: forge-backlog --> forge-sep-5
     
  • Dave Brondsema

    Dave Brondsema - 2014-08-22
    • Size: --> 2
     
  • Igor Bondarenko - 2014-08-26
    • status: in-progress --> code-review
     
  • Igor Bondarenko - 2014-08-26

    Phew! This was a tough one. Make sure you test it against different wiki pages with different markup, macros, etc, since it changes markdown pipeline a bit.

    Closed #635. je/42cc_4771

     
  • Dave Brondsema

    Dave Brondsema - 2014-09-08
    • Milestone: forge-sep-5 --> forge-sep-19
     
  • Dave Brondsema

    Dave Brondsema - 2014-09-22
    • Milestone: forge-sep-19 --> forge-oct-3
     
  • Dave Brondsema

    Dave Brondsema - 2014-10-06
    • Milestone: forge-oct-3 --> forge-oct-17
     
  • Dave Brondsema

    Dave Brondsema - 2014-10-18
    • QA: Dave Brondsema
     
  • Dave Brondsema

    Dave Brondsema - 2014-10-18

    Can you comment (here or in the code) about what the change in ForgeMacroPattern really does? The inline comment seems to be about the strip() and join() but it's not obvious to me what calling etree.fromstring() does in this context.

    The custom slugify function took me a bit to get (maybe just because its a friday afternoon) but looks like its the same as the original but with better unicode handling.

     
  • Igor Bondarenko - 2014-10-20

    The change in ForgeMacroPattern is essentially about making included page's content available to TOC extension. We need to include processed page contents (html) into parent page, so that TOC will find it. Old code just put macro result onto html stash, which is processed after all the extensions (and there's no way around it), so it was hidden from TOC extension.

    Returning html as a string does not work in this context. Actually, it includes page contents as needed, but TOC extension still can't find it (don't sure why). If we return html as a etree, then TOC can find it and do the job.

    The except might happen if macro returned error, which is just text and can't be parsed to html, so it's ok to return that text, since there will not be any headers that we need to include in TOC anyway.

    Regarding slugify you're completely right, it is only for better unicode handling. I was getting some "TypeError: must be unicode, not str" errors from within markdown, bacause etree that I return from ForgeMacroPattern is always returns a bytestring, when coerced into string, and slugify needed unicode.

    Hope, this clarifies it.

     
  • Dave Brondsema

    Dave Brondsema - 2014-10-20
    • status: code-review --> in-progress
     
  • Dave Brondsema

    Dave Brondsema - 2014-10-20

    If the include macro is used to include a repo file, the line numbers get squashed together. HTML changed from

    <div class="linenodiv"><pre>1
    2</pre></div>
    

    to:

    <div class="linenodiv">
    <pre>12</pre>
    </div>
    
     
  • Dave Brondsema

    Dave Brondsema - 2014-10-20
    • Milestone: forge-oct-17 --> forge-oct-31
     
  • Dave Brondsema

    Dave Brondsema - 2014-10-30
    • Labels: support, p3, 42cc, sf-2 --> p3, support, 42cc, sf-current, sf-2
     
  • Igor Bondarenko - 2014-10-31
    • status: in-progress --> review
     
  • Igor Bondarenko - 2014-10-31

    Closed #672. ib/4771

    I have added comments and fixed the case when page includes file from a repo directly, but there's still a problem if you include a wiki page, which, in turn, includes a file from a repo. I managed to fix issues with the line numbers in that case, but not with file contents (code). It gets extra <br>'s which break layout. I've tried a lot of things to fix that, but nothing works.

    Entire approach is very fragile... Any ideas on how to tackle it in more robust way and fix the issue with multi-level includes?

     
    • Dave Brondsema

      Dave Brondsema - 2014-11-07

      I'm actually not seeing the extra <br>s with a double include. Current example: http://sf-dbrondsema-1015.sb.sf.net/p/testit/wiki/Home/

       
      • Igor Bondarenko - 2014-11-07

        I forgot to mention it. Add a header to the included page. If there's no headers entire TOC-related logic just skipped to avoid those nasty effects

         
  • Dave Brondsema

    Dave Brondsema - 2014-11-11

    Latest commits cause text to be omitted sometimes. Specifically it seems like a child page with a header will only include the text from the first line after the header.

    Test case improvement:

    diff --git Allura/allura/tests/test_globals.py Allura/allura/tests/test_globals.py
    index a68096d..5631026 100644
    --- Allura/allura/tests/test_globals.py
    +++ Allura/allura/tests/test_globals.py
    @@ -256,34 +256,32 @@ def test_macro_project_admins_one_br():
     @td.with_wiki
     def test_macro_include_no_extra_br():
         p_nbhd = M.Neighborhood.query.get(name='Projects')
         p_test = M.Project.query.get(shortname='test', neighborhood_id=p_nbhd._id)
         wiki = p_test.app_instance('wiki')
         with h.push_context(p_test._id, app_config_id=wiki.config._id):
             p = WM.Page.upsert(title='Include_1')
    -        p.text = 'included page 1'
    +        p.text = '# page 1 title\nincluded page 1 line 1\nincluded page 1 line 2\nincluded page 1 line 3'
             p.commit()
             p = WM.Page.upsert(title='Include_2')
             p.text = 'included page 2'
             p.commit()
             p = WM.Page.upsert(title='Include_3')
             p.text = 'included page 3'
             p.commit()
             ThreadLocalORMSession.flush_all()
             md = '[[include ref=Include_1]]\n[[include ref=Include_2]]\n[[include ref=Include_3]]'
             html = g.markdown_wiki.convert(md)
    
         expected_html = '''
     <div class="markdown_content">
    -<p>
    -<div><div class="markdown_content"><p>included page 1</p></div></div>
    +<div><div class="markdown_content"><h1 id="page-1-title">page 1 title</h1>
    +<p>included page 1 line 1<br />included page 1 line 2<br />included page 1 line 3</p></div></div>
     <div><div class="markdown_content"><p>included page 2</p></div></div>
     <div><div class="markdown_content"><p>included page 3</p></div></div>
    -</p>
    -<p></p>
     </div>
     '''.strip().replace('\n', '')
         assert html.strip().replace('\n', '') == expected_html, html
    
     @with_setup(setUp, tearDown)
     @td.with_wiki
     @td.with_tool('test', 'Wiki', 'wiki2')
    
     
  • Dave Brondsema

    Dave Brondsema - 2014-11-11
    • status: review --> in-progress
     
  • Dave Brondsema

    Dave Brondsema - 2014-11-11

    I agree this approach is fragile, and getting quite messy. I don't really have a complete picture of how all the pieces fit together, so I can't really help with a specific recommendation. What it looks like to me though is the challenge is getting the includes to run before the TOC. And you've worked on moving includes earlier in the process but that is causing problems with how they get rendered. What about moving the TOC to later in the process, and leave the includes how they originally were? Would that be any better?

     
    • Igor Bondarenko - 2014-11-12

      That was my first thought too. Unfortunately, I don't see how it's possible. According to markdown's code treeprocessors run before postprocessors. The TOC is implemented as treeprocessor. But output of "include" macros inserted into final page only on postprocessors stage.

      The only way we can make TOC run later is re-implementing it from scratch as a postprocessor, which would search for headers and modify final html string to include table of contents. This approach would be also error-prone and hard to maintain...

       

      Last edit: Igor Bondarenko 2014-11-12
1 2 > >> (Page 1 of 2)

Log in to post a comment.