Skip to content

Maintain zoom when moving focus#11046

Merged
3 commits merged intomicrosoft:mainfrom
Rosefield:feature/gh7215-keep-zooming
Sep 2, 2021
Merged

Maintain zoom when moving focus#11046
3 commits merged intomicrosoft:mainfrom
Rosefield:feature/gh7215-keep-zooming

Conversation

@Rosefield
Copy link
Contributor

@Rosefield Rosefield commented Aug 26, 2021 •
edited
Loading

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-UserInterface 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 •
edited
Loading

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

zadjii-msft 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.

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

Rosefield commented Aug 26, 2021

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

Rosefield commented Aug 26, 2021

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.

carlos-zamora
carlos-zamora approved these changes Aug 26, 2021
View reviewed changes
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)

@Rosefield
Copy link
Contributor Author

Rosefield commented Aug 26, 2021

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

zadjii-msft
zadjii-msft approved these changes Sep 2, 2021
View reviewed changes
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
@Rosefield Rosefield mentioned this pull request Sep 6, 2021
Merged
@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:

@ghost ghost mentioned this pull request Oct 20, 2021
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

@Rosefield @DHowett @zadjii-msft @carlos-zamora