Fix bug which caused duplicate applet orders when dropping them in the same position

[samw-26]

No description provided.

[fredcw]

I can't figure out what this does. What bug does it fix?

[samw-26]

@fredcw Occasionally, if you drag and drop an applet that is next to another applet in the same position, it will cause two applets to have an order of 0. In cinnamon looking glass you can tell if it occurred when you see that the applet was reloaded when dropping it.

[fredcw]

Yes, I see the problem. This solution doesn't work though because you can end up with an applet having a panel position of -1. I think instead that the function should quit early if this._dragPlaceholder is null and a panel child actor that is that of the source applet should be ignored (not counted as a position).

I would rewrite it like this: 4021e1d

[samw-26]

You're right, that is better. Thanks!

[samw-26]

Ended up reverting to the previous way of doing this. The early return can prevent the user from dropping an applet in an empty location. The added check child._delegate !== source.actor._applet prevented the user from dropping in the same place which was a bad ux.

My way works fine, and an applet cannot end up with a panel position of -1. This check prevents it
if (!isSameLocation) source.actor._applet._newOrder = insertAppletPos === -1 ? 0 : insertAppletPos;

[fredcw]

an applet cannot end up with a panel position of -1

You're right sorry, I miss-edited the code I tested with. You can however have positions 1 and 2 instead of 0 and 1 in a zone with 2 applets:

'panel3:center:1:Cinnamenu@json:153', 'panel3:center:2:CinnVIIStarkMenu@NikoKrause:155'

The early return can prevent the user from dropping an applet in an empty location.

This is due to an error in handleDragOver:

        if (pos !== this._dragPlaceholderPos) {
            this._dragPlaceholderPos = pos;
            // Don't allow positioning before or after self

            if (this._origAppletPos != -1 && (pos == this._origAppletPos || pos == this._origAppletPos + 1)) {
                this._clearDragPlaceholder();
                return DND.DragMotionResult.CONTINUE;
            }

here this._dragPlaceholderPos is set to pos but if this._dragPlaceholder is null then the this._clearDragPlaceholder(); line doesn't reset this._dragPlaceholderPos back to -1. The this._dragPlaceholderPos = pos; line should come after the possible function return (return DND.DragMotionResult.CONTINUE;) like this:

         if (pos !== this._dragPlaceholderPos) {
            // Don't allow positioning before or after self

            if (this._origAppletPos != -1 && (pos == this._origAppletPos || pos == this._origAppletPos + 1)) {
                this._clearDragPlaceholder();
                return DND.DragMotionResult.CONTINUE;
            }

            // If the placeholder already exists, we just move
            // it, but if we are adding it, expand its size in
            // an animation
            
            this._dragPlaceholderPos = pos;

If this._dragPlaceholderPos is set to a value other than -1 then the condition pos !== this._dragPlaceholderPos in the line if (pos !== this._dragPlaceholderPos) { can be false even though this._dragPlaceholder is null.

The added check child._delegate !== source.actor._applet prevented the user from dropping in the same place which was a bad ux.

I'm not sure what you mean, the icon just floats back to it's original position indicating an invalid drop or no change.

The problem with the way you've done it is that it's complicated enough to be hard to follow the logic and figure out what's going on even if it does work. Code needs to be readable and easy to follow.