New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix focus and configuration handling of the integrated terminal #15958

Merged
merged 11 commits into from Nov 30, 2016

Conversation

Projects
None yet
4 participants
@kaiwood
Copy link
Contributor

kaiwood commented Nov 23, 2016

This PR fixes 2 problems with the focus handling of the integrated terminal (see #15103 and #14969) .

Although they result in a same usability/keybinding problem I initially described in #15103, my description was a little fuzzy because I didn't understand the cause fully.

There are currently 2 problems:


Problem Nr. 1

Whenever a second (or third, …) terminal instance gets exited via the shell command exit or by pressing ctrl-d (which basically is "exit" because it sends EOF), the terminal panel loses focus and the editor gets focus.

This hinders the workflow when working with multiple terminals, because you can't close a second one without refocusing the panel again, worst case scenario would be that a user write exit into an editor window without being aware of it.

The only case where focus has to change is if there is only 1 instance left, in this case the panel closes itself so the user has a visual clue about whats going on.

Just in case anyone asks, the reasoning I used focus(true) in setActiveInstanceByIndex() instead of inside _removeInstance() just in case there are some edge cases I didn't stumble upon yet. I'd guess you always want to focus your active instances, but correct me if I'm wrong.


Problem Nr. 2

To make the terminal work with various keyboard shortcuts, the setting terminal.integrated.commandsToSkipShell was introduced. The first part of my Issue I described in #15103 and I later found out to be described in #14969 as well is caused by the following:

Currently, the settings are only handled in the panel itself when _updateCommandsToSkipShell() is called (whenever the panel gets opened). If you create a new instance with either the +-Button in the GUI or via a shortcut, a new terminal instance get's created, gets attached – but _skipTerminalKeybindings is empty.

Calling the function to skip the commands from inside TerminalService#createInstance() made most sense to me because it's already aware of the settings, but correct me again if I'm wrong with this assumption.

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Nov 23, 2016

@kaiwood, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Tyriar, @octref and @chrmarti to be potential reviewers.

@Tyriar Tyriar added this to the November 2016 milestone Nov 24, 2016

@Tyriar
Copy link
Member

Tyriar left a comment

Thanks a lot for looking into this @kaiwood! I left a couple of comments.

@@ -80,6 +80,7 @@ export class TerminalService implements ITerminalService {
this.setActiveInstanceByIndex(0);
}
this._onInstancesChanged.fire();
terminalInstance.setCommandsToSkipShell(this.configHelper.getCommandsToSkipShell());

This comment has been minimized.

@Tyriar

Tyriar Nov 24, 2016

Member

You've actually stumbled upon a bug bigger than this in that no settings values are updated correctly, including scrollback and cursorBlinking as well. Having a brief look at the code, I think the best way to fix this might be to move TerminalPanel._updateConfig to TerminalService.updateConfig and then call into that after creating the new instance.

This comment has been minimized.

@kaiwood

kaiwood Nov 27, 2016

Author Contributor

Ok, after taking a closer look, there is quite some refactoring involved to do this right. _updateCursorBlink, _updateCommandsToSkipShell and _updateScrollback have to move too, makes no sense to leave them in the panel. But _updateFont has to move out of the old config update and stay on its own, its basically the same "class" of responsibility as _updateTheme. That means the event listeners need to be modified too… I'll figure it out, will take a little while until complete…

@@ -127,6 +128,7 @@ export class TerminalService implements ITerminalService {
terminalInstance.setVisible(i === terminalIndex);
});
this._onActiveInstanceChanged.fire();
this.getActiveInstance().focus(true);

This comment has been minimized.

@Tyriar

Tyriar Nov 24, 2016

Member

I believe the reason focus is not run in setActiveInstanceByIndex is because on launch we do not want the terminal to be focused when the workbench is restored, instead the editor needs to be focused.

Could we isolate this particular case? When a terminal exits AND it is not the last terminal AND the terminal was focused previously. The last case prevents the terminal hijacking focus from the editor if the shell process dies for example.

This comment has been minimized.

@kaiwood

kaiwood Nov 27, 2016

Author Contributor

@Tyriar Ok, I moved it into the block I had previously in mind, the first two isolation conditions are already met there. The third one was a little trickier because the last focus state had to be remembered, therefor c46e284 to cover this case.

@kaiwood

This comment has been minimized.

