-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update MSTerminalSettings.psm1 for v0.4.2382.0 #34
Conversation
In 0.4.2382.0 the profile path was moved from RoamingState to LocalState
@arnydo this should have a catch for a "notfound" exception and then fallback to roamingstate for backwards compatibility (if we even care about that since Terminal is an alpha currently) |
Considering it is alpha and the profile will likely stay in LocalState going forward I don't know if the check is necessary. If you think it should, where would be the best place to put that catch? May need to declare a $script:DEV_PATH_ALT and $script:RELEASE_PATH_ALT so that they can be used in https://github.com/arnydo/MSTerminalSettings/blob/preview/src/ExportedFunctions/Find-MSTerminalFolder.ps1. |
Add ALT paths for backward compatibility with Terminal versions prior to 0.4.2382.0
Okay, added the additional ALT paths to the Find-MSTerminalFolder.ps1. That should solve the backward compat concern. |
Thanks for doing this, I had seen the change in terminal but hadn't had a chance to get in here and add it yet :) |
Happy to help! |
$Script:DEV_PATH = "packages/WindowsTerminalDev_8wekyb3d8bbwe/RoamingState" | ||
$Script:RELEASE_PATH = "packages/Microsoft.WindowsTerminal_8wekyb3d8bbwe/RoamingState" | ||
$Script:DEV_PATH = "packages/WindowsTerminalDev_8wekyb3d8bbwe/LocalState" | ||
$Script:RELEASE_PATH = "packages/Microsoft.WindowsTerminal_8wekyb3d8bbwe/LocalState" | ||
$Script:STANDALONE_PATH = "Microsoft/Windows Terminal" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the old paths back as the *_ALT variables?
$Script:DEV_PATH_ALT = "packages/WindowsTerminalDev_8wekyb3d8bbwe/RoamingState"
$Script:RELEASE_PATH_ALT = "packages/Microsoft.WindowsTerminal_8wekyb3d8bbwe/RoamingState"
Also maybe add a comment that says the *_ALT paths are for supporting < 0.4.2382 so we can remember why they exist and when we can get rid of them in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gpduck Weird, I thought for sure I had included those. I have added them back and updated the order they are called in Find-MSTerminalFolder. Do I need to open another pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please.
@@ -6,7 +6,9 @@ function Find-MSTerminalFolder { | |||
$Paths = @( | |||
(Join-Path $WellKnownPaths.LocalAppData $Script:RELEASE_PATH), | |||
(Join-Path $WellKnownPaths.AppData $Script:STANDALONE_PATH), | |||
(Join-Path $WellKnownPaths.LocalAppData $Script:DEV_PATH) | |||
(Join-Path $WellKnownPaths.LocalAppData $Script:DEV_PATH), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in order to preserve the path resolution behavior I would prefer this to go in this order:
RELEASE_PATH
RELEASE_PATH_ALT
STANDALONE_PATH
DEV_PATH
DEV_PATH_ALT
In 0.4.2382.0 the profile path was moved from RoamingState to LocalState.