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

zadjii-msft · 2019-10-04T16:20:44Z

Summary of the Pull Request

Adds a small border with the accent color to indicate a pane is focused

PR Checklist

Closes Add a UI to indicate a pane is focused. #994
I work here
[😢] Tests added/passed
[n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

I've removed the simple Grid we were using as the pane separator, and replaced it with a Border that might appear on any side of a pane.

When we add a split, we'll create each child with one of the Border flags set (each child with one of a pair of flags). E.g. creating a horizontal split creates one child with the Top border, and another with the Bottom.

Then, if one of those panes is split, it will pass it's border flag to is new children, with the additional flag set. So adding another Vertical split to the above scenario would create a set of panes with either (Top|Left, Top|Right) or (Bottom|Left, Bottom|Right) borders set, depending on which pane was split.

Validation Steps Performed

Some playing with it, but honestly it needs more for me to be totally confident.

zadjii-msft · 2019-10-04T16:28:13Z

(I don't intend to get this in before I leave on vacation)

src/cascadia/TerminalApp/Pane.cpp

DHowett-MSFT · 2019-10-04T20:53:32Z

I bet there's a way to bind this that doesn't require us to propagate bitfields of border sizes down, down, down.

mcpiroman · 2019-10-05T14:04:45Z

On your screenshots, under the window there is thin strip of what appears to be accent color. Is this right?

Secondly, this could be (optionally) done tmux-style, with highlight on separators rather than borders. This would save half a space of a now-quite-thick border. While borders can be shrinked right now, it would make the highlight less visible as it only covers half of the border.

src/cascadia/TerminalApp/Pane.h

src/cascadia/TerminalApp/Pane.cpp

…com/Microsoft/Terminal into dev/migrie/f/994-pane-focus-borders

This reverts commit 3fc14da.

I tried adding the pane seperators to the Pane::_GetMinWidth calculation. That works for prevent the crash, but the resizing is wonky now. If you add a Vertical split, then a second, then resize the middle pane really small, you'll see that the _last_ resize doesn't work properly. The text seems to overhand into the border. Additionally, there's really weird behavior resizing panes to be small. They don't always seem to be resizable to the smallest size.

This reverts commit 2fd8323.

Again, no idea what I really did that worked, but it does

This makes so much more sense now

DHowett-MSFT

I'm still a bit confused about the bitfield for borders. Why do we need to keep track of [who has a left, top, bottom, right] when we could just say [the leafmost focused pane has all four, and any other panes have zero of them]? Would our UI look strange or wrong that way?

zadjii-msft · 2019-10-29T18:24:36Z

@DHowett-MSFT and I resolved the discussion offline - there was some confusion on how I implemented this. Panes only have borders on the sides that are adjacent to other panes. Case in point:

The bottom-right pane only have top and left borders, so when it's focused, only those sides are highlighted.

DHowett-MSFT · 2019-10-29T21:27:04Z

src/cascadia/TerminalApp/Pane.cpp

@@ -446,10 +475,13 @@ void Pane::UpdateFocus()
                                    _control.FocusState() != FocusState::Unfocused;

        _lastFocused = controlFocused;
+
+        _border.BorderBrush(_lastFocused ? s_focusedBorderBrush : nullptr);


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?

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

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

src/cascadia/TerminalApp/Pane.cpp

DHowett-MSFT · 2019-10-29T21:29:19Z

src/cascadia/TerminalApp/Pane.cpp

@@ -554,15 +593,18 @@ 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);


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

This is not a bad idea IMO.

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)

carlos-zamora · 2019-10-31T22:13:33Z

src/cascadia/TerminalApp/Pane.cpp

@@ -446,10 +475,13 @@ void Pane::UpdateFocus()
                                    _control.FocusState() != FocusState::Unfocused;

        _lastFocused = controlFocused;
+
+        _border.BorderBrush(_lastFocused ? s_focusedBorderBrush : nullptr);


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

miniksa

This looks fine to me, but I'm not going to merge it because there are a few Dustin comments outstanding.

zadjii-msft · 2019-11-01T20:06:56Z

