Positioncustomnodesv2

[jrm-unamur]

Hi Alex, As stated in comments on the previous pull request, I kept special handling (skipping) of 'myhome' and 'coursehome' for the following reasons:

• myhome: it is the root node and thus, I think, nothing can be put before it in the hierarchy
• coursehome: boostnavigation plugin "fumbles" with the global navigation, and coursehome node only exists in the flat navigation context and thus cannot be found as a descendent of the $node passed into local_boostnavigation_build_custom_nodes function.

As for the handling of problematic nodes in the global custom nodes context, again the problem here is about the navigation object. In the global navigation, direct children of myhome are : home, 1, myprofile, currentcourse, mycourses and users !!! Calendar and private files nodes are direct children of '1' node which is indeed Site pages node in the global hierarchy. Thus they are not "whitelisted" in the navdrawer, they are pushed a hierarchy level up. In my understanding so far, these two nodes (calendar and privatefiles are the only ones in that situation, other children of Site pages not being added to the flatnavigation. To be more generic, I would change my patch with a test on the presence of the "beforenodekey" in $node->get_children_key_list() and only search for the parent node accordingly. In this way, all the modifications are integrated in the "case 9" Cheers Jean-Roch

[abias]

Hi Jean-Roch,

I am sorry that I didn't respond anymore in #31 and thank you for submitting an improved pull request here.

I agree with your change in https://github.com/moodleuulm/moodle-local_boostnavigation/pull/37/files#diff-528c47baf0e31a7dcd6c3042a2182b9cR241 as these two blacklisted nodes can't be used as beforenodes.

In my understanding so far, these two nodes (calendar and privatefiles are the only ones in that situation, other children of Site pages not being added to the flatnavigation. To be more generic, I would change my patch with a test on the presence of the "beforenodekey" in $node->get_children_key_list() and only search for the parent node accordingly.

I understand your reasons, but I don't agree with the generic approach as it would have added an overhead for fetching the key list with get_children_key_list() for every node using a before node key and for searching for the before node in the navigation tree, even if the before node key does not exist at all (especially because of a typo in the before node key). This would all have been done just to be prepared that Moodle core adds any new node to the nav drawer in a future version.

So, I dared to change your approach back to a whilelist for the calendar and privatefiles node which you have highlighted to be affected. You can see my changes in https://github.com/moodleuulm/moodle-local_boostnavigation/commit/809fa502592fedce3988a1b694731d29c0f33551 which has already been merged to master. I hope that they are fine for you.

The only unclear thing for me is the question how to use "home" as before node key. You listed this node in your extended explanation in the language pack. However, I was not able to use this node in my tests. That's why I changed the language pack and would be grateful if you could submit another pull request if you have a solution for this node, too.

Cheers, Alex

[jrm-unamur]

Hi Alex, I understand your approach that is less generic but more efficient ;-) Moodle has a whole is slow enough to avoid unnecessary overheads ! So I will delete my positioncustomnodesv2 branch and rebase my fork on to you master and keep it clean for maybe further collaboration ;-)

As for the Home node as before node key, it's working here without anything more ... See the screenshots below (first is the configuration of the custom node, second is the upper part of the nav drawer including the custom node)

Do you receive any error messages ?

Cheers Jean-Roch

[abias]

As for the Home node as before node key, it's working here without anything more ... See the screenshots below (first is the configuration of the custom node, second is the upper part of the nav drawer including the custom node)

Ah, your patch introduced a bug then!

Your simple configuration works, but something like

beforecalendar|/foo||||||fa-pencil|beforecalendar|calendar
beforehome|/bar||||||fa-pencil|beforehome|home

does not work as in the first line, you change the $node to add the custom node to and thus in the next line the home node is not found anymore.

This is now fixed on https://github.com/moodleuulm/moodle-local_boostnavigation/commit/1aae52cd468fe2700422a3e3187634ec6eb1ed62.

Cheers, Alex

[jrm-unamur]

Uh, sorry for that but thanks a lot for having integrated the functionality Cheers Jean-Roch