Bootstrap theme shows no language menu

The language menu is added to the end of the custom menu. A code merging mistake meant that the language menu wasn't displayed unless you had entries in the custom menu.

https://github.com/bmbrands/theme_bootstrap/commit/56bd79de3c2865716ea7f6607783ba4cf42660a3

While fixing that I noticed that we seem to be ignoring the admin setting about whether to show the language menu or not. A current workaround is to limit the language menu to one item. Presumably we should check that setting in the same place as the above fix.

This commit makes it respect the $cfg->langmenu setting:

https://github.com/bmbrands/theme_bootstrap/commit/bc7b32f6a0cfef49affccf3ddb43a575cbe32df7

@David: You also need to add the option for language menu for the Frontpage layout in bootstrap/config.php

    'frontpage' => array(
        'file' => 'frontpage.php',
        'regions' => array('side-pre', 'side-post'),
        'defaultregion' => 'side-pre',
        'options' => array('langmenu'=>true),        
    ),

I'm just guessing but I assume that was taken out by Bas because the lang menu was instead being injected into the custom menu? It seems to appear everywhere for me despite that setting not being there.

Just checked layout/general.php in Base and Bootstrap. That setting is checked in Base but isn't used in Bootstrap. I'm not sure if it's appropriate to check it or not. Why wouldn't you want the menu displayed on some screens?

The patch needs to be a branch off Moodle master. Assigning this to you as you know more about Bootstrap.

David is this fixed as I see a language menu in the menubar. Or is this becasue I have set my site to use custom menu?

If this is the case then if no menu is set perhaps Bootstrap should add the lang menu as normal inside headermenu?

I tested the current 2.5beta (20130418).
The language menu is not shown when the custom head menu is not shown.
The language menu is always shown when the custom menu is shown ... even the language menu is switched off.

Thanks Ralf.

I'll see what I can do with this.

Thanks for fixing this David,

While your patch seems to do the trick, I am not sure we should head in that direction. I think copying the logic from lang_menu() to custom_menu() creates 2 problems:

1. It makes it harder for core developers to maintain the code. The same code is in 2 different places, which are not even related to each other in the first place.
2. As Bootstrap is a base, other themers will have to override 2 methods in order to customize their language menu.

I know this patch does not introduce the issue, I understand the reason why it had been done like that in the first place, and surely we should have a renderable object for the language menu, as well as render_lang_menu(), but it's now too late to introduce them in core now.

An alternative, though not the best, is to use lang_menu() to return an array of languages which you can then use in custom_menu() too. The above problems still remain, but at least the logic is constrained in the same method.

Regardless of my comments above, strictly about the patch you provided, here are a really few minor comments:

1. 93: $this->page->course != SITEID and !empty($this->page->course->lang) should be encapsulated in between brackets.
2. 105: Needs identation.
3. Your commit message could use the component name: http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages

Many thanks!

Fred

Hi Fred,

I agree the the duplication is unfortunate, but I think Bas was right to put the languages in the main custom/Bootstrap menu. The collapsing at small sizes just didn't work otherwise, and mobile is the big focus for Moodle adopting this theme. There's more than this area where things could be improved by changing a bit of core, rather than working around things in the theme.

I've added those brackets and pulled that line up onto the previous. I know it's a long line, but given the indention it would still be about the same width on it's own line. I also added bootstrapbase to the commit message.

I think it's ready for integration if someone can put it forward.

Thanks David! Pushing forward.

I think this is implied in the testing instructions, but perhaps they should explicitly state that custom menu items should be used. There should probably also be a test that checks for the language menu when there are no custom menu items.

Clarifying testing instructions based on above feedback.

I have created an issue, and added a commit for the code duplication: MDL-39565

Integrated and tested during integration. Thanks David.

We can address that duplication properly in the linked issue.

Did you think this day was not going to arrive ever?

Your patience has been rewarded, yay, sent upstream, thanks!

Closing...ciao

Linking to another issue relating to the lang menu in Bootstrap/Clean.