Skip to content
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

Jump to bottom

Indicate which pane is focused with the Accent color on the pan… #3060

Merged
merged 19 commits into from
Nov 1, 2019
Merged

Indicate which pane is focused with the Accent color on the pan… #3060

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c5e8361
start work on this by tracking border state
zadjii-msft Oct 3, 2019
a26b1c9
Colorize the border
zadjii-msft Oct 3, 2019
b078e9a
Use the accent color for highlighting
zadjii-msft Oct 3, 2019
f49e57f
Cleanup the accent color code
zadjii-msft Oct 3, 2019
c914f64
Don't buy any rural real estate when closing a pane
zadjii-msft Oct 3, 2019
d9d49d4
Closing panes works well now too
zadjii-msft Oct 3, 2019
79872c1
Cleanup for review
zadjii-msft Oct 4, 2019
3226db3
Update src/cascadia/TerminalApp/Pane.cpp
zadjii-msft Oct 4, 2019
127f8da
Merge branch 'master' into dev/migrie/f/994-pane-focus-borders
zadjii-msft Oct 25, 2019
dda2e29
Merge branch 'dev/migrie/f/994-pane-focus-borders' of https://github.…
zadjii-msft Oct 25, 2019
3fc14da
try some things that don't work to fix the resizing crash
zadjii-msft Oct 25, 2019
855f591
Revert "try some things that don't work to fix the resizing crash"
zadjii-msft Oct 25, 2019
fa244e2
this _does_ work, but I think it's not semantically correct
zadjii-msft Oct 28, 2019
2fd8323
This doesn't seem to work either.
zadjii-msft Oct 28, 2019
5997f05
Revert "This doesn't seem to work either."
zadjii-msft Oct 28, 2019
700e9c1
Merge the changes from the "this is the one" branch
zadjii-msft Oct 29, 2019
cb24cfe
Cleanup from my mess of a commit
zadjii-msft Oct 29, 2019
f39ab04
Other PR feedback from @carlos-zamora
zadjii-msft Oct 29, 2019
7951a1a
Fix a typo
zadjii-msft Nov 1, 2019
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
159 changes: 119 additions & 40 deletions src/cascadia/TerminalApp/Pane.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,28 @@
#include "Pane.h"

using namespace winrt::Windows::Foundation;
using namespace winrt::Windows::UI;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Core;
using namespace winrt::Windows::UI::Xaml::Media;
using namespace winrt::Microsoft::Terminal::Settings;
using namespace winrt::Microsoft::Terminal::TerminalControl;
using namespace winrt::TerminalApp;

static const int PaneSeparatorSize = 4;
static const int PaneBorderSize = 2;
static const int CombinedPaneBorderSize = 2 * PaneBorderSize;
static const float Half = 0.50f;

winrt::Windows::UI::Xaml::Media::SolidColorBrush Pane::s_focusedBorderBrush = { nullptr };

Pane::Pane(const GUID& profile, const TermControl& control, const bool lastFocused) :
_control{ control },
_lastFocused{ lastFocused },
_profile{ profile }
{
_root.Children().Append(_control);
_root.Children().Append(_border);
_border.Child(_control);

_connectionClosedToken = _control.ConnectionClosed({ this, &Pane::_ControlClosedHandler });

// Set the background of the pane to match that of the theme's default grid
Expand All @@ -37,6 +44,24 @@ Pane::Pane(const GUID& profile, const TermControl& control, const bool lastFocus
_root.Style(style);
}
}

if (s_focusedBorderBrush == nullptr)
{
const auto accentColorKey = winrt::box_value(L"SystemAccentColor");
if (res.HasKey(accentColorKey))
{
const auto thing = res.Lookup(accentColorKey);
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
// If SystemAccentColor is _not_ a Color for some reason, use
// Transparent as the color, so we don't do this process again on
// the next pane (by leaving s_focusedBorderBrush nullptr)
auto c = winrt::unbox_value_or<Color>(thing, Colors::Transparent());
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
s_focusedBorderBrush = SolidColorBrush(c);
}
else
{
s_focusedBorderBrush = SolidColorBrush{ Colors::Transparent() };
}
}
}

// Method Description:
Expand Down Expand Up @@ -108,7 +133,7 @@ bool Pane::_Resize(const Direction& direction)
// actualDimension is the size in DIPs of this pane in the direction we're
// resizing.
auto actualDimension = changeWidth ? actualSize.Width : actualSize.Height;
actualDimension -= PaneSeparatorSize;
actualDimension -= CombinedPaneBorderSize;

