Implement InitialPosition and CenterOnLaunch in the SUI #13605

PankajBhojwani · 2022-07-26T20:53:47Z

Summary of the Pull Request

InitialPosition and CenterOnLaunch can now be edited in the SUI

PR Checklist

Closes SUI: Add settings for initalPosition, centerOnLaunch #9075
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

InitialPosition follows the same style as LaunchSize, with a number box for the x coordinate and a number box for the y coordinate. When there is no value for either of these coordinates, the respective number box is empty (and displays the text Undefined).

Validation Steps Performed

They work

DHowett · 2022-07-26T21:11:06Z

Should we put Center on Launch inside the expander for position? it's categorically related

src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp

carlos-zamora

Design Questions/Topics

Do we want to add a toggle switch to x/y-pos labeled something like "Use system default", and when it's enabled, we disable the x/y-pos?
- My concern is that the "Undefined" (or NaN value) isn't discoverable. I assume most people would want to keep this as "Use system default".
Ideally, we would have a better UI than just putting the number in. I envisioned something like using your cursor as a crosshair to pick the position on a monitor, or pick the monitor itself. Then, that position gets converted to the InitialPosition in the settings UI. That said, that requires a lot more design and work. If we're not going down that path (or at least a path that's more user friendly), I suggest we file a work-item and throw it in the backlog to track this work.
Building on that, maybe we want to expose this completely differently without the complex control above. Consider this...
- a ComboBox with...
  - "Let windows decide" or "Use system default"
  - "On a monitor"
    - when selected, we show a NumberBox for "Monitor ID" that refers to the same monitor as that of Settings app > System > Display (hmm, this part might be hard, ugh)
    - we could probably just use this this as an alias for "center on launch" too?
  - "Custom position in pixels"
    - show the two number pickers for x and y
- The main thing missing here is just the case where "fullscreen"/"maximized" mode have you pick the monitor. :/. Maybe we make it so that if you change the launch mode to either of those values, we update this control to be "on a monitor"?

Corner cases

What happens if the user puts in (via SUI)...
- invalid input --> do we change to "Undefined"?
- a decimal number --> do we store it as an int in the settings model, but (more importantly) show it as a decimal or int in the SUI?

carlos-zamora · 2022-07-28T19:28:32Z

src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp

