Skip to content

Fix some smaller broadcast bugs #15993

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.

Sign up for GitHub

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

Merged
Mike Griese (zadjii-msft) merged 6 commits into from
Sep 19, 2023
Merged

Fix some smaller broadcast bugs #15993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
49f967a
fix some smaller broadcast bugs
zadjii-msft Sep 5, 2023
53a6e73
I hate this but it does work
zadjii-msft Sep 5, 2023
07342f5
comments
zadjii-msft Sep 7, 2023
2d8aa85
Merge remote-tracking branch 'origin/main' into dev/migrie/b/15812-br…
zadjii-msft Sep 7, 2023
e66f904
Merge branch 'main' into dev/migrie/b/15812-broadcast-nits
zadjii-msft Sep 18, 2023
8cdd3f0
Merge remote-tracking branch 'origin/main' into dev/migrie/b/15812-br…
zadjii-msft Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/cascadia/TerminalApp/Pane.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2518,6 +2518,7 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitDirect
_profile = nullptr;
_control = { nullptr };
_firstChild->_isDefTermSession = _isDefTermSession;
_firstChild->_broadcastEnabled = _broadcastEnabled;
}

_splitState = actualSplitType;
Expand Down Expand Up @@ -3173,6 +3174,9 @@ void Pane::EnableBroadcast(bool enabled)
if (_IsLeaf())
{
_broadcastEnabled = enabled;
_control.CursorVisibility(enabled ?
CursorDisplayState::Shown :
CursorDisplayState::Default);
UpdateVisuals();
}
else
Expand Down
15 changes: 15 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4698,6 +4698,21 @@ namespace winrt::TerminalApp::implementation
// the settings, change active panes, etc.
_activated = activated;
_updateThemeColors();

if (const auto& tab{ _GetFocusedTabImpl() })
{
if (tab->TabStatus().IsInputBroadcastActive())
{
tab->GetRootPane()->WalkTree([activated](const auto& p) {
if (const auto& control{ p->GetTerminalControl() })
{
control.CursorVisibility(activated ?
Microsoft::Terminal::Control::CursorDisplayState::Shown :
Microsoft::Terminal::Control::CursorDisplayState::Default);
}
});
}
}
}

winrt::fire_and_forget TerminalPage::_ControlCompletionsChangedHandler(const IInspectable sender,
Expand Down
47 changes: 44 additions & 3 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1051,8 +1051,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// _cursorTimer doesn't exist, and it would never turn on the
// cursor. To mitigate, we'll initialize the cursor's 'on' state
// with `_focused` here.
_core.CursorOn(_focused || DisplayCursorWhileBlurred);
if (DisplayCursorWhileBlurred)
_core.CursorOn(_focused || _displayCursorWhileBlurred());
if (_displayCursorWhileBlurred())
{
_cursorTimer->Start();
}
Expand Down Expand Up @@ -1968,7 +1968,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
TSFInputControl().NotifyFocusLeave();
}

if (_cursorTimer && !DisplayCursorWhileBlurred)
if (_cursorTimer && !_displayCursorWhileBlurred())
{
_cursorTimer->Stop();
_core.CursorOn(false);
Expand Down Expand Up @@ -3660,4 +3660,45 @@ namespace winrt::Microsoft::Terminal::Control::implementation
SelectionContextMenu().Hide();
_core.ContextMenuSelectOutput();
}

// 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;
}
Comment on lines +3664 to +3669

@carlos-zamora Carlos Zamora (carlos-zamora) Sep 18, 2023

Copy link
Copy Markdown

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.

@lhecker Leonard Hecker (lhecker) Sep 19, 2023

Copy link
Copy Markdown
Member

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.

Control::CursorDisplayState TermControl::CursorVisibility() const noexcept
{
return _cursorVisibility;
}
void TermControl::CursorVisibility(Control::CursorDisplayState cursorVisibility)
{
_cursorVisibility = cursorVisibility;
if (!_initializedTerminal)
{
return;
}

if (_displayCursorWhileBlurred())
{
// If we should be ALWAYS displaying the cursor, turn it on and start blinking.
_core.CursorOn(true);
if (_cursorTimer.has_value())
{
_cursorTimer->Start();
}
}
else
{
// Otherwise, if we're unfocused, then turn the cursor off and stop
// blinking. (if we're focused, then we're already doing the right
// thing)
const auto focused = FocusState() != FocusState::Unfocused;
if (!focused && _cursorTimer.has_value())
{
_cursorTimer->Stop();
}
_core.CursorOn(focused);
}
}
}
9 changes: 6 additions & 3 deletions src/cascadia/TerminalControl/TermControl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
TerminalConnection::ITerminalConnection Connection();
void Connection(const TerminalConnection::ITerminalConnection& connection);

