Page 1 of 1

Refactor toc() and li()

Posted: Thu May 08, 2014 12:00 am
by cmb
Hello Community,

does anybody understand what's going on inside of toc() and li()?

I pick up on this issue, because I recently commited a fix which introduced the funtion XH_buildHc(); this function is closely related to toc() and $hc is often used in templates as first argument to li() (or rather xli() of xtoc). This pointed my attraction to toc() and li() again, and IMHO both functions are way too long and too cryptic and should be refactored ASAP.

I suggest to apply "Replace Method with Method Object" on both functions first (toc() becoming the model and li() the view), and after having unravelled the algorithms to move related functionality (such as XH_buildHC()) to the respective class(es). Especially, I suggest to keep an eye on potential customizations and to create an API that allows these without restricting them to current functionality. To better understand what I mean, consider the original xtoc: this wasn't able to cater for $cf['menu']['sdoc']="parent", which was introduced with CMSimple 2.8, and had to be replaced by xtoc28--however, existing templates most likely weren't updated, so the new functionality could not be used.

Frankly, I don't know how much flexibility wrt. customization we can achieve, but we should strive for some sensible maximum. After all, I consider xtoc, AdvancedToc etc. merely as workarounds.

As we already have a quite comprehensive test suite regarding toc() and li() in place (see trunk/tests/unit/MenuTest.php), and the current API (toc() and li()) won't change, I dare to propose this refactoring for XH 1.6.3.

Christoph

Re: Refactor toc() and li()

Posted: Fri May 09, 2014 6:05 pm
by svasti
$hc seems to be missing in the docu... even in the dev-doc I cannot find it.
When + by whom was it introduced? What does h + c stand for? headlines of content?
Anyhow, $hc is useful in drop down and fold out menus.

Would be nice to have toc() and li() more readable, and maybe integrate some functionality so that xtoc isn't necessary any more.

$cf['menu']['sdoc']="parent" seems to me a setting, which should belong to the template and not to the core. It was probably introduced, because it was a new feature that shouldn't break existing templates. As it was introduced long ago I guess that the time has come to skip this setting and making "parent" standard. Probably there are only very few templates that use a new XH version and still need $cf['menu']['sdoc'] switched off. This could easily be adjusted in the stylesheet.

Re: Refactor toc() and li()

Posted: Fri May 09, 2014 7:30 pm
by cmb
svasti wrote:When + by whom was it introduced? What does h + c stand for? headlines of content?
$hc was first introduced in CMSimple 2.3 by Peter Harteg. In CMSimple 2.8 its population was moved from rfc() to functions.php, with the comment "Backward compatibility for DHTML menus". The changelog mentions:
CMSimple version 2.8 beta 1
CMSimple hide functionality recoded - $hc, $hl and $hs are not used anymore.
CMSimple version 2.8 beta 2
Backward compatibility for ie. DHTML menus added to functions.php ($si, $hc and $hl is set).
So it seems $hc originally had a different purpose. Ah, I found the following explanation in the archived forum:
Harteg wrote:I have removed the use of $hc (hidden content) to simplify the code and rebuild the CMSimple hide functionality and made a complete recode of toc/submenu-function.
(Interestingly, $hc contained the indexes of the visible pages, though.)
svasti wrote:$cf['menu']['sdoc']="parent" seems to me a setting, which should belong to the template and not to the core. It was probably introduced, because it was a new feature that shouldn't break existing templates. As it was introduced long ago I guess that the time has come to skip this setting and making "parent" standard. Probably there are only very few templates that use a new XH version and still need $cf['menu']['sdoc'] switched off. This could easily be adjusted in the stylesheet.
Hm, I don't think that either setting of menu_sdoc will break a template (except for some very contrived cases); when set to "parent" it just adds the sdoc(s) CSS class to the ancestors of the currently selected page. Whether it belongs to the core or the template might be debatable, but as it's in the core, we may stick with it. We may consider to hide this option, though.

What I was trying to explain with the mention of menu_sdoc above, was that such new config options and other features will always need an update to any custom toc() implementation bundled with a template, so it's better if it wouldn't be necessary to have a full replacement for toc()/li(), but being able to replace a tiny bit of it's implementation. Consider version 3 of xli() for instance: this only "turns a clicked button to a clickable button" (i.e. wraps the currently selected page item in an <a> element). To accomplish that, Till had to deliver a complete copy of li() with only a small modification. If there had been a more flexible way to modify li(), xtoc would still have worked with CMSimple 2.8's menu_sdoc.

Re: Refactor toc() and li()

Posted: Fri May 09, 2014 8:41 pm
by cmb
Oops, nearly forgot the most important thing:
svasti wrote:$hc seems to be missing in the docu... even in the dev-doc I cannot find it.
Made good for it: r1272.

Re: Refactor toc() and li()

Posted: Sun May 11, 2014 8:43 am
by svasti
cmb wrote:but as it's in the core, we may stick with it. We may consider to hide this option, though.
+1

Re: Refactor toc() and li()

Posted: Sun Sep 21, 2014 7:16 pm
by cmb
I have done some basic refactoring (r1355, r1375-r1376). To not further delay the release of CMSimple_XH 1.6.3, I will do the rest for XH 1.6.4.
cmb wrote:Consider version 3 of xli() for instance: this only "turns a clicked button to a clickable button" (i.e. wraps the currently selected page item in an <a> element). To accomplish that, Till had to deliver a complete copy of li() with only a small modification. If there had been a more flexible way to modify li(), xtoc would still have worked with CMSimple 2.8's menu_sdoc.
Now it is possible to replace this variant of xtoc with:

Code: Select all

<?php

require_once $pth['folder']['classes'] . 'Menu.php';

class MyLi extends XH_Li
{
    function renderMenuItem($i)
    {
        global $h;

        return $this->renderAnchorStartTag($i) . $h[$this->ta[$i]] . '</a>';
    }
}

function xtoc($start = null, $end = null)
{
    return toc($start, $end, array(new MyLi, 'render'));
}

function xli($ta, $st)
{
    $li = new MyLi();
    echo $li->render($ta, $st);
}

?>
svasti wrote:
cmb wrote:but as it's in the core, we may stick with it. We may consider to hide this option, though.
+1
I have not done this, because I'm not sure, if that was part of the voting. Anyway, we have Hide all folder_* config options on the roadmap for XH 1.6.4, where svasti already suggested to hide further config options, so we can do it then.

Re: Refactor toc() and li()

Posted: Tue Jan 20, 2015 8:27 am
by cmb
I have refactored a bit more (r1467+r1471). This is still far from what I consider understandable code, but due to lack of time and features (PHP 5), I'll leave it for now.