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

Maintain zoom when moving focus #11046

Merged
3 commits merged into from
Sep 2, 2021

Conversation

Rosefield
Copy link
Contributor

@Rosefield Rosefield commented Aug 26, 2021

Summary of the Pull Request

Make it so you can navigate pane focus without unzooming.

PR Checklist

  • Closes Moving focus with a zoomed pane should just zoom the adjacent pane #7215
  • 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

  • Slight refactor to bring the MRU pane logic into the NavigateDirection function
  • The actual zoom behavior was not a problem, the only issue is that because most of the panes weren't in the UI tree I had to disable using the actual sizes. There is nothing wrong with that, since the synthetic sizing is required anyways, but I'm curious what other peoples' thoughts are.

Validation Steps Performed

output

@ghost ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 26, 2021
@Rosefield Rosefield marked this pull request as ready for review August 26, 2021 12:57
@Rosefield
Copy link
Contributor Author

Rosefield commented Aug 26, 2021

Apparently I can't revert to being a draft pr. Regardless, the question about how to handle directional navigation is open. Its a tad bit weird to use "visual" navigation when all you see is one pane, but presumably the person knows what their layout under the zoomed pane looks like.

@DHowett
Copy link
Member

DHowett commented Aug 26, 2021

Its a tad bit weird to use "visual" navigation when all you see is one pane, but presumably the person knows what their layout under the zoomed pane looks like.

Yeah, I'd agree with this assertion. Worst case, somebody can flail around for a couple seconds until they find the pane they wanted anyway -- the harm in choosing the wrong one is pretty small.

@zadjii-msft
Copy link
Member

Its a tad bit weird to use "visual" navigation when all you see is one pane, but presumably the person knows what their layout under the zoomed pane looks like.

for the record, the reason I wanted in the first place is because I generally do know the layout of which pane is which in my head and want to be able to switch around exactly like this 😋 An animation for switching between zoomed panes might be cool, but hey, one step at a time

@Rosefield
Copy link
Contributor Author

Well, unless someone can think of a downside to never using actual dimensions, I guess I'll just remove all of the Actual{Height,Width} code? The other option would be to pass in a "isZoomed" flag on the PanePoint that functionally does the same thing in this case?

@Rosefield
Copy link
Contributor Author

Eh, removed the ActualSize based navigation. Doing so cleans things up a bunch, anyways. Also I think the logic is now basically identical to PreCalculate* so there is room for refactoring there. Easy enough to revert that part of the PR if needed.

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.

Most of the suggestions I added weren't even on you haha. Great work! So excited for this feature to land!! (I was literally thinking about it earlier this week haha)

src/cascadia/TerminalApp/TerminalTab.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Pane.cpp Show resolved Hide resolved
@Rosefield
Copy link
Contributor Author

@carlos-zamora Well, most of your suggestions were me circa 1 month ago :)

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.

Thanks for doing this one! Excited to be able to use it day-to-day ☺️

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 2, 2021
@ghost
Copy link

ghost commented Sep 2, 2021

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 ghost merged commit 13bc71d into microsoft:main Sep 2, 2021
@Rosefield Rosefield deleted the feature/gh7215-keep-zooming branch September 2, 2021 15:37
@ghost
Copy link

ghost commented Oct 20, 2021

🎉Windows Terminal Preview v1.12.2922.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-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Moving focus with a zoomed pane should just zoom the adjacent pane
4 participants