Usually, if a menu entry is overriden, the title is adapted to the custom entry. In our case, this does not carry over to the breadcrumbs, because we set it to fixed values. I propose we change the part of the breadcrumb that points to the current page to menu_get_active_title. E.g. in node_gallery_list_galleries():

 // old, fixed:
$breadcrumbs[] = l(t('Galleries'), 'galleries');
// new, variable:
$breadcrumbs[] = l(check_plain(menu_get_active_title()), 'galleries');
CommentFileSizeAuthor
#28 garland_issue.jpg75.41 KBaitala
#28 garland_issue2.jpg76.29 KBaitala
#26 issue.jpg47.4 KBaitala
#26 issue2.jpg78.92 KBaitala
#21 1043532.21-breadcrumb_menu_items.patch17.01 KBdeviantintegral
#20 1043532.20-breadcrumb_menu_items-2.patch7.27 KBscroogie
#17 1043532.17-breadcrumb_menu_items.patch11.84 KBjustintime
#16 1043532.14-breadcrumb_menu_items.patch10.29 KBjustintime
#13 1043532.14-breadcrumb_menu_items.patch10.29 KBdeviantintegral

Comments

nasi’s picture

nasi commented

There seem to be a couple of more fundamental issues here with the breadcrumb:

1. Surely the breadcrumbs should not include a link to the current page, which is what it currently does? The breadcrumbs should show levels above the page currently displayed. As it is, there is duplication between the breadcrumb 'Galleries' and the page title 'Gallery List'. When the user clicks on a link in the breadcrumbs, they do not expect to be taken to exactly the same page.

2. The code in node_gallery_list_galleries() seems to assume that the galleries page is at the top level in the site – it is building a breadcrumb trail from scratch as Home -> Galleries. Shouldn't the breadcrumbs be generated with reference to the menu system and the page's position within the menu structure?

scroogie’s picture

scroogie commented

Regarding 1., I think it should contain the current page, but not as a link.
Regarding 2, you're probably right. It might turn out a bit buggy to realise that, though. As far as I know, the active trail is quite broken in Drupal 6 (partially even in Drupal 7). It should be better than the current hardcoded way, though.

nasi’s picture

nasi commented

In terms of 1., I don't know how other people organise their sites, but none of mine contain the page title in the breadcrumbs (as a link or not). So at the moment there is an inconsistence when on the /galleries page. I might be able to override it with Custom Breadcrumbs, but haven't checked yet.

As for 2., you're right about things being a little broken. But, if NG does what it can within the current system then people can always fix things with something like Custom Breadcrumbs if they wish. But at least in the simple case, the breadcrumb would correctly reflect the page's position in the menu tree.

justintime’s picture

justintime commented

Re: first point: drupal_set_breadcrumb() docs specify:

... starting with "home" and proceeding up to but not including the current page.

No time to check into the second point, I need some sleep. While this is something I'd like to get fixed, it's not a release blocker, we can fix this in the normal maintenance cycle. If someone could submit a patch to get the ball rolling, it would help a lot.

scroogie’s picture

scroogie commented

Now this is a bit crazy. There is no way to retrieve the title of the menu link to the galleries page without knowing its mlid. The mlid can only be found by knowing the menu name, though. As we don't know in which menu the user put our link, there is nothing we can do except querying the database directly. There seems to be no API function to do that.

Second, I think we should change the behaviour of node_gallery_list_galleries_title. Currently we override any custom title of the menu link with "My galleries" or "%user's Galleries". I think what we should really do is introduce another level in the breadcrumb, so the original title of the summary is preserved. So the breadcrumb would look like this: Home -> Galleries -> scroogie

We need to look after the different gallery types / relationships as well though. All this confuses me a bit. I think we'll need to discuss it first.

scroogie’s picture

scroogie commented

Then there is also this: #1160388: Use view title when listing all galleries.

While this looks like a nice idea at first, it's a bit inconsistent. Links to the page will use the menu link title, but the page itself uses the View title.

dddave’s picture

dddave commented

