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

Add support for "focus follows mouse" mode #8965

Merged
8 commits merged into from
Feb 9, 2021

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Jan 30, 2021

PR Checklist

@ghost ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Jan 30, 2021
@Don-Vito Don-Vito changed the title Teach terminal control to gain focus on pointer hover Teach pane to gain focus on pointer hover Jan 30, 2021
@Gursu78
Copy link

Gursu78 commented Jan 31, 2021

Merhaba Hata kodu 0X80092012 Unknow Error .bunu nasıl çözemedim.

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Jan 31, 2021

Merhaba Hata kodu 0X80092012 Unknow Error .bunu nasıl çözemedim.

@Gursu78 - Hata açıklaması içeren bir bilet açmanız gerekiyor. Rastgele bir PR ile yazmak yerine

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Feb 1, 2021

@zadjii-msft - WDYT?

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

So yea, this would definitely work for "focus follows mouse". I hate to be that guy though - I think this needs to be a (global) setting. I don't think I'd personally want this setting on, though it's definitely a good idea.

If this is a setting, then we'd need another way of fixing #8925 regardless if the setting was enabled or not. I'm gonna block for discussion, but the implementation is sound.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 1, 2021
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Feb 1, 2021

So yea, this would definitely work for "focus follows mouse". I hate to be that guy though - I think this needs to be a (global) setting. I don't think I'd personally want this setting on, though it's definitely a good idea.

If this is a setting, then we'd need another way of fixing #8925 regardless if the setting was enabled or not. I'm gonna block for discussion, but the implementation is sound.

So I will also be that guy 😄 - if the user selects not to use "focus follow mouse", they will need to focus the pane to get the hyperlink instead of #8475.

WDYT?

Agree regarding the setting (planned to add it once you agree with the idea)

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 1, 2021
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Feb 2, 2021

@zadjii-msft - WDYT about my previous comment.
Also should I add a setting and productize it?

@zadjii-msft
Copy link
Member

Oh. Well that's annoying. I figured there'd still be a way to get the unfocused hyperlink hovering to work, even without focus follows mouse.

The team did discuss this and came to the conclusion that "focus follows mouse" would definitely have to be a setting, probably defaulted to false. You okay with that?

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Feb 3, 2021

Oh. Well that's annoying. I figured there'd still be a way to get the unfocused hyperlink hovering to work, even without focus follows mouse.

The team did discuss this and came to the conclusion that "focus follows mouse" would definitely have to be a setting, probably defaulted to false. You okay with that?

@zadjii-msft - I can absolutely solve #8925 separately from this PR. I assumed that probably the users who want to interact with other panes, will also want them focused.

If this is not the case I will

  • Add this with a setting defaulted to false.
  • Create another PR that unselects hyperlinks in non-focused tabs.

BTW I am somewhat worried with the current solution, as right now we send a mouse input when hovering over unfocused panes. Is it intentional?

@zadjii-msft
Copy link
Member

I am somewhat worried with the current solution, as right now we send a mouse input when hovering over unfocused panes. Is it intentional?

😬 almost certainly not

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Feb 4, 2021

So yea, this would definitely work for "focus follows mouse". I hate to be that guy though - I think this needs to be a (global) setting. I don't think I'd personally want this setting on, though it's definitely a good idea.

If this is a setting, then we'd need another way of fixing #8925 regardless if the setting was enabled or not. I'm gonna block for discussion, but the implementation is sound.

@zadjii-msft -

Please review (if you like it, I will add a documentation)

@Don-Vito Don-Vito removed their assignment Feb 4, 2021
@zadjii-msft zadjii-msft self-assigned this Feb 8, 2021
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

that-was-easy.jpg

@zadjii-msft zadjii-msft removed their assignment Feb 8, 2021
@zadjii-msft zadjii-msft changed the title Teach pane to gain focus on pointer hover Add support for "focus follows mouse" mode Feb 8, 2021
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Feb 9, 2021

I guess it will continue getting conflicts as I added a setting to the end 😄
@zadjii-msft - is anyone assigned to take a second look or everybody hates this feature? 😄

@zadjii-msft
Copy link
Member

Oh, just I think the rest of the team is a little bogged down...

oh wait nope, nevermind. I never threw Needs-Second on this. My b.

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Feb 9, 2021
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.

A few things:

  • If the window is not focused, does it get focus when we hover over a terminal?
  • If the tab switcher or command palette is open, does this immediately close it because the terminal gets focus? Or what happens?
  • How does this work when the search box is open in multiple panes?
  • Update the schema please.
  • I have no issues with this being a setting. So please make a PR for the docs repo too 😊

@ghost ghost 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 Feb 9, 2021
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Feb 9, 2021

A few things:

* If the window is not focused, does it get focus when we hover over a terminal?

Nope. The window will remain unfocused. But the pane will become active. Looks good to me, but let me know if you see otherwise.

* If the tab switcher or command palette is open, does this immediately close it because the terminal gets focus? Or what happens?

The CmdPal/ATS remain open as they "hide" the terminal behind them (no pointer move is reported)

* How does this work when the search box is open in multiple panes?

In this case the behavior is similar as if you were clicking on the second pane. This one is somewhat questionable. WDYT?

* Update the schema please.

Done.

* I have no issues with this being a setting. So please make a PR for the docs repo too 😊

Doing it right now. Might introduce some extra / miss some "the" 😊

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.

Thanks for looking into those scenarios! I think they're all fine. The search box one is somewhat questionable but I think this makes more sense than closing/ignoring the search box on hover.

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 9, 2021
@ghost
Copy link

ghost commented Feb 9, 2021

Hello @carlos-zamora!

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 ghost merged commit 47f4b41 into microsoft:main Feb 9, 2021
@ghost
Copy link

ghost commented Mar 1, 2021

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

Handy links:

@ghost ghost mentioned this pull request Mar 1, 2021
@zadjii-msft zadjii-msft mentioned this pull request May 7, 2021
59 tasks
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-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. 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.

Pane focus follows mouse
4 participants