Skip to content

Commit

Permalink
Use .rc files in TSM instead of string literals (#16844)
Browse files Browse the repository at this point in the history
More prerequisite work for Action IDs - turns out if we add the action
IDs to the actions defined in `defaults.json` the string ends up being
too large and the compiler complains about it. Use a `.rc` file for
`defaults.json` instead and also for `enableColorSelection.json` +
`userDefaults.json`.
  • Loading branch information
PankajBhojwani authored Mar 14, 2024
1 parent 806d5e2 commit 566b660
Show file tree
Hide file tree
Showing 15 changed files with 117 additions and 67 deletions.
10 changes: 10 additions & 0 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,16 @@ winrt::com_ptr<Profile> Model::implementation::CreateChild(const winrt::com_ptr<
return profile;
}

std::string_view Model::implementation::LoadStringResource(int resourceID)
{
const HINSTANCE moduleInstanceHandle{ wil::GetModuleInstanceHandle() };
const auto resource = FindResourceW(moduleInstanceHandle, MAKEINTRESOURCEW(resourceID), RT_RCDATA);
const auto loaded = LoadResource(moduleInstanceHandle, resource);
const auto sz = SizeofResource(moduleInstanceHandle, resource);
const auto ptr = LockResource(loaded);
return { reinterpret_cast<const char*>(ptr), sz };
}

winrt::hstring CascadiaSettings::Hash() const noexcept
{
return _hash;
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model

namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
std::string_view LoadStringResource(int resourceID);
winrt::com_ptr<Profile> CreateChild(const winrt::com_ptr<Profile>& parent);

class SettingsTypedDeserializationException final : public std::runtime_error
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <fmt/chrono.h>
#include <shlobj.h>
#include <til/latch.h>
#include "resource.h"

#include "AzureCloudShellGenerator.h"
#include "PowershellCoreProfileGenerator.h"
Expand All @@ -17,15 +18,9 @@
#include "SshHostGenerator.h"
#endif

// The following files are generated at build time into the "Generated Files" directory.
// defaults.h is a file containing the default json settings in a std::string_view.
#include "defaults.h"
// userDefault.h is like the above, but with a default template for the user's settings.json.
#include <LegacyProfileGeneratorNamespaces.h>

#include "userDefaults.h"
#include "enableColorSelection.h"

#include "ApplicationState.h"
#include "DefaultTerminal.h"
#include "FileUtils.h"
Expand Down Expand Up @@ -349,7 +344,7 @@ void SettingsLoader::FinalizeLayering()
// actions, this is the time to do it.
if (userSettings.globals->EnableColorSelection())
{
const auto json = _parseJson(EnableColorSelectionSettingsJson);
const auto json = _parseJson(LoadStringResource(IDR_ENABLE_COLOR_SELECTION));
const auto globals = GlobalAppSettings::FromJson(json.root);
userSettings.globals->AddLeastImportantParent(globals);
}
Expand Down Expand Up @@ -972,10 +967,10 @@ try

// Only uses default settings when firstTimeSetup is true and releaseSettingExists is false
// Otherwise use existing settingsString
const auto settingsStringView = (firstTimeSetup && !releaseSettingExists) ? UserSettingsJson : settingsString;
const auto settingsStringView = (firstTimeSetup && !releaseSettingExists) ? LoadStringResource(IDR_USER_DEFAULTS) : settingsString;
auto mustWriteToDisk = firstTimeSetup;

SettingsLoader loader{ settingsStringView, DefaultJson };
SettingsLoader loader{ settingsStringView, LoadStringResource(IDR_DEFAULTS) };

// Generate dynamic profiles and add them as parents of user profiles.
// That way the user profiles will get appropriate defaults from the generators (like icons and such).
Expand Down Expand Up @@ -1127,7 +1122,7 @@ void CascadiaSettings::_researchOnLoad()
// - a unique_ptr to a CascadiaSettings with the settings from defaults.json
Model::CascadiaSettings CascadiaSettings::LoadDefaults()
{
return *winrt::make_self<CascadiaSettings>(std::string_view{}, DefaultJson);
return *winrt::make_self<CascadiaSettings>(std::string_view{}, LoadStringResource(IDR_DEFAULTS));
}

CascadiaSettings::CascadiaSettings(const winrt::hstring& userJSON, const winrt::hstring& inboxJSON) :
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Microsoft Visual C++ generated resource script.
//
#include "resource.h"

#define APSTUDIO_READONLY_SYMBOLS
/////////////////////////////////////////////////////////////////////////////
//
// Generated from the TEXTINCLUDE 2 resource.
//
#include "winres.h"

/////////////////////////////////////////////////////////////////////////////
#undef APSTUDIO_READONLY_SYMBOLS

/////////////////////////////////////////////////////////////////////////////
// English (United States) resources

#if !defined(AFX_RESOURCE_DLL) || defined(AFX_TARG_ENU)
LANGUAGE LANG_ENGLISH, SUBLANG_ENGLISH_US
#pragma code_page(65001)

/////////////////////////////////////////////////////////////////////////////
//
// RCDATA
//
IDR_DEFAULTS RCDATA "Generated Files\defaults.json"
IDR_USER_DEFAULTS RCDATA "Generated Files\userDefaults.json"
IDR_ENABLE_COLOR_SELECTION RCDATA "Generated Files\enableColorSelection.json"

#endif // English (United States) resources
/////////////////////////////////////////////////////////////////////////////
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
<Import Project="$(OpenConsoleDir)src\cppwinrt.build.pre.props" />
<!-- ========================= Headers ======================== -->
<ItemGroup>
<ClInclude Include="resource.h" />
<ClInclude Include="ActionArgsMagic.h" />
<ClInclude Include="NewTabMenuEntry.h">
<DependentUpon>NewTabMenuEntry.idl</DependentUpon>
Expand Down Expand Up @@ -238,6 +239,9 @@
<PRIResource Include="Resources\en-US\Resources.resw" />
<OCResourceDirectory Include="Resources" />
</ItemGroup>
<ItemGroup>
<ResourceCompile Include="Microsoft.Terminal.Settings.Model.rc" />
</ItemGroup>
<!-- ========================= Project References ======================== -->
<ItemGroup>
<!--
Expand Down Expand Up @@ -320,26 +324,13 @@
<Import Project="$(OpenConsoleDir)src\common.nugetversions.targets" />

<!-- PowerShell version check, outputs error if the wrong version is installed. -->
<Target Name="_WindowsPowershellVersionCheck" BeforeTargets="BeforeClCompile">
<Import Project="$(SolutionDir)build\rules\CollectWildcardResources.targets" />
<Target Name="BeforeResourceCompile" Inputs="defaults.json;userDefaults.json;enableColorSelection.json" Outputs="Generated Files\defaults.json;Generated Files\userDefaults.json;Generated Files\enableColorSelection.json">
<Exec Command="powershell.exe -NoProfile -ExecutionPolicy Unrestricted -File &quot;$(OpenConsoleDir)\tools\WindowsCheckPSVersion.ps1&quot;" />
</Target>
<Target Name="_PowershellVersionCheck" BeforeTargets="BeforeClCompile">
<Exec Command="pwsh.exe -NoProfile -ExecutionPolicy Unrestricted &quot;$(OpenConsoleDir)\tools\CheckPSVersion.ps1&quot;" />
<Exec Command="pwsh.exe -NoProfile -ExecutionPolicy Unrestricted &quot;$(OpenConsoleDir)\tools\CompressJson.ps1&quot; -JsonFile defaults.json -OutPath &quot;Generated Files\defaults.json&quot;" />
<Exec Command="pwsh.exe -NoProfile -ExecutionPolicy Unrestricted &quot;$(OpenConsoleDir)\tools\CompressJson.ps1&quot; -JsonFile userDefaults.json -OutPath &quot;Generated Files\userDefaults.json&quot;" />
<Exec Command="pwsh.exe -NoProfile -ExecutionPolicy Unrestricted &quot;$(OpenConsoleDir)\tools\CompressJson.ps1&quot; -JsonFile enableColorSelection.json -OutPath &quot;Generated Files\enableColorSelection.json&quot;" />
</Target>
<!-- This target will take our defaults.json and stamp it into a .h file that
we can include in the code directly. This way, we don't need to worry about
failing to load the default settings at runtime. -->
<Target Name="_TerminalAppGenerateDefaultsH" Inputs="defaults.json" Outputs="Generated Files\defaults.h" BeforeTargets="BeforeClCompile">
<Exec Command="pwsh.exe -NoProfile -ExecutionPolicy Unrestricted &quot;$(OpenConsoleDir)\tools\GenerateHeaderForJson.ps1&quot; -JsonFile defaults.json -OutPath &quot;Generated Files\defaults.h&quot; -VariableName DefaultJson" />
</Target>
<!-- Same as above, but for the default settings.json template -->
<Target Name="_TerminalAppGenerateUserSettingsH" Inputs="userDefaults.json" Outputs="Generated Files\userDefaults.h" BeforeTargets="BeforeClCompile">
<Exec Command="pwsh.exe -NoProfile -ExecutionPolicy Unrestricted &quot;$(OpenConsoleDir)\tools\GenerateHeaderForJson.ps1&quot; -JsonFile userDefaults.json -OutPath &quot;Generated Files\userDefaults.h&quot; -VariableName UserSettingsJson" />
</Target>
<!-- Same as above, but for the enableColorSelection actions -->
<Target Name="_TerminalAppGenerateEnableColorSelectionSettingsH" Inputs="enableColorSelection.json" Outputs="Generated Files\enableColorSelection.h" BeforeTargets="BeforeClCompile">
<Exec Command="pwsh.exe -NoProfile -ExecutionPolicy Unrestricted &quot;$(OpenConsoleDir)\tools\GenerateHeaderForJson.ps1&quot; -JsonFile enableColorSelection.json -OutPath &quot;Generated Files\enableColorSelection.h&quot; -VariableName EnableColorSelectionSettingsJson" />
</Target>
<Import Project="$(SolutionDir)build\rules\CollectWildcardResources.targets" />

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@
<AdditionalIncludeDirectories>$(OpenConsoleDir)\dep\jsoncpp\json;%(AdditionalIncludeDirectories);</AdditionalIncludeDirectories>
</ClCompile>
<Link>
<AdditionalDependencies>%(AdditionalDependencies);user32.lib</AdditionalDependencies>
<AdditionalDependencies>%(AdditionalDependencies);user32.lib;$(IntDir)\..\Microsoft.Terminal.Settings.Model.lib\Microsoft.Terminal.Settings.Model.res</AdditionalDependencies>
<!-- TSM Lib contains a DllMain that we need to force the use of. -->
<AdditionalOptions Condition="'$(Platform)'=='Win32'">/INCLUDE:_DllMain@12 %(AdditionalOptions)</AdditionalOptions>
<AdditionalOptions Condition="'$(Platform)'!='Win32'">/INCLUDE:DllMain %(AdditionalOptions)</AdditionalOptions>
Expand Down
7 changes: 7 additions & 0 deletions src/cascadia/TerminalSettingsModel/resource.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
//{{NO_DEPENDENCIES}}
// Microsoft Visual C++ generated include file.
// Used by WindowsTerminal.rc
//
#define IDR_DEFAULTS 101
#define IDR_USER_DEFAULTS 102
#define IDR_ENABLE_COLOR_SELECTION 103
30 changes: 14 additions & 16 deletions src/cascadia/UnitTests_SettingsModel/DeserializationTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,10 @@

#include "../TerminalSettingsModel/ColorScheme.h"
#include "../TerminalSettingsModel/CascadiaSettings.h"
#include "../TerminalSettingsModel/resource.h"
#include "JsonTestClass.h"
#include "TestUtils.h"

#include <defaults.h>
#include <userDefaults.h>

using namespace Microsoft::Console;
using namespace WEX::Logging;
using namespace WEX::TestExecution;
Expand Down Expand Up @@ -541,7 +539,7 @@ namespace SettingsModelUnitTests
]
})" };

const auto settings = winrt::make_self<implementation::CascadiaSettings>(settings0String, DefaultJson);
const auto settings = winrt::make_self<implementation::CascadiaSettings>(settings0String, implementation::LoadStringResource(IDR_DEFAULTS));

VERIFY_ARE_EQUAL(0u, settings->Warnings().Size());
VERIFY_ARE_EQUAL(4u, settings->AllProfiles().Size());
Expand Down Expand Up @@ -619,7 +617,7 @@ namespace SettingsModelUnitTests
]
})" };

const auto settings = winrt::make_self<implementation::CascadiaSettings>(settings0String, DefaultJson);
const auto settings = winrt::make_self<implementation::CascadiaSettings>(settings0String, implementation::LoadStringResource(IDR_DEFAULTS));

VERIFY_ARE_EQUAL(0u, settings->Warnings().Size());
VERIFY_ARE_EQUAL(4u, settings->AllProfiles().Size());
Expand Down Expand Up @@ -656,7 +654,7 @@ namespace SettingsModelUnitTests
]
})" };

const auto settings = winrt::make_self<implementation::CascadiaSettings>(settings0String, DefaultJson);
const auto settings = winrt::make_self<implementation::CascadiaSettings>(settings0String, implementation::LoadStringResource(IDR_DEFAULTS));
const auto profiles = settings->AllProfiles();
VERIFY_ARE_EQUAL(5u, profiles.Size());
VERIFY_ARE_EQUAL(L"ThisProfileIsGood", profiles.GetAt(0).Name());
Expand Down Expand Up @@ -1065,7 +1063,7 @@ namespace SettingsModelUnitTests
const auto guid1String = L"{6239a42c-1111-49a3-80bd-e8fdd045185c}";
const winrt::guid guid1{ Utils::GuidFromString(guid1String) };

