Only focus the active pane once initialization is complete #10978
Conversation
This file contains 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
This works well for ``` wtd sp ; sp ; sp ``` and for ``` wtd sp ; sp ; sp ; fp -t 0 ``` But it doesn't work at all for ``` wtd sp ; sp ; sp ; mf left ``` Presumably because we can't `move-focus` directionally during startup.
Primarily with splitting not on the commandline. Also with using the cmdpal to execute commands
ghost
added
Area-Commandline
|
I'm curious, why doesn't the directional movement work in startup? Is it because the actual controls don't have a size yet? I will happily report that mf {next,previous}InOrder does work during startup should you need the functionality. |
Exactly that. See for example, |
|
Would you try adding this commit and see if directional movement works during startup with it? Rosefield@003125e If so I can clean it up and make a separate PR with it. |
DHowett
approved these changes
Aug 20, 2021
There was a problem hiding this comment.
I can't immediately see any issues here, so LGTM! Interested to see you try @Rosefield's suggestion!
Oh my god it works fantastically. |
Thanks!
ghost
requested review from
miniksa,
carlos-zamora,
leonMSFT,
PankajBhojwani and
lhecker
PankajBhojwani
approved these changes
Aug 23, 2021
zadjii-msft
added
the
AutoMerge
|
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 (
|
ghost
pushed a commit
that referenced
this pull request
Aug 24, 2021
During startup we do not have real dimensions, so we have to guess what our dimensions should be based off of the splits. We'll augment the state of the pane search to also have a size in each dimension that gets incrementally upgraded as we recurse through the tree. References #10978
DHowett
pushed a commit
that referenced
this pull request
Aug 25, 2021
Since the days immemorial of the Terminal, the TermControl has auto-focused itself when it finalizes its layout. This has led to the problem that `wt ; sp ; sp ; sp...` ends up focusing one of these panes at random. This PR fixes this issue by getting rid of the auto-focusing. Panes now manually get focused when created. We manually focus the active pane when a commandline is dispatched. since we're internally tracking "active" separate from "focused", this ends up working as you'd hope. * [x] Closes #6586 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated I also had to turn the cursor off by default. Most `TermControl`s would never get the `LostFocus` event, so their cursors would get left `On`, and that's not right. I've run the following things a bunch of times to make sure they work: * `wtd sp ; sp ; sp` * `wtd sp ; sp ; sp ; fp -t 0` * `newTab` * `splitPane` * use the command palette to do the above as well Where the result used to be random (cases 1 & 2), the result is exactly what you'd expect now. It doesn't work at all for ``` wtd sp ; sp ; sp ; mf left ``` Presumably because we can't `move-focus` directionally during startup. However, that doesn't work _today_ either, so it's not making it worse. Just highlights that single scenario doesn't work right.
|
🎉 Handy links: |
ghost
mentioned this pull request
|
🎉 Handy links: |
ghost
pushed a commit
that referenced
this pull request
Sep 8, 2021
This commit adds initial support for saving window layout on application close. Done: - Add user setting for if tabs should be maintained. - Added events to track the number of open windows for the monarch, and then save if you are the last window closing. - Saves layout when the user explicitly hits the "Close Window" button. - If the user manually closed all of their tabs (through the tab x button or through closing all panes on the tab) then remove any saved state. - Saves in the ApplicationState file a list of actions the terminal can perform to restore its layout and the window size/position information. - This saves an action to focus the correct pane, but this won't actually work without #10978. Note that if you have a pane zoomed, it does still zoom the correct pane, but when you unzoom it will have a different pane selected. Todo: - multiple windows? Right now it can only handle loading/saving one window. - PR #11083 will save multiple windows. - This also sometimes runs into the existing bug where multiple tabs appear to be focused on opening. Next Steps: - The business logic of when the save is triggered can be adjusted as necessary. - Right now I am taking the pragmatic approach and just saving the state as an array of objects, but only ever populate it with 1, that way saving multiple windows in the future could be added without breaking schema compatibility. Selfishly I'm hoping that handling multiple windows could be spun off into another pr/feature for now. - One possible thing that can maybe be done is that the commandline can be augmented with a "--saved ##" attribute that would load from the nth saved state if it exists. e.g. if there are 3 saved windows, on first load it can spawn three wt --saved {0,1,2} that would reopen the windows? This way there also exists a way to load a copy of a previous window (if it is in the saved state). - Is the application state something that is planned to be public/user editable? In theory the user could since it is just json, but I don't know what it buys them over just modifying their settings and startupActions. Validation Steps Performed: - The happy path: open terminal -> set setting to true -> close terminal -> reopen and see tabs. Tested with powershell/cmd/wsl windows. - That closing all panes/tabs on their own will remove the saved session. - Open multiple windows, close windows and confirm that the last window closed saves its state. The generated file stores a sequence of actions that will be executed to restore the terminal to its saved form. References #8324 This is also one of the items on #5000 Closes #766
|
🎉 Handy links: |
3 tasks
ghost
pushed a commit
that referenced
this pull request
Jan 10, 2022
I'm not 100% sure that this is the right solution, but it does seem to work well enough. This is unfortunately a classic heisenbug. It was already hard enough to repro originally, but attaching a debugger made it totally impossible to hit. My theory is that it's possible for the GotFocus event to fire before the LayoutUpdated event does. If that were to occur, then we'd try to turn on the cursor timer before it exists, gracefully do nothing, then create the timer. In that case, we'd never get a subsequent message to start the blinking. I tested that theory by just initializing the cursor blinker to our `_focused` state. In that case, if the control has already been focused at the time of the LayoutUpdated event, then we can init the cursor to the correct state. Testing that out, I couldn't once get this to repro, which makes me think this works. I've opened some 900 (<sup>hyperbole</sup>) tabs now, so I'm pretty confident I'd have seen it by now. * Regressed in #10978 * [x] fixes #11411 * [x] I made sure I didn't regress #6586 * [x] I work here
DHowett
pushed a commit
that referenced
this pull request
Jan 21, 2022
I'm not 100% sure that this is the right solution, but it does seem to work well enough. This is unfortunately a classic heisenbug. It was already hard enough to repro originally, but attaching a debugger made it totally impossible to hit. My theory is that it's possible for the GotFocus event to fire before the LayoutUpdated event does. If that were to occur, then we'd try to turn on the cursor timer before it exists, gracefully do nothing, then create the timer. In that case, we'd never get a subsequent message to start the blinking. I tested that theory by just initializing the cursor blinker to our `_focused` state. In that case, if the control has already been focused at the time of the LayoutUpdated event, then we can init the cursor to the correct state. Testing that out, I couldn't once get this to repro, which makes me think this works. I've opened some 900 (<sup>hyperbole</sup>) tabs now, so I'm pretty confident I'd have seen it by now. * Regressed in #10978 * [x] fixes #11411 * [x] I made sure I didn't regress #6586 * [x] I work here (cherry picked from commit 55aea08)
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary of the Pull Request
Since the days immemorial of the Terminal, the TermControl has auto-focused itself when it finalizes its layout. This has led to the problem that
wt ; sp ; sp ; sp...ends up focusing one of these panes at random.This PR fixes this issue by getting rid of the auto-focusing. Panes now manually get focused when created. We manually focus the active pane when a commandline is dispatched. since we're internally tracking "active" separate from "focused", this ends up working as you'd hope.
References
PR Checklist
Detailed Description of the Pull Request / Additional comments
I also had to turn the cursor off by default. Most
TermControls would never get theLostFocusevent, so their cursors would get leftOn, and that's not right.Validation Steps Performed
I've run the following things a bunch of times to make sure they work:
wtd sp ; sp ; spwtd sp ; sp ; sp ; fp -t 0newTabsplitPaneWhere the result used to be random (cases 1 & 2), the result is exactly what you'd expect now.
It doesn't work at all for
Presumably because we can't
move-focusdirectionally during startup. However, that doesn't work today either, so it's not making it worse. Just highlights that single scenario doesn't work right.