Component reordering not working properly in GoldenLayout 2

[andreasmueller92]

Current behavior

• Build GoldenLayout v2.2.1 and run the apitest as described here.
• Go to http://localhost:3000/ in your browser
• Select miniRow and hit Load Layout
• Realize that the Golden component cannot be reordered, while the Layout one can be dragged to a new position.

Expected behavior:
It should be possible to reorder all components, independent of their closability.

Code example:
After a lot of trial and error, it comes down to component closability and header configuration, as described below.
Sorry in advance for not providing a standalone CodePen example, but I could not find a bundled GoldenLayout 2 JS source file as in the v1.5.9 CodePen example.

The following minimal example allows all components to be moved around (while being closable):

// UI layout configuration
const config = {
    header: {
        close: 'close', // (1)
        maximise: false,
        popout: false,
    },
    root: {
        type: 'row',
        content: [{
            type: 'component',
            title: 'Golden',
            isClosable: true, // (2)
            componentType: 'test',
        },
        {
            type: 'component',
            title: 'Layout',
            isClosable: true, // (3)
            componentType: 'test',
        }],
    },
};

const myLayout = new GoldenLayout(document.getElementById('layoutDiv'));

myLayout.registerComponentFactoryFunction('test', () => {});

myLayout.loadLayout(config);

• Setting (1) to false will break reordering behavior for all components. In addition, the close buttons on the tabs won't work anymore, is any of these consequences intended?
• Setting (2) or (3) to false will break reordering behavior for that specific component. This is what happens in the apitest example.

And, even more confusing, setting (2) to false and following the next steps will result in both tabs being unable to be reordered:

• Drag Layout onto the same stack as Golden
• Now drag Golden (which suddenly becomes dragable) away from the stack so that is above/below/right of/left of Layout
• Both tabs can no longer be reordered

[pbklink]

Discussed in #647 (near end)

[andreasmueller92]

Hi Paul,
thanks for your quick response!
I went through the ticket and this is what I got from what you said:

1. Closability and dragability/popout should be independent from each other
2. "checks for popouts and drag initiation should remain the same"

I reported two things in this issue:

1. Dragability (wrongly) connected to closability
2. When close is set to false in the header, but components are marked as closable, their individual close buttons don't work.

Your first point is identical to my first point, but I'm not sure if I get your second point right. Does this mean "no additional configurability" or "we keep things as they are"? The second would mean no fix for this and different behavior compared to GoldenLayout 1.5.9

In case any of my points is officially recognized as a bug, please keep this ticket open, as the other ticket was originally on a completely different topic and is already closed.

[martin31821]

@andreasmueller92
The first two things you said are correct, although at the moment we have the closability and dragability/popout checks connected because it is easier to enforce consistency in this case.

The second one that close is set to false globally and to true in specific components indeed sounds like a bug to me, so I'll flag that as a bug for now.

As an additional explanation:
When popping out a component, we need to completely teardown the component and recreate it in a different window, because otherwise event handlers are still bound to the old (parent) window and there might be unforeseen errors because different windows may not share a single Javascript execution context.
Taking this into account, we decided to only allow popping out a component if it's also closeable, because the component needs to handle the close/reopen process and save any state it might has to either componentState or another persistent storage at its option. If you have any better idea how to solve this or improve the situation, feel free to share, maybe I've overlooked something important.

[andreasmueller92]

@martin31821:
Your explanation why you connected popout to closability makes sense.

If those reasons do not apply for drag as well, wouldn't it help to take dragability out of this constraint?

Meaning that one can drag any component (independent of closability), but popouts only work for closable components?

[martin31821]

It is true that having that connected to dragability as well is a bit over-restricted, and I think for us it would be good to liften that restriction.

@pbklink what do you think about that?

[pbklink]

Agree with all the above. I think the question is what to do about it.

As mentioned in #647, the code for this inside Golden Layout is already quite messy. I do not want to add another 'fix' making it even more confusing. What it needs is some kind of clean up. Probably with new configuration properties which make more sense.

I think the next step is to write up how it should work and get feedback. I have some ideas. Will post my thoughts after Virtual Components (#679) is released and bedded down and spare time permitting.

[NotWearingPants]

What about the following solution? AFAICT it fixes the problem.
The close button of a stack even knows to hide itself when a non-closable component is dragged into it (thanks to Header.prototype.updateClosability).
Either I'm missing something or a year has passed and the code was improved to the point where this is now an easy fix.

diff --git a/src/ts/controls/header.ts b/src/ts/controls/header.ts
index 7f279dd..9aa5f45 100644
--- a/src/ts/controls/header.ts
+++ b/src/ts/controls/header.ts
@@ -358,15 +358,11 @@ export class Header extends EventEmitter {

     /** @internal */
     private handleTabInitiatedDragStartEvent(x: number, y: number, dragListener: DragListener, componentItem: ComponentItem) {
-        if (!this._canRemoveComponent) {
-            dragListener.cancelDrag();
-        } else {
             if (this._componentDragStartEvent === undefined) {
                 throw new UnexpectedUndefinedError('HHTDSE22294');
             } else {
                 this._componentDragStartEvent(x, y, dragListener, componentItem);
             }
-        }
     }

[matteematt]

@NotWearingPants I have come across this issue today and found the same reponsbile code as you causing the issue. Did you try using Golden Layout with that code removed and not notice any regressions? If so, potentially it is worth raising a PR

[matteematt]

FYI I have fixed the issue in our fork here genesiscommunitysuccess#2