Add CenterOnLaunch property #8414

ghost · 2020-11-27T06:32:47Z

Summary of the Pull Request

If the users set true to this property, the terminal will appear on the center of the user's display.

References

PR Checklist

Closes Have an option to centre the terminal if a certain InitialPosition value is given #7722
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated.
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

github-actions · 2020-11-27T07:27:17Z

New misspellings found, please review:

dimentions

To accept these changes, run the following commands

perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/5757ec679b03a4240130c3c53766c91bbc5cd6a7.txt
.github/actions/spell-check/expect/655f007265b351e140d20b3976792523ad689241.txt
.github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"AAAAA Bopomofo CParams CSV GENERATEPROJECTPRIFILE hhhh Inlines MAKEINTRESOURCEA Reserialize rgus SGRXY UDK UDKs Unfocus xe xlang "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/d8ca6bc08a7d505877d6af320c75ba7d77cfa897.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"csv dimentions inlines reserialize udk unfocus "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/d8ca6bc08a7d505877d6af320c75ba7d77cfa897.txt is added to your repository...'

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spell-check/dictionary/names.txt.
... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

github-actions · 2020-11-27T14:28:47Z

New misspellings found, please review:

dimentions

To accept these changes, run the following commands

perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/5757ec679b03a4240130c3c53766c91bbc5cd6a7.txt
.github/actions/spell-check/expect/655f007265b351e140d20b3976792523ad689241.txt
.github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"AAAAA Bopomofo CParams CSV GENERATEPROJECTPRIFILE hhhh Inlines MAKEINTRESOURCEA Reserialize rgus SGRXY UDK UDKs Unfocus xe xlang "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/5f27ad5491f55126a1854a5f3f3e44aafdd51b68.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"csv dimentions inlines reserialize udk unfocus "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/5f27ad5491f55126a1854a5f3f3e44aafdd51b68.txt is added to your repository...'

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spell-check/dictionary/names.txt.
... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

carlos-zamora

A few things:

Please change "centre" to "center". This helps make the en-us locale consistent in our repo 😊
Concerns about the design
- centerOnLaunch seems to have a conflict with initialPosition. Rather than prioritizing centerOnLaunch, how about we consolidate them like this: initialPosition: "," | "#," | ",#" | "#,#" | "center"
- I definitely want another team member to sign off on a design though. Especially since I apparently have a bad habit of consolidating settings a bit too zealously haha

Aside from that, the implementation looks solid. Thanks!

carlos-zamora · 2020-12-02T18:28:26Z

src/cascadia/TerminalApp/AppLogic.cpp

+        int _width = 0;
+        int _height = 0;


Suggested change

int _width = 0;

int _height = 0;

int desktopWidth = 0;

int desktopHeight = 0;

We generally use _ to denote that it's a member. So let's remove that to be consistent.

And added desktop since you introduced appWidth and appHeight below. That way there's a better distinction between the two 😉

zadjii-msft · 2020-12-03T11:44:43Z

Alright, team sanity check - are we cool with adding a centerOnLaunch:bool property that overrides initialPosition? If centerOnLaunch is set to true, then initialPosition will be ignored.

The only other possible design would be overloading initialPosition so it's "initialPosition": coord|"center", but I hate that.

@DHowett and @carlos-zamora are the usual suspects for having strong opinions on this

carlos-zamora · 2020-12-03T18:22:57Z

Alright, team sanity check - are we cool with adding a centerOnLaunch:bool property that overrides initialPosition? If centerOnLaunch is set to true, then initialPosition will be ignored.

The only other possible design would be overloading initialPosition so it's "initialPosition": coord|"center", but I hate that.

@DHowett and @carlos-zamora are the usual suspects for having strong opinions on this

I'm a sucker for "initialPosition": coord|"center" hahaha. And I'm really not a fan of a setting that makes another setting ignored. If we don't go with the overloaded initialPosition design, could we at least come up with a third design that doesn't cause another setting to be ignored?

zadjii-msft · 2020-12-03T18:41:48Z

Maybe I was a little strong in the morning - I guess I don't hate it. But if I abstain, then we'll only have two people voting, so we'd need consensus, or more voters. I suppose launchMode could be used? That's already used for maximized, which overrides initialPosition, but I guess then we'd need centered and centeredFocus and we're getting dangerously close to the states problem haha.

Yea I like overloading launchMode the least, let's not do that

DHowett · 2020-12-03T18:44:23Z

The interaction between the two properties initialPosition and centerOnLaunch is unclear, and deserves its own mini spec.

Here's the problem:

launchMode maximized = maximize the window
- no initialPosition = maximize on main monitor
- initialPosition x<0 = maximize on left monitor
- initialPosition y<0 = maximize on top monitor
- initialPosition center = ?
initialPosition center = center the window
- how do I specify which monitor to center on?

zadjii-msft · 2020-12-03T18:57:53Z

That's a good point. I suppose I thought launchMode was a good example of a property whos value already made initalPosition ignored, but I guess I'm wrong about that.

So initialPosition+centered:true is "I want it to be centered on this monitor", and that makes sense to me

ghost · 2020-12-11T06:00:09Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

ghost · 2020-12-11T06:46:03Z

I will fix this.

zadjii-msft · 2021-01-27T18:41:52Z

Hey I just want a quick ACK, I think nearly 2 months ago we discussed offline being cool with adding this as a separate bool setting. We still on board with that? So the interactions are like:

initialPos: x,y, centered: true, launchMode: default: center on the monitor that x,y is on
initialPos: x,y, centered: true, launchMode: maximized: maximized on the monitor that x,y is on (centered adds nothing)
initialPos: <omitted>, centered: true, launchMode: default: center on the default monitor
initialPos: <omitted>, centered: true, launchMode: focus: center, focus mode on the default monitor
initialPos: <omitted>, centered: true, launchMode: maximized: maximized on the default monitor (centered adds nothing)

@DHowett

carlos-zamora · 2021-02-01T22:42:22Z

Hey I just want a quick ACK, I think nearly 2 months ago we discussed offline being cool with adding this as a separate bool setting. We still on board with that? So the interactions are like:

initialPos: x,y, centered: true, launchMode: default: center on the monitor that x,y is on

initialPos: x,y, centered: true, launchMode: maximized: maximized on the monitor that x,y is on (centered adds nothing)

initialPos: <omitted>, centered: true, launchMode: default: center on the default monitor

initialPos: <omitted>, centered: true, launchMode: focus: center, focus mode on the default monitor

initialPos: <omitted>, centered: true, launchMode: maximized: maximized on the default monitor (centered adds nothing)

@DHowett

This seems right to me!

zadjii-msft · 2021-02-04T13:16:37Z

Moved to #9036

This PR is a resurrection of #8414. @Hegunumo has apparently deleted their account, but the contribution was still valuable. I'm just here to get it across the finish line. This PR adds new global setting `centerOnLaunch`. When set to `true`, the Terminal window will be centered on the display it opens on. So the interactions are like: * `initialPos: x,y`, `centered: true`, `launchMode: default` center on the monitor that x,y is on * `initialPos: x,y`, `centered: true`, `launchMode: maximized` maximized on the monitor that x,y is on (centered adds nothing) * `initialPos: <omitted>`, `centered: true`, `launchMode: default` center on the default monitor * `initialPos: <omitted>`, `centered: true`, `launchMode: focus` center, focus mode on the default monitor * `initialPos: <omitted>`, `centered: true`, `launchMode: maximized` maximized on the default monitor (centered adds nothing) ## Validation Steps Performed I've played with it on multiple different monitors, and it seems to work on all of them. Closes #8414 (original PR) Closes #7722 Co-authored-by: Kiminori Kaburagi <yukawa_hidenori@icloud.com>

ghost · 2021-03-01T18:03:36Z

🎉This issue was addressed in #9036, which has now been successfully released as Windows Terminal Preview v1.7.572.0.:tada:

Handy links:

Add CentreOnLaunch property

d8ca6bc

code format

5f27ad5

correct spelling

6a3a852

zadjii-msft self-assigned this Nov 30, 2020

carlos-zamora requested changes Dec 2, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 2, 2020

zadjii-msft assigned DHowett and unassigned zadjii-msft Dec 4, 2020

ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Dec 11, 2020

ghost removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Dec 11, 2020

zadjii-msft self-assigned this Jan 27, 2021

zadjii-msft mentioned this pull request Feb 4, 2021

Add centerOnLaunch setting #9036

Merged

zadjii-msft closed this Feb 4, 2021

DHowett mentioned this pull request Feb 19, 2021

Introduce setting override tracking and update SettingContainer #9079

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CenterOnLaunch property #8414

Add CenterOnLaunch property #8414

ghost commented Nov 27, 2020

github-actions bot commented Nov 27, 2020

github-actions bot commented Nov 27, 2020

carlos-zamora left a comment

carlos-zamora Dec 2, 2020

zadjii-msft commented Dec 3, 2020

carlos-zamora commented Dec 3, 2020

zadjii-msft commented Dec 3, 2020

DHowett commented Dec 3, 2020

zadjii-msft commented Dec 3, 2020

ghost commented Dec 11, 2020

ghost commented Dec 11, 2020

zadjii-msft commented Jan 27, 2021

carlos-zamora commented Feb 1, 2021

zadjii-msft commented Feb 4, 2021

ghost commented Mar 1, 2021

Add CenterOnLaunch property #8414

Add CenterOnLaunch property #8414

Conversation

ghost commented Nov 27, 2020

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

github-actions bot commented Nov 27, 2020

New misspellings found, please review:

⚠️ Reviewers

github-actions bot commented Nov 27, 2020

New misspellings found, please review:

⚠️ Reviewers

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora Dec 2, 2020

Choose a reason for hiding this comment

zadjii-msft commented Dec 3, 2020

carlos-zamora commented Dec 3, 2020

zadjii-msft commented Dec 3, 2020

DHowett commented Dec 3, 2020

zadjii-msft commented Dec 3, 2020

ghost commented Dec 11, 2020

ghost commented Dec 11, 2020

zadjii-msft commented Jan 27, 2021

carlos-zamora commented Feb 1, 2021

zadjii-msft commented Feb 4, 2021

ghost commented Mar 1, 2021