Make sure caption controls "dim out" when window loses NC focus

[AnuthaDev]

Hello frands, I gib smol code

Summary

Make the caption controls "dim out" when window loses focus

References

https://docs.microsoft.com/en-us/windows/win32/winmsg/wm-ncactivate
#5881
#3025

PR Checklist

• Applies to Epic: Fix remaining issues with non-client drawing #1625
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Requires documentation to be updated
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Added a handler for WM_NCACTIVATE to propagate the focus change to the Titlebar Control. I believe the current implementation using VisualStateManager is Naïve and it would be great if somebody could tell me how to improve it 😊.

The unfocused color is currently set to red because I haven't figured out the correct color to use, I will update it soon :)

Validation Steps Performed

None
Typed code on phone, PC is too weak to compile the whole project and I'm too lazy to make a relatively small POC, so I don't know if it even works!

Also, My combined knowledge of c++ and xaml is still miniscule, so please have mercy😅😅

[DHowett]

YAY

[AnuthaDev]

Okay, some improvement I guess...

[github-actions]

New misspellings found, please review:

• NCACTIVATE

To accept these changes, run the following commands

perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"atg BKMK consoleaccessibility FFF hyperlink NRCS remoting SCS shobjidl xab xb xbc xca xce xdd xffff "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/b470a63bc79d420727aefbbf45c955f592e0edc6.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"NCACTIVATE nrcs Remoting Scs Shobjidl "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/b470a63bc79d420727aefbbf45c955f592e0edc6.txt is added to your repository...'

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

• ... misspelled, then please correct them instead of using the command.
• ... names, please add them to .github/actions/spell-check/dictionary/names.txt.
• ... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
• ... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
• ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

[AnuthaDev]

If only the CI could tell me what exactly is wrong with the formatting, I wouldn't need to run the code format from my PC 😑😑

ktLint and detekt on android are much helpful in this regard

[AnuthaDev]

Okay, at least the build is fine now

[beviu]

It's amazing that you're able to type the code on a phone. This is what you need to change for the formatting:

diff --git a/src/cascadia/TerminalApp/MinMaxCloseControl.cpp b/src/cascadia/TerminalApp/MinMaxCloseControl.cpp
index 074c1e5b..9eda3f8f 100644
--- a/src/cascadia/TerminalApp/MinMaxCloseControl.cpp
+++ b/src/cascadia/TerminalApp/MinMaxCloseControl.cpp
@@ -84,9 +84,7 @@ namespace winrt::TerminalApp::implementation
             VisualStateManager::GoToState(CloseButton(), L"WindowStateUnfocused", false);
             break;
 
-
-
-        case   WindowVisualState::WindowVisualStateFocused:
+        case WindowVisualState::WindowVisualStateFocused:
         case WindowVisualState::WindowVisualStateNormal:
         case WindowVisualState::WindowVisualStateIconified:
         default:
diff --git a/src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp b/src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp
index 94d13f9a..8c5cf432 100644
--- a/src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp
+++ b/src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp
@@ -515,32 +515,31 @@ int NonClientIslandWindow::_GetResizeHandleHeight() const noexcept
     return 0;
 }
 
-
 // Method Description:
 // - Responds to the WM_NCACTIVATE message by changing the focus state of
 //   the window.
 [[nodiscard]] LRESULT NonClientIslandWindow::_OnFocusChanged(const WPARAM wParam, const LPARAM lParam) noexcept
 {
-	const auto windowStyle = GetWindowStyle(_window.get());
+    const auto windowStyle = GetWindowStyle(_window.get());
     const auto isIconified = WI_IsFlagSet(windowStyle, WS_ICONIC);
-    
+
     if (isIconified)
     {
-    	return DefWindowProc(_window.get(), WM_NCACTIVATE, wParam, lParam);
+        return DefWindowProc(_window.get(), WM_NCACTIVATE, wParam, lParam);
     }
-    
+
     if (_titlebar)
     {
         _isFocused = wParam;
         const auto state = _isFocused ? winrt::TerminalApp::WindowVisualState::WindowVisualStateFocused :
-                                          winrt::TerminalApp::WindowVisualState::WindowVisualStateUnfocused;
+                                        winrt::TerminalApp::WindowVisualState::WindowVisualStateUnfocused;
         try
         {
             _titlebar.SetWindowVisualState(state);
         }
         CATCH_LOG();
     }
-    
+
     return TRUE;
 }

I am wondering about some things:

• Shouldn't we use properties (something like CanMaximize and WindowHasFocus) on the TitlebarControl and MinMaxCloseControl instead of using functions to change the state? I don't write a lot of XAML so I don't know but to me it looks like using properties make more sense here.
• I have a branch (master...greg904:titlebar-ui, warning: there are a lot of many unrelated changes there because it was to experiment with stuff) where I tried changing the title bar color when the window has focus, which is related to this and I have a bug where when I press the mouse left button on the title bar, without moving the mouse, the colors change a fraction of a second after the window gets focus and it's borders change. You might have the same bug because I think I did it the same was as you. I don't know how to fix it though ☹.

[AnuthaDev]

Thanks @greg904 I did the changes but its still not passing, It would be great if you could suggest changes again🙏🙏

