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.

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

Fix clipped progress ring in tab when tab title is too long #14167

Merged
1 commit merged into from Oct 10, 2022

Conversation

jonathan-meier
Copy link
Contributor

@jonathan-meier jonathan-meier commented Oct 9, 2022

It turns out that the negative margin for the progress ring is causing
the clipping in case the tab title gets too long:

<mux:ProgressRing x:Name="HeaderProgressRing"
Width="15"
Height="15"
MinWidth="0"
MinHeight="0"
Margin="-7.5,0,8,0"
IsActive="{x:Bind TabStatus.IsProgressRingActive, Mode=OneWay}"
IsIndeterminate="{x:Bind TabStatus.IsProgressRingIndeterminate, Mode=OneWay}"
Visibility="{x:Bind TabStatus.IsProgressRingActive, Mode=OneWay}"
Value="{x:Bind TabStatus.ProgressValue, Mode=OneWay}" />

The negative margin was introduced in #8113 because the progress ring is
supposed to replace the tab icon but the TabView still reserves space
even if no icon is set (see
#8133 (comment)).
However, it is not actually the TabView reserving space even when
there is no icon, but a workaround for a crash in the
IconPathConverter that returns a BitmapIconSource with a nullptr
source instead of a nullptr IconSource:

if (!iconSource)
{
// Set the default IconSource to a BitmapIconSource with a null source
// (instead of just nullptr) because there's a really weird crash when swapping
// data bound IconSourceElements in a ListViewTemplate (i.e. CommandPalette).
// Swapping between nullptr IconSources and non-null IconSources causes a crash
// to occur, but swapping between IconSources with a null source and non-null IconSources
// work perfectly fine :shrug:.
BitmapIconSource<TIconSource>::type icon;
icon.UriSource(nullptr);
iconSource = icon;
}

The workaround in IconPathConverter could probably be removed as I did
not find any instance where it is still used in a way that could trigger
the mentioned crash, but I did not dare to just remove it as I do not
know enough about the code by far. Hence, I opted to just locally
instantiate the IconSource with a nullptr directly in TerminalTab.

Fixes #8910

@ghost ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Oct 9, 2022
@jonathan-meier
Copy link
Contributor Author

@microsoft-github-policy-service agree

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.

Thanks for doing this! It seems so straightforward when you lay it out like that...

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.

Love it! Thanks!

@carlos-zamora
Copy link
Member

@msftbot merge this in 5 minutes

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 10, 2022
@ghost
Copy link

ghost commented Oct 10, 2022

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Mon, 10 Oct 2022 17:41:24 GMT, which is in 5 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit 21a9c55 into microsoft:main Oct 10, 2022
@DHowett DHowett added this to To Cherry Pick in 1.16 Servicing Pipeline via automation Oct 13, 2022
@DHowett DHowett moved this from To Cherry Pick to Cherry Picked in 1.16 Servicing Pipeline Oct 13, 2022
DHowett pushed a commit that referenced this pull request Oct 13, 2022
It turns out that the negative margin for the progress ring is causing
the clipping in case the tab title gets too long:

https://github.com/microsoft/terminal/blob/43dbbd590fa4b46c37e9970415f8150d3c399598/src/cascadia/TerminalApp/TabHeaderControl.xaml#L18-L27

The negative margin was introduced in #8113 because the progress ring is
supposed to replace the tab icon but the `TabView` still reserves space
even if no icon is set (see
#8133 (comment)).
However, it is not actually the `TabView` reserving space even when
there is no icon, but a workaround for a crash in the
`IconPathConverter` that returns a `BitmapIconSource` with a `nullptr`
source instead of a `nullptr` `IconSource`:

https://github.com/microsoft/terminal/blob/43dbbd590fa4b46c37e9970415f8150d3c399598/src/cascadia/TerminalSettingsModel/IconPathConverter.cpp#L143-L154

The workaround in `IconPathConverter` could probably be removed as I did
not find any instance where it is still used in a way that could trigger
the mentioned crash, but I did not dare to just remove it as I do not
know enough about the code by far. Hence, I opted to just locally
instantiate the `IconSource` with a `nullptr` directly in `TerminalTab`.

Fixes #8910

(cherry picked from commit 21a9c55)
Service-Card-Id: 86209056
Service-Version: 1.16
@DHowett DHowett moved this from Cherry Picked to Shipped in 1.16 Servicing Pipeline Dec 1, 2022
@ghost
Copy link

ghost commented Dec 14, 2022

🎉Windows Terminal Preview v1.16.3463.0 and v1.16.3464.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

A very long title causes only half of the progress ring to be visible
3 participants