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 22 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.
this seems like dangerous use of unique pointers. you're sharing ownership by popping the pointers out of them, but the first one of these things to get destructed is going to explode the entire tree of them by deleting the child nodes.
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.
(like: two unique_ptrs holding the same underlying pointer are never ever ever okay)
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.
Sorry, I don't understand what's wrong here. I only pop the underlying pointer to pass it to
_AssignChildNode
, which just reads the pointed object and copies it (copies object, not the pointer). It could receive it by reference as well, except it has to be nullable. Or it could receive reference tounique_ptr
but that's anti pattern in such situations (not to force implementation). And I don't see any twounique_ptr
s to hold the same value.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.