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

Feature: Open in Terminal RichCommand #11445

Merged

Conversation

ferrariofilippo
Copy link
Contributor

Notes

  • Work in progress
  • @cinqmilleans I need your help: how can I pass a parameter to Execute without messing up all your code?

Validation
How did you test these changes?

  • Built and ran the app
  • Tested the changes for accessibility

@ferrariofilippo ferrariofilippo marked this pull request as draft February 23, 2023 22:16
@marcofranzen99
Copy link
Contributor

My idea would be to create an own interface IAction<T> with the method ExecuteAsync(T parameter)
With that it would be possible to pass any kind of parameter to the Execute method

@cinqmilleans
Copy link
Contributor

Hello @ferrariofilippo . This is a feature that will be coming soon.

The solution proposed by @mafra99, which at first glance seems the right one, poses too many problems and greatly reduces the interest of richcommands because the parameter would only be given when the command is executed.

  • The action can no longer manage its descriptions (Label, ...) and it's still the view that will have to do it.
  • The action can no longer manage its state (IsExecutable), which differs depending on the parameter.
  • The different versions will not be able to be in the command manager, because only the views will know the parameters.
  • The command should be called as an ICommand using the CommandParameter. Can't call it with Tapped, which richcommands can do.
  • It is not possible to assign a hotkey to its commands since the action would not know which parameter to use.

Here is the solution that will be implemented.

  • ICommandManager will have a new way to register an custom action (and also to delete it).
  • It will therefore be possible in code behind to add several richcommands using the same Action class. The parameter will be passed in its constructor.
  • The commands created will be visible in the command manager and the user will be able to assign them a hotkey.
  • The Action class can itself manage all its properties (label, IsExecutable, hotkey by default) because each instance knows its parameter.
  • To save a new action, you will need to give it a unique key. This key will save custom hotkeys between sessions.
  • These custom commands will not be accessible directly through a property of ICommandManager. It will be necessary to use the instance of IRichCommand provided by registering the action.
  • You can find a custom command using its unique key.

All comments are welcome.

@cinqmilleans
Copy link
Contributor

I just read your code, and I better understand your need. Actions need to be able to observe the application to run but also to update properties. This is currently too difficult to do. The solution will be to use contexts.

A context is an observable service that easily returns useful information to actions. There will be several depending on the type of action. I am just preparing the most important context which will make it very easy to know the active folder, the selected elements and others. It should be released this weekend and with this context you will have everything you need.

@ferrariofilippo
Copy link
Contributor Author

That's great, thank you

@QuaintMako QuaintMako mentioned this pull request Feb 26, 2023
78 tasks
@cinqmilleans
Copy link
Contributor

@ferrariofilippo You can now use the new IContentPageService which will provide you with what you need.

@ferrariofilippo ferrariofilippo marked this pull request as ready for review February 27, 2023 17:30
@ferrariofilippo
Copy link
Contributor Author

We can't bind that directly to a KeyboardAccelerator since there's no VirtualKey for `

@yaira2
Copy link
Member

yaira2 commented Feb 28, 2023

Is this ready for review?

@ferrariofilippo
Copy link
Contributor Author

Yes

@yaira2 yaira2 changed the title Rich Command: Open in terminal Feature: Open in Terminal RichCommand Feb 28, 2023
hecksmosis
hecksmosis previously approved these changes Feb 28, 2023
Copy link
Contributor

@hecksmosis hecksmosis left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 requested a review from cinqmilleans March 6, 2023 18:42
@yaira2
Copy link
Member

yaira2 commented Mar 8, 2023

@cinqmilleans can you look this over?

@ferrariofilippo
Copy link
Contributor Author

ferrariofilippo commented Mar 11, 2023

  • This action is not observable. It doesn't matter if it's only a hotkey or a menu. If we add it to an always visible button, it will be problematic. Notify IsExecutable changes by subscribing to IContentPageContext changes (PageType,HasSelection,SelectedItem)

I won't subscribe to HasItem and SelectedItem since if there's no selected item, it will open base folder` Never mind

@ferrariofilippo
Copy link
Contributor Author

ferrariofilippo commented Mar 11, 2023

This is not finished yet, I still need to open all paths in a single wt instance, but I need to figure out how to know which shell is running to do this Done

@cinqmilleans
Copy link
Contributor

If you open the terminal at the root of a drive, the terminal indicates that the folder is invalid. You must use the path C:\ instead of C: for drives, but especially not for other folders.

@ferrariofilippo
Copy link
Contributor Author

If you open the terminal at the root of a drive, the terminal indicates that the folder is invalid. You must use the path C:\ instead of C: for drives, but especially not for other folders.

Actually the issue is that the back slash escapes the following characther

@yaira2
Copy link
Member

yaira2 commented Mar 14, 2023

@cinqmilleans is this ready to merge?

@cinqmilleans
Copy link
Contributor

cinqmilleans commented Mar 15, 2023

@ferrariofilippo The problems:

  • Methods with poor visibility (private/public).
  • IsExecutable difficult to read.
  • Add a limit to avoid opening 1000 folders per accident.
  • Update too frequent because not filtered.
  • If a selected item is not a folder, it opens the parent folder. The most logical thing is to do nothing.
  • Do not use ShellPage when values are available directly in Context. This avoids null problems.

Here is an improved version. The 4 must be replaced by a constant to be placed in the constants file.

using CommunityToolkit.Mvvm.ComponentModel;
using CommunityToolkit.Mvvm.DependencyInjection;
using Files.App.Commands;
using Files.App.Contexts;
using Files.App.Extensions;
using System;
using System.ComponentModel;
using System.Diagnostics;
using System.Linq;
using System.Threading.Tasks;
using Windows.Storage;
using Windows.System;

namespace Files.App.Actions
{
	internal class OpenTerminalAction : ObservableObject, IAction
	{
		private readonly IContentPageContext context = Ioc.Default.GetRequiredService<IContentPageContext>();

		public virtual string Label { get; } = "OpenTerminal".GetLocalizedResource();

		public virtual HotKey HotKey { get; } = new((VirtualKey)192, VirtualKeyModifiers.Control);

		public RichGlyph Glyph { get; } = new("\uE756");

		private bool isExecutable;
		public bool IsExecutable => isExecutable;

		public OpenTerminalAction()
		{
			isExecutable = GetIsExecutable();
			context.PropertyChanged += Context_PropertyChanged;
		}

		public Task ExecuteAsync()
		{
			var startInfo = GetProcessStartInfo();
			if (startInfo is not null)
			{
				App.Window.DispatcherQueue.TryEnqueue(() =>
				{
					try
					{
						Process.Start(startInfo);
					}
					catch (Win32Exception)
					{
					}
				});
			}
			return Task.CompletedTask;
		}

		protected virtual ProcessStartInfo? GetProcessStartInfo()
		{
			var paths = GetPaths();
			if (paths.Length is 0)
				return null;

			return new()
			{
				FileName = "wt.exe",
				Arguments = string.Join(" ; nt ", paths.Select(path => $"-d \"{path}\"")),
			};
		}

		private string[] GetPaths()
		{
			if (context.HasSelection)
			{
				return context.SelectedItems
					.Where(item => item.PrimaryItemAttribute is StorageItemTypes.Folder)
					.Select(item => item.ItemPath)
					.ToArray();
			}
			else if (context.Folder is not null)
			{
				return new string[1] { context.Folder.ItemPath };
			}
			return Array.Empty<string>();
		}

		private bool GetIsExecutable()
		{
			if (context.PageType is ContentPageTypes.None or ContentPageTypes.Home or ContentPageTypes.RecycleBin or ContentPageTypes.ZipFolder)
				return false;
			if (!context.HasSelection)
				return context.PageType is not ContentPageTypes.SearchResults && context.Folder is not null;
			if (context.SelectedItems.Count > 4)
				return false;
			return context.SelectedItems.Any(item => item.PrimaryItemAttribute is StorageItemTypes.Folder);
		}

		private void Context_PropertyChanged(object? sender, PropertyChangedEventArgs e)
		{
			switch (e.PropertyName)
			{
				case nameof(IContentPageContext.PageType):
				case nameof(IContentPageContext.Folder):
				case nameof(IContentPageContext.SelectedItems):
					SetProperty(ref isExecutable, GetIsExecutable(), nameof(IsExecutable));
					break;
			}
		}
	}
}

@ferrariofilippo
Copy link
Contributor Author

@ferrariofilippo The problems:

I didn't apply GetProcessStartInfo() changes since they would cause the previous bug with drives

@ferrariofilippo
Copy link
Contributor Author

The 4 must be replaced by a constant to be placed in the constants file.

I changed it to 5 to be more consistent with Open in New Tab and similar

@yaira2
Copy link
Member

yaira2 commented Mar 19, 2023

Anything else @cinqmilleans or is this ready to merge?

@cinqmilleans
Copy link
Contributor

@yaira2 @ferrariofilippo It is mandatory to select a folder to activate the hotkey. It doesn't work on parent folder without seletion. Yet the action handles this case everywhere except in GetIsExecutable. Modify this method to enable it.

@cinqmilleans
Copy link
Contributor

LGTM

@yaira2 yaira2 merged commit 91b803d into files-community:main Mar 20, 2023
@ferrariofilippo ferrariofilippo deleted the Open_In_Terminal_Rich_Command branch March 20, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants