Skip to content

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

Merged
4 commits merged intomainfrom
dev/lhecker/4015-followup
Dec 1, 2022
Merged

Clean up til::point/size/rect member usage#14458
4 commits merged intomainfrom
dev/lhecker/4015-followup

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