-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Move ICore/ControlSettings to TerminalControl project #7167
Conversation
This comment has been minimized.
This comment has been minimized.
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.
I'm at 35/67 but it's 5p here, so here's some initial feedback
@@ -344,7 +344,7 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting | |||
|
|||
// 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)); |
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 one seems weird. should we just be usinjg ClampSub<int>
or something?
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.
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 :/
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 is bizarre that this is needed now
@@ -303,7 +303,7 @@ class CommonState | |||
const CONSOLE_INFORMATION& gci = Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().getConsoleInformation(); | |||
// 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 = |
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.
interesting. not necessary for this change, i presume, but I'll allow it? 😁
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.
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)
🤷♂️
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.
Alright lets do it
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 (
|
Summary of the Pull Request
Move
ICoreSettings
andIControlSettings
from the TerminalSettings project to the TerminalCore and TerminalControl projects respectively. Also entirely removes the TerminalSettings project.The purpose of these interfaces is unchanged.
ICoreSettings
is 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
TerminalSettings
interfaces #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.