1: Yeah, I mentioned it already in the PR that there should be a better implementation, if you have the same code thrice, you are doing it wrong(at least that's what I believe)

2: If I remember correctly, there was an issue (Edit: microsoft/microsoft-ui-xaml#759) that the xaml island window was resizing slower than the parent window, it might be related to that. I guess that is just a (performance?) limitation of xaml islands ATM

[beviu]

You forgot to remove the two lines before case WindowVisualState::WindowVisualStateFocused.

[AnuthaDev]

It's amazing that you are able to type the code on a phone

Termux 😍

[AnuthaDev]

You forgot to remove the two lines before case WindowVisualState::WindowVisualStateFocused.

Oh Well, forgot I sure did😅😅

[AnuthaDev]

Yay! Format passed!!!🎉🎊🎇🎆🧨🎊🎉

Thank you very much @greg904 !

[AnuthaDev]

Here are ideas for after this PR is merged:

1. Change titlebar color on focus change
2. Change the font color in tabs on focus change
3. Change the tab background color on focus change

I am not touching these in this PR as people are really picky about the color of those things, especially the titlebar color (me want acrylic🙏) and it would be better to do it as part of the xaml theming or something like: #6491

[AnuthaDev]

@zadjii-msft Does this work?😅

[zadjii-msft]

Immediate thoughts after testing out the branch - if you mouse-over any of the caption buttons, the color will reset to the "focused" color, and then moving the mouse out of the window leaves the color as the focused color, not the unfocused color.

Otherwise, this seems to work pretty well, certainly better than any code I could have written on my phone 😅

[DHowett]

So, this is a really great start. Thank you for doing it!

I also played around with it a bit, and it looks like when you're maximized and switch the window into and out of focus the controls go back to their non-maximized states.

I think we need to separate the focus state out as a separate boolean, and offer a Focused setter method on the idl (and therefore on the class itself).

I'd probably go for...

Boolean Focused { get; set; };

which will require two functions in the header/source:

bool Focused() const;
void Focused(bool focused);

I'm also a little bit concerned about the use of Red as the inactive color 😄
Would you mind testing this under High Contrast mode, too?

Very impressive what you've done from your phone!

[AnuthaDev]

I also played around with it a bit, and it looks like when you're maximized and switch the window into and out of focus the controls go back to their non-maximized states.

Yup, that's because focus case falls through to normal rn

I think we need to separate the focus state out as a separate boolean, and offer a Focused setter method on the idl (and therefore on the class itself).

I'd probably go for...

Boolean Focused { get; set; };

which will require two functions in the header/source:

bool Focused() const;
void Focused(bool focused);

Hmm, okay I'll try to do it

Would you mind testing this under High Contrast mode, too?

I would've, if I could've, spent 5 hours trying to get it to compile on my potato pc, achieved nothing😭. Maybe if a successful x64 build in azure pipelines outputs an msix, I can test it....

Very impressive what you've done from your phone!

Thanks😊

[AnuthaDev]

Immediate thoughts after testing out the branch - if you mouse-over any of the caption buttons, the color will reset to the "focused" color, and then moving the mouse out of the window leaves the color as the focused color, not the unfocused color.

Yeah, I will separate focus from VisualStates, that should fix it.

Otherwise, this seems to work pretty well, certainly better than any code I could have written on my phone 😅

😊

[AnuthaDev]

@DHowett Okay, Even if I were to separate it out as a boolean, we would still need the VSM code right? Here's how I think it should be right now:
In the Focused(bool focus) method, check if we are windowed, if yes,then set all buttons to normal state, if no, set all buttons to maximised state, if we are unfocused, change the path stroke of all buttons to unfocused color.(after reading this again, would this even work? Lol, I got reading to do...)

Okay, second attempt:
In Focused(bool focus) method, change the required brushes manually...

So, we wouldn't need another visualstategroup of focus states and the code should integrate nicely with the existing code, and should prevent the problems that @zadjii-msft is facing.

What do you think??🤔🤔

[DHowett]

I think we do need a separate group, for the same reason as we need a separate field in the class! Otherwise, the XAML runtime won’t be able to tell them apart: it would think that because “focused” and “maximized” are in the same group, that it has to turn off the Maximized styles to enable the Focused styles!

State grouping prevents this by marking only options inside the same group as being exclusive.

[AnuthaDev]

I think we do need a separate group, for the same reason as we need a separate field in the class! Otherwise, the XAML runtime won’t be able to tell them apart: it would think that because “focused” and “maximized” are in the same group, that it has to turn off the Maximized styles to enable the Focused styles!

State grouping prevents this by marking only options inside the same group as being exclusive.

Okay, okay cool. Let me just write the code and I'll get back to you...

[AnuthaDev]

I have separated the focused case from the others. I think this should work now. There's probably not really a need of a separate boolean value if this works.... Please test🙏

[WSLUser]

Can we use the existing gray you see on an unfocused tab? (The focused one is white by default).

[DHowett]

I'm not kidding: we need a separate boolean for this.

The problem with putting them in the WindowVisualState is this:

There are five options in WindowVisualState

• Normal
• Maximized
• Iconified
• Focused
• Unfocused

It is impossible to have two of them turned on, because it is a normal enumerator. This means that, no matter what you do, you cannot have a "Maximized Focused" window, you would have to choose just one of those things.

This is, unfortunately, how enum types work. There can only be one active value, unless you do explicit work to make it support multiple values. However, I don't think that that work is worth doing here. 😄

If you introduce another boolean, you now have TWO SEPARATE option sets.

Window State

• Max
• Icon
• Normal

Window Focus

• Yes
• No

Now, you don't need to choose just one thing. You can choose one of EACH.

[DHowett]

Also, it looks like the inactive state is still set to Red 😄

[AnuthaDev]

This is, unfortunately, how enum types work. There can only be one active value, unless you do explicit work to make it support multiple values.

Well, TIL I am stupid. Sorry, for wasting your time.

Fixing code soon™

[ghost]

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.