Merged
Conversation
carlos-zamora
added
Product-Terminal
carlos-zamora
force-pushed
the
dev/cazamor/set/winrt-app-obj-prof
branch
from
b8f476b to
1a20cb3
Compare
carlos-zamora
force-pushed
the
dev/cazamor/set/winrt-app-obj-glob
branch
from
37d6010 to
4b4dd7a
Compare
carlos-zamora
force-pushed
the
dev/cazamor/set/winrt-app-obj-prof
branch
from
6051822 to
8396b2d
Compare
carlos-zamora
force-pushed
the
dev/cazamor/set/winrt-app-obj-glob
branch
from
4b4dd7a to
5fe8795
Compare
carlos-zamora
force-pushed
the
dev/cazamor/set/winrt-app-obj-prof
branch
from
8396b2d to
32b9223
Compare
carlos-zamora
force-pushed
the
dev/cazamor/set/winrt-app-obj-glob
branch
2 times, most recently
from
269f005 to
58dcf17
Compare
DHowett
reviewed
Aug 25, 2020
zadjii-msft
approved these changes
Aug 25, 2020
Member
zadjii-msft
left a comment
zadjii-msft
left a comment
There was a problem hiding this comment.
Huh, none of these look blocking to me. Good work!
| class ColorSchemeTests; | ||
| }; | ||
|
|
||
| namespace TerminalApp |
Member
There was a problem hiding this comment.
protip: hide whitespace changes
| GETSET_PROPERTY(winrt::Microsoft::Terminal::TerminalControl::CopyFormat, CopyFormatting, 0); | ||
| GETSET_PROPERTY(bool, WarnAboutLargePaste, true); | ||
| GETSET_PROPERTY(bool, WarnAboutMultiLinePaste, true); | ||
| GETSET_PROPERTY(winrt::TerminalApp::LaunchPosition, InitialPosition, nullptr, nullptr); |
Member
There was a problem hiding this comment.
I'm shocked that nullptr, nullptr worked here, but happy that it does
Member
There was a problem hiding this comment.
clever macro. uses __VA_ARGS__ to splat "everything at the end"
| proposedRect.left = gsl::narrow_cast<long>(initialPosition.X); | ||
| proposedRect.top = gsl::narrow_cast<long>(initialPosition.Y); | ||
| auto initialPos = _logic.GetInitialPosition(proposedRect.left, proposedRect.top); | ||
| proposedRect.left = static_cast<long>(initialPos.X); |
Member
There was a problem hiding this comment.
of course long !== int64_t. That's annoying
DHowett
requested changes
Aug 27, 2020
| GETSET_PROPERTY(winrt::Microsoft::Terminal::TerminalControl::CopyFormat, CopyFormatting, 0); | ||
| GETSET_PROPERTY(bool, WarnAboutLargePaste, true); | ||
| GETSET_PROPERTY(bool, WarnAboutMultiLinePaste, true); | ||
| GETSET_PROPERTY(winrt::TerminalApp::LaunchPosition, InitialPosition, nullptr, nullptr); |
Member
There was a problem hiding this comment.
clever macro. uses __VA_ARGS__ to splat "everything at the end"
ghost
added
the
Needs-Author-Feedback
carlos-zamora
force-pushed
the
dev/cazamor/set/winrt-app-obj-glob
branch
from
a150e15 to
6d5c578
Compare
ghost
removed
the
Needs-Author-Feedback
DHowett
reviewed
Aug 28, 2020
DHowett
reviewed
Aug 28, 2020
DHowett
approved these changes
Aug 28, 2020
ghost
closed this
carlos-zamora
changed the base branch from
dev/cazamor/set/winrt-app-obj-prof
to
master
carlos-zamora
force-pushed
the
dev/cazamor/set/winrt-app-obj-glob
branch
from
c8398bc to
7cd72aa
Compare
carlos-zamora
added
the
AutoMerge
|
Hello @carlos-zamora! 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 
|
🎉 Handy links: |
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.
GlobalAppSettings is now a WinRT object in the TerminalApp project.
References
#7141 - GlobalAppSettings is a settings object
#885 - this new settings object will be moved to a new TerminalSettingsModel project
PR Checklist
Detailed Description of the Pull Request / Additional comments
This one was probably the easiest thus far.
The only weird thing is how we handle InitialPosition. Today, we lose a
little bit of fidelity when we convert from LaunchPosition (int) -->
Point (float) --> RECT (long). The current change converts
LaunchPosition (optional) --> InitialPosition (long) --> RECT
(long).
NOTE: Though I could use LaunchPosition to go directly from TermApp to
AppHost, I decided to introduce InitialPosition because LaunchPosition
will be a part of TerminalSettingsModel soon.
Validation Steps Performed