"starting level follow active menu item" broken by max_depth variable

I have the same issue.
What I expect using both 'depth' and 'follow' settings is to have a menu starting at the active item and having a maximum depth.

Diggind into the code it seems that the level and the depth are applied on the entire tree before trying to find the active item.
I think this is related to performances issues on some website with huge menus.

We may try to force the parent-mlid to the current active menu when 'follow' is set. Or we may set the $max_depth variable to NULL when the 'follow' setting is set.

I'd like to have the opinion of a maintainer before working on a patch.

I'm seeing this as well. I was hoping "maximum depth" would be relative to the current active menu item if "follow" is set.

Just adding a related issue #1572408: Limit Tree Pruning to maximum depth.

Subscribe

The attached patch adds an option to allow the max_depth to be relative to the active item (rather than the root). This seems to work well, and may often be the desired functionality when the "Make the starting level follow the active menu item" is selected. I implemented this as a new option rather than the default behavior, as I can see a use-case for both scenarios.

If this is something that might be considered for inclusion, a few niceties would be:

1) Hide/show this option when the "Make the starting level follow the active menu item" option is selected (for clarity)
2) Review terminology and potentially change the labels/descriptions
3) Add documentation

#7 seems fine to me.

Tested #7 - works as advertised.

We've been running this on several sites over the last month without any issues.

Given that the menu list never displays more than two levels (and an optional title) when following the active menu item, it seems this will only have an effect if the maximum depth is set to 1, in which case this will just hide the children of the active menu item. Is that correct?

I just posted a somewhat related issue: #2394547: Allow starting level to be "Ancestor of active menu item". I'm proposing that the menu tree be allowed to expand to the maximum depth, even when following the active menu item, rather than always using the active menu item (or its children) as the starting level. I thought this might interest some people here.

I'm not sure that issue is related? And this patch should also have an effect for a depth of 2 (which will show the children of the current menu item)

Perhaps you'd want to install the patch and see what it changes just to confirm -- or use the previous version of Menu Block (which had the same behavior as checking the checkbox introduced by this patch).

I've tested the patch. Menu Block will never show anything below the children of the active menu item, and that will happen for any value of depth >= 2. The easiest way to achieve that is simply to set depth to "unlimited". So, from what I can tell, the only reason to set a depth on a menu, given this patch, when you are following the active menu item, is if you set depth = 1 and hide the children of the active menu item.

The issue I posted is related in that it would have the set depth actually follow the active menu item on sites with deeper menu hierarchies. It would support depths greater than two by padding the top of the menu. For instance, if I was deep in a site's hierarchy and I had a depth of 3, then the menu would show the parent of the active menu item and all of it's siblings, as well as the active menu item and all of its siblings, and all the children of the active menu item. Basically, the menu would act more like it does when you don't follow the active menu item, except that it would start pruning upper levels off the menu once you reached the maximum depth.

Confirmed this seems to work as expected. Is 'relative' the best machine name to use for this setting? I don't really know what that means off hand. Maybe 'depth_relative' would be better instead and a bit more self-descriptive? I also have a UI question/screenshot:

I like "depth_relative" as a more descriptive machine name.

As for the UI state improvement, I think this option should be shown under the following circumstances:
1) "Make starting level follow the active menu item" is checked
2) "Maximum depth" is *not* set to unlimited

I also agree that moving it under "Maximum depth" would make sense, since the user would need to change this value from its default to see the new option anyway.

Yes, those changes make sense...

