Users don't get any feedback when they try to add more than the allowed number of shortcuts

Tagging. This would be a really easy one for a new contributor.

I think you're close here.

First, it should be an 'info' message instead of a 'warning' message.

Second, the maybe the second sentence could be closer to this: "The previously last link has been disabled in favor of the link that was just added."

Third, we might want to display a notice on the shortcut creation page as well. Something to the effect of "The shortcut bar can only display @limit links. If a new shortcut is added without first disabling one of the other shortcuts, the new shortcut will take the place of the last shortcut in the list"

Verbiage for things like this is not my strong point - might be better to have someone else chime in for the exact wording.

Finally, when you post a patch, you should set the issue statue to "needs review" so that people know to look at it :)

Whoops, cross post.

Thanks for the fast feedback. I appreciate every hint. :)

@Second: I'll take your sentence.
@Third: The message should be on admin/config/user-interface/shortcut/shortcut-set-* ?

Actually, I was thinking it would be a good addition to admin/config/user-interface/shortcut/shortcut-set-*/add-link

Already added it on admin/config/user-interface/shortcut/shortcut-set-*
Maybe the place is not so wrong, because normally you will be there, before you add a link.
But I can change it.

That's not something that should go in hook_help().

I think the shortcut add screen is a much better place for that message.

Here is a patch for the drupal_set_message part of the issue. I changed the language a bit. Hopefully it gives clear instruction on what has just happened.

Should we split the display message on the shortcut add screen into its own issue? If not, I can work on that next.

Would it be better to get confirmation from the user before making the change, than informing the user after making the change?

Or, as an alternative, provide an undo link. That should be relatively easy to do. Mhh ... not really. Simply creating a link will lead to the same message being displayed again.

I agree with Rocket777... it seems like the correct approach would be to notify the user attempting to add another item that they are approaching, or at the limit of items for a given set. I neglected to do this the right way to create a patch, but I added some code to shortcut.admin.module that checks the set limit and the number of items in the set, and outputs a message if the user is about to throw an item into "disabled" by adding a new shortcut.

//@ line 419 - 

function shortcut_link_add($form, &$form_state, $shortcut_set) {
$limit = shortcut_max_slots();
$number_enabled = 0;
	foreach ($shortcut_set->links as &$link) {
      if (!$link['hidden']) {
           $number_enabled++;
        if ($number_enabled == $limit) {
           $warningText = 'Adding another link will set your last link to "disabled"';
        }

      }
    }

  drupal_set_title(t('Add new shortcut..'));
  
  $form['prefix'] = array(
   '#markup' => t('Your site is configured to have '.$limit.' shortcuts per set.  You have used '.$number_enabled. '. '. $warningText.'.')
  );
  
  $form['shortcut_set'] = array(
    '#type' => 'value',
    '#value' => $shortcut_set,
  );
  $form += _shortcut_link_form_elements();
  
  return $form;
}

Since #1304486: Completely remove the ability to limit the number of shortcuts per set was committed (see #11) this doesn't seem to apply to Drupal 8. Does it still need the 8.x-dev version flag?

Here is a backport to D7.

Removing backport tag.

The backport tag stays on. :)

Except, duh, we determined this issue is 7.x only. Sorry for the noise.

However, can we add a test for this? :)

as by xjm request, I tested the patch for D7 and toke screenshots before-and-after

I attached the images, and my steps was:

added 5 shortcuts before the patch, it happened as this issue summary

after the patch, I removed the Test final shortcut, enabled the Test 5, and added the After patch shortcut

In the Add shortcut page, it already displays the new warning: Only 7 links can be enabled in the shortcut menu. New shortcut links will replace the last enabled shortcut in the list.

After I saved it the same warning shows up, and the new link got inserted as last enabled one, with the previous new shortcut moved to Disabled

so, patch looking good!

+++ b/modules/shortcut/shortcut.admin.incundefined
@@ -423,6 +423,19 @@ function shortcut_link_add($form, &$form_state, $shortcut_set) {
+    if ($shortcuts_enabled == $shortcut_limit) {

Shouldn't this be '>=', not '==' ?

good catch, Tim
I agree, it should be >=

Reviewed and tested.

Back to CNW for now, this still needs tests.

Attached you'll find the test for this issue.
Attemp for our first core contribution along with Frederico

#26: shortcut-maxlimitreached_testonly-1279910-26.patch queued for re-testing.

#26: shortcut-maxlimitreached-1279910-26.patch queued for re-testing.

+++ b/modules/shortcut/shortcut.testundefined
@@ -132,6 +137,16 @@ class ShortcutLinksTestCase extends ShortcutTestCase {
+      // Check if message for maximum number of visible shortcuts is displayed when necessary.

comment lines must wrap at 80 chars. http://drupal.org/node/1354

+++ b/modules/shortcut/shortcut.testundefined
@@ -132,6 +137,16 @@ class ShortcutLinksTestCase extends ShortcutTestCase {
+        $this->assertText($max_visible_shortcuts_message, t('The maximum number of visible shortcuts has been reached message.'));
...
+        $this->assertNoText($max_visible_shortcuts_message, t('The maximum number of visible shortcuts has not yet been reached. Do not show message.'));

I dont think we do t() in the assert like that. http://drupal.org/node/500866

Are the following two points out of the scope of the issue:

1. Also, I'm not sure about the naming convention of tests... http://drupal.org/node/325974
"Foo[Unit]Test extends [Web|Unit]TestCase"

2. think the location of the test needs to be in a tests directory.

oh... it says:
"For modules, a single [module name].test file is the general rule. It should be placed directly in the module folder.
For core facilities (all API functions that are in includes/), tests files are named [facility name].test and are placed in the modules/system/tests folder."

so if it's a single test file, I guess it's ok.

Re-rolled patch from current head. Fixes issues in #30 - comment lines must wrap at 80 chars and removed t() from the asserts.

The naming convention of the test seems to be consistent with most of the other Drupal7 tests. Seems to be a general inconsistency throughout the Drupal7 code base.

This patch fixes the issue on the shortcut form page only. The inline code still overwrites the last shortcut with no warning. Possibly adding a confirm_form would aid in usability.

The issue of the t() in the assertions is a separate issue: #1797510: Remove t() from assertion messages in tests for the shortcut module. Not sure we should track in both places.

+++ b/modules/shortcut/shortcut.testundefined
@@ -133,6 +138,17 @@ class ShortcutLinksTestCase extends ShortcutTestCase {
+
+      // Check if message for maximum number of visible shortcuts is ¶

trailing whitespace

Thanks for the reroll leslieg and eporama! In the future, when you're rerolling and making changes, it's nice to do them one step at a time so you can create an interdiff of what changed. It also makes it really easy to reverse apply the interdiff in the case above where eporama noted that there was a separate issue for removing t() from tests.

Here's how I would have done it:

1. reroll patch
2. git commit
3. make additional changes
4. git commit
5. git diff 8.x > drupal-user_shortcut-1279910-32.patch
6. git diff HEAD~ > interdiff.txt
7. Upload patch and interdiff to issue queue

@disasm, thanks for the tips.

I re-rolled the patches according to your suggestion and removed the changes to t() since those are in the other thread.

However, I actually think that there's a couple of easy improvements that might make more sense than the current patch as it stands. There are two issues that I see. 1) You don't actually know which link was disabled. So I have actually rolled the patch with a couple of more changes that tell you which shortcut was disabled. And 2) the inline add link gave no indication that a shortcut was disabled to make room.

And the other attachment.

Thanks for fixing that eporama!

disasm, yeah, had two attachments for 36, the interdiff was there, but the "attach" failed when I had wireless issues, then it showed as attached, but only uploaded the patch.

Thanks again for this, however; I don't see your changes in the actual patch. The patch you upload should always be the complete thing (with the exception of test-only patches, but they should always be attached with a combined patch). The interdiff is used for seeing the changes you've made since the last committed patch, or in the case of a reroll, the changes made after the reroll was complete.

I've included a review of both the interdiff and the patch for that reason, but please upload a patch combining the too (if you don't change anything else, no interdiff is required since we already have it).

Thanks again!

+++ b/modules/shortcut/shortcut.testundefined
@@ -115,6 +115,11 @@ class ShortcutLinksTestCase extends ShortcutTestCase {
+    variable_set('shortcut_max_slots', $shortcut_limit);

shortcut module has been converted to CMI. Please use config() instead of variables_set/get

+++ b/modules/shortcut/shortcut.testundefined
@@ -139,9 +139,10 @@ class ShortcutLinksTestCase extends ShortcutTestCase {
+      // Check if message for maximum number of visible shortcuts is displayed
+      // when necessary.

multi-line comments should use C-style comments.

Okay, I'm going to try again, but just making a patch of the reroll and the change in comment wrapping that YesCT mentioned. I'll add the new functionality separately to see what I'm doing wrong. So this is a very minor change. Just the new roll and the comment.

I'm not sure that your last two reviews make sense as this is a D7 issue at the moment... config() is only for D8 and the module has many multiline comments in the style that I used (in fact all of them but the documentation blocks). See http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/sh... for example.

I rerolled the patch to apply cleanly to 7.x, then essentially I did the following:
change the comment wrapping in file
git commit -m "wrapping comment at 80 chars" modules/shortcut/shortcut.test
git diff 7.x > /tmp/shortcut-maxlimitreached-1279910-43.patch
git diff HEAD~ > /tmp/interdiff.patch

So this patch and interdiff should be what you're looking for.

If you want to know more of the process I followed: https://gist.github.com/eporama/b04dd4568949ff273bb6

Your completely right! That's why reviews at 10:00 at night aren't a good idea ;-) Yup, since this is a D7 issue, ignore everything I said about CMI.

So, the text in the previous patches is a little inconsistent in that it uses the term "shortcut menu" whereas they are called "shortcut sets". They also don't tell you which shortcut link was just disabled. and it doesn't actually "replace" it, but moves it to a disabled state.

Also, the inline link to add a shortcut didn't give any indication that there was a disabling of a previous shortcut either. So I added a drupal_set_message on the page when a link is added that bumps an enabled link.

Only 7 links can be enabled in a shortcut set. The previous last shortcut "Modules" was disabled.

meant to set the status to needs review...

Coding standards:

} else {

should be:

}
else {

+++ b/modules/shortcut/shortcut.testundefined
@@ -133,6 +138,19 @@ class ShortcutLinksTestCase extends ShortcutTestCase {
+      // Check if message for maximum number of visible shortcuts is displayed
+      // when necessary.

Leave this as-is. I was wrong, just checked the standards.

+++ b/modules/shortcut/shortcut.testundefined
@@ -133,6 +138,19 @@ class ShortcutLinksTestCase extends ShortcutTestCase {
+      //$links = array_reverse($set->links);

This probably shouldn't be here. Adding commented out code in a patch doesn't make sense. If there is a reason, please add a comment staying why.

+++ b/modules/shortcut/shortcut.testundefined
@@ -133,6 +138,19 @@ class ShortcutLinksTestCase extends ShortcutTestCase {
+      } else {

This is one of the malformed else blocks.

+++ b/modules/shortcut/shortcut.testundefined
@@ -140,11 +158,18 @@ class ShortcutLinksTestCase extends ShortcutTestCase {
+      } else {

here's another malformed else block

Okay, here's another shot with better coding standards.

and resetting status as I should have done.

Updated issue summary.

The latest patch does look fine.

Glad to see something being done about this. It was needed.

Patch applied cleanly to Drupal 7.x. Changes recommended by David in comment #54 were applied.
David, you said: "Interesting this works when it's asserting the wrong text (@last_shortcut vs %last_shortcut) but I guess assertText() strips out HTML anyway..." and "Perhaps I'm missing something, but this looks like it's tracking the total number of shortcuts (regardless of whether they are enabled or not). Is the test working correctly?" but i wasn't sure if I needed to do any changes.

Fixing issue with quotes.

Matched text between .test and .admin.inc files.

Resetting status to Needs review