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.

Sign up for GitHub

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

Pt run - Shell selection #28121

Merged
merged 12 commits into from
Sep 11, 2023
Merged

Pt run - Shell selection #28121

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d0f1268
[PTRun] LeaveShellOpen condition added to run command
gokcekantarci Jul 27, 2023
9bec422
Merge branch 'main' into PTRun]-Keep-Shell-open
gokcekantarci Aug 17, 2023
50dca83
[PTRun] Keep shell open added to shell plugin settings.
gokcekantarci Aug 18, 2023
5211135
[PTRun] Unnecessary variable deleted. Formatting.
gokcekantarci Aug 21, 2023
1155b31
[PTRun] Variable name changed.
gokcekantarci Aug 22, 2023
09cc544
[PTRun] Shell selection
gokcekantarci Aug 22, 2023
f263c85
[PTRun] Bugfix
gokcekantarci Aug 22, 2023
bf8cdba
[PTRun] Shell execution selection UI changes.
gokcekantarci Aug 22, 2023
11e9ea6
[PTRun] Conflict resolved.
gokcekantarci Aug 24, 2023
83c7911
[PTRun] Review comments.
gokcekantarci Aug 25, 2023
8aae265
[PTRun] Revert commit
gokcekantarci Sep 4, 2023
32ec073
[PTRun] An enumaration is added to PluginAdditionalOption for selecti…
gokcekantarci Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/modules/launcher/Plugins/Microsoft.Plugin.Shell/Main.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using System.Reflection;
using System.Windows.Input;
using ManagedCommon;
using Microsoft.Plugin.Folder.Sources;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

using Microsoft.Plugin.Shell.Properties;
using Microsoft.PowerToys.Settings.UI.Library;
using Wox.Infrastructure.Storage;
Expand Down Expand Up @@ -47,6 +48,18 @@ public class Main : IPlugin, IPluginI18n, ISettingProvider, IContextMenu, ISavab
DisplayLabel = Resources.wox_leave_shell_open,
Value = _settings.LeaveShellOpen,
},

new PluginAdditionalOption()
{
Key = "ShellCommandExecution",
DisplayLabel = Resources.wox_shell_command_execution,
Option = (int)_settings.Shell,

ComboBoxOptions = Enum.GetValues(typeof(ExecutionShell))
.Cast<ExecutionShell>()
.Select(shellEnumValue => ShellPluginSettings.GetDescription(shellEnumValue))
.ToList(),
},
};

private PluginInitContext _context;
Expand Down Expand Up @@ -418,12 +431,17 @@ public List<ContextMenuResult> LoadContextMenus(Result selectedResult)
public void UpdateSettings(PowerLauncherPluginSettings settings)
{
var leaveShellOpen = false;
var shellOption = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


if (settings != null && settings.AdditionalOptions != null)
{
var optionLeaveShellOpen = settings.AdditionalOptions.FirstOrDefault(x => x.Key == "LeaveShellOpen");
leaveShellOpen = optionLeaveShellOpen?.Value ?? leaveShellOpen;
_settings.LeaveShellOpen = leaveShellOpen;

var optionShell = settings.AdditionalOptions.FirstOrDefault(x => x.Key == "ShellCommandExecution");
shellOption = optionShell?.Option ?? shellOption;
_settings.Shell = (ExecutionShell)shellOption;
}

Save();
Expand Down
11 changes: 11 additions & 0 deletions src/modules/launcher/Plugins/Microsoft.Plugin.Shell/Properties/Resources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/modules/launcher/Plugins/Microsoft.Plugin.Shell/Properties/Resources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,4 +144,7 @@
<data name="wox_leave_shell_open" xml:space="preserve">
<value>Keep shell open</value>
</data>
<data name="wox_shell_command_execution" xml:space="preserve">
<value>Shell command execution</value>
</data>
</root>
24 changes: 24 additions & 0 deletions src/modules/launcher/Plugins/Microsoft.Plugin.Shell/ShellPluginSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Reflection;

namespace Microsoft.Plugin.Shell
{
Expand Down Expand Up @@ -31,13 +34,34 @@ public void AddCmdHistory(string cmdName)
Count.Add(cmdName, 1);
}
}

public static string GetDescription(Enum value)
{
FieldInfo fi = value.GetType().GetField(value.ToString());

DescriptionAttribute[] attributes =
(DescriptionAttribute[])fi.GetCustomAttributes(typeof(DescriptionAttribute), false);

if (attributes != null && attributes.Length > 0)
{
return attributes[0].Description;
}
else
{
return value.ToString();
}
}
}

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

Choose a reason for hiding this comment

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

These are user-facing strings and have to be localized

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Cmd = 0,
[Description("Run command in PowerShell (PowerShell.exe)")]
Powershell = 1,
[Description("Find executable file and run it")]
RunCommand = 2,
[Description("Run command in Windows Terminal (wt.exe)")]
WindowsTerminal = 3,
}
}
6 changes: 6 additions & 0 deletions src/settings-ui/Settings.UI.Library/PluginAdditionalOption.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;

namespace Microsoft.PowerToys.Settings.UI.Library
{
public class PluginAdditionalOption
Expand All @@ -16,5 +18,9 @@ public class PluginAdditionalOption
public string DisplayDescription { get; set; }

public bool Value { get; set; }

public List<string> ComboBoxOptions { get; set; }

public int Option { get; set; }
}
}
20 changes: 15 additions & 5 deletions src/settings-ui/Settings.UI/SettingsXAML/Views/PowerLauncherPage.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,11 +444,21 @@
BorderThickness="0,0,0,0"
ContentAlignment="Left"
CornerRadius="0">
<controls:CheckBoxWithDescriptionControl
Margin="56,0,0,0"
Description="{x:Bind Path=DisplayDescription}"
Header="{x:Bind Path=DisplayLabel}"
IsChecked="{x:Bind Path=Value, Mode=TwoWay}" />
<StackPanel Orientation="Vertical">
<controls:CheckBoxWithDescriptionControl
Margin="56,0,0,0"
Description="{x:Bind Path=DisplayDescription}"
Header="{x:Bind Path=DisplayLabel}"
IsChecked="{x:Bind Path=Value, Mode=TwoWay}"
Visibility="{x:Bind Path=ShowCheckBox, Converter={StaticResource BoolToVisibilityConverter}}"/>
<ComboBox
Margin="56,0,0,0"
Description="{x:Bind Path=DisplayDescription}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its optional. Not used

Header="{x:Bind Path=DisplayLabel}"
ItemsSource="{x:Bind Path=ComboBoxOptions}"
SelectedIndex="{x:Bind Path=Option, Mode=TwoWay}"
Visibility="{x:Bind Path=ShowComboBox, Converter={StaticResource BoolToVisibilityConverter}}" />
</StackPanel>
</labs:SettingsCard>
<Rectangle
Height="1"
Expand Down
25 changes: 23 additions & 2 deletions src/settings-ui/Settings.UI/ViewModels/PluginAdditionalOptionViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.ComponentModel;
using System.Runtime.CompilerServices;
using System.Windows.Input;
using Microsoft.PowerToys.Settings.UI.Library;

namespace Microsoft.PowerToys.Settings.UI.ViewModels
Expand All @@ -17,9 +19,9 @@ internal PluginAdditionalOptionViewModel(PluginAdditionalOption additionalOption
_additionalOption = additionalOption;
}

public string DisplayLabel { get => _additionalOption.DisplayLabel; }
public string DisplayLabel => _additionalOption.DisplayLabel;

public string DisplayDescription { get => _additionalOption.DisplayDescription; }
public string DisplayDescription => _additionalOption.DisplayDescription;

public bool Value
{
Expand All @@ -34,6 +36,25 @@ public bool Value
}
}

public List<string> ComboBoxOptions => _additionalOption.ComboBoxOptions;

public int Option
{
get => _additionalOption.Option;
set
{
if (value != _additionalOption.Option)
{
_additionalOption.Option = value;
NotifyPropertyChanged();
}
}
}

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

public bool ShowCheckBox => ShowComboBox == false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

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 agree. We are saving data in JSON file. One combobox or checkbox only responsible for one field in that JSON file. Changes are reverted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 ?

Copy link
Collaborator

@htcfreek htcfreek Sep 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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 ?


public event PropertyChangedEventHandler PropertyChanged;

private void NotifyPropertyChanged([CallerMemberName] string propertyName = "")
Expand Down