Drag and Drop of shortcuts between Enable and Disable Does not respect the Shortcuts Limit

I didn't get a chance to properly review this, but I tested it and see the issue. The fix looks appropriate, and reducing the use of an anonymous closure is anyway a good thing IMO.

I do think the coding standards are to use camelCase though for JS functions instead of an underscore "._row_status_change". I'm not sure about that though.

Also, the whitespace thing below.

+++ b/core/modules/shortcut/shortcut.admin.jsundefined
@@ -59,13 +64,17 @@ Drupal.behaviors.shortcutDrag = {
+      };
+      ¶
+      tableDrag._row_status_change = function (rowObject) {
       };
 

Whitespace above tableDrag._row_status_change.

Looks like a nice fix.

1. I agree with @JacobSingh on the function name; something like rowStatusChange() is better. I'm also not sure the function should be part of the tableDrag object itself? (It's specific to the shortcut module so it would be better kept separate if possible; looking at how the Block module deals with a similar situation, it does not add its helper function to the tableDrag object either.)

2. +            swappedRowObject = new tableDrag.row(swappedRow, 'mouse', self.indentEnabled, self.maxDepth, true);
   +            swappedRowObject.markChanged();
   

   Not being familiar with how tabledrag works, I have no idea what this code does, so it seems like it could use a code comment...

   Also, are we sure it's necessary to even get the full row object here? (The new helper function added by this patch looks like it only uses rowObject.element, so in that case maybe the full object is never needed?)

3. We probably also need a validation handler for this form in the server-side code (to prevent the shortcut limit from being bypassed by someone who is not using drag-and-drop). However, that's a sufficiently minor problem that we can probably get away with making a followup issue for it rather than fixing it here.

David said "Also, are we sure it's necessary to even get the full row object here? (The new helper function added by this patch looks like it only uses rowObject.element, so in that case maybe the full object is never needed?)"

I'm also not sure, but I don't think it matters much. THe code is clean and the performance difference would be negligible I imagine.

I think this is ready to go.

This patch needs a little minor code style cleanup before it goes in... there's trailing whitespace and a comment line over 80 chars. Also, comments need to end with a period (and "Swapped" should not be capitalized midsentence). Thanks, and sorry; I know these are very trivial things. :)

Also, would it be possible to test? We can't test the actual drag-and-drop, obviously, but could we emulate it?

You can test it. I tested it w/ and w/o the patch.

Oh you mean automated test? Not without selenium :)

So there's absolutely no server-side component to this and the extra shortcuts disappear on a page reload? Edit: Are you sure? What happens when "Save" is clicked? Sounds like something is not being enforced properly server-side.

I think we should probably fix #1358140: Drag and Drop of shortcuts between Enable and Disable should have server side validation for shortcut links limit in this issue as well.

Regarding the object issue, I wasn't thinking of it in terms of performance, just in terms of simplifying code. Basically, I was wondering if these two lines could be removed entirely:

+            swappedRowObject = new tableDrag.row(swappedRow, 'mouse', self.indentEnabled, self.maxDepth, true);
+            swappedRowObject.markChanged();

Playing around with it a little now, I guess I can see why you added them; normally, Drupal only marks the row that was actually dragged with the asterisk (indicating that it was changed), but here you want to do it to the other row too because we're forcing one of its properties (enabled vs disabled) to change as well. I can buy that, I suppose.

If so, can we turn that explanation into a code comment?... In general, I think the code comments in this patch could be improved in the following way: Right now they're talking about what the code is doing, but it's better to use code comments to explain why. For example, here:

+          // Check if the Swapped row is draggable.
+          if ($(swappedRow).hasClass('draggable')) {

I think the code comment can be left out, since what it says is more-or-less obvious from the code.

Whereas here:

+            // Mark the swapped row as changed.
+            // swappedRow variable is not tableDrag.row object,
+            // so created tableDrag.row object & called markChanged function.
+            swappedRowObject = new tableDrag.row(swappedRow, 'mouse', self.indentEnabled, self.maxDepth, true);
+            swappedRowObject.markChanged();

This is a good start, but it's still just telling me what the code does. As per above, this would be the place to explain why it is that we want to go out of our way to mark this particular row as changed (when the tableDrag system doesn't normally do that). Does that make sense?

Overall this patch is looking very good. Like I said in #3, I'm not sure whether we should merge #1358140: Drag and Drop of shortcuts between Enable and Disable should have server side validation for shortcut links limit with this or not.... Normally I'd say yes, but in this case I don't care as much since we want to remove the limit anyway, so if people can sneak their way around it, it really doesn't bother me that much :) The reason the JavaScript version of the bug (i.e. this issue) is worse is that you see one thing on the screen when you drag and drop, but then something totally different happens after you save.

+1 Nice work Prajwala!

Here's an attached version which just fixes the grammar a tiny bit.

+            // The dropped element will automatically br marked as changed.
+            // Here the swappedRow is moved to the Disabled section but
+            // not marked as changed. Since it is not marked as changed,
+            // what we see on the screen will not save the same as it is at server side.
+
+            // When a row is disabled, marking it as changed it will be
+            // disabled at the server side

This looks much better, but I think it needs one more rewrite. There's a typo ("br") and words like Disabled shouldn't be capitalized. Plus, the last couple of sentences duplicate each other a bit and aren't totally clear. Maybe something along the lines of this (feel free to tweak the wording):

The dropped element will automatically be marked as changed by the tableDrag system. However, the row that swapped with it has moved to the "disabled" section, so we need to force its status to be disabled and mark it as changed also.

Thanks!

I was going to mark it RTBC, but.... trying out the patch, it doesn't seem to work always? I found that if I drag something from "disabled" to a place near the bottom of the enabled list, it works as intended. But if I drag from "disabled" all the way to the top (or close to the top), the swap doesn't happen correctly and you still wind up with 8 enabled shortcuts after you save (even though you only see 7 on the screen before you save).

Can anyone else reproduce this?

+  swappedRowObject = new tableDrag.row(swappedRow, 'mouse', self.indentEnabled, self.maxDepth, true);

Looking at the tabledrag code, it also occurs to me that 'mouse' probably isn't right here. How do we know they are using a mouse as opposed to the keyboard?

The tabledrag.js code itself always seems to check before calling this.

If that bug proves too hard to fix, we could probably just get this patch committed without it (and leave the remaining work for a followup)..... as is, the patch still represents a big improvement over the current behavior.

However, I think we do need to clarify the 'mouse' vs 'keyboard' issue I raised in #18. It's important to make sure this patch doesn't cause any harm for people who are doing drag-and-drop via the tab+arrow keys rather than a mouse.

Good point.

I had some issues when I tried this with a keyboard (could not drag something up from the disabled section to enabled) which I thought were due to this patch, but you're right, they're not, and there's no reason they could be. The same thing happens without the patch, so it's a preexisting bug.

Nice work!

Please open a follow-up for the remaining JavaScript bug so that doesn't get lost. I'm OK with not fixing everything in one patch but there's already two follow-ups spun off this one.

#16: edit_shortcuts_drag_drop_issue-1344752-16.patch queued for re-testing.

Created #1386514: Dragging and dropping a shortcut from disabled to enabled doesn't switch the status if the dragging happens fast. I think there's nothing that can be done there until this one is committed though.

Thanks! Committed/pushed to 8.x.

This wasn't tagged for backport, but seems like it should be?

Let's try again...

Patch looks good, and it works fine in Drupal 7 also.

Cool. This can't be unit tested, so good to go.

Committed and pushed to 7.x. Thanks!