const auto settings = winrt::make_self<implementation::CascadiaSettings>(settings0String, DefaultJson);
const auto settings = winrt::make_self<implementation::CascadiaSettings>(settings0String, implementation::LoadStringResource(IDR_DEFAULTS));

VERIFY_ARE_EQUAL(guid1String, settings->GlobalSettings().UnparsedDefaultProfile());
VERIFY_ARE_EQUAL(4u, settings->AllProfiles().Size());
Expand Down Expand Up @@ -2006,7 +2004,7 @@ namespace SettingsModelUnitTests
]
})" };

implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
implementation::SettingsLoader loader{ std::string_view{}, implementation::LoadStringResource(IDR_DEFAULTS) };
loader.MergeInboxIntoUserSettings();
loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
loader.FinalizeLayering();
Expand All @@ -2029,7 +2027,7 @@ namespace SettingsModelUnitTests
]
})" };

implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
implementation::SettingsLoader loader{ std::string_view{}, implementation::LoadStringResource(IDR_DEFAULTS) };
loader.MergeInboxIntoUserSettings();
loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
loader.FinalizeLayering();
Expand All @@ -2054,7 +2052,7 @@ namespace SettingsModelUnitTests
]
})" };

implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
implementation::SettingsLoader loader{ std::string_view{}, implementation::LoadStringResource(IDR_DEFAULTS) };
loader.MergeInboxIntoUserSettings();
loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
loader.FinalizeLayering();
Expand Down Expand Up @@ -2088,7 +2086,7 @@ namespace SettingsModelUnitTests
]
})" };

implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
implementation::SettingsLoader loader{ std::string_view{}, implementation::LoadStringResource(IDR_DEFAULTS) };
loader.MergeInboxIntoUserSettings();
loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
loader.FinalizeLayering();
Expand Down Expand Up @@ -2123,7 +2121,7 @@ namespace SettingsModelUnitTests
]
})" };

implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
implementation::SettingsLoader loader{ std::string_view{}, implementation::LoadStringResource(IDR_DEFAULTS) };
loader.MergeInboxIntoUserSettings();
loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
loader.FinalizeLayering();
Expand All @@ -2149,7 +2147,7 @@ namespace SettingsModelUnitTests
]
})" };

implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
implementation::SettingsLoader loader{ std::string_view{}, implementation::LoadStringResource(IDR_DEFAULTS) };
loader.MergeInboxIntoUserSettings();
loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
loader.FinalizeLayering();
Expand All @@ -2175,7 +2173,7 @@ namespace SettingsModelUnitTests
]
})" };

implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
implementation::SettingsLoader loader{ std::string_view{}, implementation::LoadStringResource(IDR_DEFAULTS) };
loader.MergeInboxIntoUserSettings();
loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
loader.FinalizeLayering();
Expand All @@ -2189,7 +2187,7 @@ namespace SettingsModelUnitTests
const auto oldResult{ oldSettings->ToJson() };

