Snap to character grid when resizing window #3181

mcpiroman · 2019-10-13T17:31:34Z

When user resizes window, snap the size to align with the character grid (like e.g. putty, mintty and most unix terminals). Properly resolves arbitrary pane configuration (even with different font sizes and padding) trying to align each pane as close as possible.

It also fixes terminal minimum size enforcement which was not quite well handled, especially with multiple panes.

This PR does not however try to keep the terminals aligned at other user actions (e.g. font change or pane split). That is to be tracked by some other activity.

References

#3060 introduces some differences in size calculations by moving from separator to borders, but it should be easy to adopt to that.

PR Checklist

Closes Snap to character grid when resizing window #2834, Closes BUG: Crash on window resize #2277
CLA signed. If not, go over here and sign the CLA
Tests added/passed - do we have UI tests yet?
Requires documentation to be 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

Snapping is resolved in the pane tree, recursively, so it (hopefully) works for any possible layout.

Along the way I had to clean up some things as so to make the resulting code not so cumbersome:

Pane.cpp: Replaced _firstPercent and _secondPercent with single _desiredSplitPosition to reduce invariants - these had to be kept in sync so their sum always gives 1 (and were not really a percent). The desired part refers to fact that since panes are aligned, there is usually some deviation from that ratio.
Pane.cpp: Fixed _GetMinSize() - it was improperly accounting for split direction
TerminalControl: Made dedicated member for padding instead of reading it from a control itself. This is because the winrt property functions turned out to be slow and this algorithm needs to access it many times. I also cached scrollbar width for the same reason.
AppHost: Moved window to client size resolution to virtual method, where IslandWinow and NonClientIslandWindow have their own implementations (as opposite to pointer casting).

One problem with current implementation is I had to make a long call chain from the window that requests snapping to the (root) pane that implements it:
IslandWindow -> AppHost's callback -> App -> TerminalPage -> Tab -> Pane
Idk if this can be done better.

Validation Steps Performed

Spam split pane buttons, randomly change font sizes with ctrl+mouse wheel and drag the window back and forth.

carlos-zamora

A few comments throughout. I'll take a second and more in-depth pass of Pane.cpp later.
If possible, I'd like @zadjii-msft to sign off on this when he gets back

src/cascadia/WindowsTerminal/IslandWindow.cpp

src/cascadia/TerminalControl/TermControl.cpp

src/cascadia/TerminalApp/Pane.cpp

src/cascadia/TerminalApp/Pane.h

DHowett-MSFT · 2019-10-16T00:42:45Z

(I am very excited about this and just need some time to give it a review.)

oising · 2019-10-19T00:01:05Z

Nice! Definitely needed.

zadjii-msft

I owe this a so much longer review, but I wanted to get these thoughts in before the weekend:

