settings.json parent folder functionality #15417

AbdullahAlmanei · 2023-05-24T09:35:34Z

Summary of the Pull Request

Added menu flyout to "Open JSON" that takes you to the JSON parent folder instead.
This is in response to #12382
Added comments for the one function I had to implement for the path. Nothing complicated.

AbdullahAlmanei · 2023-05-24T09:43:38Z

src/cascadia/TerminalSettingsEditor/MainPage.cpp

Messy formatting, should clean up.

tools\runformat.cmd from this repo will help reformat files automatically 😉

If you prefer PowerShell (like me), you can also do the following:

Import-Module .\tools\OpenConsole.psm1 Invoke-CodeFormat

Then, just be sure to commit the changes with git.

AbdullahAlmanei · 2023-05-24T21:43:36Z

@microsoft-github-policy-service agree

zadjii-msft

Overall, I'm cool with this. If you get really stuck with the Resources thing, then we can always just merge as-is and have the core team fix in post ☺️

(sorry for the delays, we were all a little preoccupied with Build last week)

zadjii-msft · 2023-05-31T11:35:19Z

src/cascadia/TerminalSettingsEditor/MainPage.cpp

tools\runformat.cmd from this repo will help reformat files automatically 😉

zadjii-msft · 2023-05-31T11:44:22Z

src/cascadia/TerminalSettingsEditor/MainPage.xaml

+                <muxc:NavigationViewItem.ContextFlyout>
+                    <MenuFlyout>
+                        <MenuFlyoutItem
+                                        x:Name="OpenJsonFolderNavItem"


If you give this a x:Uid, like:

<muxc:NavigationViewItem x:Name="OpenJsonFolderNavItem" x:Uid="Nav_OpenJsonFolder"

Then you should be able to set the text in the TerminalSettingsEditor\Resources\en-us\resources.resw like:

<data name="Nav_OpenJsonFolder.Text" xml:space="preserve"> <value>Open JSON folder</value> </data>

And even the tooltip should be settable with

<data name="Nav_OpenJsonFolder.[using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip" xml:space="preserve"> <value>Open the folder holding your settings.json file.</value> </data>

And if that doesn't work, then there's something wacky going on with XAML

This is exactly what I attempted originally. The thing is, NavigationViewItem.ContextFlyout doesn't take a x:Name and a x:Uid attribute. Moreover, adding the attributes to the Menuflyout or the MenuflyoutItem simply crashes the settings menu all together.

I have successfully set up different navigation items with the exact same x:Name and x:Uid, and I set the data through the .resw file. It works for most Navigation Items, but not with Menuflyouts for some reason.

I think this has something to do with the XAML or how it's wired to the rest of the program.

Hmm... Mind committing your changes anyways? I tried the suggested changes on my end, and it worked fine. I'd be happy to take a closer look at your approach and let you know how to fix it :)

Also, you're not referencing the name OpenJsonFolderNavItem anywhere, so you might as well remove it.

It's a relic of me trying to get the resources to work! Curious that it works on your end. Must be my system. I'll commit the changes, maybe I am doing something wrong, too. I appreciate the feedback!

carlos-zamora

Good progress! Just a few things before I sign off.

I'm also going to add the "Needs-Discussion" tag so we discuss this as a team. Specifically...

I have concerns that this feature might not be discoverable
Should we update the "new tab dropdown" "open settings" item so that if you click it with a specific modifier, it'll open the folder.
- Should the same be done for the settings button in the settings UI (the one you're messing with right now), instead of adding a flyout?

Feel free to brainstorm and discuss in this PR too. :)

carlos-zamora · 2023-06-08T18:00:09Z

src/cascadia/TerminalSettingsModel/ActionArgs.idl

@@ -63,7 +63,8 @@ namespace Microsoft.Terminal.Settings.Model
        SettingsFile = 0,
        DefaultsFile,
        AllFiles,
-        SettingsUI
+        SettingsUI,
+        SettingsFolder


(Related to this PR, but not necessary to make this change in this PR) IMO, at this point, we should fully commit and also add this as a parameter to the openSettings action. If you're interested in doing that, I think you'd mainly have to update...