Log::Comment(L"Now, create a _new_ settings object from the re-serialization of the first");
implementation::SettingsLoader newLoader{ toString(oldResult), DefaultJson };
implementation::SettingsLoader newLoader{ toString(oldResult), implementation::LoadStringResource(IDR_DEFAULTS) };
// NOTABLY! Don't load the fragment here.
newLoader.MergeInboxIntoUserSettings();
newLoader.FinalizeLayering();
Expand Down Expand Up @@ -2223,7 +2221,7 @@ namespace SettingsModelUnitTests
]
})" };

implementation::SettingsLoader loader{ settings1Json, DefaultJson };
implementation::SettingsLoader loader{ settings1Json, implementation::LoadStringResource(IDR_DEFAULTS) };
loader.MergeInboxIntoUserSettings();
loader.FinalizeLayering();

Expand Down
7 changes: 3 additions & 4 deletions src/cascadia/UnitTests_SettingsModel/NewTabMenuTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,10 @@
#include "../TerminalSettingsModel/NewTabMenuEntry.h"
#include "../TerminalSettingsModel/FolderEntry.h"
#include "../TerminalSettingsModel/CascadiaSettings.h"
#include "../TerminalSettingsModel/resource.h"
#include "../types/inc/colorTable.hpp"
#include "JsonTestClass.h"

#include <defaults.h>

using namespace Microsoft::Console;
using namespace winrt::Microsoft::Terminal;
using namespace winrt::Microsoft::Terminal::Settings::Model::implementation;
Expand All @@ -37,7 +36,7 @@ namespace SettingsModelUnitTests

try
{
const auto settings{ winrt::make_self<CascadiaSettings>(settingsString, DefaultJson) };
const auto settings{ winrt::make_self<CascadiaSettings>(settingsString, LoadStringResource(IDR_DEFAULTS)) };

VERIFY_ARE_EQUAL(0u, settings->Warnings().Size());

Expand Down Expand Up @@ -71,7 +70,7 @@ namespace SettingsModelUnitTests

try
{
const auto settings{ winrt::make_self<CascadiaSettings>(settingsString, DefaultJson) };
const auto settings{ winrt::make_self<CascadiaSettings>(settingsString, LoadStringResource(IDR_DEFAULTS)) };

VERIFY_ARE_EQUAL(0u, settings->Warnings().Size());

Expand Down
3 changes: 1 addition & 2 deletions src/cascadia/UnitTests_SettingsModel/ProfileTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@

#include "../TerminalSettingsModel/ColorScheme.h"
#include "../TerminalSettingsModel/CascadiaSettings.h"
#include "../TerminalSettingsModel/resource.h"
#include "JsonTestClass.h"

#include <defaults.h>

using namespace Microsoft::Console;
using namespace winrt::Microsoft::Terminal::Settings::Model;
using namespace WEX::Logging;
Expand Down
Loading

0 comments on commit 566b660

Please sign in to comment.