Make sure to update the selection pivot while circling #14636
Conversation
ghost
added
Area-TerminalControl
This comment has been minimized.
This comment has been minimized.
ghost
requested review from
| // | ||
| // Failure to do this might lead to GH #14462 |
There was a problem hiding this comment.
| // | |
| // Failure to do this might lead to GH #14462 |
I respect that this is true, but also... git blame could probably tell you that, and I don't know that every time we make a change we need to decorate the code with signposts that say "this was a bug back when xyz" :)
There was a problem hiding this comment.
sorry missed this - FWIW I still add these out of habit. After being through 4 different source control trees, I find it's 1000% easier to leave the history for weird lines of code next to them in the source. Then, when one of these lines is inevitably refactored, we can see the comment moved/deleted and check the original bug for regressions, rather than manually git blameing every time we think a line of code comes from a weird bug report.
That's my 2c. LOC are cheap.
| Log::Comment(fmt::format(L"expectedAnchor:({},{})", expectedAnchor.x, expectedAnchor.y).c_str()); | ||
| Log::Comment(fmt::format(L"anchor:({},{})", anchor.x, anchor.y).c_str()); | ||
| Log::Comment(fmt::format(L"end:({},{})", end.x, end.y).c_str()); |
There was a problem hiding this comment.
this seems like it should be one really long fmt::format instead of three, but.. \_(shrug)_/
zadjii-msft
added
the
AutoMerge
|
Hello @zadjii-msft! Because this pull request has the 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 (
|
|
🎉 Handy links: |
ghost
mentioned this pull request
This builds upon #10749. When we added a separate pivot to track the "active" anchor, we forgot to update the pivot while circling. What does that mean? Moving the mouse would trigger us to update the selection using new endpoint and the old pivot, which hadn't been updated.
There's probably a more elegant way of doing this, but it's good enough.
Updated the test to cover this as well.
Closes #14462