I know that the Moodle 2.0 navigation system automatically highlights one of the navigation notes as 'active', if the URL of that node exactly matches the current URL. (Hmm, interesting it uses the actual URL in the browser's address bar, not $PAGE->url. I wonder if that is the best choice.)
However, sometimes you want to override that, and manually specify which the active navigation node is. (For example, if you sometimes have extra URL parameters to control slightly different variants of the same page. The extra params will stop the automatic matching from working.)
If this case, there must be some $PAGE->settignsnav->something... API you are supposed to call. What is the right API?
My initial guess was $PAGE->settignsnav->find(...)->make_active(), but I can't find any other code that does that, so I must be wrong.
Oh, I see, I was right, but all the other places that do this do it with $PAGE->navigation instead of $PAGE->settignsnav, which is why my first grep did not find anything.
However, you need to do it as two lines:
if ($node = $PAGE->settignsnav->find(...)) {
$node->make_active();
}
to avoid possible errors.
Hmm. lib/navigationlib.php could do with some more PHP doc comments. What is the difference between navigation_node_collection::get and navigation_node_collection::find?
Also, why is the second $type argument to find optional for the find method of some classes, but not for other classes?
Also, looking at quiz_extend_settings_navigation (which Sam Hemmelryk wrote, and so is hopefully exemplary) it seems to use short things like 'edit' as the $key for navigation nodes. That does not seem sufficiently unique to me. I would not be surprised if $PAGE->settignsnav->find('edit') was not unique, which would be bad.
I wonder what the best practice is here?
Actually, about the need for the separate if statement, instead of $PAGE->settignsnav->find(...)->make_active(). It would be nice if find could return a null_navigation_node object if it does not find a real object. That object would have a make_active() method that does nothing. Look up the null object design pattern, if you are interested. Of course, that would be better if PHP classes had a magic __toBool method, so that null_navigation_node worked the same as false.
Hi Tim,
I can answer one of your questions, thanks to my experience fixing this when it got broken on the Groups admin pages a few weeks back...
The difference between navigation_node_collection::get and navigation_node_collection::find is that ::get will only find a node that's an immediate child of the collection, but ::find will do a recursive search through the entire tree until it either finds a match or runs out of nodes to search. The ::find function was broken until recently - as I discovered when trying to use it to get around the lack of a proper key on the "users" node - but should now work as expected.
I'd also suggest specifying the type (second param) if possible, as it allows it to try to find the node without having to iterate over all the nodes at that level until it finds the one you're after. This applies to both ::get and ::find.
I like your idea about returning a null_navigation_node object - it would indeed make code cleaner, easier to write and more foolproof - rather than breaking the whole page, it'd just fail to highlight the menu item...
As for uniqueness, that can be dealt with in part by chaining ::get (since any conflicts should be in different branches / at different levels) rather than using ::find, and in some cases perhaps by specifying a type (perhaps this is at least part of the reason that some make this param required in ::find?). It's probably still a good idea to try to make keys reasonably unique, though, so you can use ::find if needed...
Edited to add:
I just remembered, the final solution to the groups subpages was actually to ditch the above entirely, and use navigation_node::override_active_url - for example:
navigation_node::override_active_url(new moodle_url('/group/index.php', array('id'=>$courseid)));
This might be an option for your case also?
HTH,
Paul
I can answer one of your questions, thanks to my experience fixing this when it got broken on the Groups admin pages a few weeks back...
The difference between navigation_node_collection::get and navigation_node_collection::find is that ::get will only find a node that's an immediate child of the collection, but ::find will do a recursive search through the entire tree until it either finds a match or runs out of nodes to search. The ::find function was broken until recently - as I discovered when trying to use it to get around the lack of a proper key on the "users" node - but should now work as expected.
I'd also suggest specifying the type (second param) if possible, as it allows it to try to find the node without having to iterate over all the nodes at that level until it finds the one you're after. This applies to both ::get and ::find.
I like your idea about returning a null_navigation_node object - it would indeed make code cleaner, easier to write and more foolproof - rather than breaking the whole page, it'd just fail to highlight the menu item...
As for uniqueness, that can be dealt with in part by chaining ::get (since any conflicts should be in different branches / at different levels) rather than using ::find, and in some cases perhaps by specifying a type (perhaps this is at least part of the reason that some make this param required in ::find?). It's probably still a good idea to try to make keys reasonably unique, though, so you can use ::find if needed...
Edited to add:
I just remembered, the final solution to the groups subpages was actually to ditch the above entirely, and use navigation_node::override_active_url - for example:
navigation_node::override_active_url(new moodle_url('/group/index.php', array('id'=>$courseid)));
This might be an option for your case also?
HTH,
Paul
Hi guys,
Tim, good questions, Paul, spot on with most of the answers.
I'll touch up the PHP docs in navigationlib now, but as Paul noted, get is looks only at the nodes children, find searches recursively.
Also as mentioned by Paul the navigation_node::override_active_url method which takes a moodle_url is another way to handle this, changing the URL used for comparisons. It should be noted that existing nodes aren't re-checked when that changes so you need to call it before the navigation initialised (either when you modify it or when you call $OUTPUT->header()).
override_active_url is useful when the page that you are working with doesn't exist on the navigation (and never will) but does relate to a page on the navigation then this is the way to go.
It allows you to trick the navigation into thinking loading for the required page.
The get/find methods are useful when you know the location of the page you want to mark active, and is especially useful when you aren't sure whether it exists or not (remember navigation can be loaded with AJAX and things may not always be there)
As for unique keys, Paul is right in that using get + find together to narrow the field before a search is how to deal with this.
The fact of the matter is that we can be sure duplicate keys exist within the navigation, when it constructs itself it commonly uses id's as keys for nodes, and where it can't it uses short strings.
So in order to *find* nodes you need to either given them truly unique keys and call find on $PAGE->navigation or $PAGE->settingsnav, or you need to need to narrow the field using get+find with specifics.
For the above reasons using navigation_node::override_active_url is normally a better solution.
The get and find methods are both absolutely essential to the internal operation of the navigation, and were made public as there are scenarios where they are required.
As for the null_navigation_node object, even if we did use a null object class those if statements would still need to exist, in many cases where get/find are being used there are at least two searches being run, the first to find a specific parent, the second to search the parent for the given node.
e.g.
In the above circumstance you would need to check $node to ensure it wasn't a null node and then run the second check.
Personally I think that would look much more confusing in code and certainly more difficult to explain, although in simple scenarios yes it would be easier and less code.
Cheers
Sam
Tim, good questions, Paul, spot on with most of the answers.
I'll touch up the PHP docs in navigationlib now, but as Paul noted, get is looks only at the nodes children, find searches recursively.
Also as mentioned by Paul the navigation_node::override_active_url method which takes a moodle_url is another way to handle this, changing the URL used for comparisons. It should be noted that existing nodes aren't re-checked when that changes so you need to call it before the navigation initialised (either when you modify it or when you call $OUTPUT->header()).
override_active_url is useful when the page that you are working with doesn't exist on the navigation (and never will) but does relate to a page on the navigation then this is the way to go.
It allows you to trick the navigation into thinking loading for the required page.
The get/find methods are useful when you know the location of the page you want to mark active, and is especially useful when you aren't sure whether it exists or not (remember navigation can be loaded with AJAX and things may not always be there)
As for unique keys, Paul is right in that using get + find together to narrow the field before a search is how to deal with this.
The fact of the matter is that we can be sure duplicate keys exist within the navigation, when it constructs itself it commonly uses id's as keys for nodes, and where it can't it uses short strings.
So in order to *find* nodes you need to either given them truly unique keys and call find on $PAGE->navigation or $PAGE->settingsnav, or you need to need to narrow the field using get+find with specifics.
For the above reasons using navigation_node::override_active_url is normally a better solution.
The get and find methods are both absolutely essential to the internal operation of the navigation, and were made public as there are scenarios where they are required.
As for the null_navigation_node object, even if we did use a null object class those if statements would still need to exist, in many cases where get/find are being used there are at least two searches being run, the first to find a specific parent, the second to search the parent for the given node.
e.g.
// We are searching for the notes node in a uses profile space.
// This node will only exist if the user has specific capabilities.
if ($node = $PAGE->navigation->get('myprofile')) {
if ($notes = $node->find('notes', navigation_node::TYPE_SETTING))) {
$notes->make_active();
}
}
In the above circumstance you would need to check $node to ensure it wasn't a null node and then run the second check.
Personally I think that would look much more confusing in code and certainly more difficult to explain, although in simple scenarios yes it would be easier and less code.
Cheers
Sam
Ah, so override_active_url is the way to go. Can I suggest you add a paragraph to the PHP doc for make_active() pointing out to people that they should probably use override_active_url instead for this use case.
The point about the null object pattern is that the null object implements the full interface of the corresponding real object, but just never does anything (or throws an exception, if more appropriate). For two examples, see question_usage_null_observer and question_null_step in http://github.com/timhunt/Moodle-Question-Engine-2/blob/new_qe/question/engine/lib.php.
Actually, there is a particular advantage in PHP, because attempting to call a method on null is a fatal error, whereas your null object can throw an exception that is catchable if need be, and that makes it clear what the problem is.
So, in your example, both get and find should return null objects when they find nothing, so you can do
$PAGE->navigation->get('myprofile')->find('notes', navigation_node::TYPE_SETTING)->make_active();
But it is just one style of coding. It is not always a good idea.
Actually, it is a bit like (but different from) the YUI code
Y.all('#myid').remove();
instead of
var node = Y.one('#myid');
if (node) {
node.remove();
}
for the situation where you don't know whether #myid is present on the page.
The point about the null object pattern is that the null object implements the full interface of the corresponding real object, but just never does anything (or throws an exception, if more appropriate). For two examples, see question_usage_null_observer and question_null_step in http://github.com/timhunt/Moodle-Question-Engine-2/blob/new_qe/question/engine/lib.php.
Actually, there is a particular advantage in PHP, because attempting to call a method on null is a fatal error, whereas your null object can throw an exception that is catchable if need be, and that makes it clear what the problem is.
So, in your example, both get and find should return null objects when they find nothing, so you can do
$PAGE->navigation->get('myprofile')->find('notes', navigation_node::TYPE_SETTING)->make_active();
But it is just one style of coding. It is not always a good idea.
Actually, it is a bit like (but different from) the YUI code
Y.all('#myid').remove();
instead of
var node = Y.one('#myid');
if (node) {
node.remove();
}
for the situation where you don't know whether #myid is present on the page.
Hi Tim,
I've updated the PHP docs now for make_active and override_active_url.
I also added to the docs earlier today, hopefully they are more informative now
I forgot to mention earlier as well that the default URL for comparisons (if you havn't called override_active_url) is $PAGE->url if it has been set, otherwise it is $FULLME.
As for the null object, I can certainly see how the exceptions can be a big advantage in providing a more meaningful response, and the idea of an all or something else situation is appealing.
I'm still not sure I like the idea of large chains of null objects however, but that is my personal preference more than anything else.
What do you honestly think? is it worth changing at this point in the cycle? I'm quite open to it, the only thing I'd add is that if we do choose to do it we would need to do it ASAP, before RC1.
Cheers
Sam
I've updated the PHP docs now for make_active and override_active_url.
I also added to the docs earlier today, hopefully they are more informative now
I forgot to mention earlier as well that the default URL for comparisons (if you havn't called override_active_url) is $PAGE->url if it has been set, otherwise it is $FULLME.
As for the null object, I can certainly see how the exceptions can be a big advantage in providing a more meaningful response, and the idea of an all or something else situation is appealing.
I'm still not sure I like the idea of large chains of null objects however, but that is my personal preference more than anything else.
What do you honestly think? is it worth changing at this point in the cycle? I'm quite open to it, the only thing I'd add is that if we do choose to do it we would need to do it ASAP, before RC1.
Cheers
Sam
New PHP docs look good.
Not worth changing how nav works now. But worth having the null object design pattern in your armoury for the future.
Not worth changing how nav works now. But worth having the null object design pattern in your armoury for the future.
Sam, if you are bored, you might like to grep make_active, and fix all the inappropriate ones.
Grrr! $PAGE->settingsnav->override_active_url($attemptobj->start_attempt_url()); does not work when previewing the quiz (it does not work on either the attempt.php or review.php page.)
Please, Sam, could you take a look and tell me why. I want to go home now.
Also, when I am on one of the quiz settings pages, the in the navigation block, it highlights the quiz node, and all its sub-nodes. I think that looks really bad. Perhaps just hi-light the quiz node, but not its children.
Please, Sam, could you take a look and tell me why. I want to go home now.
Also, when I am on one of the quiz settings pages, the in the navigation block, it highlights the quiz node, and all its sub-nodes. I think that looks really bad. Perhaps just hi-light the quiz node, but not its children.
Hi Tim,
The override_active_url function is static - call it directly rather than trying to call it on $PAGE->settingsnav, like the example I posted earlier:
navigation_node::override_active_url(new moodle_url('/group/index.php', array('id'=>$courseid)));
Also, make sure that the return from $attemptobj->start_attempt_url() is a moodle_url object, as that's what override_active_url expects...
HTH,
Paul
The override_active_url function is static - call it directly rather than trying to call it on $PAGE->settingsnav, like the example I posted earlier:
navigation_node::override_active_url(new moodle_url('/group/index.php', array('id'=>$courseid)));
Also, make sure that the return from $attemptobj->start_attempt_url() is a moodle_url object, as that's what override_active_url expects...
HTH,
Paul
Hi Tim,
Paul is quite right, override_active_url is a static method.
I've updated the fours instance I found where it was being called as a method.
I'm not a big fan of the highlighting either. I'll create a tracker issue for it and assign it to Patrick. I think at one point I had it applying a background colour, which I was a bigger fan of. Can't remember why it changed now, but I'll leave it up to him to decide what he would like to do for each of the themes.
Cheers
Sam
Paul is quite right, override_active_url is a static method.
I've updated the fours instance I found where it was being called as a method.
I'm not a big fan of the highlighting either. I'll create a tracker issue for it and assign it to Patrick. I think at one point I had it applying a background colour, which I was a bigger fan of. Can't remember why it changed now, but I'll leave it up to him to decide what he would like to do for each of the themes.
Cheers
Sam
Highlighting issue: MDL-23691
Thanks Sam and Paul.
Hi Sam Hemelryk,
I created a new block where I need to list only the courses and activities inside, as in the navigation block. How can i call the courses tree structure here and what all links i need to include here. Pleas kindly help me.
I created a new block where I need to list only the courses and activities inside, as in the navigation block. How can i call the courses tree structure here and what all links i need to include here. Pleas kindly help me.
More questions:
1. because $PAGE->navigation links back to $PAGE, then doing
print_object($PAGE->navigation);
when trying to debug is almost useless. Actually, for me, it seems to cause an infinite loop.
I wonder if we need a $PAGE->navigation->debug_display() method or something like that.
2. In the old tabbed interface for the question bank, if you were looking at the questions in the 'Cool questions' category, and then switched to the 'Export' tab, it would remember that 'Cool questions' was the current category. Should the links in the new settings navigation work in the same way? And if so, how should we implement that?
1. because $PAGE->navigation links back to $PAGE, then doing
print_object($PAGE->navigation);
when trying to debug is almost useless. Actually, for me, it seems to cause an infinite loop.
I wonder if we need a $PAGE->navigation->debug_display() method or something like that.
2. In the old tabbed interface for the question bank, if you were looking at the questions in the 'Cool questions' category, and then switched to the 'Export' tab, it would remember that 'Cool questions' was the current category. Should the links in the new settings navigation work in the same way? And if so, how should we implement that?