-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Fix flipped sense in TerminalDispatch::CursorPosition #4113
Conversation
(thanks @j4james) I chose to flip the sense here instead of rewriting the function. Options include...
and
(this works because we have a |
I was going to say you actually shouldn't even need those If a parameter is greater than a short, it should be clamped rather failing, i.e. you should be able to use a sequence like And note that |
Ah dammit I was about to submit a PR myself after debugging this issue today. 😣 I agree with @j4james that values should be capped because such a lenient behavior is probably what you'd want anyways. From a cursory glance bool TerminalDispatch::CursorPosition(const size_t line,
const size_t column) noexcept
{
const auto clamp = [](const size_t val) noexcept -> short {
constexpr size_t min = 1;
constexpr size_t max = std::numeric_limits<short>::max();
return static_cast<short>(std::clamp(val, min, max) - min);
};
return _terminalApi.SetCursorPosition(clamp(column), clamp(line));
} (It's certainly not beautiful code, so please feel free to use it. 😄 A wrapper class with saturating arithmetic would surely be helpful here in the future.) |
@lhecker Note that the
Yeah, that's being discussed in #4013. In the sort term, though, adding the clamping back in the state machine would at least solve a lot of the problems. |
@j4james @lhecker those are both good suggestions, though I think I'd probably want to get this fix merged in ASAP, since the Terminal's pretty horribly broken without it. I'd probably just file a follow up with those suggestions personally, though I'd be curious to hear @DHowett-MSFT's thoughts on the matter. |
We'll talk about prioritizing #4013 for this week since it's becoming a problem and will support other work. |
We'll likely end up doing 4013 this week. |
This is an unblocker. |
Fixes #4107.