7124: Add focus and maximizedFocus launch modes #7873

Don-Vito · 2020-10-09T14:16:53Z

This commit introduces two new launch modes: focus and maximizedFocus.

Focused mode, behaves like a default mode, but with the Focus Mode
enabled.
Maximized focused mode, behaves like a Maximized mode, but with the
Focus Mode enabled.

There two ways to invoke these new modes:

In the settings file: you set the "launchMode" to either "focus" or
"maximizedFocus"
In the command line options, you can path -f / --focus, which is
mutually exclusive with the --fullscreen, but can be combined with the
--maximized:
- Passing -f / --focus will launch the terminal in the "focus" mode
- Passing -fM / --focus --maximized will launch the terminal in the
  "maximizedFocus" mode

This should resolve a relevant part in the command line arguments
mega-thread #4632

Closes #7124
Closes #7825
Closes #7875

… If one of them is set, focusedMode should be toggled on startup

zadjii-msft · 2020-10-09T14:38:49Z

I love this. I think you've done the focused mode thing exactly right. My one concern - I'd yank the maximizedFocus mode. In my opinion, that's just the same thing as fullscreen mode 😄

Don-Vito · 2020-10-09T14:53:13Z

I love this. I think you've done the focused mode thing exactly right. My one concern - I'd yank the maximizedFocus mode. In my opinion, that's just the same thing as fullscreen mode 😄

