D7UX: Implement customizable shortcuts

hi Gabor, thanks for opening this up.

What Mark & I had envisaged for the toolbar was:

- initial state = role based shortcuts (following our 'make smart defaults' principle. So we would need to define what roles and associated shortcuts come out of the box. The only one we have done to date is Content Creator so we welcome input to for others

- then, once you're set up in Drupal you can over-ride the default state and choose your own set of shortcuts. What we had envisaged here was to identify a defined set of shortcuts/icons that the user can select - probably something in the region of 2 dozen options, which would probably include a completely customisable option.

Something we think is very important is to limit the number of shortcuts in this bar so that they actually still remain shortcuts. Once you start putting more than 5/6 icons in there then the efficiency intended for the icons will begin to reduce significantly and you effectively create a second, competing Information Architecture.

We have seen Jeff Noyes' proposal at http://blip.tv/file/2304921 and would strongly recommend that this *not* be the default implementation of shortcuts as they introduce far more complexity than is necessary for the average user. Having said that, it would probably make an excellent contrib option for people to install should they so desire, it is quite complex but also quite fun and for power users may definitely be a useful tool.

So, if we are to proceed as per the original vision, tasks remaining include:
- define a complete but limited set of optional shortcuts
- define sets of role based shortcuts as defaults.

Subscribe.

As of Drupal 7, we have three default roles:

1. Anonymous :: No Toolbar
2. Authenticated :: (Do they have any default permissions?)
3. Administrators :: Sky is the limit, all permissions by default

I'd say that Authenticated users only get a shortcut bar that includes the ability to list their own content, edit their profile, and logout.

Administrators should be able to add content, find content, see reports, and visit the dashboard.

Oh... and... uh... subscribe.

Josh

Added tag d7ux.

Authenticated users won't see the toolbar at all unless they have permissions, which seems unlikely on anything but intranet sites, so for me the main thing would be an 'admin user' set of shortcuts, but make sure there's the ability to show more. Adding another role to core we can discuss in a separate issue, shouldn't let that discussion affect work here.

Couple of implementation questions - if we're having shortcuts per role, then we probably need to either merge shortcuts for users with more than one role, or add role weighting so one can have precedence. Not sure about either of those options in relation to this (although there's an issue for role weighting somewhere).

Subscribing.

I think we really need to see some traction on this issue this week, or figure out what else to do with that toolbar (such as remove it).

Is there an easy way we could offer the user to choose an icon to his custom shortcut? The icons could be predefined - I know this from Software like Freemind or Mindmeister. So technically it could be a sprite that contains all options.

Installing the module after applying my patch is giving me a WSOD, not sure why. But, I've completed the schema and most of toolbar.module, now what's left is interface.

Fixed the problem, patch forthcoming.

Here is today's progress - http://screencast.com/t/DStGmKZbXQm

whoops, without that content_types.js hunk

very slightly better JS logic

I'm having trouble getting the submit function to fire. it's not related to the fact that this form has a custom theme - somehow, drupal_get_form is getting an empty $_POST array. However, when I print out the $_POST array before the page loads (e.g. before drupal_bootstrap in index.php), everything looks fine.

Oops, found the problem. Switching sets now works. Next up: adding the link next to the title (I need the graphics please) and saving the main page. Then editing and deleting shortcuts, last tests.

Could you clarify why you are using a custom implementation for this rather than trying to add a "per-user" capability to the menu module or perhaps block module itself?

I realize we're painfully close to code freeze, but at first glance it seems to me like the toolbar module really should integrate with existing systems in Drupal rather than coming up with its own entirely separate system. And a more generic per-user capability is something that might be reusable in many places -- for example, the possibility of having per-user dashboards in #544360: D7UX dashboard module rather than being limited to a single site-wide admin dashboard.

Personally, I'm OK with using a custom storage model. I don't see why we need to force this into the menu system, especially because we don't want to leverage the menu system's UI.

Tagging.

I think the text "Add shortcut" is a little unclear. It almost implies that you're adding a new shortcut or link to the current page you're on. Maybe a change to "Add to shortcut bar" or just "Add to shortcuts" would be better?

Also, I'm getting an error when attempting to edit or delete a shortcut:

Notice: Undefined index: toolbar_admin_shortcut_delete in drupal_retrieve_form() (line 458 of /Applications/XAMPP/xamppfiles/htdocs/drupal/includes/form.inc).
Warning: call_user_func_array(): First argument is expected to be a valid callback, 'toolbar_admin_shortcut_delete' was given in drupal_retrieve_form() (line 474 of /Applications/XAMPP/xamppfiles/htdocs/drupal/includes/form.inc).

More Awesomeness!

The link text is not very clear..... I could not imagine what would happen by clicking on the link

My suggestion would be: "Add shortcut to toolbar" (does a new user know difference between toolbar and shortcut bar?)

Now i have added "People" to the shortcut bar. I still see "Add shortcut" link on the page. When I click on it, I see "People twice in the shortcut bar.

Get the same error when i want to delete the second "People" on the "Edit shortcuts" page.

This needs to be a separate module.

I'm just saying that a shortcut/bookmark functionality like this should live in a separate module, because the functionality is not bound to Toolbar module. There are other modules in contrib, for example, Administration menu, which would be able re-use the data without having the Toolbar enabled, if the shortcut functionality is properly separated.

Hence, just move the functions into a shortcut.module.

Seems like it wouldn't be too hard to have the shortcuts and expand/collapse buttons only appear if module_exists('shortcuts'); - or have shortcut.module as a requirement for toolbar.module

The link will change (it will look like the mockup) - that link it just to ensure that the functionality works. re: the delete page - that's not yet implemented - i'll upload a patch for that in a few minutes.

Ok this should work, now I just need tests and the graphic.

other than the icon and the fact that this needs tests, please review

This really looks like shortcut.module and a dependency for toolbar to me. Will try to review functionality later. Also can't see that an icon should be a dependency - could just use a link and fix after freeze IMO.

I did the graphic myself and i think it came out really well, but i can't seem to install head. will try again tomorrow morning. If anyone can install head, please please PLEASE try out this patch.
The graphic belongs in modules/toolbar/toolbar.png

I don't see how the fails have anything to do with the patch...

Most functions header comments are not following the coding standard. They should all start with a verb in the third form, even the one-line header comment. This also concerns the theme functions AFAIK and since recently hooks implementations ("Implement" should be now "Implements" according to the current version of Coding standards).

+++ modules/toolbar/toolbar.admin.inc	31 Aug 2009 01:43:04 -0000
@@ -0,0 +1,294 @@
+// $Id$
+
+

I think you can remove one of those new line.

+++ modules/toolbar/toolbar.admin.inc	31 Aug 2009 01:43:04 -0000
@@ -0,0 +1,294 @@
+ * @file Administrative items for the toolbar module.

The description should be on a new line under @file.

+++ modules/toolbar/toolbar.admin.inc	31 Aug 2009 01:43:04 -0000
@@ -0,0 +1,294 @@
+function toolbar_admin_shortcuts_submit($form, &$form_state) {
...
+function toolbar_admin_shortcuts_switch_set_submit($form, &$form_state) {
...
+function toolbar_admin_shortcut_edit_submit($form, &$form_state) {
...
+function toolbar_admin_shortcut_delete_submit($form, &$form_state) {

Missing function header.

+++ modules/toolbar/toolbar.admin.inc	31 Aug 2009 01:43:04 -0000
@@ -0,0 +1,294 @@
+    'path'   => $_GET['link'],
+    'name'   => $_GET['name'],

Hmm, this is not passing though FAPI, shouldn't we be more careful with the values used here ?

+++ modules/toolbar/toolbar.admin.inc	31 Aug 2009 01:43:04 -0000
@@ -0,0 +1,294 @@
+/**
+ * Switch toolbar sets.
+ */
+function toolbar_admin_shortcuts_switch_set(&$form_state) {

It could be before another function, but you should describe how are handled toolbar sets and what they are.

+++ modules/toolbar/toolbar.admin.inc	31 Aug 2009 01:43:04 -0000
@@ -0,0 +1,294 @@
+/**
+ * Shortcut editing screen.
+ */
+function toolbar_admin_shortcut_edit(&$form_state, $shortcut) {
...
+/**
+ * Shortcut deleting screen.
+ */
+function toolbar_admin_shortcut_delete(&$form_state, $shortcut) {

These should have complete function headers, you are not describing the parameters.

+++ modules/toolbar/toolbar.admin.js	31 Aug 2009 01:43:04 -0000
@@ -0,0 +1,100 @@
+/**
+ * Handle the concept of a fixed number of slots.
+ *
+ * This behavior is dependent on the tableDrag behavior, since it uses the
+ * objects initialized in that behavior to update the row.
+ */
+Drupal.behaviors.shortcutDrag = {

Should also start with a verb in the thrid form, even if it is Javascript :)

+++ modules/toolbar/toolbar.admin.js	31 Aug 2009 01:43:04 -0000
@@ -0,0 +1,100 @@
+/**
+ * Make it so when you enter text into the "New set" textfield, the
+ * corresponding radio button gets selected.
+ */
+Drupal.behaviors.newSet = {

Would be more clear like this : Selects the corresponding radio button when user enters a text into the "New set" textfield.

+++ modules/toolbar/toolbar.module	31 Aug 2009 01:43:04 -0000
@@ -39,6 +123,20 @@ function toolbar_page_alter(&$page) {
+    $page['content']['add_shortcut']['#markup'] = l('+ Add to shortcuts', 'admin/settings/shortcuts/add-link', array('query' => http_build_query($query)));

The text of the link should be localized with t(). And maybe not include the "+" which is a visual aid, but for people with screenreader it is not really helpful.

+++ modules/toolbar/toolbar.module	31 Aug 2009 01:43:04 -0000
@@ -153,7 +387,7 @@ function toolbar_menu_navigation_links($
+      $links[$item['link']['mlid']] = $link;

Why are you removing the path based CSS ID ? It is buggy #551172: Use path based CSS classes instead of ID to style toolbar items individually (for icons) (reviews appreciated) but I don't see why you would remove it in this patch.

I leave it "CNR" to let you retest your patch, but you should set it to CNW if the testbot does not do it for you ;)

I got a chance to take a quick spin with this, only as a user viewing the user experience.

It was easy enough to figure out how to use it.

It is very disconcerting that when you click the "Add to shortcuts" button you *always* get a new element named "shortcuts", pointing to admin/settings/shortcuts. I would expect a form to open with the title and link fields ready for editing, rather than an unuseful pre-created link.

This really looks like shortcut.module and a dependency for toolbar to me.

uhm, please no dependency on Toolbar. That would be the same as stuffing the code right into Toolbar module, because Toolbar would have to be enabled for the Shortcut module.

I agree that we need to implement this in a reusable way. As mentioned above, doing it within the menu system seeems like a serious option that is important to explore.

So, here's an alternative patch that implements things via the menu system (but still uses a large part of @dmitrig01's code, especially the theme-related parts). I think having the patches side by side will help compare the two options.

Possible advantages of this patch:

1. Any menu in Drupal can now be customized per-user (via the API, although only the shortcut menu currently implements a UI for it).
2. A clean, simplified user interface similar to those discussed above for the shortcuts is now available for any menu. I separated the menu editing screen into two tabs, Basic and Advanced (see screenshots), where Advanced has all the complicated tree-related stuff that Drupal has now, but Basic just focuses on rearranging and editing the top level items in the menu.
3. I'm not 100% sure about the plans for getting icon support into this issue, but with my approach, if we add icon functionality then every menu item in Drupal will automatically support optional icons, which is a huge win.
4. Overall, this patch has less code, and no duplication of functionality.
5. I did not include any kind of "limited slots" feature in this patch. I don't think it makes much sense, since in either patch we are talking now about generalizing the shortcut feature so it's reusable, and therefore we cannot assume these shortcuts are going to be used in a particular way in the toolbar. In general, Drupal core does not take the approach of limiting people based on assumptions about how they will be using a feature, and I don't see why we should do that here.

Things that don't work right yet:

1. Like the other patch, this one puts code in the existing toolbar.module rather than creating a new shortcut.module, but that's not hard to fix later. There isn't really that much code that would even need to move.
2. In the "Basic" user interface, I did not implement separate "Enabled" and "Disabled" regions like in the earlier patches. Mostly because I'm not good enough at JavaScript to easily figure out how to do it :) But then, I actually started wondering if it would even be necessary. You can access the "disabled" functionality via the Advanced tab if you really need to, and it seems to me like "delete" is generally more useful, and that is already included as can be seen from the attached screenshots.
3. HEAD is somewhat broken for me right now; in order to get any drag and drop behavior working (either with or without this patch), I had to reverse apply the patch from #444344: jQuery .once() method for adding behaviors once first.

Anyway, I don't mean to hijack this issue or anything :) But I think we should consider the two approaches and make a decision so that the code here is as effective and reusable as possible.

@sun #48 - I mean make the toolbar module depend on shortcuts rather than vice versa.

Since we've got a perfectly good mechanism for storing links to internal pages in Drupal, it makes sense to rely on the menu system here rather than bypass it, having said that I've not looked at David's patch at all.

I've looked at dmitri's patch and am reviewing David's now, but in the meantime, here's a reroll of David's that applies to HEAD.

I think it is probably wisest to reuse the menu system, rather than duplicate half its functionality. I have a few concerns:

* At the moment, the shortcuts in the toolbar don't change when I edit the shortcut menu. Not sure what's up with that.
* I'm a little iffy about the term "parent" for menus. "Parent" is already specific in the menu context, and this is an entirely different meaning for it.
* It seems odd to create an entirely separate "basic" UI just for toolbar shortcuts. I don't suppose there's any way around it. Maybe we should call it what it really is -- a UI specifically for shortcut menus -- instead of basic/advanced? Just thinking out loud here.
* How will per-user menus scale? Are we going to end up with 200 menu blocks exposed on a site with 200 toolbar users?

Mostly, my concern is putting this feature in core hours before code freeze, when only a handful of people have even tried out either of the extant patches. I guess there's nothing to be done about that, unless we kick the toolbar out into contrib. Certainly if the toolbar is in core, this needs to be, and I think the menu-based approach is the most sensible one.

* At the moment, the shortcuts in the toolbar don't change when I edit the shortcut menu. Not sure what's up with that.

Probably you've already edited the shortcut bar, which means that the one in the toolbar is your own personal version, not the sitewide one that shows up in the Menu Module UI.

We'd need to fix that confusion by putting something like the following when you try to edit the global version of a customized menu: You have a custom version of this menu which you can <link>edit instead</link>.

It seems odd to create an entirely separate "basic" UI just for toolbar shortcuts.

Maybe it's just me, but I feel like I'd use that UI all the time. 90% of the time when I'm editing a menu, I only care about the top-level items, not the entire tree with all its complex options.

* How will per-user menus scale? Are we going to end up with 200 menu blocks exposed on a site with 200 toolbar users?

In the current version of my patch, per-user menus are not listed in the UI, and are not exported as blocks. I think that qualifies as a feature, not a bug, but I'm not sure :)

I split out simple v. advanced into another patch - #564886: Remove 'expanded' checkbox from menu item overview. I'll re-roll this patch without it.

Ok, this would be great to get into D7.

There's a spelling error - /admin/structure/menu-customize/admin_shortcuts1/edit
The Admininstration shortcuts menu contains commonly used links for administrative tasks.

I can move around the links in the Administration shortcuts, but I can't add one. Well, not really. It looks like I can, but clicking on 'Add link' sends me - /admin/structure/menu-customize/admin_shortcuts1/add

Which is all fine until I hit save, when I jump from being in 'Administration shortcuts' to being in 'Main menu'.

All the links I add go into the wrong menu.

Anyways, do think that customizable links is a very good idea!

From a UX perspective, the patch is almost there, but on closer inspection there are bigger issues.
Random notes:

It's a good idea to have separate menus per user.

This functionality should be in its own module or in menu.module, certainly not toolbar.

On clicking the add to shortcuts link, where do we want to go? I suggest staying where we are rather than taking the user to the menu_customize page.

Could potentially make the menu_links table HUGE.
There is a load of duplication for each menu link.
We should look at another way of storing these custom menus, a new table is called for with the following fields (possibly):
{menu_name} {uid} {mlid}
So, instead of creating a seperate menu_name for each user's customised menu, we check the new table to see if the user has customised that menu, and if they have, pull in their list of mlids.
Would need to add rows to menu_items for paths that don't already have an mlid and delete rows that don't appear in any user's custom menus to clean this table up.
Could add a flag for each menu to allow it to be customised by users.

Users should be able to add *any* path (i.e. not already in menu_links) to the shortcuts.
Potential scenario: an admin user has a page (node) they edit often, would like to have a quick link to that.
Potential scenario: a registered user on the site has a page they like to refer to often, would like a quick link to that.
I think these are useful scenarios. It's going to be weird to users if they add certain pages to their shortcuts, but not others.

Administrators need to be able to set defaults for these menus, including defaults for roles.

> This functionality should be in its own module or in menu.module, certainly not toolbar.
There is not much left in toolbar module.

> On clicking the add to shortcuts link, where do we want to go? I suggest staying where we are rather than taking the user to the menu_customize page.
I disagree.

> Could potentially make the menu_links table HUGE.
this table is built to be huge.

> Users should be able to add *any* path (i.e. not already in menu_links) to the shortcuts.
They can.

> Administrators need to be able to set defaults for these menus, including defaults for roles.
They can.

> This functionality should be in its own module or in menu.module, certainly not toolbar.
There is not much left in toolbar module.
OK

> On clicking the add to shortcuts link, where do we want to go? I suggest staying where we are rather than taking the user to the menu_customize page.
I disagree.
This could be argued both ways, but I found it strange that it took me to the menu_customize page. If you add a bookmark in your browser, it doesn't take you away from the page you're on.

> Could potentially make the menu_links table HUGE.
this table is built to be huge.
But does all the info need to be repeated like that?

> Users should be able to add *any* path (i.e. not already in menu_links) to the shortcuts.
They can.
They can once they're at the menu_customize page. I mean, for the admin pages there's the Add to shortcuts link next to the page title, but not for normal nodes.

> Administrators need to be able to set defaults for these menus, including defaults for roles.
They can.
It's not clear how this is done. I may be being thick, but I can't see it.

Here's an updated version of the patch that I think will pass all tests.

No other changes yet.

Regarding some of the other issues above:

1. Administrators can set site-wide defaults for the shortcuts menu by going through the normal Drupal menu interface and editing the "Administration shortcuts" menu that you see there. That one is the site-wide default, and all user-specific menus inherit from it when they're created.

2. Currently, there isn't an ability to have default menus per role. However, you can sort of approximate part of that because Drupal's menu system only shows each user links they have permission to use. So if you stuff a bunch of administrative menu items into the site-wide default menu, then a user who only has content creation privileges will not see most of them on their own personal toolbar; they will only see the ones they have access to and that are relevant for them. Personally, I think that's all that's necessary for core to support directly; however, we should make sure it is possible for contrib modules to implement full shortcuts-per-role functionality. One way to do that is that currently in the patch, the toolbar module hardcodes the name of its default shortcuts menu as 'admin_shortcuts'. We could maybe add some kind of hook that allows modules to modify that.

3. In terms of duplicating information in the {menu_links} table, duplicating it is what allows each user to fully customize each menu item any way they want. I can't think of an easy way to accomplish that without duplicating it, but maybe there is.

4. I would tend to agree that the "add to shortcuts" link should appear on every page, not just admin pages. Right now, the placement of that link is controlled by the theme, and only the Seven theme implements it in the patch. So it would be easy to add to other themes, such as Garland. We should probably think about whether that's the best way to do that, or if there's a good way for the module to automatically add it to any page regardless of the theme.

5. A number of the bugs mentioned above (such as the inability to add a new menu item by clicking on "add link") probably come from the same root cause. By default, the user-customized menus are hidden and don't show up on menu lists. This is good in most cases (since otherwise the list would be overwhelmed by all the menus), but causes problems on any page that has a menu dropdown. If you're editing a menu item that's in a user-specific menu, that menu does not exist in the dropdown, so an incorrect item is chosen when you save it. I'm not immediately sure how to solve that, but maybe the answer is that you simply shouldn't see a dropdown at all when you are editing a personal menu item. There probably isn't much of a use case for moving one of these personalized/duplicated items from one menu to another...

add tag

Here's an updated version of the patch:

• Lots of general improvements to API and schema, including moving some code from the toolbar module to the menu module
• Changed "parent_menu" to "original_menu", as per #46
• Fixed the security issue hinted at in #38 (i.e., I added token validation to eliminate cross site request forgeries)
• Fixed the bug mentioned in #52 where new menu items could not be added (and related bugs caused by the same issue)
• Fixed a bunch of other bugs, including some of the smaller ones mentioned above
• Added some text to cross-link the personalized and site-wide versions of the same menu, when the user has permission to edit both

Still not done:

1. We need to pull more code out of the toolbar module to make it reusable, possibly into a separate shortcuts module, but I think @dmitrig01 may have had some ideas of how to generalize it even more so that it could essentially all be in the menu module, and that way there wouldn't even need to be a separate shortcuts module?
2. Stop relying on the Seven theme.
3. Enable contrib modules to add a per role shortcut feature (or somehow do it in core).
4. Figure out some more things with the UI -- e.g., the question raised above of what page (if any) it should take you after you added a new shortcut.
5. Tests?

I'm probably going to put aside work on this for a little while, so if anyone wants to take up those issues, please do!

need to read in detail, but Moshe pointed me to this issue.

1st thought is that having a single per-user menu for the toolbar makes sense - making every menu be per user sounds like a bit of a disaster, especially in terms of caching the menu structure.

The current patch does not make every menu per-user. It only does so for the toolbar menu. So we're all on the same page, at least I hope :)

this seems rather specific to the toolbar module - is there a common enough use case for this to much with the menu schema and access callbacks?

What's the purpose of 'original_menu'? In this use case it is always just one menu. I'm not sure I'd want these per-user menus even exposed in the menu module UI - that seems like a real usability problem for admins

this seems rather specific to the toolbar module - is there a common enough use case for this to much with the menu schema and access callbacks?

See comments by @sun above for one possible use case outside of the toolbar module.

What's the purpose of 'original_menu'? In this use case it is always just one menu.

Contrib modules could clone other menus, and without that database column I don't think there would be any way to track which menu came from which.

I'm not sure I'd want these per-user menus even exposed in the menu module UI

They aren't :)

To clarify my comment "they aren't" -- the menus are editable via the UI, but they are not linked to from any of the listings in the menu module's administration pages.

It looks to me like trying to reuse the menu system gets us down into a rathole that negatively impacts both performance, the DX experience of the menu system, and the usability of the shortcut bar.

Leveraging the menu system at the API level might make some sense although things like 'original_menu' really smell like a hack to me. The new API functions don't bring me a lot of joy either. I'm also worried about the performance implications.

Leveraging the menu system at the UI level doesn't make sense as illustrated by the fact that we had to create #564886: Remove 'expanded' checkbox from menu item overview. Furthermore, UI strings like "You have a personalized version of this menu which you can edit instead." really confuse me. All of a sudden, the user needs to know that shortcut are menus?

How does a site administrator go about editing user ABC's shortcuts bar? For example, ABC created a custom shortcut but needs help removing or configuring it?

Leveraging the menu system at the UI level doesn't make sense as illustrated by the fact that we had to create #564886: Toggle advanced settings on menu settings. Furthermore, UI strings like "You have a personalized version of this menu which you can edit instead." really confuse me. All of a sudden, the user needs to know that shortcut are menus?

I agree 100%

How does a site administrator go about editing user ABC's shortcuts bar? For example, ABC created a custom shortcut but needs help removing or configuring it?

Actually using the menu system, the site admin can type in a URL and get it right but currently there is no interface for them to do that with the non-menu version of the patch.

Hm. I just put some more thought into this. Here are the issues I see with shortcuts:

- Those shortcuts should be treated as bookmarks. Just like in a browser, the user should be able to just click a link/icon to add a new one. Clicking a link appearing on hover to remove them. (they are non-critical, so this can be done without any form or whatever). Hence, we do not expose the menu UI at all.

- I actually do not see a use-case for shortcuts; the most visited pages are contained in the upper management menu bar already.

- Albeit not per-user, the top-level management menu links in the upper bar can be considered as shortcuts already. If you want a new, you just move an item to the top-level.

- Lastly, providing a very limited set of per-user bookmarks is not really the job of a web site. Your browser already allows you to do that, so I really think we're trying to duplicate existing functionality. If users fail to use the bookmarks of their browser, then that's a usability issue with their browser, but not Drupal.

Those shortcuts should be treated as bookmarks. Just like in a browser, the user should be able to just click a link/icon to add a new one. Clicking a link appearing on hover to remove them. (they are non-critical, so this can be done without any form or whatever). Hence, we do not expose the menu UI at all.
- This makes total sense. I'm starting to see this shortcut bar as an extension to browser bookmarks, but specific to a site.

I actually do not see a use-case for shortcuts; the most visited pages are contained in the upper management menu bar already.
- We don't know which pages will be the most visited for any and every site.

Albeit not per-user, the top-level management menu links in the upper bar can be considered as shortcuts already. If you want a new, you just move an item to the top-level.
- This wouldn't be as user-friendly or obvious.

Lastly, providing a very limited set of per-user bookmarks is not really the job of a web site. Your browser already allows you to do that, so I really think we're trying to duplicate existing functionality. If users fail to use the bookmarks of their browser, then that's a usability issue with their browser, but not Drupal.
- I think it would be a very useful addition to a website.
Think of your browser bookmark bar as top-level navigation for the web
The per-site shortcut bar is secondary-level. So, once the user has used their browser bookmark to reach the site, then can continue to dig deeper to favourite pages by using the shortcut bar.

For a busy admin, or user on a site with a lot of content/screens that they visit regularly, this could be a great help.

If you're setting up a site and are having for a short while to jump between a small handful of pages (as often happens with Drupal), being able to easily add (and eventually easily remove) some bookmarks would certainly help productivity.

When I want to work on a client's site, i don't want to see bookmarks from the other client's site. With browser bookmarks, I'd always have to see both. With our solution, you'd only see bookmarks specific to your site.

Using the menu system means having to rewrite most of it from what I can figure -- we'd end up with mlids per user in the menu_link table and so on.

We can't stored just mlids per user or role because not everything that the user may want to save is an mlid: eg node/4.

So why not just store an array of path strings? If we have a 'add current page to my toolbar' link then we already know the user has access to the current path.
If roles or permisssions change then the user gets to an Access Denied page, but meh. They can just go edit their toolbar.

1. If there are specific performance concerns, it would be useful if someone could explain them in detail, so that they could be benchmarked. I'm having trouble understanding what they might be, exactly.
2. I am not sure in what way this hurts the DX experience of the menu module, but creating a separate, parallel "menu-like" API doesn't seem like it would help DX much either...

3. Leveraging the menu system at the UI level doesn't make sense as illustrated by the fact that we had to create #564886: Toggle advanced settings on menu settings

   It's very simple to substitute in a different UI for this particular menu if we really want to, but I'm not sure that's a good idea. A "menu" is basically just a list of links -- when an administrator looks around their site, they see a bunch of these, and the shortcut bar is one of them. How does it help usability to have the UI for editing some of them be totally different than the others? For example, how do you explain to someone why, if they want to edit the list of links in the top level of the toolbar, that's a "menu" and they have to go one place to do it, but if they want to edit the bottom level, that's something else entirely, and they need to go to a totally different place and use a totally different UI? Providing consistent patterns is a major aspect of usability.

   I think the decision to split out #564886: Remove 'expanded' checkbox from menu item overview to its own issue was the right one, because it is about making the menu UI simpler and easier to use. That's a good idea in its own right, regardless of what happens with the shortcuts.

4. Furthermore, UI strings like "You have a personalized version of this menu which you can edit instead." really confuse me. All of a sudden, the user needs to know that shortcut are menus?

   Anyone seeing that particular message would already know that they are menus, but the message that appears in the reverse direction, maybe not. Anyway, there certainly needs to be some kind of cross-linking between the different shortcut sets; I'm not aware of any design that considered this, so @dmitrig01 and I each came up with our own mini-design for it :) I think something closer to his might be better (basically, a "change" link inline with the name of the thing you are editing).

   Either way, there's certainly no particular reason the word "menu" has to appear in this text.

5. How does a site administrator go about editing user ABC's shortcuts bar? For example, ABC created a custom shortcut but needs help removing or configuring it?

   Like @dmitrig01 said, this is already possible; the question is whether it's an important enough use case to provide a direct UI for within core. If we go with @dmitrig01's "'change' link" implementation mentioned in the previous point, that might provide an automatic solution (but the list of links would not scale well if the site gave many different users the ability to personalize their shortcuts). Alternatively, there could be some link to the user's shortcut menu from their user account page, I suppose.

   If this is an important enough problem to solve, it probably needs some dedicated thought around the design.

6. Leveraging the menu system at the API level might make some sense although things like 'original_menu' really smell like a hack to me.

   The 'original_menu' information could be moved to a dedicated database table pretty easily (at the cost of a small amount of complexity in the code) -- then it would just be a simple mapping. Or it could be removed entirely, but then any module which wanted to keep track of these kinds of relationships would need to figure out how to keep track of it on its own...

7. Those shortcuts should be treated as bookmarks. Just like in a browser, the user should be able to just click a link/icon to add a new one. Clicking a link appearing on hover to remove them. (they are non-critical, so this can be done without any form or whatever). Hence, we do not expose the menu UI at all.

   @sun, I think there is room to run with this idea, but I feel like you would still need some other UI as a backup. It seems to me like there are four things you really need to be able to do with these shortcuts: (1) Add them, (2) remove them, (3) edit them (i.e., change their titles), and (4) rearrange them. I am not sure how easy it would be to do all of those inline, especially in an accessible way.

   It could be possible, however, to try to do something here like what is being done with #544360: D7UX dashboard module -- where there's a spiffy UI for adding/dragging things, etc, but you can still go in and use the block module UI as a backup method to configure your dashboard.

8. Using the menu system means having to rewrite most of it from what I can figure -- we'd end up with mlids per user in the menu_link table and so on.

   @joachim, why do you think that would be necessary? The existing patch works fine without doing any of that. It doesn't touch the "core" part of the menu system at all (i.e., includes/menu.inc) and only makes a few small changes to the existing code in the menu module.

My two cents in trying to understand this and address change for this design:
http://skitch.com/jeff.noyes/b6shr/administration-shortcuts-d7sandbox

Tried this latest patch with the overlay. Unfortunately it does not work well.

Any suggestions on what to do? At first glance, this sounds like a more generic problem with the overlay - since all this patch is doing is grabbing a link, saving it, and using a drupal_goto()...

Also, (unrelated to overlays), it is entirely possible to add as many shortcuts to the shortcut bar as one wants, and when the number grows high enough, the edit shortcuts link drops to the bottom of the shortcut bar instead of the top.

The rationale for not limiting the number of shortcuts is as follows:

• There is no way to detect when there are too many links to fit, so any limit would be arbitrary.
• It's unclear what we'd do in the user interface when they hit their limit and try to add another one. Any message or confirmation we put there seems like it would just get in the way.
• The current patch effectively already does put such a "message" on the screen, but in a less intrusive way. Once they add too many links, they'll notice the layout looks wrong and realize themselves that they need to remove one. Nice and simple, in my opinion.

I agree that the particular way in which the page layout breaks could be improved, but it seems like a minor problem compared to the other issues that still need to be resolved.

Finally (also unrelated to overlays), there still seems to be no way to assign/edit icons related to these items, which should be an important consideration item when building the underlying implementation.

It's easy to add something simple like an 'icon_path' database column to the underlying storage mechanism (which, as mentioned above, might be especially nice if we reuse the menu system, because it would then provide contrib modules an easy way to add icon support for other menus as well).

It does seem like there needs to be more work done on the interaction, though. It sounds like we'd need to have (a) a set of icons included with core, (b) some kind of default mapping between URLs and icons, (c) a way for contrib modules to expand on both of these, (d) a default fallback icon to use when none is specified, and (e) some user interface for people to change the icon after the shortcut was already added?

That's a lot to do, and not even getting into things like allowing users to upload their own - are we trying to support that here (and if so, does it require something like ImageField)?

Also, looking at the screenshot from @Noyz, it seems we could address most of that (if we stick within the menu system) by going back to some of the code and ideas that already exist above:

1. We could use some kind of "change" link rather than the green message.
2. We could go back to the code I had above, which had a "basic" and "advanced" tab, only the difference would be that we wouldn't actually have the tabs - rather, it would just be that a module would use hook_menu_alter() to declare that it wanted to use the "basic" interface for this particular menu. It seems like a bit of a copout compared to actually trying to make the menu UI easier to use in general (as in #564886: Remove 'expanded' checkbox from menu item overview), but it would work fine...

Here's a copy of Jeff's screenshot from #72 - for posterity's sake, it seems better to save it here than to rely on an outside website :)

I still think implementing per-user menus throughout the menu system for this is pretty horrendous, and total overkill. There is going to be a huge amount of duplication in the menulinks table, for starters.
Furthermore, I think the matter in hand calls for a per-role toolbar too.

> Finally (also unrelated to overlays), there still seems to be no way to assign/edit icons related to these items, which should be an important consideration item when building the underlying implementation.

The need for icons suggests this is not really a menu, but a slightly different kind of entity.

There is going to be a huge amount of duplication in the menulinks table, for starters.

If we have per-user shortcuts, there is going to be duplication somewhere. Why is it worse to have it in menu_links (which is optimized in a variety of ways)? As mentioned above, we can benchmark something if there are actual, specific performance concerns...

Also, it's worth pointing out that the shortcuts don't need to be per-user (earlier versions of the patches in this issue did not do that - they had different shortcut sets that you could switch between, but no per-user separation). I think that by piecing together code from the various patches in this issue, it would actually be possible to come up with a solution for the shortcuts that didn't involve changing any database tables or adding new ones, but without the per-user functionality. And for some use cases, it might actually be simpler and better (e.g., sites with a single administrator).

The reason I moved to per-user shortcuts initially was that it seemed like where this issue was heading anyway, plus the user interface that was proposed seemed to really assume that. (If I see a convenient "add to shortcuts" link on every page of my site, I would find it pretty odd if clicking that link also caused some other person's shortcuts to be modified, etc.) But, it's a possibility.

I was looking at this during the DrupalCon code sprint, and when I considered having menus per user my immediate reaction was: the core hackers will say it's crazy.
Storing this kind of data in this way just seems bats to me: it's overkill and duplication. Most menus will never get customized per user. Most users will add a couple of links and keep the defaults and the menu_links table fills with crud.

Let's take a step back here.
We're suggesting a non-trivial rejig of the menu system to get something like this module into core: http://drupal.org/project/menu_per_role, which has a mighty 2k users.
This is for a feature that on most sites will get turned off.
And being pragmatic, there are far better candidates for improvement to the menu system -- I don't know if http://drupal.org/project/menu_breadcrumb has made it into core for D7, but that is basically an essential fix for behaviour in D6 that's considered broken by most developers I speak to. Another candidate might be getting breadcrumbs on nodes to work (http://drupal.org/project/node_breadcrumb and other similar modules) -- for me, trying to navigate the DrupalCon Paris site showed up that our out-of-the-box breadcrumbs are not very good.

Furthermore, making menus per user doesn't even address the requirements of this feature! On sites that COULD make use of the toolbar, I expect admins will want to create default toolbars for different roles: regular member, editor, site admin. This strikes me as a far more useful feature than per-user; if I had to choose between per-role or per-user I would choose per-role.

I think if this feature is to go into core, then toolbar module needs to handle the customization itself -- ideally in some way that can then be extended by contrib modules for people who really want custom menus.

The best idea I can come up with is to put all links that users add to their menu into the menu_links table, and then have a masking table per user and role.
But there are better minds than me on this issue, I hope :D

Most menus will never get customized per user. Most users will add a couple of links and keep the defaults and the menu_links table fills with crud.

Just to clarify: With the proposed approach, most menus wouldn't be allowed to be customized per user, and those menus would not put anything different into the menu_links table than they do now.

Only the toolbar shortcuts would do this. (Unless you have a contrib module that allowed it to extend to other menus.) So the amount of extra data in the menu_links table would likely be small.

This is for a feature that on most sites will get turned off.

I agree this is a serious concern. Everything that is being discussed in this issue involves adding new front-and-center features to Drupal core, somewhat last minute, that have never really been tested or fully vetted. It is worrisome.

The situation we're in is that the standard Drupal "toolbar solution" is admin_menu which uses dropdowns, but for whatever reason, this D7 toolbar module decided not to do that. Without dropdowns, there is no way to use the toolbar to quickly get to links deep within your site. By itself, therefore, the toolbar would be pretty useless.

The only way it avoids being useless is via these shortcuts, which, because they are highly customizable, provide a targeted way to get to particular, important links deep within your site. It's an interesting idea, but whether people will use it or not, I really have no idea.

It seems like the issue to really discuss that all is at #543914: Administration menu or admin_menu should be in core ... but if the current D7 toolbar is to be of any use at all, I think it needs this shortcut feature and needs it to work well.

On sites that COULD make use of the toolbar, I expect admins will want to create default toolbars for different roles: regular member, editor, site admin.

I think if we really want to have toolbar-per-role functionality within Drupal core, the best way to do that involves making the toolbar into a block. I know that was discussed and rejected elsewhere though, for different reasons (I don't have the issue number handy).

With blocks, you get per-role visibility for free, and then, figuring out which shortcuts to show to a particular user is just a matter of the toolbar module having a sortable list - the default toolbar that each user sees is the first one on the list they have access to (we're doing/proposing something very similar for text formats in D7 in #11218: Allow default text formats per role, and integrate text format permissions, and it works well).

Without the toolbar being a block, is it really worth coming up with a totally different per-role method?

Remember - it was mentioned above but worth repeating - one advantage of using the menu system is that menu access gives you a rudimentary form of "per role" toolbars for free, as long as each role's toolbar is a subset of the main one. For example, suppose you create a single toolbar for your site that contains five items: "Add Content", "Find Content", "Permissions", "Blocks", and "Menus". An administrator sees all five items. But if you then make an Editor role on your site who only has the relevant permissions for administering content, their toolbar will automatically contain only the first two items - no extra work required. I think it would not be so bad for Drupal core to stop there (as long as there is a way for modules to expand on it).

The best idea I can come up with is to put all links that users add to their menu into the menu_links table, and then have a masking table per user and role.

It seems like this would save some rows in the table, but at the cost of some complexity. What happens when a shared menu link is edited - do all users automatically see the changes, or if not, do we have to find some way to avoid that happening? (Not sure if I understand your proposal completely, but it seems to me that would be an issue.)

I don't think it's a bad idea per se -- as long as we use the menu_links table, we get all the benefits of the menu system (using the menu_custom table or not, by contrast, doesn't matter as much). But it seems more complicated, unless we really have a reason to be very concerned about not adding some extra rows to the menu links.

i think if a shortcut link is edited, it probably should affect everyone - say you move your custom admin panel to a different path or whatever, and we don't need to allow for per-user icons and text IMO. In fact that's almost an argument for a uid | path or uid | mlid mapping table, so that admins can easily go in and change stuff like this for people in an editor role. However we'd need to handle removing a shortcut so it first deletes rows from the mapping table then eventually the link if no-one's using it. Probably neither approaches are ideal.

With the current approach, even with 100 users having access to the toolbar, we're only talking about a maximum of 1,000-ish entries in {menu_links}, that's comparatively nothing. Note that book module adds a load of entries to {menu_links} too, which more or less 'duplicate' the bid/pid table and that works fine.

Most menus will never get customized per user. Most users will add a couple of links and keep the defaults and the menu_links table fills with crud.

I think that's a sweeping generalisation and certainly not what the testing we've done thus far would indicate. I think for most Drupal developers, that may be the case. In fact, most Drupal developers would probably turn this off completely. Which is fine. It's primarily designed for a completely different audience.

At this point, I'd like to reiterate - again - what the toolbar is, and who it is for.

The situation we're in is that the standard Drupal "toolbar solution" is admin_menu which uses dropdowns, but for whatever reason, this D7 toolbar module decided not to do that. Without dropdowns, there is no way to use the toolbar to quickly get to links deep within your site. By itself, therefore, the toolbar would be pretty useless.

How so? The toolbar is designed not to be a replacement for admin menu. Neither is it an an IA builder with unlimited options and flexibility. The toolbar is there to help content creators, or people administering sites. It's designed to provide quick links down to specific, repetitive tasks. It's also designed to be limited.

This issue - #543914: Administration menu or admin_menu should be in core - is completely separate. As the underpinning UX strategy of the shortcut bar is completely different to what the admin menu does.

As to how it's implemented, then that's up for discussion.

1) Seeing the impact of this patch on the menu system, I think it adds a major WTF to a system that is totally complex already. Forking an entire menu into another and optionally assigning that to a specific user looks like an edge-case the menu system was not built/intended for. The data in these tables is already one of the most challenging performance hits for Drupal sites. -- Don't get me wrong. We definitely have a great need for context support in menus. But it's definitely not a per-user context; instead, we have a need for per language context, site context, role context, domain context, etc. pp. The current approach hacks a single per-user context into the menu system, which means to hack a single use-case into a system that was meant to be as generic as possible.

2) Shortcuts neither can be expanded, nor need descriptions, nor need to be disabled, nor need a parent link. All they need is a title, path, and weight. Whereas the user shouldn't have to type or specify any path, because you want to add the current page and not an arbitrary one to your shortcuts.

3) After talking to Dries in Paris about this issue, it seems like per-user personalization may be wanted for smaller sites. To make any sense of this on larger sites, however, those sites would rather want to have different shortcuts per role, which may or may not can be overridden per user. Thus, we need default shortcuts that can be optionally specified differently per role.

4) Summarizing the above, we want to

a) build a settings page where default shortcuts can be specified (optionally per role)

b) use an own storage for those shortcuts

c) use an own UI for those shortcuts

5) Stop thinking about icons unless you want to waste your time on an utopian feature for D7.

FWIW, I do not agree that Toolbar does not make sense without those shortcuts. I can drive my car without a cup-holder, too. Drupal has to be usable without such helpers.

That sounds like dmitri's patch from #20 to me, although this issue had already gone towards the menu system approach before I had a chance to look at that one.

@markboulton: Would it be possible to provide more details here of the testing that has been done on the customizable shortcuts feature? (I think any results would be useful, as we seem to be stumbling around this issue a bit trying to decide which components are the most important.)

i think if a shortcut link is edited, it probably should affect everyone - say you move your custom admin panel to a different path or whatever, and we don't need to allow for per-user icons and text IMO.

@catch: What would you do about weights? Editing text and icons might not be necessary, but I think users need to be able to sort their own shortcuts.

And is moving the admin panel to a different path common enough to worry about? Worst case, people can delete or change back from their personal shortcuts to the sitewide one. Also, it is not hard to bulk-update a bunch of menu items (by which I mean, the database query is not hard).

5) Stop thinking about icons unless you want to waste your time on an utopian feature for D7.

@sun: So is the alternative to release D7 with the big gray boxes instead? :)

Using the menu system, icons shouldn't be a big problem - it's really simple to add an "icon path" key in hook_menu() and then each module's URLs can very easily specify an icon, or automatically inherit defaults from their parents if they don't. Plus hook_menu_alter() can be used to change it. Compared to the other issues here, this isn't complicated to implement.

I think you're probably right that a UI for changing or uploading these icons is not going to happen for D7 core, but that would not be a big deal.

To make any sense of this on larger sites, however, those sites would rather want to have different shortcuts per role

So if there's agreement that only larger sites would need more per-role granularity than what was described previously, it seems OK to delegate that to a contrib module?

I think it's actually pretty simple to make any of the patches in this issue compatible with per-role in contrib. For example, in the latest patch, it might even be as easy as changing this:

return menu_get_menu_name_for_user('admin-shortcuts');

to this:

return menu_get_menu_name_for_user(variable_get('toolbar_shortcuts_default_menu', 'admin-shortcuts'));

Shortcuts neither can be expanded, nor need descriptions, nor need to be disabled, nor need a parent link. All they need is a title, path, and weight.

And the same is true for many other menus on a typical Drupal site. At least from the end user's perspective, it's especially true for the top level of the toolbar (which has an entire tree underneath it that is visible when you edit the menu, but none of that tree is ever going to display in the toolbar). There's no particular reason shortcuts are a special case. That was the rationale for moving #564886: Remove 'expanded' checkbox from menu item overview to a separate issue - to at least try to solve the problem for real rather than working around it by inventing an entirely different UI for this one particular use case.

Don't get me wrong. We definitely have a great need for context support in menus. But it's definitely not a per-user context; instead, we have a need for per language context, site context, role context, domain context, etc.

Well, sometimes code is not hard to generalize after it is written. Notice, the new functions in the latest patch basically take $account as a parameter and are hardcoded to check $account->uid against a 'uid' column in the database. Maybe it's not so hard to change that to $key + $object as parameters, and then compare $key and $object->key to two separate columns in the database? That's a fair amount of context support right there.

Probably we don't want to generalize this more than necessary. So if it's too hard to correctly use the {menu_custom} table, then so be it. But overall, there are a variety of options which range from not using the menu system at all to using it completely and we need something along that continuum. Using the menu system in general has these advantages:

1. Easier support for icons (see above)
2. Automatic access checks on each link (although writing this, I'm realizing that even if you don't use the menu system for storage, you can still use it for access checks without too much work)
3. Caching
4. Reusability (e.g., the main shortcuts are automatically a block which can appear elsewhere on the site)

This needs to be balanced against other considerations, but going too far towards ignoring the menu system seems like it leads to a lot of duplicate code that is not very flexible.

Also, I should not have said that the toolbar is "useless" without these customizable shortcuts - that's too strong, and I apologize. But I think to be such a front-and-center feature of core like this, it should really be targeted at a wide group of people. With easily customizable shortcuts, it can be, because the shortcuts you see every day adapt as you do. Without them, it's less useful for everyone.

> It's designed to provide quick links down to specific, repetitive tasks. It's also designed to be limited.

This is what makes me think this shouldn't be a menu -- and all that sun says in #85.

The menu is a hierarchy of locations.
The toolbar is just shortcuts to paths.

If we stick with menus, and want to attach icons, what about something like http://drupal.org/project/menu_attributes then? (note I haven't reviewed the code).

It looks like that module uses the existing 'options' column in the database, which would work, I think. But the possible advantage of adding a new column for the icon is that you get inheritance - e.g., core could define a default icon for admin/config/people/* and a different default icon for admin/config/media/*, and any module's pages that exist under those paths would automatically use the default icon for that section (unless they provide their own).

Rereading the above comments, it also occurs to me that this isn't actually quite true:

Shortcuts neither can be expanded, nor need descriptions, nor need to be disabled, nor need a parent link. All they need is a title, path, and weight.

First of all, descriptions could actually be useful (since they can be made to appear as hover text). Second, while we don't seem to need parenting features directly here, they would be necessary to support something like "bookmark folders", as in Jeff Noyes' design at http://blip.tv/file/2304921 (which was linked to near the beginning of this issue). It doesn't seem like we're likely to implement that design here, but it's a very standard "shortcut" functionality (used in many web browsers, for example), and we should support contrib modules adding that.

*****

Anyway, we really need to come to some sort of decision here. I think we definitely need to use menu_links. In theory, I think we should be using menu_custom also, but the arguments for that are less overwhelming - and it would involve more API changes which at this point in the development cycle we probably want to avoid if at all possible. Based on some comments above, it sounds like we really could model this after the book.module (which uses menu_links but otherwise has its own custom system) and that wouldn't be so bad.... we could then try to improve it further in Drupal 8, so that both the book module and this could use even more of the menu API.

Re. icons:
We need a non-icon fallback that's better looking than the current situation anyway. Customizable means that links for which no icon is defined will get added. I'm not a coder, but from mentoring the icon.module GSOC project last year and talking with devs who worked on this, it seems unwise to 'make icons work in core' part of this issue. It's hard. I'd strongly advise to make it a seperate issue as it burdens the already broad scope of this discussion.

testbot?

Okay, I think this at least brings the patch back to working order.

It was a lot of cut-and-paste because many hunks were failing, apologies if I missed anything.

Some APIs changed and some urls, but it looks right now. I'm not familiar enough yet with the current intention to decide if it is 100% back, but at least no errors, and I can edit shortcuts for my user :)

Okay... Jumping into the fray :) Looks like this is a pretty contentious issue. I've tried to read everything up to this point and grok it. I realize I'm restating some arguments from before, but I just wanted to get a reset on these topics, because it feels like some resolution here is needed before we can make meaningful progress (and of course, I want to get my 2 bits in too).

Per user vs. per role

I think there is a much bigger use case for Per Role menus. In an organization, you have editors which edit stuff, writers which write stuff, admins who admin stuff, and a users who read stuff. They have different primary tasks, hence this issue. If a user needs their own bookmarks, that's what the browser is for. It's a better interface, and is accessible from anywhere, why are we replicating it?

Menu per Role is a popular module for a reason, lots of clients want this functionality. It is cleaner to implement from a front-end and backend standpoint and has no performance implications (unlike the menu-per-user one which IMO does).

There is the bookmarks module, and this sounds to me like something which belongs in contrib.

Icons

We should start a separate issue for this. It is generally a very good idea, and not really part of this.

Use of menu system

I can see both sides, but I have to say, it's duplicating code and making a new cluster-f of an API to do it any other way.

Proposal for Menu per-role and how it works

• Admin defines multiple shortcut bars or just edits the main one
• Admin defines which roles can use which bars (this would be menu_role in core)
• When a user logs in, they get the shortcut bar the admin has picked for them, if there are many, they get the first one
• If there is more than one shortcut bar available, user can choose a different one

If we implement it this way, we can do it cleanly (add a menu_role table with a weight), provide for the biggest use case, and provide functionality a lot of sites want anyway (menu per role).

I can't see a big performance hit, and because we have a n:n relationship between menus and roles, there can be some level of per-user choice involved (if still pre-determined by the admin).

Some stuff from looking at the code:

+++ modules/menu/menu.install
@@ -58,6 +58,31 @@ function menu_schema() {
+      'personalized' => array(
+        'type' => 'int',
+        'not null' => TRUE,
+        'default' => 0,
+        'size' => 'tiny',
+        'description' => 'This field is set to 1 for menus that are personalized for a particular user, and 0 for site-wide menus shared by all users.',
+      ),
+      'uid' => array(
+        'type' => 'int',
+        'not null' => FALSE,
+        'description' => 'The {users}.uid that this menu is personalized for. This field is only relevant if the menu is personalized.',
+      ),
+      'original_menu' => array(
+        'type' => 'varchar',
+        'length' => 32,
+        'not null' => TRUE,
+        'default' => '',
+        'description' => 'The name of the menu that this menu was cloned from, if there is one.',
+      ),
+    ),
+++ modules/menu/menu.module
@@ -477,21 +692,27 @@ function menu_node_form_submit($form, &$form_state) {
  *   titles as the values.
  */
-function menu_get_menus($all = TRUE) {
+function menu_get_menus($include_system_menus = TRUE, $include_personalized_menus = FALSE) {

I don't love this approach, but don't have a better idea at the moment (if we're trying to support menus per user). But even if we do go this route, why two columns (personalized and uid?).

+++ modules/menu/menu.module
@@ -369,10 +581,13 @@ function menu_node_prepare($node) {
-        $mlid = db_query_range("SELECT mlid FROM {menu_links} WHERE link_path = :path AND module = 'menu' ORDER BY mlid ASC", 0, 1, array(
+        $mlid = db_query_range("SELECT ml.mlid FROM {menu_links} ml INNER JOIN {menu_custom} mc ON ml.menu_name = mc.menu_name WHERE ml.link_path = :path AND ml.module = 'menu' AND (mc.personalized = 0 OR mc.uid = :uid) ORDER BY mc.personalized, ml.mlid ASC", 0, 1, array(
           ':path' => 'node/' . $node->nid,
+          ':uid' => $user->uid,

I don't mean to spread FUD, because I haven't profiled this, but this query will likely get called a lot right? The conditional logic in it will slow it down at least somewhat, perhaps more than a little if you have a lot of users customizing their menus.

I'm on crack. Are you, too?

Just talked w/ David and Gabor. Here's what we were thinking:

Not per role or per user

These can be done in contrib (and are).

Per role is widely used, however not applicable in 90% of sites (the dinky ones)
For organizations with multiple people using this feature, they are probably using different roles. Shortcuts by roles makes sense, by user does not. I.e. the sales team has common tasks, each sales guy doesn't

Per user creates kinda hacky implementations in the menu structure
Since the use case of individual users needing to use Drupal to manage thier bookmarks for a given site seems low enough, and the code to implement is nasty enough, we were thinking this should also be done in contrib. This also does little to benefit them because they cannot inherit changes from the parent they cloned from.

This is for the "WTF did that go?" use case

The Navigate module does a good job of this, and is popular with new admins to Drupal. It's easy to forget where that settings page is, or how to get to the user listing sorted by email or whatever. For these people, we want to make a simpler implementation of the menu module that is usable by non geeks and allows you to just "add this page to shortcuts"

Implementation

Dead simple now. With one site-wide shortcut bar, we just need to provide a customized interface for re-ordering / editing the shortcuts, and a button to "add this page to shortcuts". We use all the underlying menu structure, this is just a "special" menu with special theming status.

We also need to make it flexible enough so that contrib modules like menu per role or menu per user would be able to dynamically change which menu was loaded.

Sound good? So to wrap up:

• One site-wide shortcut bar
• Simplified admin interface for changing the shortcut items with the following:
  • Remove the Show Expanded column (why is it there anyway - rarely used)
  • Make it so you cannot drag a menu item to be a child of another item
  • Change all language to use the words "shortcut" and "shortcut bar" instead of "menu item" and "menu"
• A link on every page with a "Add to shortcut bar" button
• We ensure that Menu per user and Menu per role stuff is possible in contrib - we build this by exposing whatever is necessary to customize the "add to" link to go somewhere different, and that the shortcut bar can be changed dynamically

I get the impression that this proposed direction is informed mostly by time constraints and not so much by the minimum use case. (understandably so).

"Per role is widely used, however not applicable in 90% of sites (the dinky ones)"
I don't understand this sentence, at all. Per role is used a lot, but it is not? Immediately thereafter you say they do make sense again. Are you dissing 90% of the Drupal sites out there? I don't get how this helps getting your point across.

Anyway.

I assume the shortcuts would still be visible according to the user's permissions? If so, the visible shortcuts would still be different for each role? I could then add a lot of shortcuts to the toolbar and through permissions and roles the toolbar would still look different per role (different sub-sets of the 1 main set). Is that what would be happening here? If so, then +1.

"Per role is widely used, however not applicable in 90% of sites (the dinky ones)"

I guess what I meant was, most of the 10% of sites who have more than a couple admins need something like that. Although it is no where near the majority of sites, in terms of the size of contracts / participation in the Drupal economy, those sites are very important. So, widely used among big sites, but not needed on many others and easily provided in contrib.

-J

I get the impression that this proposed direction is informed mostly by time constraints and not so much by the minimum use case. (understandably so).

Partially. However, if you define the minimum use case as a site with a single administrator, then the proposed direction is actually perfect for that.

Basically, I think we just came to the conclusion that there is no way to satisfy everyone... so core shouldn't try to :) Depending on the site, there are valid use cases for per-user, per-role, or the "shortcut sets" idea from the original patch (which is kind of a hybrid between the two). It's not even so much about the size of the site, but more about how it is structured - generalizing a bit, sites which are more "hierarchical" have more need for per-role shortcuts, while sites which are more decentralized (think drupal.org) have more need for per-user. Complicating things further is that the "add to shortcuts" button (which is probably the most interesting thing about this feature) can get messy for some of these use cases.

Ultimately, then, the best thing we can do for core right now is to provide the simplest solution possible and let contrib modules build off of it for specific use cases.

I assume the shortcuts would still be visible according to the user's permissions? If so, the visible shortcuts would still be different for each role? I could then add a lot of shortcuts to the toolbar and through permissions and roles the toolbar would still look different per role (different sub-sets of the 1 main set). Is that what would be happening here? If so, then +1.

Yup, exactly! I tried to say that somewhere in a previous comment above, but understandably it got lost in the voluminous discussion :) What it means is that a site can actually take things pretty far with core before it reaches the limit where it would need to move to a contrib module.

User interface:

We will probably progress best if we stick to something that looks as similar to the normal menu interface as possible, but simplified where necessary, exactly as Jacob said above. A key point is that we don't want to totally reinvent the wheel here, because the last thing Drupal needs is yet another UI pattern for people to learn in addition to the ones that are already there :) Another key point is that hopefully, most users won't need to visit the UI much at all (the "add to shortcuts" button itself provides most of what they need).

For a start at how to form_alter() the menu UI to make it simpler for the case of the shortcuts, note that #42 had some code along those lines (at the top of the patch file). If we can farm out a simplification or two to the menu system as a whole (for example, the "show as expanded" row of checkboxes is pretty pointless for all menus, in my opinion), then we can try that at #564886: Remove 'expanded' checkbox from menu item overview and so much the better, but for here, we can focus on form-altering as the quickest way to get the shortcuts the way we want them.

Icons:

I could have sworn someone already spun off a separate issue about that, but apparently not, so I went and dug up #42493: Enable custom bullets/icons for specific menu items and posted a potential patch there just now. This is hopefully a lot, lot simpler than the icon module discussed in #91 -- that's a really neat module, but what it's trying to do (generic sets of icons that can be swapped between themes) is much more complicated than what we need here :) The idea of the patch I posted at #42493: Enable custom bullets/icons for specific menu items is to provide a default set of icons that themes can use if they choose, or ignore/replace if they want to, so hopefully we will still have a chance of getting something in.

Okay, I commented and/or posted to the other two issues:

#42493: Enable custom bullets/icons for specific menu items
and
#564886: Remove 'expanded' checkbox from menu item overview

Both of these are useful in their own write, and don't need to be duplicated code here.

This patch just got a lot smaller. It no longer patches the menu module at all, just adds the interface elements to toolbar.module. It still lacks a good way to be extended from the outside I think. Before we commit this, I think it would be nice to brainstorm how a contrib module would solve some this common use case:

I have a team of Moderators and I want them to be focused on the task at hand. The shortcut bar helps them get to their most frequently used screens quickly, and makes documenting their job / training a snap. When I want to add a new link to this bar, how do I do it while not screwing up the bar I have defined for the Translators.

-J

Helps if you include the patch

d'oh, left in my patch that makes CLI installs in core work again. :(

Btw, that needs some love:
#557542: Cache module_implements()

Here's another go

eek.. all kinds of skeletons, sorry, tried to sneak one in at the end of the day :)

Here's a slightly better version. BUT there are two outstanding issues AFAICT:

1. We have a separate perm to "create shortcuts". Right now, it is pretty useless IMO. How do we reconcile this with giving someone permission to edit ALL menus. Does it matter at this point? Should we just ditch the perm, make the links go straight to the menu page, and make this about just providing a quick way to build / edit a menu? (If so, I've got not problem with that - sounds good to me).

2. How do we make the "shortcut" construct extend-able in contrib? As I mentioned in my last comment, there is a use case we should be able to solve in core.

How to set which bar to show Currently, I used a var, because I figured people could mess around with $config, but this isn't great. The only other option though is to do something like this $shortcut_menu = array_shift(module_invoke_all('shortcut_bar')); or something. That's also not pretty, because only one can win, and we already use vars for primary and secondary menu.

How to change the behavior of the "add to" button
I was thinking the best thing here would be to include a place to specify an optional interim form, where a contrib module could ask the question: "Apply to the global shortcut bar or the bar for Moderators", etc.

But in that case, maybe the best thing to do is to just let them override the menu item.

Best,
J

I like the smaller patch size as well - although it's cheating a bit, since we don't yet have any of the potential code in there that would form_alter or otherwise change terminology in the menu UI for the shortcuts :)

Let's definitely keep it simple: If someone wants to override the "add to shortcuts" button with a form, they can do it via hook_menu_alter(). As long as we provide a good API for adding the shortcuts, that definitely sounds like the best way to go.

Also, using a variable for the shortcut menu name is probably the simplest method we have for making sure it is infinitely configurable. In addition, the shortcut menu itself is added to the overall page array, so hook_page_alter() is another method that modules have to dynamically replace it. (Not a great one, though, since it means two menus would get generated during the page request, even though only one is used.)

I'm not sure exactly what's planned for terminology changes but what we're adding here is a not-yet-graphical shortcut menu with some extra features attached. In that case, it seems completely reasonable to allow that to be called 'the shortcuts menu' and have 'menu links' in it - I don't think we need to mess around altering the menu interface changing all mentions of the word menu to shortcuts when the implementation is still going to resemble what people think of as a menu - my browser bookmarks are in a menu too, for example.

Okay, here's another go.

I think I got Gabor's last review items.

Also:
Removed the perms for shortcut making (all are administer menu)
Documented some stuff which wasn't doc'd yet

One more question:

Should we alter Garland to support this? I'm thinking yes.

Best,
J

Should we alter Garland to support this? I'm thinking yes.

I think if we have to add yet another template variable for themes to keep track of, it should definitely be something much more generic than $add_to_shortcuts. Hopefully we can find another way to do this, though...

- I don't see why admin/config/shortcuts/edit and it callback gets to live on?! It is just a redirect to a different path with the same structure.

I was thinking so that contrib authors would have something to override when that link was pressed. They don't really want to override every menu edit page, just the one which is meant for shortcuts. Is there a better way to do this?

given the amount of markup used by the add_to_shortcuts item, it sounds sensible to have a #theme callback for it instead and only add the data in the array

Good point - added.

- on the phpdoc of toolbar_save_link_to_shortcuts(), Drupal does not include type names in its phpdoc, so your phpdoc is non-standard

Huh, okay non-standard non-standard phpdoc, fair enough. Is there a reason why we don't include types? I've changed it, but really, that just seems crazy to me. That's one of the main reasons to document code, to make it easier for people to figure out what a function requires.

also, on the same phpdoc, you should end it with */ and not with **/, as I've noted above

Got it.

on the phpdoc of toolbar_shortcut_edit_links(), you have an extra empty line, but you should actually have a one line summary and the longer explanation after an empty line (again part of our phpdoc standard)

Got it.

Sub.

+  -moz-border-radius-topright: 5px;
+  -moz-border-radius-bottomright: 5px;
+  -webkit-border-radius-topright: 5px;
+  -webkit-border-radius-bottomright: 5px;

Any reason why we're not also using plain old border-radius here as well? Seems like it might be good to have for completeness, as D7 will be around for quite some time (i.e., long enough for browsers to start accepting that property).

Thoughts?

This review is powered by Dreditor.

I read up on this issue, and here is what we are going to do:

1. Per user shortcuts in core (with the option to do per-user defaults/starting points in contrib)
2. We will not leverage the menu system and use a custom storage mechanism

I like the decisiveness!

Sounds to me like we need flag module in core (/me ducks). Otherwise, we get more one off infrastructure.

subscribe

I like Moshe's comments because they're always open to interpretation :)

But, much more important than decisiveness is ideas and/or code. If someone can figure out how to make this happen without duplicating half the menu system and adding lots of unmaintainable code to core, while still providing all the features (including icons) that we want, then splendid.

Until then, we have working patches and approaches that use the menu system - and are very simple as a result.

The current patch does not implement per-user shortcuts which is a big part of Mark and Leisa's design and the whole point of having a shortcut bar to begin with. The current implementation may be simple, but it does not meet the functional requirements. So storage mechanism aside, this patch is still incomplete and we should not rely on contrib to make it functional.

> Sounds to me like we need flag module in core.

So the toolbar would essentially be users flagging menu link items?
I like the idea. But:

First problem: users will want to bookmark things that aren't menu links. What I call the 'node/4' case.
Second problem: flag module allows per-user flags and global flags on entities. How will contrib extend this to have per-role toolbars?

Gabor - firstly, thanks so much for making that video, it makes it so much easier to grok the work you've been doing and to give feedback to it. ++ to screencasts in the issue queue!

A few quick thoughts in response:

- suggest we rename 'create set' to something a little more descriptive like 'create custom shortcuts'
- suggest that when a custom set is created it starts with the default shortcuts included, will probably be faster for the user to delete these than to refind and add them and the reason they've been chosen is because they are quite universally required
- suggest that we come up with an alternative to designing icons for every single shortcut possible and instead design icons for those where there are really good clear, obvious icons where imagery will add value and then design a simple 'bullet' like image that can be used for the rest, as a visual target more so than to assist the user in determining what the shortcut is. (This will assist both with workload and also the overall quality of the icons, I think)

With regards to Gabor's frequent mention of Mark & I insisting on restricting the number of shortcuts, I think Gabor explained this quite well but the rational behind this is that if you create too many of these they very quickly become not-short-cuts because it will take the end user a long time to distinguish between all the possible options to choose the one they require, thereby negating the whole purpose of the shortcut. We wanted to discourage people doing this to themselves so that they would maintain the benefit of the shortcut.

Brought to you by Acquia, it's...

Proposal #7,522,608 (the "final" proposal)

So it turns out that 'decisiveness' and 'Drupal' don't mix, and that's for everyone's benefit :)

Here's what happened: A bunch of people had a long, long conversation about this earlier today. This included a number of people who have participated heavily in this issue (Dries, Gábor, me, Jacob, Jeff Noyes). In the end, we all came up with a new (again!) proposal that sort of combines a lot of ideas from above and - at the risk of crying wolf - might even make sense. Some kittens may die as a result of this proposal, but far fewer than could have died otherwise.

If you're interested in this issue, please read the proposal carefully and try to share comments ASAP if you have them, because we don't have a lot of time. I tried to organize it by section and put references to specific comments from the above discussion (e.g., "#123").

Thanks to Jacob and Linea for taking good notes, but any mistakes or digressions in the writeup are mine :)

Overall functionality

• Neither "per-role" nor "per-user" shortcuts in core... depending on what you mean by "per-user". Turns out there is some confusion over this terminology. Shortcut sets (as implemented in #126 and earlier) can be associated with users, but do not provide any click-the-add-to-shortcuts-button-and-get-your-own-personal-bookmark-that-no-one-else-might-accidentally-change ability. However, one nice thing about the concept of shortcut sets is that they are very generic and therefore potentially flexible (go Dmitri!).
• Therefore, the proposal is to use the overall concept of shortcut sets. But to carefully split out permissions and other functionality so that the site administrator - or contrib modules - can push them towards one use case or another.

Specific use cases

• What if my site only has a single administrator? Simple, just use Drupal core, stick with the default shortcut set, never create another, and click on the "add to shortcuts" button all you want.
• What if I want per-user shortcuts? If you only have a few users who need this, you'll do it with Drupal core. Create, by hand, shortcut sets for each user (you'll be able to assign it on their user account page). Then, give these users the edit shortcuts permission but not the switch shortcut sets permission. If you have too many users for this to be practical, you'll need a contrib module to help you out - a contrib module can easily be written that intercepts the "add to shortcut" action and automatically creates a new shortcut set when necessary (#42).
• What if I want per-role shortcuts? Since we'll use the menu links system (see below), we get 'pseudo-per-role' for free in Drupal core using a single shortcut set (#99, #101). Also, if you're comfortable having users decide for themselves what "role" they want to be associated with, you can create different shortcut sets with different purposes (e.g., Moderation, Translation - see #103) and give your users the switch shortcut sets permission but not give them the edit shortcuts permission. If you need more flexibility, that's where a contrib module comes in; there will be no ability to explicitly map shortcut sets to roles within Drupal core.

Data storage

• We'll store the shortcuts themselves (the links) in the menu system, but the shortcut sets in a custom table (a.k.a., we'll use the {menu_links} table but not the {menu_custom} table). Pretty similar to how the book module does it (#83, #90). If you're worried about the performance impact, don't be (see #83) - or limit the use of shortcuts on your site.
• Why? We get loads of menu system functionality without duplicating it. We allow contrib modules to implement "shortcut folders", as in Jeff Noyes' designs (#1, #90) without changing the underlying storage mechanism. By doing it this way we might also allow a contrib module to expose the shortcut sets as standard Drupal menus or blocks and thereby make them reusable via those mechanisms... I'd need to try coding that to be sure, though :)

Icons

• Work on this can continue in the separate issue already started at #42493: Enable custom bullets/icons for specific menu items. This would ideally allow core to provide default icons on a "per-section" basis, as well as an easy way for contrib modules to provide their own. As a fallback plan if that fails, we won't use as many icons and will have a single "bullet" icon as the default (#125). So worst case, we have a bunch of bullets and not too many real icons, but it still works.. and even then, there are CSS IDs already in the above patches so contrib modules can at least define icons if they are willing to specifically target it towards appearance in the toolbar.

User interface

Some of these are questions and (hopefully) on more of a November 15 deadline so can use some help and advice.

• Does the "add to shortcut" button do double duty as an administration tool and bookmark tool, and if so, how? The issue is that if you have permission to switch shortcut sets and also to add shortcuts, then clicking on "add to shortcuts" might be ambiguous... which set does the new shortcut go in? The suggested solution was to do it Firefox-style and have a dialog that prompts you which set to add it to, or whether to add it to a new one, or whether to add it to all sets. Is that good? Does it help, or make the button less convenient than it would be otherwise?
• We aren't using the menu module's user interface because it has too many options we don't need. But should this look and feel like a simplified version of it? In particular, the page for organizing shortcuts within a shortcut set - right now, the "slots" and "disabled section at the bottom" features of the patch seem to be a different UI pattern than used elsewhere in Drupal; is that good or bad? Note that we already have an initial commit at #564886: Remove 'expanded' checkbox from menu item overview for simplifying the equivalent page in the menu system (and hopefully some followups to come).
• What to do about limiting the number of shortcuts that can be created? If we go ahead with a limit, see #74 for questions to answer. Most basically: What exactly should happen when they already have seven shortcuts and try to add the eighth? We also may need to think about how this would work with different theming (e.g., if the shortcuts are organized into a dropdown folder, where the reasonable limit - if any - is larger). Obviously because of the "slots" this is related to the above question about the UI pattern also.
• What about the page for switching shortcut sets? Right now it's pretty sparse; is that good or bad? Is there other information about each shortcut set that should appear there, or is the simplicity of the current UI better?

Somewhat scared to respond after this lengthy writeup, but what does the user benefit from a set of shortcuts over a per-user shortcut bar? I am not really following the added simplicity, or perhaps the not-added-simplicity for contrib sake? I hope you get to a patch soon, so I can use it to understand it.

Still seems very complex for just bookmarking perhaps 1 or 2 more links to my shortcut bar (lets remember to stay realistic, the majority of websites do not have 20 different administration account's with different needs, who will all customize their shortcut bar).

I'm glad you discussed before we reached #200. :P

99% of that makes sense.

UI issues raised:

1) No, just drop any switcher. Core assigns a set to each user account by default and that set is the target. The user account form allows to switch.

2) There's no point in disabled shortcuts or anything like that. If anything, it will be horribly confusing for the user. We have a set of links, and nothing else, no regions and nothing. Add or remove, and be done. A contrib module wanting to expand on core needs to override, just like everywhere else.

3) Drop the limitation. It's theme-dependent. The themer is guilty for taking care of a potentially too long list of shortcuts. The system can do it. The presentation is a different layer, and core shouldn't make assumptions about the ugliness or awesomeness of the presentation layer.

4) User account form.

And, please: You do not touch anything in modules/toolbar/* to implement this, unless absolutely required.

I'd also say drop the limit. My firefox live shortcuts start to get impractical over 8-10 or so - since at that point I have to click to see more. So when it gets that bad, they get pruned. There's no usability issue there at all. Far more annoying would be trying to add a shortcut, being told I can't, then having to choose which one to delete, then trying to add it again which is the point where I'd start looking for the setting to switch it off.

@catch: but how do you know which shortcut to prune? Did I miss something?

FYI, I am working on an updated patch for this and intend to have something to post tomorrow.

If we're taking votes on the UI questions, I agree with @sun on #2 and #3, and on #4 as well -- although not sure that #4 was a complete answer to the question :) I'm on the fence about #1.

Regarding the possibility of not having disabled shortcuts but only the ability to delete them (as @sun suggested): Something in favor of that point is that I think this is a place where it would be really easy to replace the current paradigm of "click delete and get a confirmation form" with the holy grail of "click delete and have it happen automatically, but then get an 'undo' link". I don't think we'd actually implement that anywhere but in a followup, but I'm mentioning it here because it might affect people's opinions of how important it is to have a disabled state.

@Bojhan in #129: Well, I mentioned in the writeup that a couple kittens were likely being killed, and I think you've identified them :) Some things would be simpler if we didn't try to satisfy as many use cases at once, but ultimately, it seemed from this issue like there are a lot of different use cases people want to support, and per-role is important to many people, enough so that they didn't want to add too much that would make it harder to do. So the suggestion is a compromise. I should mention, though, that doing per-user shortcuts is probably not as bad as I made it sound in the writeup. Playing around with the permissions and all that would only be necessary if you wanted to make absolutely certain that there was no overlap between users. But if, for example, you and I were administering a site together, we could simply make one set of shortcuts called "Bojhan" and one called "David", agree not to use each other's, and everything would work fine, no extra work required.

By the way, the updated patch will not (at this point) make any changes to the UI from the previous patch -- except to add the setting to the user account form.

@mcrittenden - look at them and work out which one I can delete or move to actual bookmarks folder without missing it too much.

@catch, sorry, I thought you were saying that the system would prune them automatically. Makes a LOT more sense now.

There might be shortcuts like the edit mode toggle, which are nearly impossible to find/add otherwise (unless you exactly now the Drupal path), so deleting them would be disallowed. If you delete them, there is no way to get them back.

Note that as per recent discussion at #473268: D7UX: Put edit links on everything the edit mode toggle is very likely going to be taken out of the shortcut bar.

Overall, it doesn't seem like a great pattern for modules to add critical things to the shortcuts that aren't available anywhere else. The intention of the shortcuts - at least in my opinion - is that they are supposed to be customizable, swappable, and safe to remove.

Here's a start at a new patch.

This is not at all functional yet (there's still a fair amount of UI-related code that needs to be copied in from the other patches and made to work), but in terms of the underlying functional code, this is more or less an idea of how it will work.

Better version - this one actually puts the new shortcut.module files in the right place (not that it really matters since the patch is non-functional right now anyway).

Tried to get going on this, but got this when installing:

Ugh Ugh Ugh...

Had a bad time trying to install and start on this patch today. Wasted a lot of time.

Not a comment on David's re factoring, anyone could have done this easily, but a comment on our lack of Exception handling - especially during the install process. The obtuse error I got was actually because toolbar.install was referenced in the info file, but didn't exist.

I had to go through the diff line by line to find this, since it doesn't show up anywhere in any error report or logs. Another error I also combed through to find is the menu refactoring for delete_links.

+++ modules/toolbar/toolbar.info 13 Oct 2009 06:03:28 -0000
@@ -1,9 +1,9 @@
files[] = toolbar.install

This should be removed

+++ modules/menu/menu.module	13 Oct 2009 06:03:28 -0000
@@ -271,21 +271,13 @@ function menu_save($menu) {
 function menu_delete($menu) {
   // Delete all links from the menu.
-  $links = db_query("SELECT * FROM {menu_links} WHERE menu_name = :menu_name", array(':menu_name' => $menu['menu_name']));
-  foreach ($links as $link) {
-    // To speed up the deletion process, we reset some link properties that
-    // would trigger re-parenting logic in _menu_delete_item() and
-    // _menu_update_parental_status().
-    $link['has_children'] = FALSE;
-    $link['plid'] = 0;
-    _menu_delete_item($link);
-  }
+  menu_delete_links($menu_name['menu_name']);

Should be:

+ menu_delete_links($menu['menu_name']);

This review is powered by Dreditor.

Borken on install because of:

http://drupal.org/node/568640

Trying to use drupal_write_record from hook_install doesn't work because the schema isn't there yet :( poo.

Fixes a couple things which were throwing fatal errors, still broken though (doesn't show shortcuts at present).

Also included is the interdiff of the two patches.

There's a general concern regarding the vertical size of the shortcut bar. Plus, creating a custom icon for each shortcut is problematic, so I propose this design change #602408: Adjust height of shortcut bar

In addition: We have a very standard for function names. It is: subject_verb()

So, without looking at those functions, the above examples translate into:

function shortcut_name_set($number = 1) {
function shortcut_set_name_get_unique() {

Also, any interdependencies between Toolbar and Shortcut module need to vanish.

oh, and btw, http://drupal.org/project/shortcut

The object is "shortcut set", so I believe the correct function names would be shortcut_set_name() and shortcut_set_get_unique_name(). However, Gábor makes a good point that in cases where the word can be a verb or a noun, this can get really confusing.

Not only http://drupal.org/project/shortcut but also http://drupal.org/project/shortcuts :( ... although the latter doesn't seem very active. Maybe we solve both of the above problems at once with "bookmark" and "bookmark group" (http://drupal.org/project/bookmarks exists, but not "bookmark")? Overall, not too worried about this aspect right now :)

The shortcut module does not depend on the toolbar in any way. I don't think we've gotten far enough to see about the other way around, but I don't see any reason why it would be a problem for the toolbar module to depend on shortcuts.

In any case, yes, the intention of the shortcut module is absolutely that it can provide support for more complicated structures such as Noyz's folder-based approach. That is one of the reasons for using menu links. Currently, the shortcut module has an API that will save any menu links you give it - it doesn't care how they are structured. So a contrib module that wanted shortcut folders would really only have to provide a UI for putting the links in the correct hierarchical structure, and then it would basically be done.

Okay, here is a start on resurrecting the interface and making a sane abstraction here. I think this is a good start, although needs a lot of work still.

When looking at this, I was trying to address the concern that shortcut really has little to do with toolbar. Shortcuts could be shown in other places, and the space where the shortcuts are could be used by other modules (as @sun says).

Problem is, the collapsible drawrer in toolbar needs to exist for shortcuts to play with it.

So instead of having toolbar-shortcuts, now we have toolbar-drawrer! This should remove a strict dependency on shortcut in toolbar, and shortcut can theme itself and just put it in the toolbar-drawrer part of $page.

This is kinda a wtf, and someone with better experience in theming would probably be able to suggest a better approach, but basically:

in function shortcut_page_build:

  $links = shortcut_links();
  $links['#attached'] = array('css' => array(drupal_get_path('module', 'shortcut') . '/shortcut.css'));
  $links['#prefix'] = '<div class="toolbar-shortcuts">';
  $links['#suffix'] = '</div>';
  $page['toolbar_drawrer'] = $links;

And in toolbar_page_build

$page['page_top']['toolbar']['toolbar_drawrer'] = isset($page['toolbar_drawrer']) ? $page['toolbar_drawrer'] : array();

This is not pretty, but because the letter S comes before the letter T, I don't know how else to pass along the fact that we have filled the toolbar_drawrer.

Anyway, small WTF to do away with a much bigger one IMO and give a lot more flexibility.

Starting on the admin interface and getting stuff to actually work....

Heh, as Gabor kindly corrected me, it is drawer, not drawrer. He speaks his 2nd language better than I speak my first it seems :(

Okay, okay misunderstanding, I was just showing how we can remove one from the other. I think there is a little CSS cleanup to do to get it nice again.

Okay, another update.

Also attached is the interdiff with 151.

Summary of updates:
* Now the button works again, and I also added the toolbar.png that it needs in this binary diff.
* The previous refactoring of menu_delete_links was actually broken, fixed
* shortcut_set_load now takes similar arguments to shortcut_set_save. Has a slightly greater performance overhead... don't know if it matters, can be made into more functions, but seems cleaner.
* New UX workflow, instead of going to a form, it just adds the shortcut directly and sends the user back to destination.
* Fixed some broken CSS from my last update.
* Put the correct dashboard path into the installer.

I'm feeling sick and might be calling it quits for tonight.

OK, I'm going to start working on this patch again (a little bit later). In the meantime:

The previous refactoring of menu_delete_links was actually broken, fixed

Interesting - that was a straight copy-paste... looks like the code may actually be broken in core? Need to remember to look at that - there don't appear to be any tests for it :)

+  if (isset($_REQUEST['token']) && drupal_valid_token($_REQUEST['token'], 'shortcut-add')) {

Nice to see the tokens added back - nothing like a little security :) They were in the patch around 100 comments ago or thereabouts, but seem to have gotten lost in the shuffle. Probably to be extra safe, they should use the user ID when generating the token.

***

Discussion question:

With regard to toolbar/shortcut dependencies, seems like Jacob's approach of a toolbar drawer definitely makes the most sense. However, it currently means that the shortcut module adds stuff to a dedicated "toolbar_drawer" region of the page which seems targeted at the toolbar module (unless other modules similarly looked for such a region). Not sure if that's what we want - I sort of envisioned the shortcut module as being more of an API, which probably only exposes the shortcuts via a simple hook_block() or perhaps also via a special theme function that is "toolbar-targeted" (if it can be made generic) but that can be called by various modules rather than trying to inject itself onto the page?

We discussed the possibility of a hook_toolbar_drawer() to solve this (which the shortcut module would implement), or possibly instead having the toolbar module use the shortcuts as its fallback for the drawer (if no other module has populated the drawer region first and if module_exists('shortcut'))...

Not sure it's critical to nail this right now, but worth mentioning for discussion - it may be that @sun is the only person who will care :)

New version, with a lot more of the user interface code copied over (almost all of it).

A bunch of things aren't working still, but it's starting to. I suspect a lot of things can be fixed by doing a find/replace in the shortcut module (especially the CSS and JS) and replacing "toolbar" with "shortcut"...

+++ modules/shortcut/shortcut.admin.inc	15 Oct 2009 04:05:14 -0000
@@ -0,0 +1,394 @@
+    // Really we should test for a dupe here.
+    $shortcut_set['links'] = $link;
+    

I'm not sure how this worked...

I'm on crack. Are you, too?

Okay I made some changes here which may or may not be popular :) But I also finished up the UI in most sections, so hopefully I am forgiven.

First, I switched most of the data structures back to using StdClass instead of assoc arrays.

Why? Because in the book of code purity by Jacob, arrays are generally for iterated structures. If you have something which has a bunch of properties and a primary key, it is an object - even if it is a fake PHP StdClass. I have no great reasons, it's easier to read and write, it's more consistent with other data structures in Drupal ($node, $user).

There were lots of other bugs, and there are still some left.

So shortcut_set is back to being a StdClass. I didn't mess with the links array, however I did fix something in menu.inc to sort by weight, because this was broken.

The screen to re-order screenshots is there, as is the button to access it. The screen to edit an existing shortcut is there. Creating a new one is not yet.

Attached is the patch and interdiff against 157.

Can I test it yet, dear bot?

Hm, not sure why that patch didn't apply. Here's a rerolled version that is the same exact thing (but not made using Git...)

I'm going to see what I can do to finish this up. It is actually pretty close, although indeed a few things are still completely broken. UI-wise, the goal is that it's supposed to be the same as it was in #124-126 (with the exception of adding something on the user account page to change shortcuts from there), so if that goal isn't met, it's only by accident :)

OK, this patch actually works now (i.e., no explosions). Definitely small bugs and UI polish not done yet, but it is now functional again, and therefore reviewable :)

Let's also see what the testbot thinks.

OK, this will fix most of the test exceptions (and with any luck, all of them).

The bug was in shortcut_page_build() which I haven't really looked at too carefully at all but is the source of some of the remaining "interesting" code... the interaction between the shortcuts and the toolbar still isn't quite clear.

+++ includes/menu.inc	15 Oct 2009 21:11:01 -0000
@@ -2257,6 +2257,74 @@ function _menu_navigation_links_rebuild(
+function menu_links_clone($links, $menu_name = NULL) {
+  foreach ($links as &$link) {
+    unset($link['mlid']);

What about 'plid' ?

Not sure whether menu links can be cloned that easily...

Maybe we can go with that and deal with the bug later on.

+++ includes/menu.inc	15 Oct 2009 21:11:01 -0000
@@ -2257,6 +2257,74 @@ function _menu_navigation_links_rebuild(
+function menu_load_links($menu_name) {
...
+function menu_delete_links($menu_name) {

The pattern is subject_verb, so

menu_links_load()
menu_links_delete()

+++ includes/menu.inc	15 Oct 2009 21:11:01 -0000
@@ -2257,6 +2257,74 @@ function _menu_navigation_links_rebuild(
+  $links = db_select('menu_links', 'ml', array('fetch' => PDO::FETCH_ASSOC))
+  ->fields('ml')
+  ->condition('ml.menu_name', $menu_name)
+  // Order by weight so as to be helpful for menus that are only one level
+  // deep.
+  ->orderBy('weight')
+  ->execute()
+  ->fetchAll();

Wrong indentation.

+++ includes/menu.inc	15 Oct 2009 21:11:01 -0000
@@ -2257,6 +2257,74 @@ function _menu_navigation_links_rebuild(
+  

Trailing white-space here.

+++ modules/shortcut/shortcut.admin.inc	15 Oct 2009 21:11:02 -0000
@@ -0,0 +1,506 @@
+function theme_shortcut_set_switch($variables) {
...
+  drupal_add_css(drupal_get_path('module', 'shortcut') . '/shortcut.admin.css');
+  drupal_add_js(drupal_get_path('module', 'shortcut') . '/shortcut.admin.js');

This should use the #attached property in the form already.

Which basically makes the entire theme function a bit obsolete.

+++ modules/shortcut/shortcut.admin.inc	15 Oct 2009 21:11:02 -0000
@@ -0,0 +1,506 @@
+  $form['set'] = array(
+    '#markup' => t('Using set "@set"', array('@set' => $shortcut_set->title)),
+    '#prefix' => '<h4 class="shortcuts-set">',
+    '#suffix' => '</h4>',
+    '#weight' => -100,
+  );

This looks like a page title...?

+++ modules/shortcut/shortcut.admin.inc	15 Oct 2009 21:11:02 -0000
@@ -0,0 +1,506 @@
+    $form['shortcuts'][$status][$mlid]['name']['#markup'] = l($link['link_title'], $link['link_path']);
...
+    $form['shortcuts'][$status][$mlid]['edit']['#markup'] = l(t('edit'), 'admin/config/system/shortcut/link/' . $mlid);
+    $form['shortcuts'][$status][$mlid]['delete']['#markup'] = l(t('delete'), 'admin/config/system/shortcut/link/' . $mlid . '/delete');

With #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable, these could use the new #type = 'link'.

+++ modules/shortcut/shortcut.admin.inc	15 Oct 2009 21:11:02 -0000
@@ -0,0 +1,506 @@
+    $form['shortcuts'][$status][$mlid]['status'] = array(
+      '#type' => 'select',
+      '#title' => t('Status'),
+      '#options' => array('disabled' => t('Disabled'), 'enabled' => t('Enabled')),

This should be a checkbox according to our UX guidelines.

+++ modules/shortcut/shortcut.admin.inc	15 Oct 2009 21:11:02 -0000
@@ -0,0 +1,506 @@
+  drupal_add_css(drupal_get_path('module', 'shortcut') . '/shortcut.admin.css');
+  drupal_add_js(drupal_get_path('module', 'shortcut') . '/shortcut.admin.js');

ditto, #attached

+++ modules/shortcut/shortcut.admin.inc	15 Oct 2009 21:11:02 -0000
@@ -0,0 +1,506 @@
+function shortcut_link_add($form, &$form_state, $shortcut_set) {
+  drupal_set_title(t('Add new shortcut'));
+  $form['shortcut_set'] = array(
+    '#type' => 'value',
+    '#value' => $shortcut_set,
+  );
+  $form += _shortcut_link_form_elements();
+  return $form;
...
+function shortcut_link_edit($form, &$form_state, $shortcut_link) {
+  drupal_set_title(t('Editing @shortcut', array('@shortcut' => $shortcut_link['link_title'])));
+  $form['original_shortcut_link'] = array(
+    '#type' => 'value',
+    '#value' => $shortcut_link,
+  );
+  $form += _shortcut_link_form_elements($shortcut_link);
+  return $form;

Instead of using a sub-form-builder, these should live in a single form builder function, conditionally setting the title and if required, additionally elements.

+++ modules/shortcut/shortcut.module	15 Oct 2009 21:11:02 -0000
@@ -0,0 +1,526 @@
+    $data['content'] = shortcut_renderable_links($shortcut_set);

We use the term "build" when a renderable array is built.

+++ modules/shortcut/shortcut.module	15 Oct 2009 21:11:02 -0000
@@ -0,0 +1,526 @@
+    // $_GET['q'] is the unaliased version.
+    $get = $_GET;
+    unset($get['q']);
+    $link = $_GET['q'];
+    if (!empty($get)) {
+     $link .= '?' . drupal_http_build_query($get);
+    }

?

You can use drupal_get_query_parameters() here.

+++ modules/shortcut/shortcut.module	15 Oct 2009 21:11:02 -0000
@@ -0,0 +1,526 @@
+     '#markup' => l('<span class="icon"></span><span class="text">' . (shortcut_set_switch_access() ? t('Add to %shortcut_set shortcuts', array('%shortcut_set' => $shortcut_set->title)) : t('Add to shortcuts')) . '</span>', 'admin/config/system/shortcut/' . $shortcut_set->set_name . '/add-link-inline', array('query' => $query, 'html' => TRUE)),

We should prepare the link text separately.

+++ modules/shortcut/shortcut.module	15 Oct 2009 21:11:02 -0000
@@ -0,0 +1,526 @@
+function shortcut_preprocess_page(&$variables) {
+  if (shortcut_set_edit_access()) {
+    $variables['add_to_shortcuts'] = drupal_render($variables['page']['add_to_shortcuts']);
+  }

Shouldn't the access be checked before something is built - so you can simply test isset() here...?

+++ themes/seven/style.css	15 Oct 2009 21:11:03 -0000
@@ -735,3 +735,10 @@ body.overlay #page {
+/* Shortcut theming */
+div.add-to-shortcuts {
+  float: left;

Maybe I overlooked it, but whenever there is a float, there must be an overflow: hidden in the parent container.

I'm on crack. Are you, too?

Awesome feedback as always... This patch just fixes the broken test, does not address any of #167 for now. I have to go offline for a while soon, but can try to do it later.

I was very tempted to delete this test rather than fix it, but refrained :) (see the interdiff) At least the testbot should be happy.

OK, the test failed because I didn't put things in alphabetical order... sigh :)

This should take care of that as well as most of @sun's feedback above. A quick recap of some of the things I did not change...

1. menu_load_links($menu_name): It's subtle but I actually think this might be the right function name. The "menu" is the object we are acting on, and "load links" is the thing we're doing to it.

2. Did not change select box to a checkbox for now (note that it gets hidden by javascript anyway, and then used in the tabledrag behavior, and I didn't feel like trying to make tabledrag work with a checkbox).

3. I left the sub-form-builder as a separate function (we had tried combining those functions before and they didn't come out looking so good, plus have separate submit handlers, etc).

4. I left as $data['content'] rather than $data['build'] because the code occurred inside hook_block_view() so it's required to be like that :)

5. I double-checked and it looks like there's already a overflow: hidden in the parent container.

Yoink. Grabbing this to give it a spin.

When applying this to an existing install without starting from fresh (to simulate a user upgrading from Drupal 6), here's what I see:

1. (commit-blocker?) When the toolbar is enabled but shortcut is disabled, you see the collapsing icon, but it doesn't do anything. I haven't looked at the implementation closely, but this seems like unwanted coupling.

2. (commit-blocker) When I first enabled the module, I got:

    *  Notice: Trying to get property of non-object in shortcut_page_build() (line 495 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
[00:08]  <webchick>     * Notice: Trying to get property of non-object in shortcut_page_build() (line 498 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
[00:08]  <webchick>     * Notice: Trying to get property of non-object in shortcut_renderable_links() (line 474 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
[00:08]  <webchick>     * Notice: Trying to get property of non-object in shortcut_page_build() (line 508 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).

2b. (commit-blocker) Apparently uninstalling it isn't pretty either, according to David. ;)

3. (commit-blocker?) My shortcuts bar has nothing in it by default. Just an "edit shortcuts" link. Not sure if this is by design or not, but I suspect not.

4. (commit-blocker) Clicking "edit shortcuts" gives me:

[00:10]  <webchick>     *  Notice: Trying to get property of non-object in shortcut_set_switch() (line 67 of /Users/webchick/Sites/core/modules/shortcut/shortcut.admin.inc).
[00:10]  <webchick>     * Notice: Trying to get property of non-object in shortcut_set_switch() (line 72 of /Users/webchick/Sites/core/modules/shortcut/shortcut.admin.inc).
[00:10]  <webchick>     * Notice: Trying to get property of non-object in shortcut_page_build() (line 495 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
[00:10]  <webchick>     * Notice: Trying to get property of non-object in shortcut_page_build() (line 498 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
[00:10]  <webchick>     * Notice: Trying to get property of non-object in shortcut_renderable_links() (line 474 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
[00:10]  <webchick>     * Notice: Trying to get property of non-object in shortcut_page_build() (line 508 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).

5. (post-commit clean-up) The description on that text field makes no sense to me:

I'm going to pause here with my testing while David works out these bugs, then have at it again.

OK, here's a new version that fixes the uninstall bug.

The issues with enabling it on an existing install, I tracked it down a bit and it seems almost certainly like it's due to #200931: Schema not available in hook_install/hook_enable. Haven't tried that patch to see if it fixes it, but seems like it should. Worth including as part of this patch or not (it seems to be a one line fix)?

Well, a one line fix, but it seems like that will cause its own testbot failures, so maybe best to leave as a separate issue if possible...

UI review

Also, note that I was not able to test contextual "Add to shortcut" because I don't see any links for that and missed the images somewhere above. This implies that we might have some accessibility issues, though David suggested that it might work with JS disabled.

Code review

Nice! I'm betting these will come in handy for contributed modules as well.

Ha! I was going to say that I can think of more than 7 links that I can't remember the paths too and lookee here. :)

The name of this hook could really use some work. I would never guess that it was intended to let me alter the shortcut set for the given user.

(follow-up) These permission names/descriptions could use some work. In particular, it's not clear to me what the difference is between "Administer shortcuts" and "Customize shortcut links".

If you do this in a db_select(), then other modules can hook_query_alter() it.

Check permissions first.

Love hunks like this. ;)

Hm? Why this name change? The old name is way clearer.

A: We don't want to couple toolbar and shortcut menu together. This makes it a generic container for "extended" data, wherever it comes from.

Conclusion
This is the first time I've really had a chance to play around with this patch, and it was an absolute joy. Finally I have a place to put all of those links I can't remember the paths to anymore. :D I really think this will be an amazing offering in D7 core for power users, and site builders alike.

However, as you can see, this isn't quite ready yet. But it is very close. I think one or two more re-rolls ought to get us there. I can't really see holding this feature back on one or two re-rolls, but let's please try and get this nailed down in the next ~24 hours. If any of these things are going to take too long to fix, go ahead and file follow-up issues for them.

It's also worth spending a little time here (again < 24 hours) talking about the name of this module. When the dashboard feature got committed, there was an absolute sh*t storm about the fact that core took a name that a contrib module had already called "dibs" on. But really, IMO the ire was less about the namespace conflict and more the fact that the functionality the two modules implemented was similar in concept, but drastically different in implementation. While there are no recorded users of dashboard module in contrib yet, Drupal.org at the very least will eventually be using this module. (Incidentally, it turns out Gábor (major contributor to dashboard patch) and Neil (maintainer of dashboard module) had already had the discussion about this namespace conflict and Neil was cool with it.)

We have another such namespace conflict here with http://drupal.org/project/shortcut. It's a module about keyboard shortcuts (nothing to do with this core module) with ~25 users. I don't personally see this as a huge deal; modules frequently change names between major version of Drupal (bio => content_profile, workflow_ng => rules, amazon_tools => amazon, etc.) so I don't see this placing an incredible hardship on users as long as it's documented (Upgrade Status module even has rules for this). It would be great for one of the pathc authors though to reach out to the shortcut maintainer before we commit this patch and check if they'd be ok with something like "keyboard_shortcuts" instead.

Another option is we go the route of naming this to something like "bookmark" instead since http://drupal.org/project/bookmark currently doesn't exist. I'm not crazy about this though, since:

a) If I saw a module named "Bookmark" i would expect it to do what http://drupal.org/project/bookmarks does (bookmark individual nodes on my site)
b) I'm concerned people will get these bookmarks confused with their browsers' bookmarks. "Shortcuts" is clear; you're emulating the dock from your operating system.

Anyway, thoughts/opinions on that stuff would be nice. Great work on this patch!! :D

This review is powered by Dreditor.

Ookee :)

Good review.

I'm going to knock these out. They all seem pretty simple except the "Save changes" one....

To add the "Front page" link you must have left this page though, right? in which case, I don't understand the problem.

I think it would be a better UI to always save changes as they worked, but this is of course more work, and you need a save changes button anyway for accessibility.

Something in #171 (or core) is seriously messed up.

Gabor and I both saw this behavior:

Depending on the page visited, we saw different shortcuts, and some times a CSS offset.

My guess is depth field

Okay.
Something in core changed which caused a massive fail on this issue and took a couple hours out of my day, but then it fixed itself! Yeah for self-healing Drupal.

Anyway, I think I fixed most of the small code things. And I've made a little movie to demonstrate the UI fixes.
You can watch it here: http://pajamadesign.com/drupal/movies/shortcut-178.mov

Here is a list of the things I didn't do:

Change the hook name
I agree w/ webchick, just don't have a great suggestion. Probably, the problem is that this pattern is not represented in Drupal, a "winner takes all" type of callback, that's why it doesn't really work. I know Crell has some strong feelings about this :)

Figure out why the shortcut bar vanishes on a 404 page
I just don't know / want to know much of menu.inc, but I fear it is somewhere in there.

General
I feel like the CRUD APIs here are redundant in a lot of places, and some cleanup would be a good idea. Sometimes we seem to load all sets into a static cache, sometimes an individual one, I think there are some overlap cases we should deal with. Particularly if we can pass around the objects returned by db_select(). Not a huge deal though.

subscribing

OK, here's a new version. I've also attached the interdiff from the last patch @webchick reviewed, as well as reattaching the icon (from #126) for modules/toolbar/toolbar.png.

I think we've now gotten all of the points in @webchick's original review, except for the second screenshot which we were a little confused about what to do with :) Also, Jacob's point in #178 is probably not fixed either.

• It turns out the confirmation form Jacob added was a little tricky (didn't work in all cases, e.g. when an administrator was editing someone else's shortcuts). I removed it for now, and replaced it with a simple message (see the interdiff file) that hopefully at least more clearly indicates when you add a shortcut set that is automatically assigned to yourself. I think there are some more complicated interaction issues with the different uses cases of this module that probably have to wait for a followup to figure out in more detail... Otherwise I left all the cleanups Jacob had (although removed the word "toolbar" from the UI since we can't know that they are actually using the shortcuts in a toolbar).
• We now have form validation that should (mostly) prevent you from adding an invalid link to the shortcut. We still have the problem where the shortcut bar disappears on a 404 page, but at least you need to find your way to the 404 page on your own now... I'm guessing, like Jacob said, that this is a deeper menu system bug.
• I'm still not sure of a good name for the hook either, but I renamed to hook_shortcut_default_set() for now to make it a little shorter, and added some documentation. I also made it so that the LAST module which returns something from that hook wins out (rather than the first one the way I had it before) since that is more in line with the way other hooks (e.g., alter-type-hooks) work.

I think this is pretty close?

Looks good. Some minor feedback -- none of which should hold up this patch.

+++ modules/shortcut/shortcut.admin.inc	16 Oct 2009 21:28:49 -0000
@@ -0,0 +1,537 @@
+function shortcut_set_switch_submit($form, &$form_state) {

This function has a lot of different code paths, and could be a candidate for some clean-up.

+++ modules/shortcut/shortcut.admin.inc	16 Oct 2009 21:28:49 -0000
@@ -0,0 +1,537 @@
+      '%user' => $account->name,
+      '%set_name' => $set->title,

- For sets, we seem to be mixng 'name' and 'title'.

- For user names, we just do 'user'. For set names, we use 'set_name'.

+++ modules/shortcut/shortcut.admin.js	16 Oct 2009 21:28:49 -0000
@@ -0,0 +1,100 @@
+ * Handle the concept of a fixed number of slots.

This is somewhat cryptic, and could be clarified and expanded upon.

+++ modules/shortcut/shortcut.api.php	16 Oct 2009 21:28:49 -0000
@@ -0,0 +1,43 @@
+ * This hook allows modules to define default shortcut sets for a particular
+ * user that differ from the site-wide default (for example, a module may want
+ * to define default shortcuts on a per-role basis).
+ *
+ * The default shortcut set is used only when the user does not have any other
+ * shortcut set explicitly assigned to them.
+ *
+ * Note that only one default shortcut set can exist per user, so when multiple
+ * modules implement this hook, the last (i.e., highest weighted) module which
+ * returns a valid shortcut set name will prevail.

It is not clear what format that default set needs to be in. Do we just need to specify the name or something else? Could be clarified in the documentation.

Alright, after a lot of testing, I committed this to CVS HEAD. I'm marking it 'code needs work' so we can follow-up with some refinements.

Great job all -- well done!

Note that for a couple minutes, HEAD was broken. This patch was only partially committed for whatever reason. It should be fixed now though. :) In default profile, anyway.

When you install expert profile, however, upon enabling this module you get:

    *  Notice: Trying to get property of non-object in shortcut_page_build() (line 524 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
    * Notice: Trying to get property of non-object in shortcut_page_build() (line 527 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
    * Notice: Trying to get property of non-object in shortcut_renderable_links() (line 503 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
    * Notice: Trying to get property of non-object in shortcut_page_build() (line 539 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).

And an empty shortcut bar, then a bevvy of errors everywhere else. (basically, see #172)

It looks like default.profile is doing some instantiation that doesn't take effect when the module is installed after Drupal. Need that fixed. ;D

Huh. Apologies if this posts twice.

Note that for a couple minutes, HEAD was broken. This patch was only partially committed for whatever reason. It should be fixed now though. :) In default profile, anyway.

When you install expert profile, however, upon enabling this module you get:

    *  Notice: Trying to get property of non-object in shortcut_page_build() (line 524 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
    * Notice: Trying to get property of non-object in shortcut_page_build() (line 527 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
    * Notice: Trying to get property of non-object in shortcut_renderable_links() (line 503 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
    * Notice: Trying to get property of non-object in shortcut_page_build() (line 539 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).

And an empty shortcut bar, then a bevvy of errors everywhere else. (basically, see #172)

It looks like default.profile is doing some instantiation that doesn't take effect when the module is installed after Drupal. Need that fixed. ;D

MenuIncTestCase::testMenuGetNames() is now failing... and that menu test is effing stupid. Let's fix that.

Fixing the fix. This one passes locally.

Committed #188, turned testing bot back on. Let's see what happens.

Restoring needs work status.

#200931: Schema not available in hook_install/hook_enable appears to fix the notices when enabling shortcut module after installation using expert profile (as referred by David_Rothstein in #174)

ok. I'm angry. So I keep it up to the point.

@sun: Since a picture says 1000 words, I'm not sure what you're talking about, but I suspect it is the "add to shortcuts" link when you are not using shortcuts. It actually has *nothing* to do with the toolbar. I made this patch not 1:1 with the toolbar by making the space the toolbar expands to not be shortcut specific and moving the shortcut related stuff into shortcut.module. This was directly in response to your (valid) complaints.

What you are seeing is a not so nice integration with the theme. Currently, there is this "add_to_shortcuts" variable which is chucked right into seven:

<?php if (isset($add_to_shortcuts)): ?><?php print $add_to_shortcuts; ?><?php endif; ?>

No one likes this. Also, no one has a better idea of how to position this thing where it belongs (next to the title) while not embedding it in the admin_theme. As a developer, you can of course use a preprocess hook to remove this var, so it doesn't show. Also, if you don't plan on offering shortcuts to users, you can just turn the shortcut modules off.

Let me know if I'm missing something here and found the wrong problem or if you have ideas of how to make this cleaner but as usable.

Best,
Jacob

This caused a 6% performance regression, including for users with no access to shortcuts. #620634: Shortcuts are built unconditionally for anonymous users

superscribe

Note that the commit of #182 caused #633234: Error when enabling the toolbar module on a minimal site since it removed the dependency on menu.module in toolbar.install but not from toolbar_install().