Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add buttons for moving the terminal around. #2704

Merged
merged 3 commits into from
May 19, 2020
Merged

Add buttons for moving the terminal around. #2704

merged 3 commits into from
May 19, 2020

Conversation

MartinGC94
Copy link
Contributor

PR Summary

This adds 3 new buttons to the titlebar for moving the terminal around, like the ISE had.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
  • Summarized changes
  • [NA] PR has tests
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

@msftclas
Copy link

msftclas commented May 14, 2020

CLA assistant check
All CLA requirements met.

@rjmholt
Copy link
Contributor

rjmholt commented May 15, 2020

These should probably only be enabled in ISE mode to prevent complicating the current layout for users using the default, non-ISE-like functionality

@TylerLeonhardt
Copy link
Member

Can you add a screenshot for what this looks like?

@MartinGC94
Copy link
Contributor Author

Dark:
Dark
Light:
Light

Personally I don't think the Powershell extension has so many buttons that it feels too complicated/bloated but I do like the idea of giving users the option for hiding these buttons as well as the run buttons previously added.

@rkeithhill
Copy link
Contributor

That looks very nice! But I agree with @rjmholt, I wouldn't add these except when in ISE mode. And/or create a setting to control the visibility of these buttons. These three buttons aren't a lot but the problem is that other extensions also add buttons. My current button set up looks like this:

image

Having more buttons would cut further into space I have for editor tabs.

@MartinGC94
Copy link
Contributor Author

Good point about other extensions also being able to add their own buttons. I've added options for hiding all of the buttons and made the new buttons hidden by default.

@rjmholt
Copy link
Contributor

rjmholt commented May 15, 2020

@MartinGC94 one last thing you might like to add is logic to activate all the buttons when ISE mode is set. That happens here:

public static settings: ISetting[] = [
{ path: "workbench.activityBar", name: "visible", value: false },
{ path: "debug", name: "openDebug", value: "neverOpen" },
{ path: "editor", name: "tabCompletion", value: "on" },
{ path: "powershell.integratedConsole", name: "focusConsoleOnExecute", value: false },
{ path: "files", name: "defaultLanguage", value: "powershell" },
{ path: "workbench", name: "colorTheme", value: "PowerShell ISE" },
];

@MartinGC94
Copy link
Contributor Author

@rjmholt I did that for the new buttons I just forgot to mention it:
https://github.com/MartinGC94/vscode-powershell/blob/346f1b3a7770ff6e85d56ad4277fde4f6f8ac518/src/features/ISECompatibility.ts#L27-L29

Should I add the old buttons as well? I thought about doing that but didn't see any reason to do so as long as they are enabled by default.

@rjmholt
Copy link
Contributor

rjmholt commented May 15, 2020

I did that for the new buttons I just forgot to mention it:

Oh! Sorry I didn't see it when I looked at the diff before.

Should I add the old buttons as well?

No no, all good! Just meant the new buttons 🙂

Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing this @MartinGC94!

package.json Outdated
Comment on lines 846 to 869
"powershell.buttons.RunButtonVisibility": {
"type": "boolean",
"default": true,
"description": "Show the Run button in the editor titlebar."
},
"powershell.buttons.RunSelectionButtonVisibility": {
"type": "boolean",
"default": true,
"description": "Show the Run Selection button in the editor titlebar."
},
"powershell.buttons.ClosePanelButtonVisibility": {
"type": "boolean",
"default": false,
"description": "Show the Close panel button in the editor titlebar."
},
"powershell.buttons.MovePanelLeftButtonVisibility": {
"type": "boolean",
"default": false,
"description": "Show the Move panel left button in the editor titlebar."
},
"powershell.buttons.MovePanelToBottomButtonVisibility": {
"type": "boolean",
"default": false,
"description": "Show the Move panel to bottom button in the editor titlebar."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really awesome and gives the user a ton of flexibility but I don't think some is going to want just 1 button or just 2 buttons from what appears to be two groups here:

powershell.buttons.showRunButtons
powershell.buttons.showPanelMovementButtons

Can you change it to these 2 settings and use these everyewhere? I discussed this with @rjmholt and @SydneyhSmith

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, it still fits my needs so I don't mind limiting the options a bit. I'm just curious why limit it if the work has already been done? I can imagine there's people out there that never use the vertical split view or always use the run selection option that might not care for those buttons.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very wary to add configuration because often times people get lost in it. There have been many instances where folks just don't realize that settings already exist or find themselves digging trying to figure out the right configuration for them.

I do want there to be some way to toggle showing these buttons and that's kinda why we want 2 settings instead of 5.

I know the difference is only 3 but we try to have this mindset with all new settings.

I can imagine there's people out there that never use the vertical split view or always use the run selection option that might not care for those buttons.

Maybe. But those same people might be just as happy seeing all the buttons... in the off chance that they use the other.

It's in situations like this where I go "let's expose the least amount of stuff that makes sense (in other words 2 settings) and wait for a user to come around to ask for more."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand and I've fixed it now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MartinGC94!

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants