Skip to content

Re-add the keychords to the new tab flyout#3903

Merged
1 commit merged intomasterfrom
dev/migrie/b/3896-dropdown-bindings-again
Dec 11, 2019
Merged

Re-add the keychords to the new tab flyout#3903
1 commit merged intomasterfrom
dev/migrie/b/3896-dropdown-bindings-again

Conversation

@zadjii-msft
Copy link
Member

Summary of the Pull Request

On this month's episode of "Mike accidentally breaks this":
Mike forgets that == is defined on a pair of winrt objects as "do these point to the SAME object", not "do they have the same values".

This just slipped right through code review.

References

Broken in #3825

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Manually checked that these are still there

 On this month's episode of "Mike accidentally breaks this": Mike forgets that `==` is defined on a pair of winrt objects as "do these point to the _SAME_ object", not "do they have the same values". This just slipped right through code review.
@zadjii-msft zadjii-msft added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Dec 10, 2019
if (otherAsUs)
{
return otherAsUs->_TerminalArgs == _TerminalArgs;
return otherAsUs->_TerminalArgs.Equals(_TerminalArgs);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we actually need a less strict check than this, right? like: if I bind a profile guid, name OR index to newTab, should it show up in the list?

@DHowett-MSFT DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label Dec 10, 2019
@DHowett-MSFT DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 11, 2019
@ghost
Copy link

ghost commented Dec 11, 2019

Hello @DHowett-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.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 3 hours 33 minutes. No worries though, I will be back when the time is right! 😉

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 ghost merged commit bb60db7 into master Dec 11, 2019
@ghost ghost deleted the dev/migrie/b/3896-dropdown-bindings-again branch December 11, 2019 04:23
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-UserInterface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dropdown menu no longer displays bound keys again (after #3825)

3 participants