Skip to content

Conversation

Techypanda
Copy link
Contributor

Summary of the Pull Request

This commit adds a new setting in which you can configure the active highlight color of panes as well as inactive and broadcast. This will only happen if configured otherwise uses the default as suggested by #3061

References and Relevant Issues

#3061

Detailed Description of the Pull Request / Additional comments

When modifying the settings you can now set pane key to have the following properties:

  • "activeBorderColor"
  • "inactiveBorderColor"
  • "broadcastBorderColor"
    This allows for more configuration of the windows terminal

Validation Steps Performed

Built Locally, Ran Locally and configured a profile with the following theme:

"themes": 
[
    {
        "name": "Under Construction",
        "tab": {
            "background": "#FFFF00FF",
            "iconStyle": "default",
            "showCloseButton": "always",
            "unfocusedBackground": "#88440088"
        },
        "tabRow": {
            "background": "#FF8800FF",
            "unfocusedBackground": "#202020FF"
        },
        "window": {
            "applicationTheme": "light",
            "experimental.rainbowFrame": false,
            "frame": null,
            "unfocusedFrame": null,
            "useMica": true
        },
        "pane": {
            "activeBorderColor": "#13A10E",
            "inactiveBorderColor": "#61D6D6"
        }
    }
]

PR Checklist

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-Settings Issues related to settings and customizability, for console or terminal Area-Theming Anything related to the theming of elements of the window Product-Terminal The new Windows Terminal. labels May 27, 2025

This comment has been minimized.

@Techypanda
Copy link
Contributor Author

@microsoft-github-policy-service agree

@Techypanda Techypanda force-pushed the pane-highlight-setting branch 2 times, most recently from 97b01e4 to b660e34 Compare May 29, 2025 03:59
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Downloaded the branch and played around with it for a bit. Looks and feels great!

Blocking mainly for the issue where "accent" and "terminalBackground" aren't handled properly.

Also don't forget to update the docs repo please

Thanks!

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 3, 2025
@Techypanda Techypanda force-pushed the pane-highlight-setting branch from b660e34 to ce1ba03 Compare June 4, 2025 04:53
@Techypanda
Copy link
Contributor Author

Downloaded the branch and played around with it for a bit. Looks and feels great!

Blocking mainly for the issue where "accent" and "terminalBackground" aren't handled properly.

Fixed in latest push can you retest when available? Thanks

Also don't forget to update the docs repo please

Can update the docs repo, will raise a PR there

@Techypanda Techypanda requested a review from carlos-zamora June 4, 2025 04:56
carlos-zamora added a commit that referenced this pull request Jun 10, 2025
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Setting all the pane's border colors to "accent" still doesn't work:

            "pane":
            {
                "activeBorderColor": "accent",
                "inactiveBorderColor": "accent",
                "broadcastBorderColor": "accent"
            },

Comment on lines 4692 to 4698
const auto paneActiveBorderColor = theme.Pane() ? theme.Pane().ActiveBorderColor() : nullptr;
const auto paneInactiveBorderColor = theme.Pane() ? theme.Pane().InactiveBorderColor() : nullptr;
const auto broadcastBorderColor = theme.Pane() ? theme.Pane().BroadcastBorderColor() : nullptr;
const auto requestedTheme{ theme.RequestedTheme() };

{
_updatePaneResources(requestedTheme);
_updatePaneResources(requestedTheme, paneActiveBorderColor, paneInactiveBorderColor, broadcastBorderColor);
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 you don't actually need the variables. In MTSMSettings.h, the last parameter in the macro is nullptr, so if the setting isn't defined, the getter should already retrieve nullptr. You should be able to pass theme.Pane().ActiveBorderColor() and the other ones directly into _updatePaneResources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tested this by removing the pane key from my json and reloading: crashed as theme.Pane() was null; I need the ternary or it goes nullptr.ActiveBorderColor() which crashes it (i could be wrong).

Will try a way that im thinking to clean this up but i dont believe i can pass this directly as you suggest unless im mistaken

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 11, 2025
@Techypanda Techypanda force-pushed the pane-highlight-setting branch from ce1ba03 to f8202e2 Compare June 16, 2025 10:41
@Techypanda Techypanda marked this pull request as draft June 16, 2025 10:55
@Techypanda Techypanda force-pushed the pane-highlight-setting branch from f8202e2 to 320267c Compare June 16, 2025 11:34
@Techypanda Techypanda marked this pull request as ready for review June 16, 2025 11:37
@Techypanda
Copy link
Contributor Author

Found the bug with accent, it was because i wasnt picking accent key sorted that and all 3 being set to accent seems to work
Also found the bug with terminalbackground, swapped to use Evaluate with backgroundBrush as TerminalTab uses (also fed this in and raised where this is used outside the updatePane method).

@Techypanda Techypanda requested a review from carlos-zamora June 16, 2025 11:38
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Tested the branch out and cycled between combinations of...

  • toggling broadcast input mode
  • setting one of the pane.XBorderColor properties to accent vs an rgb value

Looks and feels great! Same with the code! Thank you so much for doing this! 😊

Sorry for the delay 😅. I was OOF on vacation.

@carlos-zamora
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Techypanda
Copy link
Contributor Author

Documentation PR - MicrosoftDocs/terminal#872

This commit adds a new setting in which you can configure the active highlight color of panes aswell as inactive and broadcast.
@Techypanda Techypanda force-pushed the pane-highlight-setting branch from 320267c to 7573799 Compare July 17, 2025 00:54
@Techypanda
Copy link
Contributor Author

Rebased & Ran Invoke-CodeFormat as i believe the failures were due to this

@Techypanda Techypanda requested a review from carlos-zamora July 17, 2025 00:55
@lhecker
Copy link
Member

lhecker commented Jul 21, 2025

@Techypanda If possible, please avoid force-pushing. Now we have to review the PR from scratch. 🙂

@carlos-zamora
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

(Sorry - i'm not blocking this one because of you, I just wanted to make sure I got a chance to review it :))

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 21, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jul 28, 2025
@carlos-zamora carlos-zamora reopened this Aug 5, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Aug 5, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Aug 12, 2025
@carlos-zamora carlos-zamora removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Aug 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Area-Theming Anything related to the theming of elements of the window Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a setting to manually set the Pane highlight color
4 participants