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

Clean up til::point/size/rect member usage #14458

Merged
4 commits merged into from
Dec 1, 2022
Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Nov 29, 2022

This is a follow-up of #13025 to make the members of til::point/size/rect
uniform and consistent without the use of unions. The only file that has
any changes is src/host/getset.cpp where an if condition was simplified.

Validation Steps Performed

  • Host unit tests ✅
  • Host feature tests ✅
  • ControlCore feature tests ✅

@lhecker lhecker added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Nov 29, 2022
Copy link
Member

@carlos-zamora carlos-zamora 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. Thanks!

Comment on lines -602 to +609
const auto requestedBufferSize = data.dwSize;
if (requestedBufferSize.X != coordScreenBufferSize.X ||
requestedBufferSize.Y != coordScreenBufferSize.Y)
const auto requestedBufferSize = til::wrap_coord_size(data.dwSize);
if (requestedBufferSize != coordScreenBufferSize)
{
auto& commandLine = CommandLine::Instance();

commandLine.Hide(FALSE);

LOG_IF_FAILED(context.ResizeScreenBuffer(til::wrap_coord_size(data.dwSize), TRUE));
LOG_IF_FAILED(context.ResizeScreenBuffer(requestedBufferSize, TRUE));
Copy link
Member

Choose a reason for hiding this comment

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

Note to other reviewers: aside from removing the union in til::point/rect/size, this is the only other change I found that isn't just renaming.

@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 1, 2022
@ghost
Copy link

ghost commented Dec 1, 2022

Hello @lhecker!

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 0eff8c0 into main Dec 1, 2022
@ghost ghost deleted the dev/lhecker/4015-followup branch December 1, 2022 00:40
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-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants