Missing shortcut edit and delete operations on shortcuts admin

Subscribing.

I believe it's true that these links just aren't there. I even remember thinking that they were missing once before :)

A little tricky, though, since with the design that was implemented, the shortcut admin page isn't really a standard admin listing, so there's less of an obvious place to put edit links, but this really should be fixed (somehow).

This is pretty bad.

See also #680500: Shortcuts violate Drupal UI standards, where I think Bojhan is advocating moving the overview page to a table, for consistency with what we do elsewhere. Maybe this issue could be re-purposed for revamping that initial screen?

Looking at this, I realize the shortcut menu system needed a somewhat significant overhaul. In addition to the first reported issue:

- Users can select their own default shortcut set, so we need a menu structure that lets us edit each set, not just the default
- There is no UI implementation of the function for deleting shortcut sets.

So I followed the model of block module to a large extent, and made each set it's own local task. I also added a local action for deleting non-default sets.

I think there's a pretty clear separation between the more abstract problem identified in #680500: Shortcuts violate Drupal UI standards and this one. It's possible a better solution there could make this obsolete, but this is the more critical one, so here's a stab at the most obvious kind of fix.

Could you post some screenshots of this patch? Thanks for working on this, matt2000! :D

here ya go.

How do people embed their screenshot sin there comment? Apparently my comment-fu is not that strong, as I do not see <img> as an allowed tag for my HTML....

You need to be on the docs team, then you get access to the "Documentation" format. :\ You can post a request to the "Documentation" queue if you want, but you should offer to do some work there in return. ;)

In the meantime, I've edited your comment to put them inline.

eck, they're too wide anyway. I've removed them, so as to avoid the false impression that they're being seen as intended. And I'll try to remember to scale my browser down next time I take screenshots.

The funny bullet is a seven style issue, will be fixed soon in the seven style topic (if i'm correct)

Took a quick look, and the delete code seems pretty reasonable - although part is commented out? (While we're at it, I don't think it's possible to rename a shortcut set either, is it...)

I don't understand the rest of the patch, though. Why are you getting rid of the %shortcut_set loader and replacing it with a foreach() loop? The current menu callbacks already work fine and allow you to edit any shortcut set. The only problem is that we don't provide links to those in the UI.

It does indeed seem like we could solve this by adding a standard "Drupal-style" admin table to the shortcut module, with inline edit/delete links for each shortcut set. However, I think that table would probably need to be its own new page rather than a replacement for any existing page. I'll have to read #680500: Shortcuts violate Drupal UI standards more carefully later and comment on it, but I'm not sure that proposal makes sense as it stands? Anyway, I think the implementation of such an admin listing page probably could be done in this issue, as it seems like the cleanest way to solve the problem.

Whoops. The commented out code was an abandoned experiment in allowing the user to select a new personal default set if they were deleting their own. It was killing kittens, so i got rid of (most of) it. I'll roll a new patch without that bit once we hash out the other details.

I don't understand the rest of the patch, though. Why are you getting rid of the %shortcut_set loader and replacing it with a foreach() loop?

Because AFAICT, paths with dynamic loaders can't be rendered as Local Tasks. They need to be explicitly declared. I just did what block module does, in essence.

So for the record, the approach taken in #5 is not favorable. Lets continue the discussion at #680500: Shortcuts violate Drupal UI standards - casey is almost done with a patch

@jhodgdon, the help text issue is different and still relevant. But I think that it should only be addressed after the other UI issues sort themselves out and not before.

New point (or maybe not): The string "default" should not be used as the label for the first set. Default is better at describing the current set and not be associated with any particular set.

Shai

So let's close this issue as a duplicate of #680500: Shortcuts violate Drupal UI standards, which will fix this in a consistent-with-Drupal way.

I don't think so. That one is a meta-issue. At best maybe mark this "postponed", but I'm not really sure about that either.

Because AFAICT, paths with dynamic loaders can't be rendered as Local Tasks. They need to be explicitly declared. I just did what block module does, in essence.

Really? Hm, if that's true, that's really weird - but the menu system sometimes is :) Well, at least in that case we could use '%' rather than a foreach loop (that seems to be what the block module does).

However, in general we should probably be aiming to model this after the menu module more than the block module? Because (as stated many many times elsewhere... grumble... grumble) that's essentially what the shortcut sets are.

This patch does the following:

* The radio buttons for selecting the per-user set now only appear on the user account page.
* An admin table with edit and delete links has been added.
* The 'Change set' link points to the user account's change set page.

Known problems:

1. Users have the option to change their set of shortcuts even if there is only one set to choose from. Bit pointless.
2. There is an operation in the table to delete the default set, which can't be deleted.
3. Needs more comments.

I'm in a bit of a rush, so probably lots of other embarrassing mistakes.

There are additional problems with the UI that are being discussed in the meta issue (#680500: Shortcuts violate Drupal UI standards), but trying to keep this patch as small as possible.

Screenshots of changes

need sreview ;)

Any reason I am not seeing that main page in my install? admin/config/system/shortcut doesn't seem to list that table.

Ok, my intend is to get this ready today. Therefor I would want to be a bit more strict, and lets not waste to much discussion on bikeshed parts.

Fix :
- Change "Customize shortcuts" to "Edit %shortcut_set_name"
- Remove "Change set" - its a confusing pattern, not anywhere else in core.
- Add help text to "Edit Shortcuts" where one can "Change set".

Discuss
- Set default
Is it worth to set a new default set? Probably less so, if we disable the inheriting of sets from the default. I am up for removing that.

Bad implementation
- Per user / per site shortcut settings
One of the things that was obviously not looked after correctly in the Shortcut initial patch, is the fact that there is now a confusing radio on the user page where you are left guessing what is in each set. I would advise being able to show whats in each set, but given the complexity and time involved - lets not do this.

wait! I just finished it!

To clarify, the above patch fixes the 'known issues' in #24, and Bohjan's first 'Fix'.

Bohjans other comments are being discussed in #680500: Shortcuts violate Drupal UI standards -- I wanted to do that here, but feared for the kittens.

ATM, jhodgdon and I are discussing doing it anyway in IRC.

One UI callback to rule them all.

See attached patch & screenshots: one screenshot for the admin page, and one for the user page with each permission.

This addresses everything but help text.

If I've done well enough, you should be able to read shortcut_set_admin() and understand everything. Everything else is really just access checks and operation callbacks.

This one might work better.

Should the user be able to even see any shortcut sets that they can neither switch to nor edit?

Hmm. That's a good point. But that issue is not introduced by this patch, it's always been the case. So I vote we do that in a follow up.

Maybe someone without either of these two permissions shoudn't even see the Shortcuts page on their account?

That's already true. See the hook_menu entry for the tab, and shortcut_user_access().

re 1st point above: I take it back. A person without permission to switch couldn't view list of shortcuts sets anywhere before. I'll fix it.

1. Why are we expanding this issue to cover so many things at once? I thought the original issue was just about adding an admin table with edit and delete links. Other proposals should be dealt with elsewhere, IMO.
2. I don't really understand the "set active" link at all. "Edit" and "delete" make sense in an operations table because they lead to other pages with forms where you can do things. But "set active" is different since it takes an immediate action. Furthermore, radio buttons are the standard UI for something where you can choose only single item from a list, which is the situation we have here.
3. I think it's clear we need to do something do get rid of the current duplicate UI for switching shortcut sets, but as I mentioned in the other issue, I think we may be going in the wrong direction. Playing around with shortcuts is mostly an administrative activity. As I recall, the only reason it was put on the user account pages at all (it was not part of the design to do so) was so that administrators could have a way to edit a particular user's shortcuts. However, that is not too common of an operation, and leads to other problems: #632210: Move shortcut settings from user/%user/shortcuts to user/%user/edit I'd personally love to find a way to get rid of that page altogether, rather than the other way around. If we force people to go to that page a lot (and furthermore, put "administrative-looking" operations tables within their user account, as can be seen from the screenshot), that will break the flow of the module quite a bit.

   I think we should think more about ways to switch shortcuts from within the admin UI, and ideally we should do that at #632210: Move shortcut settings from user/%user/shortcuts to user/%user/edit.

but feared for the kittens.

As did I.

Like webchick suggested. I wanted to use the other issue as a meta issue then produce the simplest change here possible so we can get an improvement committed before I have to say things like...

...I have a problem with the latest patch which is that it is continuing the current problem of mixing site wide configuration and per-user configuration together. While it has been pointed out that this design works well if we have only one user per shortcut set, we haven't designed the module with that in mind. We're basically relying on the administrator to make clever decisions with setting the permissions depending on the number of users; otherwise the design breaks.

Can we please reawaken #680500: Shortcuts violate Drupal UI standards and continue the meta stuff there? Some of the stuff in #41 is already mentioned there too. We don't have to fix everything in one issue.

OK. Quick responses so I can get back to actually writing code.

* Please don't re-open #680500: Shortcuts violate Drupal UI standards. The conversation has moved back and forth too many times already. it's difficult for those of use who are trying to define tasks & do them.

* AFAIC, #632210: Move shortcut settings from user/%user/shortcuts to user/%user/edit is not relevant, because shortcuts seem to be designed for sub-admins, not regular users. You need a way to expose editing shortcuts for people who don't have higher level administrative permissions.

* @#42 pt 2 - We use links on the admin/themes page. But it really doesn't matter. Once we get the missing functionality in, you can write a patch to change the widget to anythign you like.

This patch fixes the issue identified in #38 and adds the help text from #44.

Webchick & I discussed #41 in IRC and agree that these are outside the scope of this issue.

This covers the important thing, which is adding the missing functionality, and fixing the biggest violations of core UI patterns. Polishing can follow once this gets committed.

[edit: typo corrected to #41 above]

Issues from #41 will continue in #684176: Shortcut Set Edit forms don't follow UI patterns

@webchick asked if I had any serious reservations about the patch.

I've looked it over, and the reservations I have are real but not overwhelming. It makes some things better and some things worse, but the overall effect is better. If this issue had remained on target, it would be 100% better :)

It makes things weirder for a major use case (single site administrator), and the admin table on the user account page is bizarre. However, we can easily continue that at #632210: Move shortcut settings from user/%user/shortcuts to user/%user/edit. The link from the "edit shortcuts" page to the user account page works well and nicely preserves the flow as much as possible, so that is a plus.

At some point, there needs to be a discussion (or a decision) about what audience the shortcut module is intended for - it probably can't keep trying to be everything for everyone, and it might make more sense to take out functionality (and relegate it to contrib modules) than to keep shuffling the UI to make it match "core patterns" which don't fit with the module's actual functionality. I'm not sure rushing this patch in helps come to a conclusion on that. But the 40% of the patch that adds the missing pages is definitely needed sooner rather than later :) Nice stuff overall.

I haven't looked super carefully at the code, but there are at least some small issues. I could do a more in-depth review of that if necessary.

I echo number #50. Though I think this is one step forward one back so I'm slightly more negative.

Very quick code review.

+ * (optional) User object for the user whose shortcuts will be administered
Missing full stop.

+ //is the the user's current set?

Space at start, capital letter, the the

+ $is_current_set = $set->set_name == shortcut_current_displayed_set($user)->set_name;

This is hard to read.

+ //Add an edit link, if the user has permission

Missing period.

} else {

Should be:

}
else {

The line afterward that else is indented too much.

+ $cols[3] = user_access('administer shortcuts') && $set->set_name != shortcut_default_set($user)->set_name ? l('delete', "admin/config/system/shortcut/$set->set_name/delete") : NULL;

Are we trying to do everything on one line?

Also, shortcut_default_set() doesn't need to be called on every foreach. Nor does shortcut_current_displayed_set()

Everything in shortcut_set_add_form_submit() is indented two spaces too much.

I just did a closer review of this from a code perspective. The code seems fine to me. I only found minor things, listed below.

 }
 
