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

Make padding applied as swap chain panel's margin #1778

Merged
merged 1 commit into from
Jul 5, 2019

Conversation

Summon528
Copy link
Contributor

@Summon528 Summon528 commented Jul 2, 2019

Summary of the Pull Request

Make padding applied as swap chain panel's margin

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

I know that this PR is trying to do the same thing as #1672. However since that one is unlikely to be merged in a short time and the issue will cause background image to be padded once #1107 is merged, I present my slightly more straight forward solution which is just set margins to swap chain panel. Hope this one gets merge sooner but we can switch to #1672's solution later if it is preferred.
Note that this PR does NOT fixx #1367 as #1672 does.

Validation Steps Performed

Set padding and observe that the scroll bar stay in the same place

@zadjii-msft
Copy link
Member

This is a perfectly fine change, but like you mention, is pretty much the same thing as #1672. I'm not sure what the benefits of that PR are over this one. This one seems like the more elegant, simpler solution, without need to muck with the renderer. Though, #1672 doesn't seem terribly complicated, and I'm pretty okay with it. I think I'd rather have @miniksa weigh in on whether the other PR makes sense. If he thinks the other is too risky or might take too long to review, I'm fine shipping this one.

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this one better because it doesn't mess with any math calculations in the DX renderer. It just applies the setting to a different element.

I just don't want the DX renderer to move too much before I get a big chance to put a concerted effort back into it, especially by adding mathematical deltas for offsets because I've burned myself that way before with complexity in rendering.

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.

Makes sense to me.

@miniksa miniksa merged commit c6ca298 into microsoft:master Jul 5, 2019
mcpiroman pushed a commit to mcpiroman/terminal that referenced this pull request Jul 23, 2019
@ghost
Copy link

ghost commented Aug 3, 2019

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scrollbar is inset by padding and probably shouldn't be
3 participants