+        // put the placeholder text in the box instead
+        if (auto xCoord{ x.try_as<int64_t>() })
+        {
+            return xCoord.has_value() ? gsl::narrow_cast<double>(xCoord.value()) : NAN;


urgh. Do we have to worry about the data getting corrupted with this narrow_cast? Like, if the JSON has a really big int that couldn't fit in the double?

src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

PankajBhojwani · 2022-08-01T21:15:32Z

Consensus from discussion:

Remove CenterOnLaunch as a setting in the UI, add it as an enum option to LaunchMode and the VM can handle the backend of mapping that enum to our internal CenterOnLaunch
Combine the LaunchMode combo box and the LaunchPosition number boxes into one expander

Follow up in a future PR:

Add a button Use current position that will populate the fields according to the Terminal's current position

DHowett · 2022-08-01T22:00:53Z

(Given the consensus opinion, How do you propose to have "centered focus mode" as a launch mode in the drop-down?)

zadjii-msft · 2022-08-02T12:01:43Z

How do you propose to have "centered focus mode" as a launch mode in the drop-down?

Okay fine, we need "Centered focus mode" in there too. But "Centered Maximized*" seemed viable to leave out. Technically yea you can have it launch centered (and maximize itself), and then restore down to centered. This design occludes that particular combination of settings for presenting the user something more sensible.

If we're dead set on including that as a separate toggle, I'd instead propose:

A set of controls under the Launch Positioning expander.

The "launch mode" dropdown
Positioning radio buttons
- Position manually
  - An X/Y input
    - The "use current position" button
- Use system default
A toggle for "center on launch

Heck, it doesn't even need to be one expando then. Could be three:

mode
position (with the system default checkbox, ala the "desktopBackground" checkbox in the profile appearance page, and eventually the "use current pos" button)
center on launch

Maybe we hoped on the "add center to the dropdown" crazy idea train too fast. <discuss more>

zadjii-msft · 2022-09-12T22:05:33Z

…/pabhoj/dxd_sui

PankajBhojwani · 2022-09-27T18:08:02Z

updated screenshots:

carlos-zamora

Good work! I all of these are small changes.

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

carlos-zamora · 2022-10-03T20:26:21Z

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

+    <comment>Header for a set of settings that determine how terminal launches.</comment>
+  </data>
+  <data name="Globals_LaunchParameters.HelpText" xml:space="preserve">
+    <value>Settings that control how the terminal launches</value>


Suggested change

<value>Settings that control how the terminal launches</value>

<value>Settings that control where the terminal launches</value>

This is no longer just 'where' though, this expander now also contains LaunchMode

src/cascadia/TerminalSettingsEditor/Launch.xaml

ghost · 2022-10-05T19:36:52Z

Hello @PankajBhojwani!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

DHowett

Post-hoc

DHowett · 2022-10-07T17:55:11Z

src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp

+
+        if (UseDefaultLaunchPosition())
+        {
+            result = fmt::format(L"{}, {}, {}", launchModeString, RS_(L"Globals_LaunchModeDefault/Content"), centerOnLaunchString);


I would have structured it a bit differently.

It would say:

Maximized, Default Position Normal, Default Position, Centered Minimized, (3, 3) Focus, (4, 100), Centered

That way, we wouldn't include the less-meaningful strings "on" and "off" and we would only show contextually-appropriate data.

DHowett · 2022-10-07T17:55:37Z

src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp

+
+        if (UseDefaultLaunchPosition())
+        {
+            result = fmt::format(L"{}, {}, {}", launchModeString, RS_(L"Globals_LaunchModeDefault/Content"), centerOnLaunchString);


(That is: replace off with nothing, and on with Centered)

DHowett · 2022-10-07T17:57:12Z

src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp

+        {
+            InitialPosX(NAN);
+            InitialPosY(NAN);
+            _NotifyChanges(L"InitialPosX", L"InitialPosY");


This will generate up to three notifications for LaunchParametersCurrentValue, right? Because it will cascade through the InitialPos# setters as well.

Addressing post-hoc comments on the launch parameters expander in the SUI (added in #13605) - Use more contextually appropriate strings (`Centered` instead of `On` / `Off`) - Don't emit an extra `NotifyChanges` References #13605

ghost · 2023-01-24T18:21:13Z

🎉Windows Terminal Preview v1.17.1023 has been released which incorporates this pull request.:tada:

Handy links:

PankajBhojwani added 2 commits July 26, 2022 13:38

launch position

ade1658

center on launch

e7fbb44

ghost added Area-Settings UI Anything specific to the SUI Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Jul 26, 2022

dictionary

265bff2

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

lhecker reviewed Jul 28, 2022

View reviewed changes

src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalSettingsEditor/LaunchViewModel.cpp Outdated Show resolved Hide resolved

carlos-zamora requested changes Jul 28, 2022

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 Jul 28, 2022

conflict

02ba2c0

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 28, 2022

PankajBhojwani added 3 commits July 28, 2022 14:16

stylistic fixes

364b692

const

ee19ae3

format, resource strings

04fe18d

PankajBhojwani added the Needs-Discussion Something that requires a team discussion before we can proceed label Jul 28, 2022

lhecker approved these changes Jul 28, 2022

View reviewed changes

zadjii-msft added this to the Terminal v1.16 milestone Aug 1, 2022

zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Aug 1, 2022

zadjii-msft modified the milestones: Terminal v1.16, Terminal v1.17 Aug 26, 2022

PankajBhojwani added the Needs-Discussion Something that requires a team discussion before we can proceed label Sep 6, 2022

zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Sep 12, 2022

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

b87bea8

…/pabhoj/dxd_sui

PankajBhojwani added 2 commits September 14, 2022 16:14

Everything works except the current value

1ee23e9

works

7067cb4

carlos-zamora requested changes Oct 3, 2022

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 3, 2022

PankajBhojwani added 2 commits October 3, 2022 16:34

nits

f8b8ff0

accessibility things

8985ac6

carlos-zamora approved these changes Oct 5, 2022

View reviewed changes

PankajBhojwani added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 5, 2022

ghost merged commit 40bc3d7 into main Oct 5, 2022

ghost deleted the dev/pabhoj/dxd_sui branch October 5, 2022 19:37

DHowett reviewed Oct 7, 2022

View reviewed changes

PankajBhojwani mentioned this pull request Oct 11, 2022

Fix some issues with the launch parameters in the SUI #14186

Merged

zadjii-msft mentioned this pull request Jan 3, 2023

SUI: Add settings for initalPosition, centerOnLaunch #9075

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement InitialPosition and CenterOnLaunch in the SUI #13605

Implement InitialPosition and CenterOnLaunch in the SUI #13605

PankajBhojwani commented Jul 26, 2022

This comment has been minimized.

This comment was marked as outdated.

DHowett commented Jul 26, 2022

carlos-zamora left a comment

carlos-zamora Jul 28, 2022

PankajBhojwani commented Aug 1, 2022

DHowett commented Aug 1, 2022

zadjii-msft commented Aug 2, 2022

zadjii-msft commented Sep 12, 2022

PankajBhojwani commented Sep 27, 2022

carlos-zamora left a comment

carlos-zamora Oct 3, 2022

PankajBhojwani Oct 3, 2022

ghost commented Oct 5, 2022

DHowett left a comment

DHowett Oct 7, 2022

DHowett Oct 7, 2022

DHowett Oct 7, 2022

ghost commented Jan 24, 2023

	<value>Settings that control how the terminal launches</value>
	<value>Settings that control where the terminal launches</value>

Implement InitialPosition and CenterOnLaunch in the SUI #13605

Implement InitialPosition and CenterOnLaunch in the SUI #13605

Conversation

PankajBhojwani commented Jul 26, 2022

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

This comment has been minimized.

This comment was marked as outdated.

DHowett commented Jul 26, 2022

carlos-zamora left a comment

Choose a reason for hiding this comment

Design Questions/Topics

Corner cases

carlos-zamora Jul 28, 2022

Choose a reason for hiding this comment

PankajBhojwani commented Aug 1, 2022

DHowett commented Aug 1, 2022

zadjii-msft commented Aug 2, 2022

zadjii-msft commented Sep 12, 2022

PankajBhojwani commented Sep 27, 2022

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora Oct 3, 2022

Choose a reason for hiding this comment

PankajBhojwani Oct 3, 2022

Choose a reason for hiding this comment

ghost commented Oct 5, 2022

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 (@msftbot) and give me an instruction to get started! Learn more here.

DHowett left a comment

Choose a reason for hiding this comment

DHowett Oct 7, 2022

Choose a reason for hiding this comment

DHowett Oct 7, 2022

Choose a reason for hiding this comment

DHowett Oct 7, 2022

Choose a reason for hiding this comment

ghost commented Jan 24, 2023

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 (`@msftbot`) and give me an instruction to get started! Learn more here.