+
 /**
  * Implements hook_permission().
  */

Extra space shouldn't be added there.

+    'title' => 'Delete this shortcut set',

The word "this" in a menu item seems odd to me?

+    'title' => 'Set Current Shortcut Set',

Shouldn't be all caps.

+ * @param $user
+ *   (optional) User object for the user whose shortcuts will be administered.

This should really be $account.

+  return theme('shortcut_set_admin', $rows);

It's better to return an array from this page callback, with '#theme' => 'shortcut_set_admin', etc.

+    $cols[0] = check_plain($set->title);

(and elsewhere)... seems like you could just use $cols[] rather than defining a specific numeric key?

+    // Check if the specified set exists.
+    if (isset($shortcut_set->set_name)) {
+      // Set the default set.
+      shortcut_set_assign_user($shortcut_set, $account);
+      $replacements = array(
+        '%user' => $account->name,
+        '%set_name' => $shortcut_set->title,
+      );
+      global $user;

Ideally global $user should be at the very top of the function.

+/**
+ * Theme function for the shortcut set admin form.
+ *
+ * @param $rows
+ *   An associative array containing a row for each Shortcut set with
+ *   - Shortcut title
+ *   - Edit link
+ *   - Delete link, if available
+ *   
+ * @return
+ *   A themed HTML string representing the content of the form.
+ *
+ * @ingroup themeable
+ * @see shortcut_set_admin()
+ */
+function theme_shortcut_set_admin($rows) {
+  $output = '';
+  $header = array(t('Name'), array('data' => t('Operations'), 'colspan' => 3));
+  $output .= theme('table', array('header' => $header, 'rows' => $rows));
+  return $output;
+}

Seems like the "@param $rows" description could use a little work ("Shortcut" not capitalized, for starters).

Also, the comments refer to this as a form, but it isn't one, right?

-    '#description' => t('The new set is created by copying items from the @default set.', array('@default' => $default_set->title)),
+    '#description' => t('The new set is created by copying items from the default set.')

What is the rationale for this change?

+  $form_state['redirect'] = 'admin/config/system/shortcut/';

Doesn't need the trailing slash.

Are the screenshots in #36 still accurate?

For the record I would like to see a slightly more polished version of #33 go in.

i.e.

I don't want this: http://drupal.org/files/issues/shortcuts-user_admin.png
I do want this: http://drupal.org/files/issues/shortcut-change-set.png

This point is being ignored but I'll drum my beat another time in case it sticks.

Having those edit and delete links on the user edit screen is going to be a big problem. Seriously. We have a module that allows one shortcut set to many users. That means the links in shortcut sets are not editable on a user by user basis. Everything else on the user account pages affects only that single user. Editing and deleting shortcut sets potentially affects EVERY user.

The current design already makes it look like each user has their own individual set of admin links. That's how I thought it worked when I first started playing with the module. It only dawned on me that this wasn't the case when I read the code.

It looks like it works quite nicely at the moment because we are not testing the design with multiple users. With one administrator, you have no problem. In fact, with one user it is great.

The proposed addition of edit and delete links on the user edit page makes this problem worse.

There is one use case where I can see this screen making sense. That's when the administrator has enforced one user to one shortcut set. In that case editing their list of shortcuts won't affect anyone else. But we haven't really designed the module for this scenario and we are relying on a very knowledgeable administrator to set permissions that make our broken design work. Sure, it makes the module slightly more flexible, but it makes it far LESS user friendly. In this case the delete links on the user page still won't make sense.

If this goes in I will create a new critical issue to get it taken out again and discussed properly. In fact we probably need to create such an issue anyway but I'd prefer not to take one step back before we try to move forward again. David_Rothstein in #50 is touching on the same issue I think although it looks like he sees the problem a bit differently to me.

Well alpritt in #56 is bringing up some good points. We definitely don't want to *introduce* WTFs here in our epic quest to solve WTFs.

But furthermore, I agree that adding edit/delete links anywhere other than admin/* is non-standard to anything else we do in core, anyway...

Kudos to everyone working so hard on this and trying to sort out the multiple thorny issues.

Big Problems Still Remain

Help-text is difficult because module is a so different depending on permissions settings
This module, even with the current attempts at fixing it, is going to be really hard to write sensible help-text for. Settings of the permissions in different combinations makes this module a completely different beast for each permutation. What we really need to do is write separate help texts for each different combination of the three permissions and display each one depending on the current users combination of permissions. If that isn't practical, we need really verbose help text that covers the different permissions permutations and simply render the pages uglier but with some chance that users can actually understand what the module is doing and how they can use it.

We must clearly tell users that their shortcut sets may be available for use or editing by other users
Because of privacy issues as well as usability problems, we must explain to users in detail how this module works.

Sets should not be editable by more than one user without a lot of functionality added
Shortcut sets that are available for editing by more than one user are a kind of collaboration tool -- sort of a shortcut set wiki. But there is no changelog, there isn't even possibility to describe what the purpose of a certain set is. We are providing functionality to collaborate on creating a shortcut set without giving people standard collaboration tool support.

I share @alpritt's concerns and I agree with @David_Rothstein when he writes

At some point, there needs to be a discussion (or a decision) about what audience the shortcut module is intended for - it probably can't keep trying to be everything for everyone, and it might make more sense to take out functionality (and relegate it to contrib modules) than to keep shuffling the UI to make it match "core patterns" which don't fit with the module's actual functionality.

I would vote for pulling this out of core as not ready for prime time. If we do keep it in, we need to add LOTS of help-text...

Shai

A specific gripe about use of the string "default" and what happens when you create a new set:

The pre-installed shortcut set should not be called "default." Drupal comes with two content-types pre-installed on a standard installation. Neither of them are called "default."

The name should be descriptive and verbose. I would recommend, "Content related links shortcut set" or even "First set of shortcut links."

Also, it makes no sense that when you create a new set of shortcuts that it copies the "Create content" and "Find content" links into it. WTF?

Shai

As far as I'm concerned, we're not releasing Drupal 7 without this module. The module is integral to our new IA strategy for power users.

I feel like this relatively simple issue (make it so you can edit/delete shortcuts) got wrapped into a whole pile of other "wouldn't it be nice?" stuff. I was hoping we could keep the meta issue for that kind of things, and we could use this issue to just. fix. the dang. bug.

Bojhan showed me http://www.screencast.com/users/Bojhan/folders/Jing/media/4759ea78-e1d1-... on IRC.

I would love a patch for this issue that just did exactly that, and nothing else. Anything else we can discuss in other issues.

Good plan! :)

Not stealing this. We spoke in IRC.

Okay this is #33 with some code and comment fixes.

I removed the half fix for:

1. Users have the option to change their set of shortcuts even if there is only one set to choose from. Bit pointless.

...which removed the link to that page but left the tab on the user account pages. I want to think of a nicer way to handle that, and it isn't critical so I vote to leave that problem be for now.

--

Take a look theme_shortcut_set_admin($rows). Not sure what I think of that. I guess its useful. But can we feed in something other than table rows perhaps. I couldn't think of a better way of handling that but perhaps there is a better way of handling that. Something to consider.

Thanks for channeling me. :)

Sorry to disappear for a bit. I spent way too much time on this yesterday, and need to get caught up on 'real' work today. I might have some time this evening, but can't promise it.

In short, I think #56 added a lot of clarity to the discussion. Thanks for the clear & dispassionate explication of the issues. I think #58 suggests a better path forward: I'd rather add help text than another UI. I have yet to hear a solid response to the issue raised in #62 paragraph 2. But we can try to solve that in another issue once we get this back to something simpler.

I'm comfortable with the idea of #68, though I haven't reviewed it myself yet.

I applied the patch from #68 to a fresh install of D7 HEAD. It applied cleanly.
admin/config/system/shortcut is certainly much better.

However, AFAIK, in order to edit a set,

1. the user must have permission to use the toolbar and the toolbar be on... OR
2. the user must have access to admin/config

Wasn't that link going to be on the shortcut tab for user/. I don't quite understand @alpritt's explanation in #68 and if he intentionally did not put a link there.

Shai

Another problem:

Based on a fresh install of D7 HEAD with the patch from #68 applied:

The only pages that have links to be able to add a set are:

• admin/config/system/shortcut
• admin/help/shortcut

Shai

@jhodgdon wrote

====> Is it possible to set up permissions so that there are users who have permission to create/edit/update/delete (CRUD) shortcut sets (or some subset of these permissions), but who don't have permission to visit the page admin/config/system/shortcut?

Yes

Shai

While we are on permissions...

I've filed an issue with a patch to fix the following problem. It's at: http://drupal.org/node/685020

When testing the latest patch I noticed a big (but small in size) mistake in the string used for one of the three permissions.

Now: 'Select own shortcut set'

What it needs to be: 'Select any shortcut set'

Note that one of the other permissions is: 'Edit own shortcut set'

In "Edit own shortcut set" "own" means the one you created while in "Select own shortcut set" "own" means the one you chose. Very confusing. Simply switching "own" with "any" in the "select" permission makes a big difference.

This one is NOT thorny! Take a breath and go over there, +1 it. :)

--

re #75

The only pages that have links to be able to add a set are:

* admin/config/system/shortcut
* admin/help/shortcut

Well help does not even have a direct link. But anyway, is this a problem?

--

re. permissions:

jhodgdon, you already explained the problem yourself in #61. That means you can't work around this issue in the UI because that's not where the problem originates. Also, I don't think it is a difficult fix (we just have one too many permissions). The module will feel less flexible when fixed, but the truth is that it was never as flexible as the UI and permissions pretended in the first place. In any case, there is no regression in the proposed patch so I don't think that is a problem for this issue. Shai's 'non-thorny' permission issue mentioned above might be a better place. :)

--

The screenshotsThe attached mockups are addressing comments in #69.

Obviously there wouldn't be a delete link next to the default set. And if we can change the name of the default set we'd have to mark that it is the default some other way than by its title (perhaps by adding '(default)' after it).

But looking around the rest of /admin this looks like the standard way more or less. Good approach?

A) As I stated above I believe the real issue is that we have a permission that doesn't make sense. Other than rewriting the module I can't see any way to tackle this issue other than removing the 'Edit my shortcut set' permission completely. Which would make this point moot. Adding links to this screen for people without admin access feels like putting a handle on a leaky bucket. It won't do any harm, by all means do it, but I don't think it really solves the issue.

B) There are no links to that admin screen because that admin screen doesn't exist. It is just a mockup, waiting for feedback (sorry I was a bit unclear about that in my comment above). Should both permissions be able to get to that screen when/if it is written? Again, I don't care, because it will be broken either way. But as long as we move one step forward I'm happy.

Sub.

Is tere any reason this isn't moving forward? We can't fix this with all the controversial fixes? I am totally fine with leaving the (user can't edit his own shortcut set) as a new bug open.

"...any reason this isn't moving forward?"

The bottom of comment #81 has mockups for changing the name of the shortcut set, which touches the same admin table etc. I'd like those mockups to be reviewed. IMO it makes sense to incorporate that here.

Yes, it does - but #81 doesn't have a patch.

Not critical. Please read and understand Priority levels of Issues, thanks.

In other words: This is the result of introducing a feature that depends on other modules being enabled to work properly.

Quick fix: Disable it.

Anyway, would be cool to get this sucker fixed. :)

This is actually critical - things that prevent you from working with it. This is defnitily an issue that qualifies as that, since you cannot delete a shortcut, edit a shortcuts name.

Patch coming later today.

This implements the mockups in #81.

"Cannot redeclare shortcut_set_delete_access() (previously declared in drupal\modules\shortcut\shortcut.module:243) in drupal\modules\shortcut\shortcut.module on line 260 "

When trying to install this

A) not to be fixed, or discussed in this patch

B) This should be fixed indeed

C) See #102

D) You should be able to delete sets, even if not your own.

E) Should be fixed

Re #105:

For (a), you should not be able to delete the sitewide default set (just like you can't delete the fallback text format in the filter module). Otherwise admins should be able to delete anything.

(b) and (d) are already features of the existing codebase - although (b) should be prevented at the UI level also, once there is a UI :)

(c) sounds like a good idea although I'm not sure how to get a truly accurate count of N...

I will review any patch that comes out of this issue, but please please please please can we keep this patch as focused as possible? This issue is already over 100 comments and keeps shifting back and forth - all of us who participated here have been guilty of expanding at one point or another :) Many of the things mentioned above already have other issues in the queue, and the ones that don't, should.

Personally, I think a patch that focuses very tightly on the issue title as well as on something like the mockup in #63 will be a lot easier to review and get committed.

Hm, regarding what I said about (a) and (b), it turns out there is actually a little bug in the existing code. I posted a patch here: #717308: Shortcut module does not properly prevent default set from being deleted

Well, this is quite a large patch and some very nice work in there - but I'm not sure it's managing to stay very focused on the main issue :)

Nonetheless, here's a review of the full patch. This is done in order, so it mixes all sorts of comments together, including some UI ones.

  * This set will be displayed to any user that does not have another set
- * assigned.
+ * assigned, unless a hook_shortcut_default_set() implementation overrides.

Maybe "...unless overridden by a hook_shortcut_default_set() implementation"?

+      $output = '<p>' . t('Define which shortcut you are using on the <a href="@user_shortcuts">Shorcuts tab of your account page</a>', array('@user_shortcuts' => url('user/' . $user->uid . '/shortcuts')));

Typo ("Shorcuts"), and missing a closing p tag.

+  $items['user/%user/shortcuts/switch'] = array(
+    'title' => 'Choose shortcut set',
+    'type' => MENU_DEFAULT_LOCAL_TASK,
+  );
+
+  $items['user/%user/shortcuts/edit'] = array(
+    'title' => 'Edit shortcuts',
+    'page callback' => 'user_edit_shortcuts',
+    'page arguments' => array(1),
+    'access callback' => 'shortcut_set_edit_displayed_access',
+    'access arguments' => array(1),
+    'type' => MENU_LOCAL_TASK,
+    'file' => 'shortcut.admin.inc',
+  );

I don't understand why we would move "edit shortcuts" to the user account tab? When I click "edit shortcuts" in the toolbar I would really expect it to take me to an admin page, but with this patch it does not. Also, by putting it under the user account, I think this gives the impression that the edits only affect that user, even though they actually affect anyone else using the shortcut set.

What is wrong with leaving it under the admin pages? There seems to be a lot of discussion above in this issue about things not working for users with certain permission combinations, but I really don't understand what those particular things are. I have tried all three "non-administrative" permission combinations (a user with "Edit own shortcuts", a user with "Select own shortcut set", and a user with both) and everything seemed to work fine - I found one little bug which I posted a quick patch for at #717766: Access denied message for certain users trying to switch their shortcut set. I don't think there is anything wrong with putting these pages in the admin section since the primary use case of the module is intended to be administrative - and a regular user with the correct permissions still has access to them there.

Now, the above all does assume that the shortcuts are being displayed in the toolbar, which shows an "edit shortcuts" link - vs the shortcut block, which does not. Ironically, this is the original problem that @jhodgdon raised at the top of this issue - but we have obviously moved a long way from that in this thread :) Given that it's such a separate problem from the rest, I created a new issue for it now, at #717812: Add "edit shortcuts" contextual link on the shortcut block. But assuming we get an "edit shortcuts" link on the block, everything should make sense for sites using that also.

I'm also pretty uneasy about this patch removing the administrative UI for choosing a different shortcut set. That one is a bit less of a problem than the above (and the duplicate UI between that and the user account page is currently a problem) - however, the same point as above remains; for most sites I'd expect this to be an administrative action and therefore it belongs in the administration section like it is now. I still think it would really be better to deal with in its own issue, such as #632210: Move shortcut settings from user/%user/shortcuts to user/%user/edit - it's a bit of a complicated discussion, and I don't see how it's related to the rest of this issue at all.

 function shortcut_theme() {
   return array(
+    'shortcut_set_admin' => array(
+      'file' => 'shortcut.admin.inc',
+    ),

Shouldn't this get a 'variables' key with the shortcut set variable defaulting to an empty array?

- * @param $shortcut_set
- *   (optional) The shortcut set to be edited. If not set, the current
- *   displayed shortcut set will be assumed.
+ * @param object $shortcut_set
+ *   The shortcut set to be edited. If not set, the currently displayed
+ *   shortcut set for the currently logged-in user will be assumed.

Here (and elsewhere):

Why remove the "(optional)"?

Also, I have to say that the @param object thing personally drives me crazy, and I think it would be much better to incorporate the word "object" into the description instead. That's the way it's done in the vast majority of places in core. But, I do see that it's in the coding standards these days... well, whatever :)

 /**
+ * Access callback for editing a particular user's displayed shortcut set.
+ *
+ * @param $user
+ *   User whose displayed shortcut set is to be edited.
+ */
+function shortcut_set_edit_displayed_access($account) {

$user should be $account.

-  return user_access('administer shortcuts') || (user_access('switch shortcut sets') && (!isset($account) || $user->uid == $account->uid));
+  return user_access('administer shortcuts') || (user_access('switch shortcut sets') && (is_null($account) || $user->uid == $account->uid));

What is the reason for changing this? My understanding is that normally isset() is preferred.

-  $default_set = shortcut_default_set();
-  if ($shortcut_set->set_name == $default_set->set_name) {
+  // Don't allow deletion of system default shortcut set.
+  if ($shorcut_set->set_name == SHORTCUT_DEFAULT_SET_NAME) {
     return FALSE;

Typo in the $shortcut_set variable name, leads to a PHP notice.

Also, probably put "the" in front of "system default shortcut set".

   // Allow modules to return a default shortcut set name. Since we can only
   // have one, we allow the last module which returns a valid result to take
   // precedence. If no module returns a valid set, fall back on the site-wide
-  // default.
-  $shortcut_set_names = array_reverse(array_merge(array(SHORTCUT_DEFAULT_SET_NAME), module_invoke_all('shortcut_default_set', $account)));
-  foreach ($shortcut_set_names as $name) {
+  // default, which is the lowest-numbered shortcut set.
+
+  $suggestions = array_reverse(module_invoke_all('shortcut_default_set', $account));
+  $suggestions[] = SHORTCUT_DEFAULT_SET_NAME;
+  
+  foreach ($suggestions as $name) {

Nice - this is much cleaner! I don't see any reason to add the blank lines here though - it's all part of the same code flow.

+/**
+ * Returns the title of a shortcut set.
+ *
+ * This is used as the title callback for the editing pages for sets.
+ *
+ * @param $set
+ *   Shortcut set object (output of shortcut_set_load).
+ */
+function shortcut_set_title($set) {
+  return $set->title;
+}

I think it would be more consistent with the rest of the module to use $shortcut_set here. Also, I think the @param description should be more like "An object representing the shortcut set, as returned by shortcut_set_load()".

\ No newline at end of file

Please add one :)

- * Menu callback; Build the form for switching shortcut sets.
+ * Builds the form for switching shortcut sets.

Here (and elsewhere):

I'm a big fan of getting rid of the incorrect capitalization, although you will find people who swear that is the way it's supposed to be done, and they seem to be having their way on a large part of the codebase :) (There should really be an issue to clean this up throughout core if there isn't one - it's totally inconsistent.)

But... I'm not sure about removing the "menu callback" part altogether. It seems to be used in most places, and in general I think it can be helpful to know what the function is there for. In this case, since it's a form, maybe it's OK to remove because it's obvious why forms exist - but for general page callbacks (at least one place in the patch) I think it's best to keep.

-  // Only administrators can add shortcut sets.
-  $add_access = user_access('administer shortcuts');
-  if ($add_access) {
-    $options['new'] = t('New set');
-  }
-
   $form['account'] = array(
     '#type' => 'value',
     '#value' => $account,
@@ -67,12 +62,6 @@
     '#default_value' => $current_set->set_name,
   );
 
-  $form['new'] = array(
-    '#type' => 'textfield',
-    '#description' => t('The new set is created by copying items from the @default set.', array('@default' => $default_set->title)),
-    '#access' => $add_access,
-  );

Why remove these?

I get why we are adding a standard local action for adding shortcut sets (elsewhere in the patch) but I don't understand why we would remove them here too. I did not design this, but I really think it makes sense as is: When "switching" shortcut sets you might very well want to switch to a new one, so keeping it on this screen preserves the flow. Plus, as someone mentioned before (either above or on another issue, I don't remember), removing these means that anyone who installs the module and sticks with a single shortcut set - probably the most common use case - is confronted with a single radio button on the page for switching shortcut sets; i.e., that page is completely useless to them.

+      l('list links', "admin/config/system/shortcut/$set->set_name"),
+      l('edit shortcut set', "admin/config/system/shortcut/$set->set_name/edit"),

While this separation makes the UI consistent with the menu module (which shortcuts are closely related to), it seems to me that the menu module is kind of the odd one - in general I think it's more common in Drupal to have single "edit" and "delete" links in this kind of admin table, with the "edit" page taking you to a form that lets you do a couple things at once.

Personally, I think that model would be way better here - i.e., why not just have a single "edit" page which consists of adding a field for editing the shortcut set name on top of the (existing) form for reordering the links? Compare, for example, the Filter module. I don't know about anyone else, but I can't tell you how many times I've clicked "edit menu" in the menu module when I actually wanted to edit the links (which inexplicably is done from a page that says "list links") and then when I get to the edit menu page it is one of the most useless pages in all of Drupal :) With shortcuts I think it would be worse because you can do even less from the "edit" page - the new page added by this patch literally only lets you edit the set name (plus click delete, but that duplicates the delete link which this patch adds to the admin table anyway).

+/**
+ * Loads the form for editing a particular user's displayed shortcut set.
+ */
+function user_edit_shortcuts($account) {
+  $set = shortcut_current_displayed_set($account);
+  return drupal_get_form('shortcut_set_customize', $set);
+}

Incorrect function name - should be prefixed by 'shortcut_'. Also, in the function description, perhaps "Builds" rather than "Loads"?

-  $form['actions'] = array('#type' => 'container', '#attributes' => array('class' => array('form-actions')));
-  $form['actions']['submit'] = array(
+  $form['submit'] = array(

Why remove the container?

+ * @param $set_name
+ *   The unique name of the set.
+ *
+ * @return
+ *   An array representing the form definition.
+ *
+ * @ingroup forms
+ * @see shortcut_set_edit_form_submit()
+ */
+function shortcut_set_edit_form($form, &$form_state, $shortcut_set) {

Mismatch between the @param and the function signature here.

+function shortcut_set_edit_form($form, &$form_state, $shortcut_set) {
+  $default_set = shortcut_default_set();
...
+  if ($shortcut_set->set_name != $default_set->set_name) {
+    $form['actions']['delete'] = array(
+      '#type' => 'submit',
+      '#value' => t('Delete'),
+      '#weight' => 10,
+    );
+  }

Looks like this introduces the same mistake I made originally which you fixed elsewhere in the patch at the API level - as per #717308: Shortcut module does not properly prevent default set from being deleted, shouldn't this be checking SHORTCUT_DEFAULT_SET_NAME instead?

+  // Find out how many users are assigned to this shortcut set.
+
+  $num = db_query('SELECT COUNT(*) FROM {shortcut_set_users} WHERE set_name = :name', array(':name' => $shortcut_set->set_name))->fetchField(0);
+  if ($num) {
+    $form['info'] = array(
+      '#type' => 'markup',
+      '#markup' => '<p>' . format_plural($num,
+        '@count user is using this shortcut set.',
+        '@count users are using this shortcut set.') . '</p>',
+    );
+  }

1. The calculation works for core, but if any module implements hook_shortcut_default_set() the calculation will likely not reflect the correct number of users actually using the set. Maybe we don't care that much - I guess they could alter this if they need to - but it seems a bit bad to potentially report incorrect information here.
2. I don't think there should be a space after the code comment.
3. Maybe $number rather than $num?
4. Should be able to just use fetchField(), rather than fetchField(0)
5. I think the '#type' => 'markup' line is unnecessary?

Or, if it is purely administrative, then I think the "switch shortcuts" page should be in admin. It doesn't make sense to me to have "switch shortcuts" and "edit your shortcuts" in two different places. Thoughts?

Yes, right now, "edit shortcuts" is in admin only, and "switch shortcuts" is in two separate places (admin, plus the user account page). I don't like that it is in two places either. I think (ideally) it should be on the admin page only, and the the "shortcuts" tab on the user account should go away -- there are other ways to let admins edit a particular user's shortcuts, which is the only functionality those tabs provide that isn't duplicated elsewhere. However, I just didn't want to get into it in this particular issue :)

- In either case, we MUST have a link somewhere for a non-"administer shortcuts" person to edit their shortcuts if they have permission.

So just to clarify again, besides #717812: Add "edit shortcuts" contextual link on the shortcut block, is there anywhere you see where such a link is currently missing?

If I understand correctly, I agree with the strategy in #114.

There seems to be a lot of discussion above in this issue about things not working for users with certain permission combinations, but I really don't understand what those particular things are. [... ]all three "non-administrative" permission combinations [...] seemed to work fine.

Yes, everything works fine. The problem isn't permissions restricting anything; it is permissions allowing you to do too MUCH. But as long as the only setting that can be changed under /user is to switch shortcuts this can be discussed in a separate issue. That isn't the case in the latest patch, but it sounds like there is agreement to get the edit page out of /user in the next patch.

So how about this strategy:
- We'll add edit shortcuts link to the block over there on that other issue. Then everyone who can will have permission to edit their chosen set of shortcuts.

Sounds good.

- We'll leave switch shortcuts on the user pages (but it should be within edit and not visible until you start editing your user profile, if it isn't already, and I think it isn't currently).

Sure, I think it's fine to leave them on the user pages for this issue.

The last patch removed them from the admin interface as well, which I was less sure of - however, that may be simplest from an implementation standpoint (since the patch needed to steal that page's URL anyway), and it did automatically get rid of the duplication. It's not the behavior shown in the mockup in #63, though, which keeps the admin screen for this - and I thought there was some consensus that that mockup is what we're aiming for in this issue? - but it's OK with me if that's what other people really want.

In terms of moving them underneath the user's edit tab, I believe the existing patch at #632210: Move shortcut settings from user/%user/shortcuts to user/%user/edit started to do that (although I haven't really looked at the patch).

- Other admin screens as in the last patch.

I'm still unclear on the reason for having a separate "list links" and "edit shortcut set" pages... some parts of Drupal do it that way, but others don't. And I believe all the parts that do have at least one other thing besides the name that you can edit from that page (e.g., menus at least have a "description"). With shortcuts, it is literally just the name. I've attached a couple screenshots below showing visually the two issues I previously mentioned above with that approach. Again, I think the best analogy in the rest of Drupal core is the Filter module; there, a text format doesn't really consist of anything besides its name and the list of filters, so there is no separate page for editing just the "text format" itself - that wouldn't really make sense. Also, the Filter module approach is the same one shown in the mockups in #63, which, again, I thought there was some consensus on.

I would like an opinion from user interface team (Bojhan?) about whether non-admins should be popped into admin pages (with potentially admin theme) when they click on an "edit shortcuts" link.

Something to point out here is that there are a number of other permissions in Drupal that don't necessarily sound like they should be limited to "admins" but still give you access to various administration pages (which would display with the admin theme if that option is enabled). Some examples include:

• "Access the content overview page"
• "View site reports"
• "View content access statistics"

In most cases, those permissions don't even give non-admins any kind of link (just access to the URL), and at least we are planning on making sure the Shortcut module gives non-admins a link :) But again, I think the main use case of the Shortcut module (out of the box) is for administrators. The permission for editing shortcut sets is maybe useful for a site with a few sub-administrators who don't want to step on each other's toes - but to (usefully) grant that permission to hundreds of normal site users, you'd need another module. The intention of the core Shortcut module is that a contrib module could easily extend it to provide that kind of functionality, but I don't think it needs to be optimized for that on its own. Personally I think it would be up to any such contrib module to, e.g., implement hook_menu_alter() so that the page for editing shortcuts is displayed without the admin theme.

The only thing I'm not sure of is if someone has shortcut edit permissions but not "access admin pages" permission (or whatever is it called), aren't they forbidden from any page whose path starts with admin? Not sure about that...

I believe the way it works is that "access admin pages" is required to view /admin itself (and therefore inherited by default by all pages under /admin). However, any page under /admin that has its own access callback will still use that instead.

I am inclined to agree with jhodgdon here, following the logic of menus and taxonomies this should make more sense. We have various patterns in core on this, there is no clear standard and most definitely not on pages which both list intensely, provide an add and will have a name.

Some suggestion on the labelling though :

edit set name - delete set.

superscribe

Can you attach screenshots, for reviewers?

Quick review, but this looks pretty close to me.

--

In shortcut_set_switch()

  $form['#attached'] = array(
    'css' => array(drupal_get_path('module', 'shortcut') . '/shortcut.admin.css'),
    'js' => array(drupal_get_path('module', 'shortcut') . '/shortcut.admin.js'),
  );

Are either of these files still relevant? If not we should tidy them up.

--

We'll leave switch shortcuts on the user pages (but it should be within edit and not visible until you start editing your user profile...

Wasn't sure what you were saying at the end there, but it sounds like you were planning to put this on the edit tab instead of giving it its own shortcut tab. I'm on the fence on that one, but am interested why you decided to keep it in its own tab still. (Or is this just so it can be discussed in the other issue?)

--

'Edit set name' for the tab is a bit of a misnomer since it has a delete button in there as well. This probably isn't a major issue since the delete link appears in the table now so users should be able to find this function; but perhaps we can think of better wording.

--

There's some general clean up of comments, whitespace, etc which should perhaps be in a different issue (which I would be happy to review).

--

+  $replacements = array(
+    '%set_name' => $set->title,
+  );
+  drupal_set_message(t('The %set_name shortcut set has been created. You can edit it from this page.', $replacements));

We can probably condense this to one line.

I've looked at the patch again but not tried it out this time -- this looks pretty close, I think! Most comments below are relatively minor and/or easy to fix. And with the "edit set name" wording rather than "edit shortcut set" I do think it makes the proposed menu-module-style UI much more clear. (Similar wording was tried on the roles administration page a little while ago and rejected in favor of leaving it at "edit role"... but hopefully that won't happen here.)

+  case 'admin/config/user-interface/shortcut':
+    if (user_access('switch shortcut sets')) {
+      $output = '<p>' . t('Define which shortcut you are using on the Shortcuts tab of your account page.') . '</p>';
+      return $output;
+    }

This should say "shortcut set" rather than "shortcut" and it should link to the account page (didn't it link there in the previous patch?).

Also, a similar message/link should be added to the page for editing a shortcut set (since the patch removes the existing link from that page). This is important because that's the page you get to directly from clicking "edit shortcuts" in the toolbar, so it's where the main workflow of customizing your personal shortcut set will probably happen from.

+  $items['admin/config/user-interface/shortcut/%shortcut_set/edit'] = array(
+    'title' => 'Edit set name',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('shortcut_set_edit_form', 4),
+    'access arguments' => array('administer shortcuts'),

Hm, I find it slightly odd that people who can edit shortcut sets don't have permission to edit the set name. Not a showstopper, though.

- *   (optional) The shortcut set to be edited. If not set, the current
- *   displayed shortcut set will be assumed.
+ *   The shortcut set to be edited. If not set, the currently displayed
+ *   shortcut set for the currently logged-in user will be assumed.

I still don't see any reason to remove "optional" here (especially in this issue). This syntax is used all over core, and makes sense.

+function shortcut_set_edit_displayed_access($account) {
+  $shortcuts = shortcut_current_displayed_set($account);
+  return shortcut_set_edit_access($shortcuts);
+}

Is this function used anywhere? I think it might be a holdover from earlier patches.

- *   (optional) The user account whose shortcuts will be switched. If not set,
- *   the account of the current logged-in user will be assumed.

+ *   The user account whose shortcuts will be switched, or NULL to check
+ *   permissions for the logged-in user.
+ *
  * @return
  *   TRUE if the current user has access to switch the shortcut set of the
  *   provided account, FALSE otherwise.
  */
 function shortcut_set_switch_access($account = NULL) {

This change seems a little confusing; we're always checking permissions for the current logged in user, right? The parameter only determines which user account's shortcuts are being switched, not whose permissions are being checked.

-  // Sufficiently-privileged users can switch their own shortcut sets, but not
-  // those of other users. Shortcut administrators can switch any user's set.
-  return user_access('administer shortcuts') || (user_access('switch shortcut sets') && (!isset($account) || $user->uid == $account->uid));
+
+  if (user_access('administer shortcuts')) {
+    // Admins can switch anyone's shortcut set.
+    return TRUE;
+  }
+
+  if (is_null($account)) {
+    $account = $user;
+  }
+
+  if (user_access('switch shortcut sets') && $user->uid = $account->uid) {
+    // Users with switch shortcut sets permission can switch own set.
+    return TRUE;
+  }
+
+  return FALSE;

There are a few problems with this change: is_null (see comment later on), an "=" instead of a "==", and it seems a bit odd to assign $account = $user only to compare it to $user later... although I guess the grouping in the last part makes sense. I'm not really sure the code needs to be changed at all, though.

- * @param $account
- *   (optional) The user account whose shortcuts will be returned. Defaults to
- *   the current logged-in user.
+ * @param object $account
+ *   The user account whose shortcuts will be switched, or NULL to check
+ *   permissions for the logged-in user.
+ *
  * @return
  *   An object representing the default shortcut set.
  */
 function shortcut_default_set($account = NULL) {

Something got messed up here matching documentation to functions, I think... :)

-  if (!isset($account)) {
+  if (is_null($account)) {

I didn't explain this well enough before, but the issue is that we shouldn't be adding any is_null calls in this patch. Throughout core, !isset() is overwhelmingly preferred (it's used in hundreds of places), whereas there are maybe only a handful of examples of is_null, most of which probably shouldn't be there. Also see (for example) issues like #530950: user_access() should use isset() instead of is_null() - although here we probably don't care about micro-optimization, it's still better practice.

+ * Title callback for the editing pages for shorcut sets.

Typo here.

 }
 
+/**
+ * Returns the title of a shortcut set.
+ *
+ * Title callback for the editing pages for shorcut sets.
+ *
+ * @param $shortcut_set
+ *   An object representing the shortcut set, as returned by
+ *   shortcut_set_load().
+ */
+function shortcut_set_title($shortcut_set) {
+  return $shortcut_set->title;
+}

Somehow you removed the "newline" message from the previous patch file without preserving the new line at the end of the file :) This is very minor, but files in Drupal should end with a blank line (like this one did before the patch). It's definitely not done consistently everywhere, but it should be kept here; there are people who are trying to keep it like that throughout core, since it makes patch files cleaner.

-  // Only administrators can add shortcut sets.
-  $add_access = user_access('administer shortcuts');
-  if ($add_access) {
-    $options['new'] = t('New set');
-  }

-  $form['new'] = array(
-    '#type' => 'textfield',
-    '#description' => t('The new set is created by copying items from the @default set.', array('@default' => $default_set->title)),
-    '#access' => $add_access,
-  );

As before, I still don't see any rationale given for removing these (especially in this issue). We need a way for people to conveniently add shortcut sets that they want to switch to. If you really want to change this part of the UI, I think it should be a separate issue unless there is some connection I'm missing here.

+    // There is only 1 option, so don't make a form, just a message.
+    $form['info'] = array(
+      '#markup' => '<p>' . t('You are currently using shortcut set <em>%set-name</em>.', array('%set-name' => $current_set->title)) . '</p>',
+    );

This will produce a double <em> ... just "You are currently using shortcut set %set-name" is all you need here. (Also, should it maybe be "You are currently using the %set-name shortcut set", for better grammar?)

+      l('list links', "admin/config/user-interface/shortcut/$set->set_name"),
+      l('edit set name', "admin/config/user-interface/shortcut/$set->set_name/edit"),
+    );
+    if (shortcut_set_delete_access($set)) {
+      $row[] = l('delete set', "admin/config/user-interface/shortcut/$set->set_name/delete");

These should all be translated; e.g., l(t('list links')...

-  $form['actions'] = array('#type' => 'container', '#attributes' => array('class' => array('form-actions')));
-  $form['actions']['submit'] = array(
+  $form['submit'] = array(

I double checked this, and it appears these containers were all added in #482816: Make a consistent wrapper around submit buttons with the intention that all form buttons be wrapped in them (even if there is only one button) - this is for theming consistency. So we shouldn't be removing them here. You're right that that change appears to have caused a bug where theme_shortcut_set_customize() is now trying to render a form element ($form['submit']) that doesn't exist, but that should be fixed by changing the theme function... we can make a separate issue for that, or include it here if you want to since we're already talking about it :)

+ * @param object $shortcut_set
+ *   Object representing shortcut set, as returned from shortcut_set_load().

"An object representing the shortcut set..."

+  $info = '<p>' . format_plural($number,
+    '@count user has chosen or been assigned to this shortcut set.',
+    '@count users have chosen or been assigned to this shortcut set.') . '</p>';

I believe that proper syntax for format_plural() is to not use @count for the singular use case but rather to use the number directly... I think this is for easier translation. So, replace "@count" with "1" in the first line.

Also, shouldn't we not be printing a message at all (or at least printing something different) if $number is 0? From the screenshot it looks a bit odd to say "0 users have chosen or been assigned to this shortcut set".

***

@alpritt: 'Edit set name' for the tab is a bit of a misnomer since it has a delete button in there as well. This probably isn't a major issue since the delete link appears in the table now so users should be able to find this function; but perhaps we can think of better wording.

@jhodgdon: The edit set name page does not have a delete button on it.

@jhodgdon: Are you sure? I think I see code for that button... I'd probably agree with getting rid of it though. It might have some use cases, but the delete link in the table works well, and I'd rather keep "edit set name" (which is nice and clear) rather than trying to reword it to accommodate the button.

I haven't read the patch yet, but I wanted to provide some quick feedback based on the screenshots:

This is great work, and important UX work too. The current patch is a big step forward so we should try to get this in Drupal 7.

I've only been vaguely following this issue, but I have been working on #680850: Tests for Shortcut module. Clearly we'll need some new tests for this, so if anyone wants to outline what tests are needed I'll get cracking. At quick glance I see:

• edit set name
• delete set
• maybe: confirming the number in the "%d users are using this set" message is correct

I can take on posting the next version of the patch (hopefully the last version) later today... I don't think there's that much more to do.

I also agree with pretty much everything #137. Way too much in this issue already :) Also, while a "make default" setting might make some sense, it would add clutter to the UI, and it's not really clear what the overwhelming use case is. We removed the "make default" feature from the Filter module in Drupal 7 because it was confusing and a security risk. It wouldn't be a security risk here, but I do think it would still be confusing - to understand what is actually happening to existing users when you switch the default from out under them. For a site with a small number of shortcut users (which the core UI is mainly optimized for), it is probably easier to just switch each user by hand, if you really need to do this kind of thing. And it would not be hard at all for a contrib module to add a "make default" functionality.

The one thing related to the Default set that I do think is relevant for this issue is how to present it in the admin listing - should it be presented specially in any way? I don't think there's any consistency to be found here: For text formats it appears in italics, for roles it is displayed with the word "locked" next to it, and lots of other places (e.g., content types, menus) the "special ones" aren't displayed in any special way at all except that they are missing the delete link. The latter is the way we're doing it here now, and seems OK to me, but worth thinking about.

As for the menu module UI - yes. If we are copying it here, we copy its usability issues also :)

This patch is an improvement over what we have now so I'm comfortable moving forward with the proposed UI. It is not perfect but I'm OK with postponing my suggestions to follow-up patches.

- 'taxonomy/menu module does it too' is not a great argument because there are also administration screens do _not_ introduce 'edit $abc' tabs. In fact, I think taxonomy/menu module is the exception, rather than the rule -- and it is an exception that I'd like to see us get rid of in the future. The basic rule is that tabs are for listings of content, not for actions.

- The fact that tabs don't look like tabs is actually a well-known usability issue (based on formal testing in usability labs), and one of the reasons why we decided to create Seven.

I don't think there are other forms that have two editing-type actions, but in my mind that isn't really relevant. If we want a predictable and consistent user experience, tabs should be for listings. Two bad examples doesn't make for a good pattern. As said, we can debate the semantics of tabs vs tasks, and how it is relevant to UIs with two editing-types, in a follow-up issue. It is not specific to this issue. I'd rather see us focus on good code reviews of the patch that we have now, and then worry about a generic solution for two editing-type pages later.

OK, here is the new (and hopefully final) patch for this issue. I'm also attaching the interdiff file so it's easy to see the changes from the last patch. There aren't too many, and most are trivial and obvious. Worth mentioning:

1. I did fix the help text to appear on the correct pages. I also changed it to link directly to the user's shortcut tab rather than their main account page - I assume that makes sense?
2. I removed some CSS that is now unused as a result of this patch. The CSS in general is indeed a mess here (I'm not sure the right files are even being included in the right places), but I made a separate issue for that: #724782: Clean up the shortcut module's CSS
3. I restored the previous uses of "(optional)" in the PHPDoc - @jhodgdon already restored one in the previous patch, so I assume there's no objection to doing it with all of them?
4. Finally, I put back the ability to add a new shortcut set and switch to it at the same time (from the user account form). As per discussion above, @jhodgdon said she thought removing that "was requested above by usability review, I thought, but now I cannot find it in the 126 comments"... and I can't find it either. The best I can find is that at #680500: Shortcuts violate Drupal UI standards there is a commented screenshot which points to that feature and notes that there is "nothing like this in core". That's not an argument for or against it - it's a statement of fact :) Potentially removing or changing it brings up some complications that we don't want to discuss here, at 140 comments, when it's not even related to this issue. So, let's leave it as is - the code for removing it (in previous patches) can certainly be revived in a followup issue if necessary.

Summary

@jhodgdon has a great set of screenshots above so no need to repeat that, but when committing or reviewing this it's worth thinking about the shortcut workflow here - this is a big improvement, but there is followup work to do. To help focus on that I attach some "before-after" screenshots for particular workflows:

• The first set shows the screen you get to after clicking the "Shortcuts" link from the admin/config page.

  Before:
  

  After:
  

  Clearly we needed the new admin page, and this is good :) We do lose some simplicity here. The previous behavior did have a nice way you could just "jump in and try out different sets for yourself" that the new behavior does not have.

• The second set shows the screen you get to after clicking "Edit shortcuts" in the toolbar.

  Before:
  

  After:
  

  Overall, this looks a lot cleaner - seems like this page started off ugly and I swear some extra ugliness crept in the past few months, all of which is fixed by this patch :) We do, however, lose some of the immediacy and focus on the administrator who actually clicked this that we had before via the "Using set Default (Change set)" link.

  In fact, we're using the word "set" a couple times on this new page without the user really knowing what it is - I might suggest that the overall page title should be something more like "Editing the Default shortcut set" rather than just "Default", but that's basically a one-line followup patch, so no worries.

• The final (single) screenshot shows what is now the only page for switching to a different shortcut set (this page existed before this patch, but was duplicated via the admin screen shown above). It's worth it to get rid of the duplication, but taking people out of the admin area for this can in some situations be confusing:

  

I will plan to post a follow-up issue to address the above - my proposal will basically be to get rid of the "Shortcuts" tab on the user account page entirely, and replace it with some kind of "Assign shortcut sets" page in the admin area. This page would look a lot like the first screenshot above - i.e., focused on letting the administrator switch between different sets without ever leaving the admin pages, but with an ability to switch someone else's shortcuts from that page too (maybe via an autocomplete?). I'm not sure if that can really happen for Drupal 7 - but if not, for Drupal 8. I think that if done right, it will remove most or all of the remaining confusion.

In the meantime, let's get this in :)

Oops, let's fix one critical bug (the "=" vs "==" thing mentioned previously) as well as a little security issue that went unnoticed until now :)

Leaving at RTBC based on the above.

Oops, that one should have had a '.txt' ending...

Reuploading the actual patch so the testbot doesn't get confused.

This is teeny tiny but it's the first thing my eye keeps going to ...

re: http://drupal.org/files/issues/click.edit_.shortcuts.toolbar.AFTER_.png

Shouldn't operations TD in the "Add content" TR have a background color?

That's not caused by this patch, though - you can see it in the "before" screenshot also...

It also (probably) isn't even a shortcut module bug. Throughout core right now, all those draggable tables look pretty broken (e.g., with the "Weight" column showing even though it is supposed to be hidden by the JavaScript), so I think there is a good chance it is tied up with that. Hopefully there is already a bug report somewhere to fix those :)

I reviewed and tested this once more, and I think this is very close. The main problem is that the breadcrumbs are broken; as you can see on the screenshot they are merely repeating the title of the page so something isn't quite right yet. If we can get that fixed, than I think this patch is RTBC.

Subscribing.

Dries, I think that bug occurs for breadcrumbs throughout core and is being fixed in the issue at #576290: Breadcrumbs don't work for dynamic paths & local tasks - I just checked that the most recent patch there seems to fix this one too. So, based on that, I'm setting this to RTBC.

We are escaping the title in the wrong place. This is actually a wider issue with the overlay module. For example, we have the same problem when editing a node with the overlay. Also, without the overlay the title will now be escaped twice. So we should remove that check here and fix in another issue.

You're right, David. Committed this patch to CVS HEAD. Thanks all for the hard work -- this is a significant improvement.

I think he was referring to my use of check_plain() in shortcut_set_title() - and it turns out he's right. This security issue only shows up when the overlay module is enabled, so I fixed it in the wrong place.

I just created #725734: Overlay doesn't escape any page titles (residual cleanup) for this - and noted there the extra check_plain() call I added to the Shortcut module, which should indeed be rolled back.

now that this is committed, anyone want to respond to #136 for me?

jhodgdon asked me to sum up the tests that were already created for the shortcuts module ... http://drupal.org/node/680850#comment-2647184