const auto firstMinSize = _firstChild->_GetMinSize();
const auto secondMinSize = _secondChild->_GetMinSize();
Expand Down Expand Up @@ -446,10 +471,13 @@ void Pane::UpdateFocus()
_control.FocusState() != FocusState::Unfocused;

_lastFocused = controlFocused;

_border.BorderBrush(_lastFocused ? s_focusedBorderBrush : nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qq: should this be when the pane is focused, or should it be tied to the control hosted inside the pane? Each UIElement/Control has a focus event that we could subscribe to, and the pane could handle updating its status when the control gains/loses focus

or will that be bad when we finally decouple active and focused?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO:
"when the pane is focused" == "active"
"when the control is focused" == "focused"

So I think the border should be there when the pane is active. We don't want the border to disappear when another UI element appears (e.g. the command palette). Also consider a pane where there are many (non-terminal) controls, any of which could be focused.

Ideally, all this will get resolved when we loop back on the active vs focused issue #1205

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes more sense for this to be on the pane than the control. Two scenarios come to mind:

  • if we support having something other than a TermControl, we can still apply the border
  • if we wanted to allow multiple panes to be "selected" for some reason, it still works

}
else
{
_lastFocused = false;

_firstChild->UpdateFocus();
_secondChild->UpdateFocus();
}
Expand Down Expand Up @@ -530,6 +558,13 @@ void Pane::_CloseChild(const bool closeFirst)
// If the only child left is a leaf, that means we're a leaf now.
if (remainingChild->_IsLeaf())
{
// When the remaining child is a leaf, that means both our childern were
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
// previously leaves, and the only difference in their borders is the
// border that we gave them. Take a logical AND of those two children to
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
// remove that border. Other borders the children might have, they
// inherited from us, so the flag will be set for both children.
_borders = _firstChild->_borders & _secondChild->_borders;
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved

// take the control and profile of the pane that _wasn't_ closed.
_control = remainingChild->_control;
_profile = remainingChild->_profile;
Expand All @@ -554,15 +589,19 @@ void Pane::_CloseChild(const bool closeFirst)
// re-attach the TermControl to our Grid.
_firstChild->_root.Children().Clear();
_secondChild->_root.Children().Clear();
_firstChild->_border.Child(nullptr);
_secondChild->_border.Child(nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should a pane just always have a border, and it's set to invisible when it doesn't need it? Instead of getting the border removed by its parent pane... it seems like we shouldn't be reaching into our childrens' visual trees..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a bad idea IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so looking into that one, that's not as bad. This case here is us removing the controls from the child's UI tree, because we're taking ownership of it. This is happening because one child closed, and we need to absorb the remaining child's state into ourselves.

When we first split, we do remove our own border, in order to add the two children directly to our root grid. Hypothetically, we could leave that border around always, but we'd need to add another grid to the tree always (since a Border can only ever have one child)


// Reset our UI:
_root.Children().Clear();
_border.Child(nullptr);
_root.ColumnDefinitions().Clear();
_root.RowDefinitions().Clear();
_separatorRoot = { nullptr };

// Reattach the TermControl to our grid.
_root.Children().Append(_control);
// _root.Children().Append(_control);
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
_root.Children().Append(_border);
_border.Child(_control);

if (_lastFocused)
{
Expand All @@ -571,12 +610,26 @@ void Pane::_CloseChild(const bool closeFirst)

_splitState = SplitState::None;

_UpdateBorders();

// Release our children.
_firstChild = nullptr;
_secondChild = nullptr;
}
else
{
// Determine which border flag we gave to the child when we first split
// it, so that we can take just that flag away from them.
Borders clearBorderFlag = Borders::None;
if (_splitState == SplitState::Horizontal)
{
clearBorderFlag = closeFirst ? Borders::Top : Borders::Bottom;
}
else if (_splitState == SplitState::Vertical)
{
clearBorderFlag = closeFirst ? Borders::Left : Borders::Right;
}

// First stash away references to the old panes and their tokens
const auto oldFirstToken = _firstClosedToken;
const auto oldSecondToken = _secondClosedToken;
Expand All @@ -585,7 +638,6 @@ void Pane::_CloseChild(const bool closeFirst)

// Steal all the state from our child
_splitState = remainingChild->_splitState;
_separatorRoot = remainingChild->_separatorRoot;
_firstChild = remainingChild->_firstChild;
_secondChild = remainingChild->_secondChild;

Expand All @@ -603,6 +655,7 @@ void Pane::_CloseChild(const bool closeFirst)

// Reset our UI:
_root.Children().Clear();
_border.Child(nullptr);
_root.ColumnDefinitions().Clear();
_root.RowDefinitions().Clear();

Expand All @@ -626,11 +679,19 @@ void Pane::_CloseChild(const bool closeFirst)
// Remove the child's UI elements from the child's grid, so we can
// attach them to us instead.
remainingChild->_root.Children().Clear();
remainingChild->_border.Child(nullptr);

_root.Children().Append(_firstChild->GetRootElement());
_root.Children().Append(_separatorRoot);
_root.Children().Append(_secondChild->GetRootElement());

// Take the flag away from the children that they inherited from their
// parent, and update their borders to visually match
WI_ClearAllFlags(_firstChild->_borders, clearBorderFlag);
WI_ClearAllFlags(_secondChild->_borders, clearBorderFlag);
_UpdateBorders();
_firstChild->_UpdateBorders();
_secondChild->_UpdateBorders();

// If the closed child was focused, transfer the focus to it's first sibling.
if (closedChild->_lastFocused)
{
Expand All @@ -640,7 +701,6 @@ void Pane::_CloseChild(const bool closeFirst)
// Release the pointers that the child was holding.
remainingChild->_firstChild = nullptr;
remainingChild->_secondChild = nullptr;
remainingChild->_separatorRoot = { nullptr };
}
}

Expand Down Expand Up @@ -682,10 +742,7 @@ void Pane::_CreateRowColDefinitions(const Size& rootSize)
{
_root.ColumnDefinitions().Clear();

// Create three columns in this grid: one for each pane, and one for the separator.
auto separatorColDef = Controls::ColumnDefinition();
separatorColDef.Width(GridLengthHelper::Auto());

// Create two columns in this grid: one for each pane
const auto paneSizes = _GetPaneSizes(rootSize.Width);

auto firstColDef = Controls::ColumnDefinition();
Expand All @@ -695,17 +752,13 @@ void Pane::_CreateRowColDefinitions(const Size& rootSize)
secondColDef.Width(GridLengthHelper::FromPixels(paneSizes.second));

_root.ColumnDefinitions().Append(firstColDef);
_root.ColumnDefinitions().Append(separatorColDef);
_root.ColumnDefinitions().Append(secondColDef);
}
else if (_splitState == SplitState::Horizontal)
{
_root.RowDefinitions().Clear();

// Create three rows in this grid: one for each pane, and one for the separator.
auto separatorRowDef = Controls::RowDefinition();
separatorRowDef.Height(GridLengthHelper::Auto());

// Create two rows in this grid: one for each pane
const auto paneSizes = _GetPaneSizes(rootSize.Height);

auto firstRowDef = Controls::RowDefinition();
Expand All @@ -715,7 +768,6 @@ void Pane::_CreateRowColDefinitions(const Size& rootSize)
secondRowDef.Height(GridLengthHelper::FromPixels(paneSizes.second));

_root.RowDefinitions().Append(firstRowDef);
_root.RowDefinitions().Append(separatorRowDef);
_root.RowDefinitions().Append(secondRowDef);
}
}
Expand All @@ -734,23 +786,36 @@ void Pane::_CreateSplitContent()
gsl::narrow_cast<float>(_root.ActualHeight()) };

_CreateRowColDefinitions(actualSize);
}

if (_splitState == SplitState::Vertical)
// Method Description:
// - Sets the thickness of each side of our borders to match our _borders state.
// Arguments:
// - <none>
// Return Value:
// - <none>
void Pane::_UpdateBorders()
{
double top = 0, bottom = 0, left = 0, right = 0;

Thickness newBorders{ 0 };
if (WI_IsFlagSet(_borders, Borders::Top))
{
// Create the pane separator
_separatorRoot = Controls::Grid{};
_separatorRoot.Width(PaneSeparatorSize);
// NaN is the special value XAML uses for "Auto" sizing.
_separatorRoot.Height(NAN);
top = PaneBorderSize;
}
else if (_splitState == SplitState::Horizontal)
if (WI_IsFlagSet(_borders, Borders::Bottom))
{
bottom = PaneBorderSize;
}
if (WI_IsFlagSet(_borders, Borders::Left))
{
left = PaneBorderSize;
}
if (WI_IsFlagSet(_borders, Borders::Right))
{
// Create the pane separator
_separatorRoot = Controls::Grid{};
_separatorRoot.Height(PaneSeparatorSize);
// NaN is the special value XAML uses for "Auto" sizing.
_separatorRoot.Width(NAN);
right = PaneBorderSize;
}
_border.BorderThickness(ThicknessHelper::FromLengths(left, top, right, bottom));
}

