page titles are broken when outside the active menu or on a callback

I think this relatively simple fix gives us pretty much all that we want.

Also fixes some calback titles that were broken.

this patch is functionally identical, but chx likes the use of an array index better then end()/reset()

I tested correct_title_2.patch by moving the administer menu to Primary Links, and it appeared to work fine, with one exception. If you go to admin/by-module, it still shows 'home' as the page title. The admin page, (admin/by-task) correctly shows "Administer" as the page title.

Other than that, everything's good to go!

Note- the page title for by-module is right if it's still in the Navigation menu, just not if it's moved. So, this patch doesn't catch this case - basically local tasks may need to set the page tittle.

ok - here's a possibly more robust fix - should get the right title (i.e. the title of the root page) for local tasks even outside the active menu.

Tested and it's working fine.

needs a little more work - found a path where this breaks: admin/settings/filters/1/configure

I think this is because the tab_root invludes a '%' wildcard.

a general explanation of the logic in the patch:

If you deconvolute the logic, on a normal page (i.e. in the active menu) it should work as before

The last item in the active train is used to set the page title. So when we're outside the active, or on a callback, there may be no active trail. So, to fix this, we append the current item to the active trail if it's not there. For tabs we want the root item, rather than the tab itself.

The previous iteration always did $item['href'] = $item['tab_root']; and this one doesn't'; now I'm assigning the whole root item.

ok - well this still isn't quite right for the filter pages - but that's because of a bug in filter module - it doesn't declare a title attribute nor set the title.

Otherwise, I think this is closer to correct.

I tested this one extensively by once again moving the Administer menu under the Primary Links section, and then clicking each and every last one of them. All links and page titles functioned as expected.

I did notice that while the Administer menu had Primary Links as its parent, the /admin page(tab) was blank, yet the /admin/by-module had the expected content. While an IRC discussion revealed that this is not the fault of the correct_title_4.patch, it should be mentioned here, just in case.

Could we make the code comments of the first chunk a bit more verbose? They don't help me understand 100% what's going on. Thanks.

Greatly expanded code comments - also noticed an error - I put 'tab_parent', rather than 'tab_root' in the explode(). These will be the same for top-level tabs, but not for sub-tabs.

Also, there are a number of pages where the root path has a wildcard where the titles are not quite right in addition to the filter module, such as: admin/content/taxonomy/1/add/term

Basically this needs to be handled in separate patches for various paths in 1 of two ways: as menu module does with a title callback, or as node module does by calling drupal_set_title() separately for node/% and node/%/edit.

Yes, this is a fine patch. I would much , much prefer title callback -- even removing some existing drupal_set_title -- mostly because that's easy alterable by a fictional i18n_menu for example...

@Dries - are the code comments now sufficient?

pwolanin: the documentation is clear. Thanks!

I can't make sense of chx' comment #12 though. chx, are you suggesting we should have dynamic titles?

@Dries - we can already have dynamic titles using the title callback (see menu module for the one example so far in core - the string "(overview)" is added to the menu name to generate the title). This may be a more effective way to handle items were the root has a wildcard in the path (e.g. different filters with different names) rather than patching each tab's page callback to have a drupal_set_title().

pwolanin: thanks for the clarification, I forgot about that callback. I think that means your patch is RTBC, and that we can discard chx comment for now? chx, do you have anything else to add? I'm still not 100% I understood what you were trying to point out.

If chx approves your patch, I'll commit it. Thanks for the great work, p.

I am sorry for the confusion, this patch is fine and ready. My confusing comment was a followup to pwolanin's

Basically this needs to be handled in separate patches for various paths in 1 of two ways: as menu module does with a title callback, or as node module does by calling drupal_set_title() separately for node/% and node/%/edit.

and basically said "use method 1".

OK, committed the fix then.