ActionArgs.cpp OpenSettingsArgs::GenerateName()

TerminalSettingsSerializationHelpers.h JSON_ENUM_MAPPER(::winrt::Microsoft::Terminal::Settings::Model::SettingsTarget)

Resources.resw in the TerminalSettingsModel project (this is where you update the resource key for GenerateName())

Since you already updated TerminalPage::_LaunchSettings(), this should just work outright!

If you're not interested though, let me know. Happy to do it as a follow up myself :)

carlos-zamora · 2023-06-08T18:05:22Z

src/cascadia/TerminalSettingsEditor/MainPage.cpp

If you prefer PowerShell (like me), you can also do the following:

Import-Module .\tools\OpenConsole.psm1 Invoke-CodeFormat

Then, just be sure to commit the changes with git.

carlos-zamora · 2023-06-08T19:15:13Z

src/cascadia/TerminalSettingsEditor/MainPage.xaml

+                <muxc:NavigationViewItem.ContextFlyout>
+                    <MenuFlyout>
+                        <MenuFlyoutItem
+                                        x:Name="OpenJsonFolderNavItem"


Hmm... Mind committing your changes anyways? I tried the suggested changes on my end, and it worked fine. I'd be happy to take a closer look at your approach and let you know how to fix it :)

Also, you're not referencing the name OpenJsonFolderNavItem anywhere, so you might as well remove it.

AbdullahAlmanei · 2023-06-09T01:18:18Z

I have concerns that this feature might not be discoverable

I definitely agree; you'd have to right click by accident to discover it the way it's set up right now.

Should we update the "new tab dropdown" "open settings" item so that if you click it with a
specific modifier, it'll open the folder.

The thing is, there's already an alt-click modifier currently. Wouldn't the tooltip get quite lengthy if we tried something like that?

