handle custom nodes positioning

[jrm-unamur]

Hi Alex, I am finished with custom nodes positioning. Edge cases are handled if the given "nodebeforenodekey" cannot be processed (misspelling, wrong context or unusable nodes 'myhome' and 'coursehome') it fallbacks to default positioning at the end of the navdrawer menu section.

• code is only in locallib.php
• updated lang file to document usage
• version bump up to install updated lang var
• travis checks passed
• french language translation posted on Amos

Cheers

[abias]

Hi Jean-Roch,

thank you for this improvement.

french language translation posted on Amos

First, I think you should only translate the strings in AMOS which have been published as official plugin versions. As this change is not published yet, your french strings might now not match the plugin's functionality anymore.

Then, looking at and testing your patch, I have to admit that I don't get the purpose neither of actively ignoring the node keys 'myhome' and 'coursehome' neither searching actively for the node parent before adding the node with the "beforekey" parameter to the navigation.

If you look at https://github.com/moodle/moodle/blob/master/lib/navigationlib.php#L957, you will see that Moodle does check itself if you have submitted a valid beforekey. If the beforekey is not valid, you will get a nice debugging message saying something like Navigation node add_before: Reference node not found coursehome, options: participants grades localboostnavigationcoursesections 50680 57111 localboostnavigationactivities localboostnavigationactivityadobeconnect localboostnavigationactivityassign localboostnavigationactivityforum localboostnavigationactivityquiz localboostnavigationactivitycustomcert. As this only appears when debugging is on, I would be willing to accept this.

Additionally, as the patch stands now, we would be searching the navigation tree anytime a custom node has a beforekey setting even if the beforekey is perfectly valid. I think this is an unnecessary overhead.

From your comment in https://github.com/moodleuulm/moodle-local_boostnavigation/pull/29#issuecomment-482588628, I had the impression that there is a whitelist of navigation nodes which are added by Moodle core to the nav drawer (your example was: calendar) and which would not be found if referenced as beforekey. I haven't tested this myself, but I relied on your experience.

Against this background, I think the current patch is not really necessary or even counter-productive (unless I am getting something wrong and you can point me into the right direction). But on the other hand I would appreciate to have a special handling for the whilelist of nodes including calendar if this is really necessary.

Please let me know what you are thinking.

Cheers, Alex

[jrm-unamur]

Hi Alex and thanks for bright feedback,

First the simple thing :-) AMOS ---> I translated only already registered lang strings of course, I'll add missing one if you accept my input and release a new version integrating it ^^

As for the rest, I'll try to be as clear a possible, but there are many things to say.

I have to admit that I don't get the purpose neither of actively ignoring the node keys 'myhome' and 'coursehome'

Indeed I think it is not possible to use myhome as beforekey since it is the root node and thus nothing can come before it in the hierarchy. As for coursehome, the problem is that the plugin "fumbles" with the global navigation nodes and not the flatnavigation object. 'coursehome' exists only in the flatnavigation context and thus cannot be matched in the boostnavigation context. (As opposed to your theme plugin which fumbles directly with the flatnavigation object to possible put home node above course nodes in course context). That is why I decided to ignore these nodes.

neither searching actively for the node parent before adding the node with the "beforekey" parameter to the navigation. If you look at https://github.com/moodle/moodle/blob/master/lib/navigationlib.php#L957, you will see that Moodle does check itself if you have submitted a valid beforekey. If the beforekey is not valid, you will get a nice debugging message saying something like Navigation node add_before: Reference node not found coursehome, options: participants grades localboostnavigationcoursesections 50680 57111 localboostnavigationactivities localboostnavigationactivityadobeconnect localboostnavigationactivityassign localboostnavigationactivityforum localboostnavigationactivityquiz localboostnavigationactivitycustomcert. As this only appears when debugging is on, I would be willing to accept this. Additionally, as the patch stands now, we would be searching the navigation tree anytime a custom node has a beforekey setting even if the beforekey is perfectly valid. I think this is an unnecessary overhead.

I thought I should make everything to avoid raising errors (even debugging messages). If that is not an issue, I totally agree to remove it to prevent the overhead.

From your comment in #29 (comment), I had the impression that there is a whitelist of navigation nodes which are added by Moodle core to the nav drawer (your example was: calendar) and which would not be found if referenced as beforekey. I haven't tested this myself, but I relied on your experience. Against this background, I think the current patch is not really necessary or even counter-productive (unless I am getting something wrong and you can point me into the right direction). But on the other hand I would appreciate to have a special handling for the whilelist of nodes including calendar if this is really necessary.

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.

What do you think? Cheers,

Jean-Roch

[jrm-unamur]

Hi Alex, I rebased my work onto your last commit and made a new proposal in another pull request, following my answers above. Details are in the new pull request. I'm closing this one Cheers Jean-Roch