Fix clipped progress ring in tab when tab title is too long by jonathan-meier · Pull Request #14167 · microsoft/terminal

jonathan-meier · 2022-10-09T13:31:41Z

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

terminal/src/cascadia/TerminalApp/TabHeaderControl.xaml

Lines 18 to 27 in 43dbbd5

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

terminal/src/cascadia/TerminalSettingsModel/IconPathConverter.cpp

Lines 143 to 154 in 43dbbd5

    
           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

jonathan-meier · 2022-10-09T13:37:06Z

@microsoft-github-policy-service agree

DHowett

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

carlos-zamora

Love it! Thanks!

carlos-zamora · 2022-10-10T17:36:16Z

@msftbot merge this in 5 minutes

ghost · 2022-10-10T17:36:25Z

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

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

ghost · 2022-12-14T20:45:04Z

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

Handy links:

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

086d5a6

ghost added Area-UserInterface 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

DHowett approved these changes Oct 10, 2022

View reviewed changes

carlos-zamora approved these changes Oct 10, 2022

View reviewed changes

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

ghost merged commit 21a9c55 into microsoft:main Oct 10, 2022

ghost mentioned this pull request Dec 14, 2022

A very long title causes only half of the progress ring to be visible #8910

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Fix clipped progress ring in tab when tab title is too long#14167
1 commit merged intomicrosoft:mainfrom
jonathan-meier:8910-clipped-progress-ring

jonathan-meier commented Oct 9, 2022 •

edited by DHowett

Loading

Uh oh!

jonathan-meier commented Oct 9, 2022

Uh oh!

DHowett left a comment

Uh oh!

carlos-zamora left a comment

Uh oh!

carlos-zamora commented Oct 10, 2022

Uh oh!

ghost commented Oct 10, 2022

Uh oh!

ghost commented Dec 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	<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}" />

	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;
	}

Conversation

jonathan-meier commented Oct 9, 2022 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathan-meier commented Oct 9, 2022

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Oct 10, 2022

Uh oh!

ghost commented Oct 10, 2022

Uh oh!

ghost commented Dec 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonathan-meier commented Oct 9, 2022 •

edited by DHowett

Loading