Should the same be done for the settings button in the settings UI (the one you're messing with right now), instead of adding a flyout?

I do believe that we should apply whatever we decide is best universally. As in, it should be the same anywhere you can access the settings JSON.

Really, I think our options are either a modifier or some kind of drop down. The latter might result in a messy user experience . A possible approach to the former might to figure out some other place we can move the current modifier to. This is, of course, if we believe that the JSON folder is a more significant and frequent operation than accessing the default JSON file. (I believe that's the current applied modifier, I have to double-check.)

AbdullahAlmanei · 2023-06-11T01:22:16Z

@carlos-zamora I committed the changes I talked about. However, do note that it's still not working on my local system. I have the output debug text file where the program crashes when I try to navigate to the settings menu at all. I have attached it here.
Output-Debug.txt

carlos-zamora

Regarding my comment earlier about discoverability. We chatted about this PR more as a team. Two things came out of it:

We feel it'd be best to prioritize this as an action and add it to the command palette. To do that, you'll have to...
- follow the instructions from this comment. This hooks it up as an action.
- add { "command": { "action": "openSettings", "target": "<JSON KEY FROM TerminalSettingsSerializationHelpers.h>" } }, to defaults.json. This adds it to the command palette for everyone. I'd add it next to the other openSettings actions.
Since the context menu isn't very discoverable, we think long-term, a better design would be something like the following:
- That way the folder icon appears on hover.
- However, this is a much more tedious to do and will have accessibility concerns/issues. It's doable, but it should definitely be its own standalone PR.

That said, we'd like to pivot this PR to focus on accomplishing the first item above and remove the context menu functionality (so basically, revert all changes made to src/cascadia/TerminalSettingsEditor). Then, as a follow-up, the UI changes can be introduced with the design from the second point above.

How does that sound? Happy to help and provide any more guidance along the way. 😊

carlos-zamora · 2023-06-15T20:03:34Z

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

@@ -577,6 +577,10 @@
  <data name="Nav_OpenJSON.Content" xml:space="preserve">
    <value>Open JSON file</value>
    <comment>Header for a menu item. This opens the JSON file that is used to log the app's settings.</comment>
+  </data>
+	<data name="Nav_OpenJSONFolder.Content" xml:space="preserve">


Suggested change

<data name="Nav_OpenJSONFolder.Content" xml:space="preserve">

<data name="Nav_OpenJSONFolder.Text" xml:space="preserve">

Found the bug! This'll fix the crash!

carlos-zamora · 2023-06-15T20:04:34Z

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

@@ -577,6 +577,10 @@
  <data name="Nav_OpenJSON.Content" xml:space="preserve">
    <value>Open JSON file</value>
    <comment>Header for a menu item. This opens the JSON file that is used to log the app's settings.</comment>
+  </data>


Also, make sure you use spaces here, not tabs.

If you want, you can also edit this file in Visual Studio. It gives you a table to work with, and always formats it correctly.

microsoft-github-policy-service · 2023-06-22T23:29:17Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

microsoft-github-policy-service · 2023-06-22T23:29:49Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

microsoft-github-policy-service · 2023-06-22T23:29:54Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

Most of the logic is taken from the original PR (#15417) and adapted to work with the palette. ## References and Relevant Issues [#12382](#12382)

microsoft#12382

abdd0e6

AbdullahAlmanei mentioned this pull request May 24, 2023

Add functionality to settings Open JSON file entry to open the settings.json parent folder in Explorer #12382

Open

AbdullahAlmanei commented May 24, 2023

View reviewed changes

zadjii-msft reviewed May 31, 2023

View reviewed changes

carlos-zamora requested changes Jun 8, 2023

View reviewed changes

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 Jun 8, 2023

carlos-zamora added the Needs-Discussion Something that requires a team discussion before we can proceed label Jun 8, 2023

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 Jun 9, 2023

These are changes that integrate Resources.

5be5db2

carlos-zamora removed the Needs-Discussion Something that requires a team discussion before we can proceed label Jun 12, 2023

carlos-zamora requested changes Jun 15, 2023

View reviewed changes

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 Jun 15, 2023

microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jun 22, 2023

microsoft-github-policy-service bot closed this Jun 30, 2023

e82eric mentioned this pull request Aug 9, 2024

Add action for opening settings directory in file explorer #17690

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

settings.json parent folder functionality #15417

settings.json parent folder functionality #15417

AbdullahAlmanei commented May 24, 2023

AbdullahAlmanei May 24, 2023

zadjii-msft May 31, 2023

carlos-zamora Jun 8, 2023

AbdullahAlmanei commented May 24, 2023

zadjii-msft left a comment

zadjii-msft May 31, 2023

zadjii-msft May 31, 2023

AbdullahAlmanei Jun 2, 2023

carlos-zamora Jun 8, 2023

AbdullahAlmanei Jun 9, 2023

carlos-zamora left a comment

carlos-zamora Jun 8, 2023

carlos-zamora Jun 8, 2023

carlos-zamora Jun 8, 2023

AbdullahAlmanei commented Jun 9, 2023 •

edited

Loading

AbdullahAlmanei commented Jun 11, 2023

carlos-zamora left a comment •

edited

Loading

carlos-zamora Jun 15, 2023

carlos-zamora Jun 15, 2023

microsoft-github-policy-service bot commented Jun 22, 2023

microsoft-github-policy-service bot commented Jun 22, 2023

microsoft-github-policy-service bot commented Jun 22, 2023

	<data name="Nav_OpenJSONFolder.Content" xml:space="preserve">
	<data name="Nav_OpenJSONFolder.Text" xml:space="preserve">

settings.json parent folder functionality #15417

settings.json parent folder functionality #15417

Conversation

AbdullahAlmanei commented May 24, 2023

Summary of the Pull Request

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbdullahAlmanei commented May 24, 2023

zadjii-msft 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

carlos-zamora 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

AbdullahAlmanei commented Jun 9, 2023 • edited Loading

AbdullahAlmanei commented Jun 11, 2023

carlos-zamora left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

microsoft-github-policy-service bot commented Jun 22, 2023

microsoft-github-policy-service bot commented Jun 22, 2023

microsoft-github-policy-service bot commented Jun 22, 2023

AbdullahAlmanei commented Jun 9, 2023 •

edited

Loading

carlos-zamora left a comment •

edited

Loading