| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | shortcut.module |
| Category: | task |
| Priority: | normal |
| Assigned: | swentel |
| Status: | needs work |
| Issue tags: | API change, needs backport to D7 |
Issue Summary
Currently the shortcut module is configured to have a limit of 7 shortcuts per set. When you try to add an eighth, it (silently) replaces the seventh.
At http://drupal.org/node/511286#comment-2038092 (while working on a version of the initial shortcut patch that did not impose a limit) I wrote:
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.
In general, we cannot assume these shortcuts are going to be used in a particular way in the toolbar, and Drupal does not normally take the approach of limiting people based on assumptions about how they will be using a feature, so I think there might be argument for configuring the module by default to not have a limit.
Also, @alpritt at http://drupal.org/node/680500#comment-2460288 wrote this:
For the record, the way we deal with the 7 shortcut limit by arbitrarily disabling the last shortcut and replacing it with a new one is a definite WTF. It is probably too late in the dev cycle to rethink that one though. A drupal_set_message() might be worthwhile in the meantime however.
Comments
#1
If this came from the UX-Team, then this is a definitive mistake - lets lose the limit. People can decide for themselves to break it or not.
#2
Agreed with Bojhan. Let's just get rid of it altogether. Most anybody is smart enough to realize that you can't keep adding shortcuts if there's no room, and if you do, then it will overlap/break.
#3
If you like I can come up with something like how browsers handle multiple tabs (button on left and right of toolbar to scroll horizontally). Somebody needs to design those buttons however...
Edit
On the other hand... its about shortcuts. Hard to reach shortcuts aren't really useful.
#4
Here is a patch to stop enforcing the limit in core - but still allow other modules to write code that would trigger it to be enforced again.
As someone who never liked it in the first place, I'm happy to see people agree that it should go... but I wonder where all this agreement was when we were arguing against it before the module was fully written? :) This wasn't really a small side thing; it was deemed to be a "very important" part of the original UX design, and in some ways informed the entire design (e.g., the concept of enabled vs disabled shortcuts is much more important if you have an enforced limit).
Anyway, along those lines, notice that on the screen for reordering existing shortcuts in a set, the idea of a limited number of "slots" is baked into the UI. We aren't going to totally change that UI at this point, so what I did here is have 7 slots on that page by default (same as before), but if you are near the limit, it will give you more; i.e., on page load, it always makes sure you have at least two empty slots available. See attached screenshot where there are 6 shortcuts in the set, so an extra slot (8 total) appears on the admin page.
This patch results in some semi-dead code hanging around the module, but it's still available for contrib modules to use if they want to enforce a limit, so it's not totally dead. Overall, I think the patch is the right thing to do :)
#5
I just marked #1045602: Add to shortcut behaves strange as we add more items as duplicate.
We've also seen this come up a couple times in the Drupal Gardens forums. Here are a couple cases of that (compiled by George Cassie):
http://www.drupalgardens.com/content/add-more-7-shortcuts
http://www.drupalgardens.com/content/usability-issue-limit-shortcut-menu...
It seems clear that a lot of people want to remove this limit and/or are confused by.
The above patch still applies. Anyone care to review it?
#6
I totally support this. With my current settings and the browser window being about 3/4 of the screen width, the shortcuts do not extend to 1/3 of the window itself...
#7
I agree, this limit should be configurable by the site admin, there could be a different limit for the toolbar and the shortcut bloc too. I added the shortcuts bloc to the dashboard, and it's limited to 7 shortcuts there too...
#8
Just hit this limit myself and thought it was a bug.
#9
If I create a new set of shortcuts, it says "The new set is created by copying items from your default shortcut set" and will import all the shortcuts from the default set, BUT all the shortcuts will be enabled, so, it does not take in consideration the imposed limit. Though, if you want another shortcut to the menu, it defaults to 7 active shortcuts.
my steps to reproduce:
1)add a number of shortcuts (more than seven)
2)click on edit shortcuts
3) click on "Show row weights"
4) check the "Enabled" status on all the shortcuts, click "Save"
5) refresh the entire page or go somewhere (so that the page is refreshed) => you have more than 7 links
unfortunately, it goes away when adding another shortcut (defaults to 7). but you can redo those steps until you are satisfied :)
i do not know if this is a bug...
but, in my opinion, it could be nice to let someone decide (at least the admin) how many shortcuts you can get...
hacking the core is never a good solution
i attached a print screen of my overlay menu, if it is helpful in some way
#10
Great! This is just a workaround but greatly improves the overall experience of site management! Thanks je jorhe!
#11
Nice trick - thanks for sharing it.
#12
Tested patch in #4. It's working nicely.
First, i created 8 shortcuts to reproduce the issue ( one beeing automatically disabled ). I then applied the patch. 2 new empty slot appeared and I was able to enabled the disabled shortcut. I then created another one so I had a total of nine shortcuts. After adding the last shortcut 2 new empty slots were available. And by the way, the shortcuts are all displayed correctly in the shortcut bar.
#13
Je_jorhe, thank you! Though it's pity a bit, that we have to use such a hack.
#14
This is horrible UX, no novice Drupal site creator would ever figure this out. They'd just be confused as to why when they add a new link, the last one disappears. Then you'll get bug reports on d.o about this non stop.
It needs to provide a warning, that you're only allowed to have 7 links and then it should link to a configuration page where you can change this setting at the very least.
IMHO, I don't really see a point of limiting it at all. I assume the point was to restrict overzealous site builders, but really. If they add too many and it messes up their theme, so be it. They can remove additional ones manually. It should wrap down anyways. If I wanted to be baby-sat, I'd use wordpress :)
#15
Commit patch #4 to Drupal 7.2. Some hunk offset warnings, but otherwise works for me. We should push to get this into next months Drupal 7.3.
#16
Thanks. It looks like this patch has been pretty well tested... Now we just need someone to review the code, and if it looks good, mark it "reviewed & tested by the community".
It will need to be committed to Drupal 8 first, but the same patch applies there equally well, and I think there's a pretty good chance it will be acceptable for Drupal 7 also.
#17
Can I have a screen, what we will do when the screen size is to small to display all the shortcuts?
#18
#17 here is a screenshot with 6 long shortcuts and a small window size. It doesn't look as pretty as it could be but it works, adding more shortcuts would just make it even worse. But I think this design issue correspond to #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop
#19
You guys are "optimizing first" and placing limitations for everyone, when not everyone has a small screen. You guys are not using the 80/20 rule. Aka, stuff should work well for 80% of the people, not force everyone into the 20% camp.
If you have a small screen, don't add many shortcuts. It will look bad on your screen. But don't force everyone to 7 shortcuts. That doesn't make any sense.
The overflow issue should be solved properly in the theme layer, not by a limitation on the number of items.
Additionally the "Admin" bar above has 13 items, but my shortcut bar can only have 7. How does that make any sense?
Personally, I work on a screen with a 1920 pixel width. 7 items takes up like 1/5th of my screen.
This limitation needs to get removed. I see no good reason for it being here. If it stays, it needs to be configurable in the admin and people need to be warned via drupal_set_message, when they reach it.
#20
@j0rd Sorry, but we are doing what you are asking - I dont understand the rant.
I think we might want something like a > that scrolls to more items? I don't think it's really "small" screens that are a problem here.
#21
The screenshot in #18 looks way better than I was expecting :)
Since the shortcut bar already looks mostly OK when it wraps (better than the toolbar!) and in some situations the overlap behavior already occurs even without the limit of 7 shortcuts being lifted, I think any attempt to add scrolling behavior should be left to a different issue.
#22
RTBC? It looks good, and yes scrolling should be a new issue.
#23
Just a reroll, didn't apply cleanly with patch p0 - and not even with git apply.
#24
great.I just stepped onto this and it was a big WTF!
#25
Mmm, if we want to remove the limit, why don't we _truly_ get rid of by removing the 'slot support' in Drupal 8?
In other words, I'd be in favor of getting rid of functions like
shortcut_max_slots()and variables likevariable_get('shortcut_max_slots').#26
@Dries This confuses me a bit, does this mean we have to roll two patches? One to fix the D8 complete API removal of the limit, and one D7 to just remove variable of the limit? Honestly I never really got the whole backport thing, everything seems on a per case basis - and it tends to end up on having to roll two patches and reviews on two patches.
#27
I think complete removal could be discussed in a separate D8-only issue.
One of the reasons for leaving this in is to make it easy for someone to add back a limit (e.g. on particular sites). For example, the original designers felt that limiting the number of shortcuts was a very important feature:
http://drupal.org/node/511286#comment-1779578
http://drupal.org/node/511286#comment-2131274
#28
I already looked at this the other day, and it still looks ok to me.
Also agree that removing the limit entirely is D8-only material. (...but then again, we're hitting the essential question of what should and needs to evolve in the slow/stable pace of core here once more; this is a pure, single-purpose product feature [and thus a patch which I'd love to simply won't fix, ignore, send to hell, and don't care about], but since it is in core already, we have to care for it, regardless of whether we like or not. So in terms of that, adjusting the baked-in default seems more backwards-compatible to me than entirely removing it. That said, tagging appropriately.)
To clarify, I've looked at this twice and I believe it's acceptable to do this change (also in D7).
#29
sun, we're not asking you to care about every patch. There are patches you can ignore, instead of sending them to hell. ;-)
#30
I'd like some one to care about this issue. It's been 9 months and still no fix for this simple problem. It's making the shortcut module useless for my installs and patching every time after an upgrade is getting annoying.
This is one reason, I wish this module was in contrib instead of core. Then less people would care about backwards compatibility and this would have been resolved and long long time ago.
#31
@j0rd I suggest you make a D7 patch to remove the limit. And a D8 patch that removes all functions etc.
#32
I think we need to add a drupal_set_message in here when the max slots variable is set to a non-zero number and the user tries to add too many shortcuts.
#33
@cweagons No, we are removing the limit.
#34
Not according to the patch in #23. It changes the max slots to a variable in the variables table.
For D8, perhaps it will be removed altogether, but we can't just remove the limit in D7.
#35
How about we just split that off to a different issue? It's kind of a separate problem anyway: #1279910: Users don't get any feedback when they try to add more than the allowed number of shortcuts
---
@j0rd, it's really not clear what you're talking about here. Backwards compatibility is not what's been holding up this issue; the patch I posted in May 2010 dealt with that already. Rather, what prevented this from getting committed for so long was lack of code reviews. Moving a module to contrib will not make code reviewers magically show up. At most it would allow the code review process to be bypassed (which in the long run leads to worse code).
#36
In my opinion, we should commit this to 7.x and than upgrade this to 'major' or 'critical' so we can fix it the right way in 8.x.
#37
It's RTBC. Go for it. :)
#38
So let's move it to #7 for webchick per #36.
#39
subscribe
#40
Patch no longer applies, but makes sense.
#41
simple reroll of patch in #23 line by line
#42
I can confirm it's just a re-roll.
#43
The testbot ignores any patches with -d7 or -d8 at the end (those extensions should just be used when explicitly trying to prevent testbot from failing a patch for a different version).
Re-uploading patch from #41 for D8.
#44
I'm not clear what's supposed to be committed where, but the D8 patch doesn't remove the variable, so needs work per previous comments suggesting that's what would happen.
#45
The patch does need work, but I believe because of this:
+ $count_shortcuts = count(element_children($form['shortcuts'][$status]));....
- $count_shortcuts = count($shortcuts_by_status[$status]);
That discrepancy probably got introduced in one of the recent rerolls. (The original intent here was to simply move that line of code higher up so it could be reused, so if the code has been improved in the meantime there's no reason to revert that while moving it.)
In terms of what this patch should contain, I thought we agreed to defer larger changes to a separate, D8-only issue? That's what I suggested above, and no one directly disagreed with me; plus, our policy at http://drupal.org/node/767608 says that's what we should do.
Actually that policy suggests we should split off the D8-only issue beforehand (so interested people can further discuss or work on that without interrupting this one), so I just did that now: #1304486: Completely remove the ability to limit the number of shortcuts per set (upgrade path and followup)
For this issue, I think all we need to do is change the default (as the above patches do), and commit that to both D8 and D7.
#46
This is why keeping track of patches for multiple versions of drupal core and/or modules in a SINGLE THREAD is not only counter-intuitive but also counter productive. The latest patches now no longer resolve the original issue, which got lost somewhere as folks started changing the version number of this, plus, anyone looking for a solution for 7.x by filtering for 7.x issues now no longer see this as a potential solution, because its assigned to 8.x.
Come on community! Get organized! Separate versions = separate issues! They have separate solutions, after all! (for example, here we simply patch 7.x, however for 8.x there has been proposed more drastic measure).
Contributing for Drupal, and finding patches at all is becoming chaotic and aggravating, primarily for this reason. I strongly urge that stopping mid-thread version modifications be considered for addition to best practices! It benefits no one, except people more concerned with keeping the issue count down.
#47
That's not how it is. We patch the most recent version, then backport what we can.
#48
Patch #41 works for me in Drupal 7.12
#49
Still needs a bit of work, per above comments.
#50
argh, here we go again, why do I run into every bug there is? ;) When will this fix be part of a core upgrade? Instead of the entire patch, couldn't I just change the one line that sets the limit to 7, to my own limit, say 10? Or is 7 hard-coded elswhere?
#51
#52
@flightrisk, the max slot can be configured through a variable called 'shortcut_max_slots' (see shortcut.admin.inc). Just set the variable to a number you'd want. You can use, eg. drush vset shortcut_max_slots 10 or a simple variable_set() call in a custom module somewhere.
#53
#43: shortcut_no_limit-682000-43.patch queued for re-testing.
#54
The last submitted patch, shortcut_no_limit-682000-43.patch, failed testing.
#55
Re-uploading patch from #43 for D8 again.
#56
#57
The last submitted patch, shortcut_no_limit-682000-55.patch, failed testing.
#58
Maybe this one works... with correct paths.
Re-uploading patch from #43 for D8 again.
#59
Thanks for the reroll! But it looks like this still has the issue described in #45, plus there are some code style problems (the indentation is off - it should always be two spaces of indentation, as per http://drupal.org/coding-standards).
#60
Maybe this is the final? :)
#61
For anyone wanting a non-patch quick-fix solution for this then you can always add your shortcut menu to the standard menu interface with SQL like so:
INSERT INTO menu_custom(menu_name,title) VALUES('shortcut-set-1', 'shortcut-set-1');Then you can just enable all the items :-)
#62
I found another way of changing the limit so that one could increase it temporarily.
Open settings.php and add this line:
$conf['shortcut_max_slots'] = 10;
Credit goes to (not me):
http://coderoach.blogspot.com/2012/05/drupal-7-increase-number-of-shortc...
#63
The maintainer of shortcut should probally take a look at this
#64
Well, I'm going to be honest, we should simply ditch this completely, there's a patch for D8 at #1304486: Completely remove the ability to limit the number of shortcuts per set (upgrade path and followup) which is so ridiculously easy to backport we can get rid of all this misery. I vote getting that in, backporting and we're done with it.
#65
Agreed.
#66
I don't see how that other issue could ever be backported to Drupal 7.
But it was committed, so I guess that makes this one a D7-only issue now?
#67
@j0rd #19
Well said. I found this bizarre and unintuitive. Why not do a CSS "overflow:hidden" on the shortcut bar, so the menu items just slide behind; or a fluid transition into two rows.
I knew I wasn't the only one bothered by this. =)
#68
#66 2 years, 6 months, 9 days of waiting for D8 to bring us back to patch #4 for D7.
This is certainly one time where the patch workflow has failed miserably for something that required a simple fix.
Not sure what the best patch for D7 is, but ideally this would make it into D7 before D7 is End Of Life :)
#69
[edited]In Drupal 7 you need to click “display line weight” to show the enable/disable select widget. Then enable as many shortcurt as one wants and save. All enable shortcuts will be displayed without any limit.
Not an efficient fix, but that works.
That being said, what it the status of this issue for Drupal 7? Looks like it's the JavaScript part that limits the number on links to 7.
#70
Here is the D7 patch reroll (for #60 approach), i've tested, and it works well :)
#71
So quick solution (patch) for fix D7 shortcut limit issue is #70.
Another fix (hacking settings.php): #62.
And follow #1304486: Completely remove the ability to limit the number of shortcuts per set (upgrade path and followup).
#72
A word on fix #62:
It's a very nice solution. But I think there is one pitfall. When configuring a site with multiple users/roles the extra shortcuts (number > 7) remain hidden. By enabling 'Use the administration pages and help' under permissions/roles this problem is fixed, but then you probably give the user/role a little bit too much access...