// Method Description:
Expand All @@ -764,14 +829,28 @@ void Pane::_ApplySplitDefinitions()
if (_splitState == SplitState::Vertical)
{
Controls::Grid::SetColumn(_firstChild->GetRootElement(), 0);
Controls::Grid::SetColumn(_separatorRoot, 1);
Controls::Grid::SetColumn(_secondChild->GetRootElement(), 2);
Controls::Grid::SetColumn(_secondChild->GetRootElement(), 1);

_firstChild->_borders = _borders | Borders::Right;
_secondChild->_borders = _borders | Borders::Left;
_borders = Borders::None;

_UpdateBorders();
_firstChild->_UpdateBorders();
_secondChild->_UpdateBorders();
}
else if (_splitState == SplitState::Horizontal)
{
Controls::Grid::SetRow(_firstChild->GetRootElement(), 0);
Controls::Grid::SetRow(_separatorRoot, 1);
Controls::Grid::SetRow(_secondChild->GetRootElement(), 2);
Controls::Grid::SetRow(_secondChild->GetRootElement(), 1);

_firstChild->_borders = _borders | Borders::Bottom;
_secondChild->_borders = _borders | Borders::Top;
_borders = Borders::None;

_UpdateBorders();
_firstChild->_UpdateBorders();
_secondChild->_UpdateBorders();
}
}

Expand Down Expand Up @@ -845,15 +924,15 @@ bool Pane::_CanSplit(SplitState splitType)

if (splitType == SplitState::Vertical)
{
const auto widthMinusSeparator = actualSize.Width - PaneSeparatorSize;
const auto widthMinusSeparator = actualSize.Width - CombinedPaneBorderSize;
const auto newWidth = widthMinusSeparator * Half;

return newWidth > minSize.Width;
}

if (splitType == SplitState::Horizontal)
{
const auto heightMinusSeparator = actualSize.Height - PaneSeparatorSize;
const auto heightMinusSeparator = actualSize.Height - CombinedPaneBorderSize;
const auto newHeight = heightMinusSeparator * Half;

return newHeight > minSize.Height;
Expand Down Expand Up @@ -891,6 +970,7 @@ void Pane::_Split(SplitState splitType, const GUID& profile, const TermControl&
// Remove any children we currently have. We can't add the existing
// TermControl to a new grid until we do this.
_root.Children().Clear();
_border.Child(nullptr);

// Create two new Panes
// Move our control, guid into the first one.
Expand All @@ -901,7 +981,6 @@ void Pane::_Split(SplitState splitType, const GUID& profile, const TermControl&
_secondChild = std::make_shared<Pane>(profile, control);

_root.Children().Append(_firstChild->GetRootElement());
_root.Children().Append(_separatorRoot);
_root.Children().Append(_secondChild->GetRootElement());

_ApplySplitDefinitions();
Expand Down Expand Up @@ -929,7 +1008,7 @@ std::pair<float, float> Pane::_GetPaneSizes(const float& fullSize)
THROW_HR(E_FAIL);
}

const auto sizeMinusSeparator = fullSize - PaneSeparatorSize;
const auto sizeMinusSeparator = fullSize;
const auto firstSize = sizeMinusSeparator * _firstPercent.value();
const auto secondSize = sizeMinusSeparator * _secondPercent.value();
return { firstSize, secondSize };
Expand All @@ -953,8 +1032,8 @@ Size Pane::_GetMinSize() const

const auto firstSize = _firstChild->_GetMinSize();
const auto secondSize = _secondChild->_GetMinSize();
const auto newWidth = firstSize.Width + secondSize.Width + (_splitState == SplitState::Vertical ? PaneSeparatorSize : 0);
const auto newHeight = firstSize.Height + secondSize.Height + (_splitState == SplitState::Horizontal ? PaneSeparatorSize : 0);
const auto newWidth = firstSize.Width + secondSize.Width + (_splitState == SplitState::Vertical ? CombinedPaneBorderSize : 0);
const auto newHeight = firstSize.Height + secondSize.Height + (_splitState == SplitState::Horizontal ? CombinedPaneBorderSize : 0);
return { newWidth, newHeight };
}

Expand Down
Loading