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
Refactor toc() and li()
Refactor toc() and li()
Christoph M. Becker – Plugins for CMSimple_XH
Re: Refactor toc() and li()
$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.
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()
$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:svasti wrote:When + by whom was it introduced? What does h + c stand for? headlines of content?
So it seems $hc originally had a different purpose. Ah, I found the following explanation in the archived forum: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).
(Interestingly, $hc contained the indexes of the visible pages, though.)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.
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.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.
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.
Christoph M. Becker – Plugins for CMSimple_XH
Re: Refactor toc() and li()
Oops, nearly forgot the most important thing:
Made good for it: r1272.svasti wrote:$hc seems to be missing in the docu... even in the dev-doc I cannot find it.
Christoph M. Becker – Plugins for CMSimple_XH
Re: Refactor toc() and li()
+1cmb wrote:but as it's in the core, we may stick with it. We may consider to hide this option, though.
Re: Refactor toc() and li()
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.
Now it is possible to replace this variant of xtoc with: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.
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);
}
?>
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.svasti wrote:+1cmb wrote:but as it's in the core, we may stick with it. We may consider to hide this option, though.
Christoph M. Becker – Plugins for CMSimple_XH
Re: Refactor toc() and li()
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.
Christoph M. Becker – Plugins for CMSimple_XH