Set Tab tooltip explicitly · Pull Request #8298 · microsoft/terminal

ghost · 2020-11-17T03:09:21Z

Summary of the Pull Request

The tab tooltip is no longer empty when it was toggle zoomed.

References

PR Checklist

Closes Tab tooltip is empty when zooming pane #8199
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema 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

Validation Steps Performed

mpela81 · 2020-11-17T09:23:22Z

Hi - I'd like to give my humble comments from a non-reviewer if you don't mind:

If you set the tooltip explicitly only when you are in zoomed mode, then when you exit zoom and switch to another pane, the tooltip is not updated automatically to reflect the selected pane. You may need to set it explicitly in all cases.
Is it required to set a Run+Textbox, instead of just the tabText?
If I remember correctly when I experimented on this, by default the 'explicit' tooltip is not positioned the same as the 'non-explicit' tooltip. You may need to adjust it.

ghost · 2020-11-18T02:25:41Z

@mpela81 From my experience it won't work if I don't use Run+Textbox.

DHowett · 2020-11-25T20:41:44Z

This will probably need to be updated for the new TabHeaderControl! 😄

zadjii-msft · 2020-11-30T17:33:17Z

Hey thanks for putting this together - mind merging main into this branch? I'm fairly certain the work in #8227 is gonna end up conflicting with this PR a decent amount 😄

ghost · 2020-12-01T17:53:20Z

I will update it for the new TabHeaderControl.

ghost · 2020-12-14T07:17:16Z

@DHowett I updated for TabHeaderControl!

DHowett · 2020-12-15T00:44:06Z

src/cascadia/TerminalApp/CommandPalette.cpp

                                         Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e)
    {
        auto key = e.OriginalKey();
-        auto const ctrlDown = WI_IsFlagSet(CoreWindow::GetForCurrentThread().GetKeyState(winrt::Windows::System::VirtualKey::Control), CoreVirtualKeyStates::Down);


This change doesn't seem related to fixing the tooltip in the tab header!

DHowett · 2020-12-15T00:45:07Z

src/cascadia/TerminalApp/TerminalTab.cpp

+        newTabRun.Text(tabTitle);
+        auto textBlock = WUX::Controls::TextBlock{};
+        textBlock.Inlines().Append(newTabRun);
+        toolTip.Content(textBlock);


we might be able to get away with tooltip.Content(winrt::box_value(tabTitle)) and avoid the Run and TextBlock completely. Does that work?

DHowett · 2020-12-15T00:45:42Z

src/cascadia/TerminalApp/TerminalTab.cpp


            // Update the control to reflect the changed title
            _headerControl.Title(activeTitle);
+            _SetToolTip(_GetActiveTitle());


Are we sure this is called at all the right times? How about during initialization when we set the first title

ghost · 2020-12-15T14:25:55Z

@DHowett I hope this is what you meant!

zadjii-msft · 2020-12-15T18:29:43Z

Notes from playing with the branch:

The tooltip doesn't seem to change when the application changes the title
The tooltip doesn't change when the tab is renamed with the renamer
The tooltip does change when the tab zooms

Additionally

There's currently no tooltip on any tabs:

so this is definitely still an improvement

ghost · 2020-12-16T06:26:11Z

It seems like putting _SetToolTip() in _UpdateTitle() resolve all the issues above.

DHowett

This looks great. Thank you!

DHowett · 2020-12-17T01:57:02Z

src/cascadia/TerminalApp/TerminalTab.cpp


+    void TerminalTab::_SetToolTip(const winrt::hstring& tabTitle)
+    {
+        auto toolTip = WUX::Controls::ToolTip{};


Suggested change

auto toolTip = WUX::Controls::ToolTip{};

WUX::Controls::ToolTip toolTip{};

optional change to improve code readability. we usually don't use "auto x = type{y}" because auto is only good when you don't have to say the type. type x{y}; is shorter 😄

zadjii-msft

This is great, thanks!

ghost · 2020-12-17T14:21:55Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2021-01-28T18:02:12Z

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

Handy links:

## Summary of the Pull Request The tab tooltip is no longer empty when it was toggle zoomed.  ## References  ## PR Checklist * [x] Closes microsoft#8199 * [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema 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  ## Validation Steps Performed

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 Nov 17, 2020

zadjii-msft self-assigned this Nov 20, 2020

zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 30, 2020

zadjii-msft assigned ghost and unassigned zadjii-msft Nov 30, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 1, 2020

kaihugo added 3 commits December 4, 2020 17:47

update for TabHeaderControl

aea4c15

delete the line from last the PR(8194)

96da7f8

code format

127eeb1

DHowett reviewed Dec 15, 2020

View reviewed changes

DHowett requested changes Dec 15, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 15, 2020

avoid the Run and TextBlock

96e77bb

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 15, 2020

kaihugo added 2 commits December 15, 2020 18:13

not this PR

7a73a8f

remove _SetToolTip from UpdateTitle()

9eba428

Put it back into UpdateTitle()

dffe560

DHowett approved these changes Dec 17, 2020

View reviewed changes

DHowett reviewed Dec 17, 2020

View reviewed changes

improve readability

52517c4

zadjii-msft approved these changes Dec 17, 2020

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 17, 2020

ghost merged commit b8e6b8e into microsoft:main Dec 17, 2020

ghost mentioned this pull request Jan 28, 2021

Tab tooltip is empty when zooming pane #8199

Closed

This pull request was closed.

	auto toolTip = WUX::Controls::ToolTip{};
	WUX::Controls::ToolTip toolTip{};

Conversation

ghost commented Nov 17, 2020

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

mpela81 commented Nov 17, 2020

Uh oh!

ghost commented Nov 18, 2020

Uh oh!

DHowett commented Nov 25, 2020

Uh oh!

zadjii-msft commented Nov 30, 2020

Uh oh!

ghost commented Dec 1, 2020

Uh oh!

ghost commented Dec 14, 2020

Uh oh!

DHowett Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

DHowett Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

DHowett Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 15, 2020

Uh oh!

zadjii-msft commented Dec 15, 2020

Uh oh!

ghost commented Dec 16, 2020

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 17, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ghost commented Jan 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.