(I spoke with Dustin - he's fine with this, his comments were answered)

ghost · 2019-11-26T17:28:30Z

🎉Windows Terminal Preview v0.7.3291.0 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft added 7 commits October 3, 2019 09:51

start work on this by tracking border state

c5e8361

Colorize the border

a26b1c9

Use the accent color for highlighting

b078e9a

Cleanup the accent color code

f49e57f

Don't buy any rural real estate when closing a pane

c914f64

Closing panes works well now too

d9d49d4

Cleanup for review

79872c1

zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Oct 4, 2019

This was referenced Oct 4, 2019

Add a setting to manually set the Pane highlight color #3061

Open

Scenario: Add support for panes #1000

Open

zadjii-msft mentioned this pull request Oct 4, 2019

Add a setting to manually set the Pane border width #3063

Open

DHowett-MSFT reviewed Oct 4, 2019

View reviewed changes

src/cascadia/TerminalApp/Pane.cpp Show resolved Hide resolved

zadjii-msft commented Oct 4, 2019

View reviewed changes

src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved

Update src/cascadia/TerminalApp/Pane.cpp

3226db3

carlos-zamora reviewed Oct 8, 2019

View reviewed changes

src/cascadia/TerminalApp/Pane.h Show resolved Hide resolved

src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved

mcpiroman mentioned this pull request Oct 13, 2019

Snap to character grid when resizing window #3181

Merged

5 tasks

zadjii-msft added 10 commits October 25, 2019 11:54

Merge branch 'master' into dev/migrie/f/994-pane-focus-borders

127f8da

Merge branch 'dev/migrie/f/994-pane-focus-borders' of https://github.…

dda2e29

…com/Microsoft/Terminal into dev/migrie/f/994-pane-focus-borders

try some things that don't work to fix the resizing crash

3fc14da

Revert "try some things that don't work to fix the resizing crash"

855f591

This reverts commit 3fc14da.

this _does_ work, but I think it's not semantically correct

fa244e2

Revert "This doesn't seem to work either."

5997f05

This reverts commit 2fd8323.

Merge the changes from the "this is the one" branch

700e9c1

Again, no idea what I really did that worked, but it does

Cleanup from my mess of a commit

cb24cfe

This makes so much more sense now

Other PR feedback from @carlos-zamora

f39ab04

zadjii-msft requested review from miniksa, DHowett-MSFT and carlos-zamora October 29, 2019 14:49

DHowett-MSFT reviewed Oct 29, 2019

View reviewed changes

carlos-zamora approved these changes Oct 31, 2019

View reviewed changes

Fix a typo

7951a1a

miniksa approved these changes Nov 1, 2019

View reviewed changes

zadjii-msft changed the title ~~Indicate which pane is focused with the Accent color on the pane border~~ Indicate which pane is focused with the Accent color on the pan… Nov 1, 2019

zadjii-msft merged commit 64943bd into master Nov 1, 2019

ghost mentioned this pull request Nov 26, 2019

Add a UI to indicate a pane is focused. #994

Closed

Studio384 mentioned this pull request Nov 26, 2019

Add option for more subtle pane focus border #3708

Open

skyline131313 mentioned this pull request Nov 27, 2019

Accent Color Should Observe Windows Dark Settings #3732

Closed

mcpiroman mentioned this pull request Dec 27, 2019

Split Pane class into LeafPane and ParentPane + small fixes #4068

Closed

5 tasks

zadjii-msft deleted the dev/migrie/f/994-pane-focus-borders branch January 31, 2020 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zadjii-msft commented Oct 4, 2019

zadjii-msft commented Oct 4, 2019

DHowett-MSFT commented Oct 4, 2019

mcpiroman commented Oct 5, 2019 •

edited

Loading

DHowett-MSFT left a comment

zadjii-msft commented Oct 29, 2019

DHowett-MSFT Oct 29, 2019

zadjii-msft Oct 31, 2019

carlos-zamora Oct 31, 2019

DHowett-MSFT Oct 29, 2019

zadjii-msft Oct 31, 2019

zadjii-msft Nov 1, 2019

carlos-zamora Oct 31, 2019

miniksa left a comment

zadjii-msft commented Nov 1, 2019

ghost commented Nov 26, 2019

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

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

Conversation

zadjii-msft commented Oct 4, 2019

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

zadjii-msft commented Oct 4, 2019

DHowett-MSFT commented Oct 4, 2019

mcpiroman commented Oct 5, 2019 • edited Loading

DHowett-MSFT left a comment

Choose a reason for hiding this comment

zadjii-msft commented Oct 29, 2019

DHowett-MSFT Oct 29, 2019

Choose a reason for hiding this comment

zadjii-msft Oct 31, 2019

Choose a reason for hiding this comment

carlos-zamora Oct 31, 2019

Choose a reason for hiding this comment

DHowett-MSFT Oct 29, 2019

Choose a reason for hiding this comment

zadjii-msft Oct 31, 2019

Choose a reason for hiding this comment

zadjii-msft Nov 1, 2019

Choose a reason for hiding this comment

carlos-zamora Oct 31, 2019

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

zadjii-msft commented Nov 1, 2019

ghost commented Nov 26, 2019

mcpiroman commented Oct 5, 2019 •

edited

Loading