I am noticing other odd behavior. Using a multilingual setup I get the correct breadcrumb for the english "My galleries" but on the german version I get a mishmash: "My Galerien". Is this related to this issue or worth a separate issue?

edit: Using the NG user profile module show this odd beahvior also on the tab created this add-on module.

scroogie’s picture

scroogie commented

Perhaps you didn't translate the single word "My"?

dddave’s picture

dddave commented

Just checked the .po file and "My Galleries" is translated correctly. There is no single string of "My" in the .po file.

scroogie’s picture

scroogie commented

Ah, yes, that is a bug. "My" is passed in as a parameter instead of special casing the whole call to t().

We really need to fix the breadcrumbs globally.

deviantintegral’s picture

deviantintegral commented

You can use menu_get_item() to get a menu item based off of a path:

$ drush php-eval 'print_r(menu_get_item('galleries'));'
Array
(
    [path] => galleries
    [load_functions] => 
    [to_arg_functions] => 
    [access_callback] => user_access
    [access_arguments] => a:1:{i:0;s:17:"view node gallery";}
    [page_callback] => node_gallery_list_galleries
    [page_arguments] => a:0:{}
    [fit] => 1
    [number_parts] => 1
    [tab_parent] => 
    [tab_root] => galleries
    [title] => Gallery List
    [title_callback] => t
    [title_arguments] => 
    [type] => 6
    [block_callback] => 
    [description] => 
    [position] => 
    [weight] => 0
    [file] => sites/all/modules/node_gallery/node_gallery.pages.inc
    [href] => galleries
    [options] => Array
        (
        )

    [access] => 
)
deviantintegral’s picture

deviantintegral commented
Assigned: Unassigned » deviantintegral

I'm working on a patch for this.

deviantintegral’s picture

deviantintegral commented
Assigned: deviantintegral » Unassigned
Priority: Minor » Normal
Status: Needs work » Needs review
StatusFileSize
new 1043532.14-breadcrumb_menu_items.patch10.29 KB

Here's a patch that contains the following commits:

  • 4550ff5 Issue #1043532: Add a function to set a breadcrumb based on menu access.
  • b0557ba Issue #1043532: Use node_gallery_set_breadcrumb() to set breadcrumbs.
  • d7122d2 Issue #1043532: Add a function to set the breadcrumb for a given user ID.

Menu titles are pulled directly from the menu system. As well, breadcrumb entries are only set if the user has access to the menu item. node_gallery_set_breadcrumb() allows setting a breadcrumb with any set of paths (while adding in "Home" automatically). node_gallery_set_user_breadcrumb($uid) is a helper function that saves on some repetition.

I've upgraded this to normal due to the issues with menu access controls.

scroogie’s picture

scroogie commented

Thanks for the patch! Can you check if the title adapts if you change the title of the item in the menu administration? Is the function node_gallery_list_gallery_titles still required after the patch? Otherwise we can delete it in the same patch.
Sorry that I dont have time for a review right now, but I'll do that tomorrow.

deviantintegral’s picture

deviantintegral commented
Status: Needs review » Needs work

I'd assumed it would since the menu is being pulled from the menu system, but it looks like that's not the case. Hopefully there's just an additional menu call to make, but I'd assumed it would have been handled by the title callback when the menu item was loaded.

I think we still need the node_gallery_list_galleries_title() as it's used by the title callback in the hook_menu() implementation.

justintime’s picture

justintime commented
Status: Needs work » Needs review
StatusFileSize
new 1043532.14-breadcrumb_menu_items.patch10.29 KB

Holy crap, that took forever to dig out. It seems there's no good way in D6 to find the title of a page that isn't the one you're currently on. I finally found some tips from custom_breadcrumbs. Attached patch only modifies the breadcrumb function, but I think it may be better suited to pull out the logic into it's own function. Thoughts?

justintime’s picture

justintime commented
StatusFileSize
new 1043532.17-breadcrumb_menu_items.patch11.84 KB

Dammit, uploaded the wrong file, sorry.

scroogie’s picture

scroogie commented

Ah yes, this is the way that I, in theory, tried to point out in #5 :)

Now this is a bit crazy. There is no way to retrieve the title of the menu link to the galleries page without knowing its mlid. The mlid can only be found by knowing the menu name, though. As we don't know in which menu the user put our link, there is nothing we can do except querying the database directly. There seems to be no API function to do that.

So yes, I agree this seems to be the best way to do it. Well done!

scroogie’s picture

scroogie commented
Status: Needs review » Needs work

Okay, I had a chance to look at this more detailed today. I really like that we finally have the breadcrumb code centralized. I found a few inconsistencies though:
1. On "/galleries" you have active page in the breadcrumb, on the other pages you don't. So I think the breadcrumb should only be "Home".
2. On gallery pages there is a trailing ">" sign
3. On image pages, the link to "galleries" is missing. You just get "Home > GalleryTitle", I think it should be "Home > Galleries > User's Galleries > Gallery Title"

2. might be because it tries to lookup a menu link to a parameterized menu entry. E.g. it tries to find a menu entry that links to "/galleries/1" instead of one that links to "/galleries/%".

3. Nearly the same as in 2., but the same effect for the node router ("node/%") where it tries to find a menu entry for a specific node id.

I think the behavior is actually proper, because if there is a menu entry that links directly to the item, than we should use the assigned title. But we need to add a fallback if there is no such entry, to link to the router (%) and need to set a title manually.

scroogie’s picture

scroogie commented
Status: Needs work » Needs review
StatusFileSize
new 1043532.20-breadcrumb_menu_items-2.patch7.27 KB

So what about this? Just a small additional check if there is a direct link at all, or otherwise fall back to the item's title. Also changed the self-link in breadcrumb on the galleries page.

deviantintegral’s picture

deviantintegral commented
StatusFileSize
new 1043532.21-breadcrumb_menu_items.patch17.01 KB

Patch in #20 looks to be working for me. Here's a patch with the two changes from #20 broken into separate commits as well as an additional patch to fix some very minor code style issues.

I think this is RTBC.

justintime’s picture

justintime commented
Status: Needs review » Needs work

@scroogie - haven't had time to look this over yet, but can you follow the procedure outlined here (ironicaly written by deviantintegral) and re-roll that? It makes it so much easier to work with patches where multiple people are working on the same area of a patch, and it also keeps all the accreditation straight.

deviantintegral’s picture

deviantintegral commented

What needs work on this? I used --author in #21 to attribute the two changes to scroogie.

justintime’s picture

justintime commented
Status: Needs work » Needs review

Sorry, I must've forgot to refresh the page, I totally overlooked your reroll in #21. I'll review it tonight and should be able to commit it into dev.

justintime’s picture

justintime commented
Status: Needs review » Fixed

Looks good, tested fine, and is certainly better than what we have in HEAD now :) Committed.

aitala’s picture

aitala commented
StatusFileSize
new issue2.jpg78.92 KB
new issue.jpg47.4 KB

Dumb question... does the patch / issue here address the problem in the image attached?

You can see the behavior at http://www.f1m.com/content/gallery/peugeot-206-tamiya-124

Note that I am running the just released 6.x-3.0-beta2...

Thanks,

Eric

justintime’s picture

justintime commented

@aitala, this issue isn't the fix for that I'm afraid. That appears to me to be a theme issue. Can you switch to a core theme and see if the links render properly?

aitala’s picture

aitala commented
StatusFileSize
new garland_issue2.jpg76.29 KB
new garland_issue.jpg75.41 KB

Nope, I switched to Garland and got the same thing. See images...

I am thinking it might be a function of the RealName module? Maybe Custom Breadcrumbs??

I dunno.. I did clear the Views cache and revert and Node Gallery views...

Eric

justintime’s picture

justintime commented

Yeah, if it's not a theme issue, then it's either something you've done customizing the menus, or quite possibly one of those modules. Can you create a new support issue for this? Try disabling realname first (I could see where it might conflict), if that doesn't work turn realname back on, then disable custom_breadcrumb. Post your results in the new issue, and I'll dig into it with you.

aitala’s picture

aitala commented

Yup, its the RealName module... I disabled it and the issue disappeared...

Eric

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.