-
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 some smaller broadcast bugs #15993
Conversation
// Should the text cursor be displayed, even when the control isn't focused? | ||
// n.b. "blur" is the opposite of "focus". | ||
bool TermControl::_displayCursorWhileBlurred() const noexcept | ||
{ | ||
return CursorVisibility() == Control::CursorDisplayState::Shown; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any value to keeping around _displayCursorWhileBlurred
? We could replace any call with this check tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think the function name is more descriptive than either of the two parts. "cursor visibility" for instance initially confused me because I though it controls the general visibility of the cursor but actually it controls whether it gets hidden on blur. The enum itself is also longer than the function name.
enum CursorDisplayState | ||
{ | ||
Default, | ||
Shown | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will there be more states or will this remain a boolean? In case of the latter, I think some code can be expressed more elegantly with a boolean like
control.CursorVisibility(
activated
? Microsoft::Terminal::Control::CursorDisplayState::Shown
: Microsoft::Terminal::Control::CursorDisplayState::Default
);
vs.
control.HideCursorOnBlur(!activated);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(a half real suggestion that is somewhat relevant) you could also use the Visibility from XAML, if you want.
EDIT: personally I think an enum is easier to read. I think the double negative in HideCursorOnBlur(!activated)
adds a little bit of mental energy to parse it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see... It's because I read it as "Hide Cursor On Blur if not activated".
I don't think the Visibility
enum is a good fit, since this setting is more like a "automatic visibility change when the window is unfocused". So it's a bit like both states simultaneously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, Default
is "the control gets to say if the cursor is visible or not", while Shown
is "Please always show your cursor".
When we "specced" this in Teams chat, we also left in a Hidden
state, for a consumer who wanted to always hide the cursor. We obv didn't need that here.
I've kinda weirdly gotten attached to binary enums as bool values, just cause then the arg literally says what the value means, rather than having to look up what true
/false
means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But would it make sense to still rename it a bit to make it more obvious what it means? We have more cursor visibility states than this one after all (like blinking visibility, IME visibility, VT visibility, etc.) and the naming clashes with those IMO.
I personally think that HideCursorOnBlur
improves on this as it ties the meaning of true/false to the function name. The function _displayCursorWhileBlurred
IMO shows this as it turns the enum back to a boolean, so why not have a DisplayCursorWhileBlurred
in the first place? (I'm mostly impartial about this though. Enums/Bools are both fine for me.)
For an enum I think that Always
instead of Shown
and HideOnBlur
instead of Default
would be easier to understand (different names are probably even better).
I'll approve it in the meantime, but maybe this is something to still noodle about. ^^
Summary of the Pull Request
Resolves the following in #15812
References and Relevant Issues
x-ref:
Detailed Description of the Pull Request / Additional comments
There was literally no logic in the original PR for starting the cursor blinking. It's entirely unknowable how that ever worked. This makes it all much more explicit.
We're taking the hacky
DisplayCursorWhileBlurred
from #15363, and promoting that to the less-hackyCursorVisibility
. Broadcast input mode can use that to force the cursor to be visible always.The last checkbox in that issue is harder, and I didn't want to further pollute this delta with the paste plumbing.