Pt run - Shell selection #28121

gokcekantarci · 2023-08-24T10:04:01Z

Summary of the Pull Request

A combobox added to choose which shell command to run command with

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

stefansjfw · 2023-08-24T12:49:57Z

/azp run

azure-pipelines · 2023-08-24T12:50:06Z

Azure Pipelines successfully started running 1 pipeline(s).

htcfreek · 2023-08-24T20:07:49Z

src/settings-ui/Settings.UI/SettingsXAML/Views/PowerLauncherPage.xaml

+                                                                            Visibility="{x:Bind Path=ShowCheckBox, Converter={StaticResource BoolToVisibilityConverter}}"/>
+                                                                        <ComboBox
+                                                                            Margin="56,0,0,0"
+                                                                            Description="{x:Bind Path=DisplayDescription}"


Please make sure that it is possible to go without description. Current it is optional.

Its optional. Not used

htcfreek · 2023-08-24T20:19:40Z

src/modules/launcher/Plugins/Microsoft.Plugin.Shell/Main.cs

@@ -418,12 +431,17 @@ public List<ContextMenuResult> LoadContextMenus(Result selectedResult)
        public void UpdateSettings(PowerLauncherPluginSettings settings)
        {
            var leaveShellOpen = false;
+            var shellOption = 0;


What was the default before implementing this? I thought it was ShellExecute. We should keep the old default behavior as new default.

stefansjfw · 2023-08-24T20:00:19Z

src/modules/launcher/Plugins/Microsoft.Plugin.Shell/Main.cs

@@ -13,6 +13,7 @@
 using System.Reflection;
 using System.Windows.Input;
 using ManagedCommon;
+using Microsoft.Plugin.Folder.Sources;


This shouldn't be here. One plugin should not depend on another

stefansjfw · 2023-08-24T20:21:48Z

src/modules/launcher/Plugins/Microsoft.Plugin.Shell/ShellPluginSettings.cs

    }

    public enum ExecutionShell
    {
+        [Description("Run command in Command Prompt (cmd.exe)")]


These are user-facing strings and have to be localized

htcfreek · 2023-08-24T20:25:51Z

src/settings-ui/Settings.UI/ViewModels/PluginAdditionalOptionViewModel.cs

+
+        public bool ShowComboBox => _additionalOption.ComboBoxOptions != null && _additionalOption.ComboBoxOptions.Count > 0;
+
+        public bool ShowCheckBox => ShowComboBox == false;


Can we make this more flexible by supporting checkbox value (Value) being not initialized (null). Then we can show the checkboox if Value is true or false.

This allows us to have three different settings combinations: DropDown, Checkbox, Checkbox&DropDown

Value is bool and its non-nullable. So a new bool HideCheckBox added and used.

We can't use bool? (Nullable<bool>)?

After thinking about this some time I am wondering if we it makes sense. 🤔 We have only one name and description and I think it's more common to have different names/descriptions for checkbox and drop-down. Maybe we should revert this change. What do you think?

I agree. We are saving data in JSON file. One combobox or checkbox only responsible for one field in that JSON file. Changes are reverted.

Looking at this, I think it's not very flexible to adding new types in the future. Can we add a field that's a sort of enum for the type of option this is? Currently there's two option types we're using, a "BooleanCheckboxOption" type (which would be default, so we don't need to change current code and still support the third party plugins we currently do) and a "SelectionDropDownOption" type.
That way, in the future, someone can add a "SelectionDropDownWithCheckboxOption", if needed?
I imagine some TextBox fields might make sense in the future if people want to add in plugins that may take API keys, for example. (Not to add in this PR)
What do you think @htcfreek ?

There is the PR #25326 for adding a string type setting which I think we can close. I am waiting for this drop-down feature to merge before implementing the string setting.

In the other PR I mentioned the exact same idea with the enum. And it would make single type (string, checkbox, drop-down, ...) settings easier to implement. (Currently I have the idea to check for stringProperty != null to decide if we should show the input box or not. And this requires a default value. At least explicit empty string. With a enum this quirk will be obsolete.) But for mixed settings we still have some complexity to implement: Multiple names and descriptions, disable input box/drop-down if checkbox is not checked and so one.

So I don't know if it makes sense to add so complex setting definitions. I mean you still can use two different PluginAdditionalOptions for two settings. (E.g. option one for "Use ChatGPT Pro" and option two for "Enter api key".) What do you think about this point @jaimecbernardo ?

jaimecbernardo

It's working well. Thank you! I left comments about changes that need to happen before this can go in.

jaimecbernardo · 2023-09-05T09:07:16Z

src/modules/launcher/Plugins/Microsoft.Plugin.Shell/Main.cs

@@ -418,12 +432,17 @@ public List<ContextMenuResult> LoadContextMenus(Result selectedResult)
        public void UpdateSettings(PowerLauncherPluginSettings settings)
        {
            var leaveShellOpen = false;
+            var shellOption = 2;


Why is this the default? cmd.exe was the default behavior before, right?

Its default is 2 (Find executable file and run it)

You're right :)

jaimecbernardo · 2023-09-05T09:11:06Z

src/modules/launcher/Plugins/Microsoft.Plugin.Shell/Main.cs

+                    "Run command in Command Prompt (cmd.exe)",
+                    "Run command in PowerShell (PowerShell.exe)",
+                    "Find executable file and run it",
+                    "Run command in Windows Terminal (wt.exe)",


These don't look like they are currently localizable.

jaimecbernardo · 2023-09-05T09:20:39Z

src/settings-ui/Settings.UI/ViewModels/PluginAdditionalOptionViewModel.cs

+
+        public bool ShowComboBox => _additionalOption.ComboBoxOptions != null && _additionalOption.ComboBoxOptions.Count > 0;
+
+        public bool ShowCheckBox => ShowComboBox == false;


Looking at this, I think it's not very flexible to adding new types in the future. Can we add a field that's a sort of enum for the type of option this is? Currently there's two option types we're using, a "BooleanCheckboxOption" type (which would be default, so we don't need to change current code and still support the third party plugins we currently do) and a "SelectionDropDownOption" type.
That way, in the future, someone can add a "SelectionDropDownWithCheckboxOption", if needed?
I imagine some TextBox fields might make sense in the future if people want to add in plugins that may take API keys, for example. (Not to add in this PR)
What do you think @htcfreek ?

jaimecbernardo · 2023-09-05T09:26:51Z

This PR solves #4283

htcfreek · 2023-09-05T10:52:53Z

I like to have a review of @niels9001 for the settings ui part and if we need to adjust the ui look and feel.

@gokcekantarci
Cab you please upload some screenshots.

gokcekantarci · 2023-09-05T12:25:12Z

I like to have a review of @niels9001 for the settings ui part and if we need to adjust the ui look and feel.

@gokcekantarci Cab you please upload some screenshots.

…on types.

jaimecbernardo

LGTM! It's likely that the Settings UI part may need a refactor as we increase the number of types, but this looks good enough for now.
Thank you! Great work!

gokcekantarci added 9 commits July 27, 2023 13:17

[PTRun] LeaveShellOpen condition added to run command

d0f1268

Merge branch 'main' into PTRun]-Keep-Shell-open

9bec422

[PTRun] Keep shell open added to shell plugin settings.

50dca83

[PTRun] Unnecessary variable deleted. Formatting.

5211135

[PTRun] Variable name changed.

1155b31

[PTRun] Shell selection

09cc544

[PTRun] Bugfix

f263c85

[PTRun] Shell execution selection UI changes.

bf8cdba

[PTRun] Conflict resolved.

11e9ea6

gokcekantarci requested a review from stefansjfw August 24, 2023 10:04

htcfreek reviewed Aug 24, 2023

View reviewed changes

stefansjfw requested changes Aug 24, 2023

View reviewed changes

htcfreek reviewed Aug 24, 2023

View reviewed changes

gokcekantarci added 2 commits August 25, 2023 18:01

[PTRun] Review comments.

83c7911

[PTRun] Revert commit

8aae265

jaimecbernardo requested changes Sep 5, 2023

View reviewed changes

[PTRun] An enumaration is added to PluginAdditionalOption for selecti…

32ec073

…on types.

jaimecbernardo approved these changes Sep 11, 2023

View reviewed changes

stefansjfw approved these changes Sep 11, 2023

View reviewed changes

jaimecbernardo merged commit e856e05 into microsoft:main Sep 11, 2023
7 checks passed

michaelmdresser mentioned this pull request Oct 14, 2023

Support custom terminals for PowerToys Run Shell plugin #29222

Open

gokcekantarci deleted the PTRun---Shell-Selection branch January 30, 2024 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pt run - Shell selection #28121

Pt run - Shell selection #28121

gokcekantarci commented Aug 24, 2023 •

edited by jaimecbernardo

Loading

stefansjfw commented Aug 24, 2023

azure-pipelines bot commented Aug 24, 2023

htcfreek Aug 24, 2023

gokcekantarci Aug 25, 2023

htcfreek Aug 24, 2023

gokcekantarci Aug 25, 2023

stefansjfw Aug 24, 2023

gokcekantarci Aug 25, 2023

stefansjfw Aug 24, 2023

htcfreek Aug 24, 2023

gokcekantarci Aug 25, 2023

htcfreek Aug 24, 2023

gokcekantarci Aug 25, 2023

htcfreek Aug 25, 2023

htcfreek Aug 26, 2023

gokcekantarci Sep 4, 2023

jaimecbernardo Sep 5, 2023

htcfreek Sep 5, 2023 •

edited

Loading

jaimecbernardo left a comment

jaimecbernardo Sep 5, 2023

gokcekantarci Sep 5, 2023

jaimecbernardo Sep 11, 2023

jaimecbernardo Sep 5, 2023

jaimecbernardo Sep 5, 2023

jaimecbernardo commented Sep 5, 2023

htcfreek commented Sep 5, 2023 •

edited

Loading

gokcekantarci commented Sep 5, 2023

jaimecbernardo left a comment


		public bool ShowComboBox => _additionalOption.ComboBoxOptions != null && _additionalOption.ComboBoxOptions.Count > 0;

		public bool ShowCheckBox => ShowComboBox == false;

Pt run - Shell selection #28121

Pt run - Shell selection #28121

Conversation

gokcekantarci commented Aug 24, 2023 • edited by jaimecbernardo Loading

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

stefansjfw commented Aug 24, 2023

azure-pipelines bot commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htcfreek Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

jaimecbernardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaimecbernardo commented Sep 5, 2023

htcfreek commented Sep 5, 2023 • edited Loading

gokcekantarci commented Sep 5, 2023

jaimecbernardo left a comment

Choose a reason for hiding this comment

gokcekantarci commented Aug 24, 2023 •

edited by jaimecbernardo

Loading

htcfreek Sep 5, 2023 •

edited

Loading

htcfreek commented Sep 5, 2023 •

edited

Loading