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

Jump to bottom

Add action for opening settings directory in file explorer #17690

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add action for opening settings directory in file explorer #17690

wants to merge 4 commits into from

Conversation

e82eric
Copy link
Contributor

@e82eric e82eric commented Aug 9, 2024 •
edited by carlos-zamora
Loading

Most of the logic is taken from the original PR (#15417) and adapted to work with the palette.

References and Relevant Issues

#12382

@e82eric
Add ability to send input settings file path. (Also open settings
0c57f86 
directory and copy settings file path to clipboard)
@DHowett
Copy link
Member

DHowett commented Aug 16, 2024

Sorry for the delay! We've been understaffed this week 🙂

@e82eric
Copy link
Contributor Author

e82eric commented Aug 17, 2024

No worries, no rush on my end.

lhecker
lhecker approved these changes Aug 20, 2024
View reviewed changes
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

Apart from the .c_str() call the PR seems fine to me. I'll approve in advance because it'll have to wait for a 2nd approval anyway.

However, I'm not quite sure if we should introduce the 2 additional options just yet.

src/cascadia/TerminalApp/TerminalPage.cpp Outdated
openFolder(CascadiaSettings::SettingsDirectory());
break;
case SettingsTarget::Clipboard:
copyToClipboard(CascadiaSettings::SettingsPath().c_str());
Copy link
Member

Choose a reason for hiding this comment

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

The .c_str() is unnecessary, if not detrimental, because constructing a string-view from a nullptr is UB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. (Sorry, I should have thought that through)

Let me know if I should remove the other 2 actions. Fwiw, the send input one is what I have found most useful, I don't think I have ever actually used the other 2 outside of testing. But that is probably just because of my personal workflows.

@lhecker lhecker added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Discussion Something that requires a team discussion before we can proceed labels Aug 20, 2024
@e82eric
Avoid potentially passing nullptr as wstring_view when calling
cbab271 
_copyToClipboard when copying settings file path to clipboard
@carlos-zamora carlos-zamora removed the Needs-Discussion Something that requires a team discussion before we can proceed label Oct 21, 2024
carlos-zamora
carlos-zamora requested changes Oct 21, 2024
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora 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 being so patient with us. Sorry it's taken us a long time to get to it.

Got a chance to discuss this with the team. We're interested in being able to open the settings directory, but not the sendInput or clipboard ones as they may be too niche.

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h Outdated
Comment on lines 443 to 444
pair_type{ "sendInput", ValueType::SendInput },
pair_type{ "clipboard", ValueType::Clipboard },
Copy link
Member

Choose a reason for hiding this comment

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

Discussed this as a team. I see value in opening the directory, but sendInput and clipboard might be too niche. We'd prefer that you back out those two values please.

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h Outdated
pair_type{ "settingsFile", ValueType::SettingsFile },
pair_type{ "defaultsFile", ValueType::DefaultsFile },
pair_type{ "allFiles", ValueType::AllFiles },
pair_type{ "settingsUI", ValueType::SettingsUI },
pair_type{ "fileExplorer", ValueType::FileExplorer },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pair_type{ "fileExplorer", ValueType::FileExplorer },
pair_type{ "directory", ValueType::Directory },

I think this would be cleaner

src/cascadia/TerminalSettingsModel/defaults.json Outdated
Comment on lines 434 to 436
{ "command": { "action": "openSettings", "target": "sendInput" }, "id": "Terminal.SendInputSettingsFile" },
{ "command": { "action": "openSettings", "target": "fileExplorer" }, "id": "Terminal.FileExplorerSettingsFile" },
{ "command": { "action": "openSettings", "target": "clipboard" }, "id": "Terminal.ClipboardSettingsFile" },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ "command": { "action": "openSettings", "target": "sendInput" }, "id": "Terminal.SendInputSettingsFile" },
{ "command": { "action": "openSettings", "target": "fileExplorer" }, "id": "Terminal.FileExplorerSettingsFile" },
{ "command": { "action": "openSettings", "target": "clipboard" }, "id": "Terminal.ClipboardSettingsFile" },
{ "command": { "action": "openSettings", "target": "directory" }, "id": "Terminal.OpenSettingsDirectory" },

src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw Outdated
<value>Send the settings file path as input to the terminal</value>
</data>
<data name="SettingsFileOpenInExplorerCommandKey" xml:space="preserve">
<value>Open the settings file directory in File Explorer</value>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<value>Open the settings file directory in File Explorer</value>
<value>Open the settings file directory</value>

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 21, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 22, 2024
@e82eric
Copy link
Contributor Author

e82eric commented Oct 22, 2024

Thanks for being so patient with us. Sorry it's taken us a long time to get to it.

Got a chance to discuss this with the team. We're interested in being able to open the settings directory, but not the sendInput or clipboard ones as they may be too niche.

Sounds good! updated.

carlos-zamora
carlos-zamora approved these changes Oct 29, 2024
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Love it. Thank you!

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h Outdated Show resolved Hide resolved
@carlos-zamora
Copy link
Member

Gif removed from PR body:

open_settings_in_explorer

@carlos-zamora carlos-zamora enabled auto-merge (squash) October 29, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@lhecker lhecker lhecker approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@e82eric @DHowett @carlos-zamora @lhecker