Control::CursorDisplayState CursorVisibility() const noexcept;
void CursorVisibility(Control::CursorDisplayState cursorVisibility);

// -------------------------------- WinRT Events ---------------------------------
// clang-format off
WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
Expand Down Expand Up @@ -194,9 +197,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation

WINRT_OBSERVABLE_PROPERTY(winrt::Windows::UI::Xaml::Media::Brush, BackgroundBrush, _PropertyChangedHandlers, nullptr);

public:
til::property<bool> DisplayCursorWhileBlurred{ false };

private:
friend struct TermControlT<TermControl>; // friend our parent so it can bind private event handlers

Expand Down Expand Up @@ -258,6 +258,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
Windows::Foundation::Collections::IObservableVector<Windows::UI::Xaml::Controls::ICommandBarElement> _originalSelectedPrimaryElements{ nullptr };
Windows::Foundation::Collections::IObservableVector<Windows::UI::Xaml::Controls::ICommandBarElement> _originalSelectedSecondaryElements{ nullptr };

Control::CursorDisplayState _cursorVisibility{ Control::CursorDisplayState::Default };

inline bool _IsClosing() const noexcept
{
#ifndef NDEBUG
Expand Down Expand Up @@ -376,6 +378,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

void _SelectCommandHandler(const IInspectable& sender, const IInspectable& args);
void _SelectOutputHandler(const IInspectable& sender, const IInspectable& args);
bool _displayCursorWhileBlurred() const noexcept;

struct Revokers
{
Expand Down
8 changes: 7 additions & 1 deletion src/cascadia/TerminalControl/TermControl.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ import "ControlCore.idl";
namespace Microsoft.Terminal.Control
{

enum CursorDisplayState
{
Default,
Shown
};
Comment on lines +15 to +19

@lhecker Leonard Hecker (lhecker) Sep 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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);

@carlos-zamora Carlos Zamora (carlos-zamora) Sep 18, 2023 •
edited
Loading

Copy link
Copy Markdown

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

@lhecker Leonard Hecker (lhecker) Sep 19, 2023

Copy link
Copy Markdown
Member

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?

@zadjii-msft Mike Griese (zadjii-msft) Sep 19, 2023

Copy link
Copy Markdown
Member Author

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.

@lhecker Leonard Hecker (lhecker) Sep 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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. ^^


[default_interface] runtimeclass TermControl : Windows.UI.Xaml.Controls.UserControl,
IDirectKeyListener,
IMouseWheelListener,
Expand Down Expand Up @@ -121,7 +127,7 @@ namespace Microsoft.Terminal.Control
// opacity set by the settings should call this instead.
Double BackgroundOpacity { get; };

Boolean DisplayCursorWhileBlurred;
CursorDisplayState CursorVisibility;

Windows.UI.Xaml.Media.Brush BackgroundBrush { get; };

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsEditor/Profiles_Appearance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
_previewControl = Control::TermControl(settings, settings, *_previewConnection);
_previewControl.IsEnabled(false);
_previewControl.AllowFocusWhenDisabled(false);
_previewControl.DisplayCursorWhileBlurred(true);
_previewControl.CursorVisibility(Microsoft::Terminal::Control::CursorDisplayState::Shown);
ControlPreview().Child(_previewControl);
}

Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/defaults.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@
{ "command": { "action": "swapPane", "direction": "previousInOrder"} },
{ "command": { "action": "swapPane", "direction": "nextInOrder"} },
{ "command": { "action": "swapPane", "direction": "first" } },
{ "command": "toggleBroadcastInput" },
{ "command": "togglePaneZoom" },
{ "command": "toggleSplitOrientation" },
{ "command": "toggleReadOnlyMode" },
Expand Down