Move ICore/ControlSettings to TerminalControl project#7167
Merged
10 commits merged intomasterfrom Aug 7, 2020
Merged
Conversation
ghost
added
Area-CodeHealth
This comment has been minimized.
This comment has been minimized.
zadjii-msft
reviewed
Aug 5, 2020
Member
zadjii-msft
left a comment
zadjii-msft
left a comment
There was a problem hiding this comment.
I'm at 35/67 but it's 5p here, so here's some initial feedback
DHowett
requested changes
Aug 5, 2020
| // If the old scrolloffset was 0, then we weren't scrolled back at all | ||
| // before, and shouldn't be now either. | ||
| _scrollOffset = originalOffsetWasZero ? 0 : ::base::ClampSub(_mutableViewport.Top(), newVisibleTop); | ||
| _scrollOffset = originalOffsetWasZero ? 0 : static_cast<int>(::base::ClampSub(_mutableViewport.Top(), newVisibleTop)); |
Member
There was a problem hiding this comment.
this one seems weird. should we just be usinjg ClampSub<int> or something?
Member
Author
There was a problem hiding this comment.
For whatever reason, this came up:
1>D:\Terminal\src\cascadia\TerminalCore\Terminal.cpp(347,104): error C2445: result type of conditional expression is ambiguous: types 'int' and 'base::internal::ClampedNumeric<short>' can be converted to multiple common types
It's also having trouble converting from ClampedNumeric to the non-clamped numeric type :/
Member
There was a problem hiding this comment.
This is bizarre that this is needed now
| // length 80 string of text with bisecting characters at the beginning and end. | ||
| // positions of き(\x304d) are at 0, 27-28, 39-40, 67-68, 79 | ||
| PWCHAR pwszText = | ||
| auto pwszText = |
Member
There was a problem hiding this comment.
interesting. not necessary for this change, i presume, but I'll allow it? 😁
Member
Author
There was a problem hiding this comment.
Similar to the ClampSub issue above, idk why this came up:
6>D:\Terminal\src\inc\test\CommonState.hpp(306,25): error C2440: 'initializing': cannot convert from 'const wchar_t [81]' to 'PWCHAR'
6>D:\Terminal\src\inc\test\CommonState.hpp(306,25): message : Conversion from string literal loses const qualifier (see /Zc:strictStrings)
🤷♂️
ghost
added
the
Needs-Author-Feedback
ghost
removed
the
Needs-Author-Feedback
DHowett
approved these changes
Aug 5, 2020
zadjii-msft
added
the
AutoMerge
|
Hello @zadjii-msft! Because this pull request has the 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 (
|
ghost
merged commit
This pull request was closed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary of the Pull Request
Move
ICoreSettingsandIControlSettingsfrom the TerminalSettings project to the TerminalCore and TerminalControl projects respectively. Also entirely removes the TerminalSettings project.The purpose of these interfaces is unchanged.
ICoreSettingsis used to instantiate a terminal.IControlSettings(which requires anICoreSettings) is used to instantiate a UWP terminal control.References
Closes #7140
Related Epic: #885
Related Spec: #6904
PR Checklist
TerminalSettingsinterfaces #7140added/passed (no additional tests necessary)Documentation updatedSchema updatedDetailed Description of the Pull Request / Additional comments
A lot of the work here was having to deal with winmd files across all of these projects. The TerminalCore project now outputs a Microsoft.Terminal.TerminalControl.winmd. Some magic happens in TerminalControl.vcxproj to get this to work properly.
Validation Steps Performed
Deployed Windows Terminal and opened a few new tabs.