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

rename profiles.json to settings.json, clean up the defaults #5199

Merged
6 commits merged into from
Apr 1, 2020

Conversation

DHowett-MSFT
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT commented Mar 31, 2020

This pull request migrates profiles.json to settings.json and removes the legacy roaming AppData settings migrator.

It also:

  • separates the key bindings in defaults.json into logical groups
  • syncs the universal terminal defaults with the primary defaults
  • removes some stray newlines that ended up at the beginning of settings.json and defaults.json

Fixes #5186.
Fixes #3291.

categorize key bindings

sync universal with main

kill stray newlines in template files

move profiles.json to settings.json

This commit also changes Get*Settings from returning a string to
returning a std::filesystem::path. We gain in expressiveness without a
loss in clarity (since path still supports .c_str()).

NOTE: I tried to do an atomic rename with the handle open, but it didn't
work for reparse points (it moves the destination of a symbolic link
out into the settings folder directly.)

(snip for atomic rename code)

auto path{ pathToSettingsFile.wstring() };
auto renameBufferSize{ sizeof(FILE_RENAME_INFO) + (path.size() * sizeof(wchar_t)) };
auto renameBuffer{ std::make_unique<std::byte[]>(renameBufferSize) };
auto renameInfo{ reinterpret_cast<FILE_RENAME_INFO*>(renameBuffer.get()) };
renameInfo->Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS | FILE_RENAME_FLAG_POSIX_SEMANTICS;
renameInfo->RootDirectory = nullptr;
renameInfo->FileNameLength = gsl::narrow_cast<DWORD>(path.size());
std::copy(path.cbegin(), path.cend(), std::begin(renameInfo->FileName));

THROW_IF_WIN32_BOOL_FALSE(SetFileInformationByHandle(hLegacyFile.get(),
                          FileRenameInfo,
                          renameBuffer.get(),
                          gsl::narrow_cast<DWORD>(renameBufferSize)));

(end snip)

Stop resurrecting dead roaming profiles

This commit also changes Get*Settings from returning a string to
returning a std::filesystem::path. We gain in expressiveness without a
loss in clarity (since path still supports .c_str()).

NOTE: I tried to do an atomic rename with the handle open, but it didn't
work for reparse points (it moves the destination of a symbolic link
out into the settings folder directly.)

Fixes #5186

--- snip for atomic rename code ---

auto path{ pathToSettingsFile.wstring() };
auto renameBufferSize{ sizeof(FILE_RENAME_INFO) + (path.size() * sizeof(wchar_t)) };
auto renameBuffer{ std::make_unique<std::byte[]>(renameBufferSize) };
auto renameInfo{ reinterpret_cast<FILE_RENAME_INFO*>(renameBuffer.get()) };
renameInfo->Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS | FILE_RENAME_FLAG_POSIX_SEMANTICS;
renameInfo->RootDirectory = nullptr;
renameInfo->FileNameLength = gsl::narrow_cast<DWORD>(path.size());
std::copy(path.cbegin(), path.cend(), std::begin(renameInfo->FileName));

THROW_IF_WIN32_BOOL_FALSE(SetFileInformationByHandle(hLegacyFile.get(), FileRenameInfo, renameBuffer.get(), gsl::narrow_cast<DWORD>(renameBufferSize)));

-----------------------------------
@DHowett-MSFT

This comment has been minimized.

@DHowett-MSFT DHowett-MSFT marked this pull request as ready for review March 31, 2020 21:09
@DHowett-MSFT DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label Apr 1, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@zadjii-msft zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Apr 1, 2020
@carlos-zamora
Copy link
Member

@leonMSFT should approve this so that we get the whole team to approve it haha

Copy link
Contributor

@leonMSFT leonMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i gotchu

@DHowett-MSFT
Copy link
Contributor Author

lol y’all

@oising
Copy link
Collaborator

oising commented Apr 1, 2020

I guess I should update my PR for WT env vars?

@DHowett-MSFT
Copy link
Contributor Author

Yeah! That'd be excellent.

@DHowett-MSFT DHowett-MSFT changed the title Rename profiles.json to settings.json and clean up the defaults rename profiles.json to settings.json, clean up the defaults Apr 1, 2020
@DHowett-MSFT DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 1, 2020
@ghost
Copy link

ghost commented Apr 1, 2020

Hello @DHowett-MSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 64489b1 into master Apr 1, 2020
@ghost ghost deleted the dev/duhowett/legacy_ii_legacy_harder branch April 1, 2020 19:09
@oising
Copy link
Collaborator

oising commented Apr 1, 2020

@DHowett-MSFT Done.

@ghost
Copy link

ghost commented Apr 22, 2020

🎉Windows Terminal Preview v0.11.1121.0 has been released which incorporates this pull request.:tada:

Handy links:

sylveon pushed a commit to LazoCoder/Pokemon-Terminal that referenced this pull request May 1, 2020
* Add Windows Terminal adapter

* [Windows Terminal] Get defaultProfile on globals

* [Windows Terminal] Update with latest version
- profiles.json to settings.json
microsoft/terminal#5199
- Change current profile
microsoft/terminal#4852

* [Windows Terminal] Rename adapter to pass tests

* [Windows Terminal] Update with default profile settings
https://github.com/microsoft/terminal/blob/master/doc/user-docs/UsingJsonSettings.md#default-settings
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename profiles.json to settings.json Roaming profile still lives; isn't deleted by the sync service
8 participants