Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Snap to character grid when resizing window #3181
Snap to character grid when resizing window #3181
Changes from 11 commits
b8a5725
68f613b
5d0f1d3
90649b2
cbda911
d69f8ed
b732e74
0ecbec5
3dcd00f
583541d
b00fef8
5e9b31c
bed02c5
76d4945
9a95e18
11fcd51
8034bb4
14cc2d8
eb67b82
b4febc8
cae3963
63ed26a
b0097d2
cb0186a
bada548
24a88a5
8cf35e8
b933528
1ebddfc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than having method named
SnapDimension
that sometimes doesn't do its job, it might be cleaner just not to call it at all inIslandWindow
.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
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just check
initialUpdate
here, and only raise the event when it's true? It looks like it's only actually doing something inPane
when it's true.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I did this because in future we may re-layout panes even on other events, like font/zoom change and so we don't make to much diff (see issue #xxx I haven't filled one yet). But it might be too future of a future to bother now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading this method, I'd maybe want this first param named something like
instead, especially lower:
if (snapToWidth && _settings.ScrollState() == ScrollbarState::Visible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 likeisWidth
, it is not so apparent what it means if it's false. Usually not-width means height, but not always.