Amazing work here
I'm definitely concerned about the _root thing I called out below
Is this disable-able by any chance? If someone wants smooth resizing like it is currently, is that something that could still be enabled with a setting? (not saying I'll block on this if it isn't, I'm just curious)
Heads up, I merged Indicate which pane is focused with the Accent color on the pan… #3060, so there's gonna be some conflicts with that 😧 I'm so sorry

src/cascadia/TerminalApp/Pane.cpp

zadjii-msft

So first off, this is definitely a change I want to see land. Resizing seems much more stable with it, and the snapping is really slick. That being said, I still have very little idea how it works. I think in _AdvanceSnappedDimension especially, additional comments would help clean things up, as well as adding more types just to help clear up the differences between the many different usages of std::pair<float, float> throughout the code.
I'm also very worried about the runtime of this calculation. Seems like it repeats a lot of work, though I'm not totally sure how at this time to avoid that step. Let's iterate on this a bit, but I'm hopeful that we can land this change. Excellent work overall 😀

zadjii-msft · 2019-11-04T22:01:14Z

src/cascadia/TerminalApp/TerminalPage.cpp

+    float TerminalPage::SnapDimension(const bool widthOrHeight, const float dimension) const
+    {
+        const auto focusedTabIndex = _GetFocusedTabIndex();
+        return _tabs[focusedTabIndex]->SnapDimension(widthOrHeight, dimension);


Note to self: This is probably where I'd implement a setting to disable the snapping. By simply not calling SnamDimension here, and just returning dimension, then the AppHost layer will act as though the proposed snap size was okay, and it'll still smooth resize.

Rather than having method named SnapDimension that sometimes doesn't do its job, it might be cleaner just not to call it at all in IslandWindow.

Secondly, that would only disable the 'window' snapping but not 'pane' snapping (so if one splits terminal vertically and changes the window's width, the left pane will still snap, although the whole window would go smoothly). Should it be this way? Maybe make this 3-way setting that would also cover that (like fullSnap, innerSnap, noSnap).

src/cascadia/TerminalApp/Tab.h

src/cascadia/TerminalControl/TermControl.h

zadjii-msft · 2019-11-04T22:16:49Z

src/cascadia/TerminalControl/TermControl.cpp

+    // - dimension: a dimension (width or height) to be snapped
+    // Return Value:
+    // - A dimension that would be aligned to the character grid.
+    float TermControl::SnapDimensionToGrid(const bool widthOrHeight, const float dimension) const


Reading this method, I'd maybe want this first param named something like

Suggested change

float TermControl::SnapDimensionToGrid(const bool widthOrHeight, const float dimension) const

float TermControl::SnapDimensionToGrid(const bool snapToWidth, const float dimension) const

instead, especially lower:

if (snapToWidth && _settings.ScrollState() == ScrollbarState::Visible)

If so, this should be renamed everywhere (so in a lot of places). I went with widthOrHeight because I find it more readable. With some thing like isWidth, it is not so apparent what it means if it's false. Usually not-width means height, but not always.

src/cascadia/TerminalApp/Pane.cpp

src/cascadia/TerminalApp/Pane.h

src/cascadia/TerminalApp/Pane.cpp

…e more

mcpiroman · 2019-11-05T19:23:13Z

About performance:

Form my measurements it appears that the introduced layouting code takes usually about 1% CPU time of whole resize operation (and the thing to note is that it runs on every mouse move event, while the actual terminal resizes runs only at the 'snaps`). On the other hand the terminal resize takes way to long.

I've tried to make this O(1) instead of the current O(n) (where n is number of cells fitting given dimension) but failed to do so ~~(and spent far too much time thinking about this. That actually gotta be on some math competition!)~~. I'm fairly convicted that the current, iterative implementation is the a good compromise between optimization and sanity.

As of caching: the XAML prop reads really did slow down the algorithm by few times, even on release config. But since I measured it the algorithm changed quite drastically and now these properties are being read many less times (in favor of character size, which does not go through xaml) + I just discovered I can cache the padding locally instead of reading it for each edge. This makes it far less buttlenecking and in fact rather irrelevant so I can safely undo the cacheing.

mcpiroman · 2019-12-11T11:18:17Z

@zadjii-msft Ok, screw it. The problem is that since panes now have borders inside them and some panes have more borders than others (because e.g. some are on the edge of the window), thus some are bigger than others, although terminals are the same. This somewhat breaks the nice order of growth that was established earlier, because now panes that are in the middle are favored, since they are smaller. But that's not terribly bad, if noticeable, so I won't block on that. Fixing this would require differentiating between content and border size and that seems a little bit hard and messy. I might take a follow up on this.

Another problem I noticed is that CanSplit can yield wrong result, because it assumes that the other pane would have the same minimum size as the one to be spitted (which is not in case of different font scale, and now with #3825). But this isn't really related to this PR, so I guess I should file an issue about that.

miniksa · 2019-12-11T22:23:19Z

I'm still concerned this should be a setting that can be turned off given that conhost originally snapped to character cells, then we removed it. I don't recall 100% why that was, but I'm going to suspect there's at least some set of users who prefer it to smooth resize.
Did you test that this still works with maximizing the screen and fullscreening through F11/Alt+Enter? One of the weirdnesses in conhost.exe when it snapped to cells was that maximizing the window wouldn't... actually maximize because it was not ready nor willing to have a partial cell rendered at the bottom/right if necessary dependent on the pixel dimensions of the monitor.

zadjii-msft · 2019-12-11T22:27:41Z

@miniksa re: point 1 - I'm fully prepared with a PR to make this a disable-able setting once this merges 😄

mcpiroman · 2019-12-12T18:55:29Z

I can't get it built ATM for some reason but I'm pretty sure it fullscreening worked well. It's only on WM_SIZING that something happens, so only when user drags an edge of window.

Regarding the disabilisness, as I mentioned somewhere, apart from the on and off states, there could be an interesting option to only snap panes, but not window. Can there be a setting that is both boolean and enum? (Typescript can handle that). If not it could be named something like fullSnap, innerSnap and noSnap.

Edit: Yup, fullscrean works.

zadjii-msft · 2019-12-12T19:50:15Z

Regarding the disabilisness, as I mentioned somewhere, apart from the on and off states, there could be an interesting option to only snap panes, but not window. Can there be a setting that is both boolean and enum? (Typescript can handle that). If not it could be named something like fullSnap, innerSnap and noSnap.

Yea that's how I planned on implementing it - we can differentiate between a boolean and a string at parse time, and assume that false == "innerSnap" (or something)

zadjii-msft

Oh yea I'm much happier with that name. Thanks for this 😄

miniksa · 2019-12-31T21:41:13Z

@mcpiroman, I know I said in your other PR 4068 that I'd try to get to this one today because I duplicate commented everywhere. But I got tied up in other PRs this morning and early afternoon and now I have to go for the day. (I was overdue on a few for @j4james...) This is now on the top of my list for Thursday morning after the New Year holiday. Thanks for your patience.

mcpiroman · 2019-12-31T22:05:41Z

@miniksa No worries, I wouldn't look at that anyway - it's almost new year already at my timezone

miniksa

I went through this a bit. I don't see anything major left except other people's comment threads that still aren't marked as resolved or closed. I didn't have much else to add. I'm going to not hit the second checkmark, though, because I feel like @DHowett-MSFT had a deeper understanding of this than me and I want to leave him the chance to be second sign off before merge.

DHowett-MSFT

Alright, I love this. I'm going to check it out, merge it, play with it, and then merge the PR if it feels right. 😄

src/cascadia/TerminalApp/Pane.LayoutSizeNode.cpp

DHowett-MSFT · 2020-01-08T20:51:40Z

src/cascadia/TerminalApp/Pane.cpp

+    const auto snapPossibilites = _CalcSnappedDimension(widthOrHeight, dimension);
+    const auto lower = snapPossibilites.lower;
+    const auto higher = snapPossibilites.higher;
+    return dimension - lower < higher - dimension ? lower : higher;


It's fine. I was thinking that we could use `((dimension - lower) < (higher - dimension)) ? lower : higher. I'm okay with this though.

DHowett-MSFT · 2020-01-08T21:19:35Z

@mcpiroman thanks for your contribution 😄

## Summary of the Pull Request Adds a setting `snapToGridOnResize` to disable snapping the window on resize, and defaults it to `false`. ## References Introduced by pr #3181 ## PR Checklist * [x] Closes #3995 * [x] I work here * [ ] Tests added/passed * [ ] Requires documentation to be updated

ghost · 2020-01-14T17:16:07Z

🎉Windows Terminal Preview v0.8.10091.0 has been released which incorporates this pull request.:tada:

Handy links:

I originally just wanted to close #1104, but then discovered that hey, this event wasn't even used anymore. Excerpts of Teams convo: * [Snap to character grid when resizing window by mcpiroman · Pull Request #3181 · microsoft/terminal (github.com)](https://github.com/microsoft/terminal/pull/3181/files#diff-d7ca72e0d5652fee837c06532efa614191bd5c41b18aa4d3ee6711f40138f04c) added it to Tab.cpp * where it was added * which called `pane->Relayout` which I don't even REMEMBER * By [Add functionality to open the Settings UI tab through openSettings by leonMSFT · Pull Request #7802 · microsoft/terminal (github.com)](https://github.com/microsoft/terminal/pull/7802/files#diff-83d260047bed34d3d9d5a12ac62008b65bd6dc5f3b9642905a007c3efce27efd), there was seemingly no FontSizeChanged in Tab.cpp (when it got moved to terminaltab.cpp) > `Pane::Relayout` functionally did nothing because sizing was switched to `star` sizing at some point in the past, so it was just deleted. From [Misc pane refactoring by Rosefield · Pull Request #11373 · microsoft/terminal](https://github.com/microsoft/terminal/pull/11373/files#r736900998) So, great. We can kill part of it, and convert the rest to a `TypedEvent`, and get rid of `DECLARE_` / `DEFINE_`. `ScrollPositionChangedEventArgs` was ALSO apparently already promoted to a typed event, so kill that too.

carlos-zamora reviewed Oct 15, 2019

View reviewed changes

mcpiroman added 10 commits October 18, 2019 18:51

Kinda works, no splitted panes

b8a5725

Calc sizes more accurately

68f613b

General work, add resolution for children along resize axis

5d0f1d3

Partially fix child displacement

90649b2

Change child layout algorithm, now 100% correct

cbda911

Cache padding and scrollbar width, fix infinite loop

d69f8ed

(drastically) optimize layout algorithm ( O(n^m) -> O(n) )

b732e74

Fix minimum pane size calculation

0ecbec5

Style, comments, format; fix layout just after split

3dcd00f

Review changes

583541d

mcpiroman force-pushed the 2834-snap-to-char-grid branch 2 times, most recently from 9cb587c to f12cc5e Compare October 18, 2019 17:57

Fix rebase

b00fef8

mcpiroman force-pushed the 2834-snap-to-char-grid branch from f12cc5e to b00fef8 Compare October 18, 2019 18:01

zadjii-msft self-requested a review October 25, 2019 13:56

zadjii-msft reviewed Nov 1, 2019

View reviewed changes

src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved

zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Nov 4, 2019

zadjii-msft requested changes Nov 5, 2019

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 Nov 5, 2019

zadjii-msft mentioned this pull request Nov 5, 2019

Scenario: Add support for panes #1000

Open

59 tasks

zadjii-msft added a commit that referenced this pull request Nov 5, 2019

A bunch of changes for #3181 that helped me really understand the cod…

e83b696

…e more

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 5, 2019

mcpiroman added 3 commits November 5, 2019 20:27

Undo caching of padding and scrollbar size

5e9b31c

Some review changes

bed02c5

Merge with master

76d4945

Move Pane.LayoutSizeNode.cpp out of lib folder

cb0186a

Merge with master 3

bada548

zadjii-msft approved these changes Dec 17, 2019

View reviewed changes

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Dec 17, 2019

zadjii-msft mentioned this pull request Dec 17, 2019

Investigate Defering Pane initialization until after layout is completed #3996

Closed

mcpiroman and others added 3 commits December 18, 2019 08:22

Merge branch 'master' into 2834-snap-to-char-grid

24a88a5

Merge branch 'master' into 2834-snap-to-char-grid

8cf35e8

Github apparently switched files to LF, so revert that

b933528

mcpiroman requested a review from DHowett-MSFT December 20, 2019 17:03

mcpiroman mentioned this pull request Dec 27, 2019

Split Pane class into LeafPane and ParentPane + small fixes #4068

Closed

5 tasks

miniksa reviewed Jan 3, 2020

View reviewed changes

Some things that miniksa mentioned in microsoft#4068

1ebddfc

DHowett-MSFT approved these changes Jan 8, 2020

View reviewed changes

DHowett-MSFT merged commit d4c5276 into microsoft:master Jan 8, 2020

This was referenced Jan 8, 2020

Add support for disabling window snapping #3995

Closed

Add a setting to disable pane snapping #4154

Merged

This was referenced Jan 14, 2020

BUG: Crash on window resize #2277

Closed

Snap to character grid when resizing window #2834

Closed

DHowett-MSFT mentioned this pull request Mar 14, 2020

Window size should snap to a multiple of font width/height #4918

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snap to character grid when resizing window #3181

Snap to character grid when resizing window #3181

mcpiroman commented Oct 13, 2019 •

edited by cinnamon-msft

Loading

carlos-zamora left a comment

DHowett-MSFT commented Oct 16, 2019

oising commented Oct 19, 2019

zadjii-msft left a comment

zadjii-msft left a comment

zadjii-msft Nov 4, 2019

mcpiroman Nov 28, 2019

zadjii-msft Nov 4, 2019

mcpiroman Nov 5, 2019

mcpiroman commented Nov 5, 2019

mcpiroman commented Dec 11, 2019

miniksa commented Dec 11, 2019

zadjii-msft commented Dec 11, 2019

mcpiroman commented Dec 12, 2019 •

edited

Loading

zadjii-msft commented Dec 12, 2019

zadjii-msft left a comment

miniksa commented Dec 31, 2019

mcpiroman commented Dec 31, 2019

miniksa left a comment

DHowett-MSFT left a comment

DHowett-MSFT Jan 8, 2020

DHowett-MSFT commented Jan 8, 2020

ghost commented Jan 14, 2020

	float TermControl::SnapDimensionToGrid(const bool widthOrHeight, const float dimension) const
	float TermControl::SnapDimensionToGrid(const bool snapToWidth, const float dimension) const

Snap to character grid when resizing window #3181

Snap to character grid when resizing window #3181

Conversation

mcpiroman commented Oct 13, 2019 • edited by cinnamon-msft Loading

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

carlos-zamora left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Oct 16, 2019

oising commented Oct 19, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Nov 4, 2019

Choose a reason for hiding this comment

mcpiroman Nov 28, 2019

Choose a reason for hiding this comment

zadjii-msft Nov 4, 2019

Choose a reason for hiding this comment

mcpiroman Nov 5, 2019

Choose a reason for hiding this comment

mcpiroman commented Nov 5, 2019

mcpiroman commented Dec 11, 2019

miniksa commented Dec 11, 2019

zadjii-msft commented Dec 11, 2019

mcpiroman commented Dec 12, 2019 • edited Loading

zadjii-msft commented Dec 12, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

miniksa commented Dec 31, 2019

mcpiroman commented Dec 31, 2019

miniksa left a comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

DHowett-MSFT Jan 8, 2020

Choose a reason for hiding this comment

DHowett-MSFT commented Jan 8, 2020

ghost commented Jan 14, 2020

mcpiroman commented Oct 13, 2019 •

edited by cinnamon-msft

Loading

mcpiroman commented Dec 12, 2019 •

edited

Loading