The only difference from the fullscreen is that in the maximized focused mode, you still see the taskbar (not sure how useful it is). Added this mode as it seems that in was originally mentioned in the feature request (#7124): "I want to have maximized and focused mode when app is launch" (#7124 (comment))

Please let me know if you prefer me to delete anyway.

zadjii-msft · 2020-10-09T15:09:32Z

Oh, fair point! Then I'm totally cool with this. I don't hate maximizedFocus either - I think maximized|focus would be wrong, because that would imply the whole set is flags that could be combined (they aren't). It's basically either maximizedFocus or focusMaximized, and I don't feel like there's a tangible difference between the two, so let's go with what's already been implemented 😛

Don-Vito · 2020-10-09T15:21:54Z

Oh, fair point! Then I'm totally cool with this. I don't hate maximizedFocus either - I think maximized|focus would be wrong, because that would imply the whole set is flags that could be combined (they aren't). It's basically either maximizedFocus or focusMaximized, and I don't feel like there's a tangible difference between the two, so let's go with what's already been implemented 😛

Yay :)
Should I add some UTs? If so - can you please guide me to the framework that I can base on?
If not - should I mark this as "Ready for review"? (I am quite new with the Draft PRs, work with Azure DevOps mostly).

zadjii-msft · 2020-10-09T15:29:34Z

Should I add some UTs?

Bless your soul ☺️

In this specific case, since this is such a UI-dependent feature, I think we're cool not adding any unit tests. The enum parsing is already pretty well tested, and I'm not sure we've got any sort of framework for testing the state of the Win32 window. I'd definitely mark this as "Ready for Review" 😄

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h

Don-Vito · 2020-10-10T08:57:58Z

@zadjii-msft - sorry for pushing another change after the approval (I was sure that this new commit will reset it, but for some reason it didn't).

I have noticed that in the megathread (#4632) there is also a request to add command-line option for this. So added it as well.

zadjii-msft

are you a mind reader?

I was gonna go ahead and sneak the cmdline argument in after this PR merged, but hey, you've gone and already done it. Thanks for the diligence! I quite like the elegance of -Mf for combining focus and maximized too

DHowett

I believe we'll need to update the schema in doc/cascadia! This looks excellent otherwise.

Don-Vito · 2020-10-13T06:43:36Z

Added the schema to the cascadia docs. Should I do anything beyond validating that it works on different profile?
This schema corresponds to the documentation PR I put in the docs repo (MicrosoftDocs/terminal#158)

lhecker

I believe you can drop MaximizedFocusMode.
You could get the same effect by calling wt.exe --focus --maximized instead, couldn't you?

lhecker · 2020-10-13T10:17:55Z

src/cascadia/TerminalApp/AppCommandlineArgs.cpp

@@ -175,14 +175,24 @@ void AppCommandlineArgs::_buildParser()
    //   -M,--maximized: Maximizes the window on launch
    //   -F,--fullscreen: Fullscreens the window on launch
    auto maximizedCallback = [this](int64_t /*count*/) {
-        _launchMode = winrt::Microsoft::Terminal::Settings::Model::LaunchMode::MaximizedMode;
+        _launchMode = (_launchMode.has_value() && _launchMode.value() == winrt::Microsoft::Terminal::Settings::Model::LaunchMode::FocusMode) ?


You can drop the winrt::Microsoft::Terminal::Settings::Model:: prefix, because the file already imports the namespace. 👍

Thanks! Done!

Don-Vito · 2020-10-13T20:14:28Z

I believe you can drop MaximizedFocusMode.
You could get the same effect by calling wt.exe --focus --maximized instead, couldn't you?

@lhecker - please guide me here. How do you suggest to encode the combination of --focus --maximized instead?

…ndLineArgs

Don-Vito · 2020-10-14T06:22:42Z

@DHowett - sorry for nagging, but CI is somewhat harsh with me today and I am not sure if I can somehow unblock this PR 😄

The github spelling complains about something I already fixed (I wrote betwen instead of between).
The OutputTests::BasicReadConsoleOutputATest fails - though it passes locally for me, and I can hardly follow the link between it and the change.

DHowett

Excellent, excellent stuff. Thank you!

DHowett · 2020-10-14T22:19:02Z

I have several questions regarding the feature and the PR

This is my first PR, please be gentle

This is excellent work for a first PR -- though your other work merged first! 😄
We're happy to have you.

I hesitated between introducing new launch modes or adding a separate setting for "startInFocused" mode. I >preferred not to introduce a new parameter, was this a good decision?

Looks like we went w/ a parameter.

Didn't updated the documentation yet - wanted to hear your opinion first.

All set!

Not sure how to test this in UT. Didn't find UT for other modes.

@zadjii-msft summed it up quite nicely.

Didn't add those modes to args. Please let me know if required (and if so, the names of the options)

All set again! 😁

ghost · 2020-10-14T22:19:30Z

Hello @DHowett!

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.

## Summary of the Pull Request Since we've started working on the Settings UI, a few settings have been added on `main`. This adds those missing settings over. ## References Missing settings include... - #7364: `disableAnimations` - #7873: `launchMode` `focus` and `maximizedFocus` - #7793: `bellStyle` ## Validation Steps Performed Verified that those settings appear properly in the Settings UI.

ghost · 2020-11-11T18:14:17Z

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

Handy links:

7124: initial commit - add focused and maximizedFocused launch modes.…

f5b5e33

… If one of them is set, focusedMode should be toggled on startup

ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Oct 9, 2020

7124: fix code formatting

a2d3662

zadjii-msft self-assigned this Oct 9, 2020

zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 9, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 9, 2020

zadjii-msft reviewed Oct 9, 2020

View reviewed changes

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h Outdated Show resolved Hide resolved

7124: rename focused -> focus, both in the settings name and in the code

bb0d3b1

zadjii-msft approved these changes Oct 9, 2020

View reviewed changes

zadjii-msft removed their assignment Oct 9, 2020

Don-Vito marked this pull request as ready for review October 9, 2020 16:04

Don-Vito changed the title ~~7124: Add focused and maximizedFocused launch modes~~ 7124: Add focus and maximizedFocus launch modes Oct 9, 2020

This was referenced Oct 9, 2020

7124: add documentation for the new launch modes MicrosoftDocs/terminal#158

Merged

Add command line flag to start in focus mode #7875

Closed

7124: add comman line option for launching in focus mode

45c466d

Don-Vito marked this pull request as draft October 10, 2020 09:49

7124: add UT for multiple commands

73a4fe1

Don-Vito requested a review from zadjii-msft October 10, 2020 12:49

Don-Vito marked this pull request as ready for review October 10, 2020 12:49

zadjii-msft self-assigned this Oct 12, 2020

zadjii-msft approved these changes Oct 12, 2020

View reviewed changes

zadjii-msft removed their assignment Oct 12, 2020

ghost requested review from DHowett, leonMSFT and PankajBhojwani October 12, 2020 22:34

DHowett requested changes Oct 13, 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 Oct 13, 2020

DHowett added the hacktoberfest-accepted label Oct 13, 2020

7124: add new launch modes to the schema in doc cascadia

cf9016c

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 13, 2020

Don-Vito requested a review from DHowett October 13, 2020 06:43

7124: add new launch modes to the schema in doc cascadia

ad03ae0

lhecker reviewed Oct 13, 2020

View reviewed changes

zadjii-msft assigned DHowett Oct 13, 2020

Don-Vito added 3 commits October 13, 2020 23:36

7124: remove explicit namespace and add comment the flags in AppComma…

97abb96

…ndLineArgs

7124: fix typo in a comment

1fb5e83

7124: fix another typo in a comment

bf1f23b

DHowett approved these changes Oct 14, 2020

View reviewed changes

ghost added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Oct 14, 2020

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 14, 2020

ghost merged commit f86045e into microsoft:master Oct 14, 2020

This was referenced Oct 30, 2020

Apply Enum Mappings to most enum settings #8090

Closed

Add missing settings to Settings UI #8116

Merged

This was referenced Nov 11, 2020

focusMode option for launchMode setting #7825

Closed

Focus mode on app startup/launch #7124

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7124: Add focus and maximizedFocus launch modes #7873

7124: Add focus and maximizedFocus launch modes #7873

Don-Vito commented Oct 9, 2020 •

edited by DHowett

Loading

zadjii-msft commented Oct 9, 2020

Don-Vito commented Oct 9, 2020

zadjii-msft commented Oct 9, 2020

Don-Vito commented Oct 9, 2020

zadjii-msft commented Oct 9, 2020

Don-Vito commented Oct 10, 2020 •

edited

Loading

zadjii-msft left a comment

DHowett left a comment

Don-Vito commented Oct 13, 2020

lhecker left a comment

lhecker Oct 13, 2020

Don-Vito Oct 13, 2020

Don-Vito commented Oct 13, 2020

Don-Vito commented Oct 14, 2020

DHowett left a comment

DHowett commented Oct 14, 2020

I have several questions regarding the feature and the PR

ghost commented Oct 14, 2020

ghost commented Nov 11, 2020

7124: Add focus and maximizedFocus launch modes #7873

7124: Add focus and maximizedFocus launch modes #7873

Conversation

Don-Vito commented Oct 9, 2020 • edited by DHowett Loading

zadjii-msft commented Oct 9, 2020

Don-Vito commented Oct 9, 2020

zadjii-msft commented Oct 9, 2020

Don-Vito commented Oct 9, 2020

zadjii-msft commented Oct 9, 2020

Don-Vito commented Oct 10, 2020 • edited Loading

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Don-Vito commented Oct 13, 2020

lhecker left a comment

Choose a reason for hiding this comment

lhecker Oct 13, 2020

Choose a reason for hiding this comment

Don-Vito Oct 13, 2020

Choose a reason for hiding this comment

Don-Vito commented Oct 13, 2020

Don-Vito commented Oct 14, 2020

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Oct 14, 2020

I have several questions regarding the feature and the PR

ghost commented Oct 14, 2020

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.

ghost commented Nov 11, 2020

Don-Vito commented Oct 9, 2020 •

edited by DHowett

Loading

Don-Vito commented Oct 10, 2020 •

edited

Loading

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.