Copy link
Contributor Author

kaiwood commented Nov 27, 2016

@Tyriar Done. All configurations update when changed and on instance creation, focus is kept when the terminal receives EOF via ctrl-d and it doesn't hijack it when I kill -9 the shell from another terminal emulator.

Feel free to review again.

@kaiwood kaiwood changed the title Fix focus handling of the integrated terminal Fix focus and configuration handling of the integrated terminal Nov 27, 2016

@Tyriar
Copy link
Member

Tyriar left a comment

This is shaping up nicely! A few more comments 😄

}

private _updateCursorBlink(terminalInstance): void {
terminalInstance.setCursorBlink(this.configHelper.getCursorBlink());

This comment has been minimized.

@Tyriar

Tyriar Nov 28, 2016

Member

I think it would make sense to just roll these 3 single line functions into updateConfig now.

this._updateTheme();
} else {
return super.setVisible(visible).then(() => {
this._terminalService.createInstance();
this._updateConfig();
this._terminalService.updateConfig();

This comment has been minimized.

@Tyriar

Tyriar Nov 28, 2016

Member

This shouldn't be needed anymore as it's called within ITerminalService.createInstance

@@ -80,6 +83,9 @@ export class TerminalService implements ITerminalService {
this.setActiveInstanceByIndex(0);
}
this._onInstancesChanged.fire();
this._updateCursorBlink(terminalInstance);

This comment has been minimized.

@Tyriar

Tyriar Nov 28, 2016

Member

These calls would be more at home inside TerminalInstance.constructor. Thinking about this further we may be able to get rid of both calls in TerminalPanel.setVisible as well by updating the config at the end of TerminalInstance.attachToElement. I believe that covers the cases we need to worry about for updating the config:

  • When the terminal is created and the panel exists (attachToElement will trigger immediately)
  • When the terminal is created via the API and the panel does not exist (attachToElement call will be deferred)
  • When the config is updated (TerminalService now listens to IConfigurationservice.onDidUpdateConfiguration)

This comment has been minimized.

@kaiwood

kaiwood Nov 29, 2016

Author Contributor

Absolutely correct, piggybacking on TerminalInstance.attachToElement makes a lot of sense. That part of the config is now no longer a concern of TerminalPanel, the TerminalInstance is only responsible for itself and TerminalService handles the global updates when settings change. Seems reasonable 👍

@@ -96,6 +102,9 @@ export class TerminalService implements ITerminalService {
if (wasActiveInstance && this.terminalInstances.length > 0) {
let newIndex = index < this.terminalInstances.length ? index : this.terminalInstances.length - 1;
this.setActiveInstanceByIndex(newIndex);
if (terminalInstance.hadFocusOnExit) {

This comment has been minimized.

@Tyriar

Tyriar Nov 28, 2016

Member

Love it! 👍

this._updateTheme();
this._updateConfig();
this._terminalService.updateConfig();

This comment has been minimized.

@Tyriar

Tyriar Nov 28, 2016

Member

I think this can safely be removed.

@Tyriar

This comment has been minimized.

Copy link
Member

Tyriar commented Nov 29, 2016

@kaiwood let me know when this is ready for another review, I'd love to land it for this release (ideally merge this week) 😄

@kaiwood

This comment has been minimized.

Copy link
Contributor Author

kaiwood commented Nov 29, 2016

I was just about to comment above, hehe 👍

The latest commit should address everything that you mentioned, ready for the next round.

@Tyriar

This comment has been minimized.

Copy link
Member

Tyriar commented Nov 29, 2016

Changes look good, I re-queued the Travis build to make sure 😃

@kaiwood

This comment has been minimized.

Copy link
Contributor Author

kaiwood commented Nov 30, 2016

Just noticed while checking for the cause of another issue that I included TerminalService here just to use configHelper. Didn't notice that _configHelper was already available, so I removed the dependency.

Last commit doesn't change any behaviour, just a cleanup. Travis was green before this, Appveyor timed out, maybe we are lucky and they get both green this time around 😆

@Tyriar

This comment has been minimized.

Copy link
Member

Tyriar commented Nov 30, 2016

Looks good, thanks again 😄

@Tyriar Tyriar merged commit d6807fc into Microsoft:master Nov 30, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@kaiwood kaiwood deleted the kaiwood:terminal-focus branch Dec 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment