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 action for opening settings directory in file explorer #17690

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

Conversation

e82eric
Copy link
Contributor

@e82eric e82eric commented Aug 9, 2024

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

I added 2 additional actions, send settings file path as input and copy settings file path to clipboard. (Totally understand if these should be removed)

open_settings_in_explorer

Summary of the Pull Request

References and Relevant Issues

#12382

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

PR Checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated
    • If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated (if necessary)

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.

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.

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
_copyToClipboard when copying settings file path to clipboard
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. Needs-Discussion Something that requires a team discussion before we can proceed Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants