All CLA requirements met.

I know @sbatten recently was looking into making the panel position more configurable with the new layout. Due to that assigning first to him. Feel free to assign back to me and I can review.

@isidorn @AlexFell-Velo I have no issues with taking this. The PR appears well done and it does not conflict in any major way with the current thinking going forward. I am testing the branch for any caveats that could have been missed and reviewing the code.

I am assigning you as well @isidorn indicating that you can review whenever.

The PR looks good overall. However if we plan to introduce a more flexible workbench layout in the future then I am not sure how much this makes sense as a temporary solution.
If the flexible workbench layou is not in our 6 month plan then I would merge this in. Otherwise I would wait for the right solution.

Since the implementation looks good, I am approving this. However please note that I did not intesively test this and if we merge this in it would be nice if we have a test plan item to cover it.
Thanks!

Feedback from testing:

• the menu seems to show all actions now:
  
• panel.border does not work when panel is left, it should work to get a border between panel and editor
  
• changing the location and reloading the window does not persist the location, it goes back to bottom

I see all of the same issues @bpasero has mentioned. I tested in High Contrast theme as it makes it easy to catch theme issues. Additionally, the chevron is incorrect when panel is on the left.

Make sure to update the setting
workbench.panel.defaultLocation so that it can also be left

Thank you for the review and testing on this PR, much appreciated.

I have managed to address:

• Border appearance
• Chevron orientation
• Contextual menu only showing correct items
• Update of default location setting

However I cannot seem to resolve the default panel location when reloading the window, I understand that this works in the current implementation, any pointers you have would be greatly appreciated.

TIA

For the default postiion on reload I would put a breakpoint somewhere around here and try to figure out what is going on. Hope that helps

Thanks @isidorn for the pointer, turns out it was actually and array ordering thing on startup here

I needed [panel, editor] and not the default [editor, panel], so this is now resolved, I'll await the outcome of any further testing.

Thanks

Thanks. I'll leave it up to @sbatten to merge it in.
I recommend a test plan item, which I can also write. Let me know if you would like me to do that @sbatten

@sbatten any movement on this one?
It wasn't clear to me what sort of gates you had around test coverage for PR's etc. Because of the way that panelPart and panelActions were linked, I've added two new commits to further separate these entities so that they can become testable via unit tests. All the setup work of registering singletons and actions is now achieved in registerPanel.ts

Hopefully that helps this along a little bit.

@AlexFell-Velo sorry about not picking this up, that's my fault. Due to the current time for us in the milestone, I'm going to move this to be merged early in December. This will give it plenty of testing in insiders.

@AlexFell-Velo while I understand where you were coming from, I don't particularly prefer the new registerPanel.ts file as none of the other parts do this pattern so I'd prefer consistency.

@AlexFell-Velo while I understand where you were coming from, I don't particularly prefer the new registerPanel.ts file as none of the other parts do this pattern so I'd prefer consistency.

Happy to back out this change, however I'd also have to back out the unit tests, as the current architectural style would require mocking out the DI and registration mechanics before the require/import is made, which seems like a lot of effort for some small tests.

You could argue that the current unit tests that I have written don't really provide any concrete value, so if you are happy for me to back out this change, then I'm happy to do this @sbatten .

@AlexFell-Velo the unit tests are not a condition for this to be merged IMO as we didn't have these tests before and this is a welcome tweak to an existing function. If you are still interested in keeping the tests, you can look at some other tests as examples to get things set up.

@AlexFell-Velo the unit tests are not a condition for this to be merged IMO as we didn't have these tests before and this is a welcome tweak to an existing function. If you are still interested in keeping the tests, you can look at some other tests as examples to get things set up.

@sbatten I found it easier to remove them. Rebased onto master as before

Nice PR - but is there any way to mimic the previous behavior? I loved my single keyboard shortcut to toggle moving bottom/right.

@jasonvarga absolutely

    {
        "key": "<your keybind here>",
        "command": "workbench.action.positionPanelRight",
        "when": "panelPosition == 'bottom'"
    },
    {
        "key": "<your keybind here>",
        "command": "workbench.action.positionPanelBottom",
        "when": "panelPosition == 'right'"
    }

Awesome 🎉 Thanks @sbatten