1. +++ b/menu_block.admin.inc
   @@ -395,6 +395,27 @@ function menu_block_configure_form($form, &$form_state) {
   +          array('value' => '1'),
   +          array('value' => '2'),
   +          array('value' => '3'),
   +          array('value' => '4'),
   +          array('value' => '5'),
   +          array('value' => '6'),
   +          array('value' => '7'),
   +          array('value' => '8'),
   +          array('value' => '9'),
   

   I think you might be able to simplify this with 'value' => range(1, 9)?

2. +++ b/menu_block.module
   @@ -215,17 +215,18 @@ function menu_block_get_all_menus() {
   +    'delta'          => $delta,
   +    'menu_name'      => 'main-menu',
   +    'parent_mlid'    => 0,
   +    'parent'         => '',
   +    'title_link'     => 0,
   +    'admin_title'    => '',
   +    'level'          => 1,
   +    'follow'         => 0,
   +    'depth'          => 0,
   +    'depth_relative' => 0,
   +    'expanded'       => 0,
   +    'sort'           => 0,
   

   Ugh, I hate equal spacing for this reason and it's easy to break other patches. I'm going to remove this upstream in 7.x-2.x.

3. +++ b/menu_block.module
   @@ -243,14 +244,15 @@ function menu_block_get_config($delta = NULL) {
   -    $config['title_link']  = variable_get("menu_block_{$delta}_title_link",  $config['title_link']);
   -    $config['admin_title'] = variable_get("menu_block_{$delta}_admin_title", $config['admin_title']);
   -    $config['level']       = variable_get("menu_block_{$delta}_level",       $config['level']);
   -    $config['follow']      = variable_get("menu_block_{$delta}_follow",      $config['follow']);
   -    $config['depth']       = variable_get("menu_block_{$delta}_depth",       $config['depth']);
   -    $config['expanded']    = variable_get("menu_block_{$delta}_expanded",    $config['expanded']);
   -    $config['sort']        = variable_get("menu_block_{$delta}_sort",        $config['sort']);
   -    $config['parent']      = variable_get("menu_block_{$delta}_parent",      $config['menu_name'] . ':' . $config['parent_mlid']);
   

   Same here...

+        ':input[name=depth]' => array('value' => range(1, 9)),

Haven't tested that this will work yet, can anyone check the UI?

I tested the UI, it looks like the latest patch with range(1,9) for the triggering state value isn't showing the new option:

Oh, I think they have to be strings, sorry. Try array_map('strval', range(1, 9))

@dblue Oh yes that would work much better, using the negation condition.

Also it would be really nice to get some reviews for #2419725: Refactor menu_block_get_config() since that would help make it easier to all these feature requests to add new configuration keys since you don't have to add the new key in four new places with each request. I'd greatly prefer to have that merged prior to this being committed.

Pity we could'nt get this into 2.5 anymore, hoping for a 2.6 to be released soon :)

I know, I'm sorry but I really wanted to get all the possible bugs squashed and in a new release so I could start evaluating new features.

Yeah cool that makes sense. I may be confused and thinking of another issue, but I think this just reverts the behavior to whatever it was before 2.4, so hoping for a 2.6 not to be too far out!

Looks good!

1. Enabled menu_block (7.x-2.x-dev), as well as devel, devel_generate.
2. $ drush generate-content 30
3. $ drush generate-menu 1 20 5
4. Configured a menu block:

* Starting level: Third level (tertiary)
* Make the starting level follow the active menu item. - checked
* Starting level will be: Active menu item
* Maximum depth: 1

Page structure:

• item 1
  • item 1.1
    • item 1.1.1
    • item 1.1.2
      • item 1.1.2.1 <-- active page
      • item 1.1.2.2

Before patch menu output (no active class):

item 1.1.1
item 1.1.2

After patch menu output (with new option "Make the maximum depth relative..." checked):

item 1.1.2.1 <-- active class
item 1.1.2.2

FWIW, also confirmed that making a selection other than "Unlimited" for Maximum depth displays the new option Make the maximum depth relative to the starting level while following the active menu item.

Patch in #26 worked for us. Thanks!

OK good, another review and this should go back to RTBC!

Still works with latest HEAD :)

IMO this is not actually a new feature, this is a fix for a BC break that was introduced in 2.4

Doesn't work for me.

If i select a item that have no children, menu_block is hidden

Configuration :

Menu : Selected by page
Start Level : Secondary level
Folow active element : checked
Start leve will be : Children of selected menu
Depth : 1
Make the maximum depth relative to the starting level while following the active menu item : Checked

@ebougerolle:

If i select a item that have no children, menu_block is hidden

I think this would be your problem:

Start level will be : Children of selected menu

Committed #26 to 7.x-2.x.

Not sure if I should worry about this but noticed on upgrade the following notices popped up.

Notice: Undefined index: depth_relative in menu_tree_block_data() (line 360 of menu_block/menu_block.module).
Notice: Undefined index: depth_relative in menu_tree_block_data() (line 350 of menu_block/menu_block.module).
Notice: Undefined index: depth_relative in menu_tree_block_data() (line 360 of menu_block/menu_block.module).

+++ b/menu_block.module
@@ -346,7 +347,7 @@ function menu_tree_block_data(array &$config) {
-    if ($config['parent_mlid']) {
+    if ($config['parent_mlid'] || $config['depth_relative']) {

@@ -356,8 +357,13 @@ function menu_tree_block_data(array &$config) {
-    $tree = menu_tree_page_data($config['menu_name'], $max_depth);
+    if ($config['depth_relative']) {

Looks like they are here, should I open a new issue or can we just add a patch here for isset/empty() checks?

Maybe this would do for things going forward? Sorry for changing the status on a fixed issue, I know some don't like it.

We could also just use !empty($config['depth_relative']) which will not throw any errors or notices if the value is not set, which will happen for upgraded sites.

@Dave Reid reason I proposed this fix is so you don't have to do the empty check and possibly if a future config gets added then you won't have to worry about that either... just a thought:

+++ b/menu_block.module
@@ -342,6 +342,12 @@ function menu_block_get_config($delta = NULL) {
+  static $defaults;
+  if (!isset($defaults)) {
+    $defaults = menu_block_default_config();
+  }
+  $config += $defaults;

Also the static is small enough that it would be negligible to memory performance I'd expect.

I ended up resolving the follow-up in #2499733: Error when upgrading to 7.x-2.6 but I still gave you a commit mention @joelpittet.

Well cool, thanks @Dave Reid!

Is this will be a part of new release? I checked 2.7 and dev, and neither have this patch.