Skip to content
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

Auto Start: "need to restart terminal" warning is too easy to miss #111890

Closed
weinand opened this issue Dec 4, 2020 · 13 comments
Closed

Auto Start: "need to restart terminal" warning is too easy to miss #111890

weinand opened this issue Dec 4, 2020 · 13 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues terminal Integrated terminal issues under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@weinand
Copy link
Contributor

weinand commented Dec 4, 2020

After turning on "Auto Attach", it never works for me until I notice the tiny warning icon on the right side of the terminal.

2020-12-04_14-52-21

IMO this is not a good flow. I'm still tricked by this every other day...

If users explicitly set an Auto Attach option, they expect that the feature works after executing the action. They do not want to hunt down a warning and then have another action...

I suggest to ask users immediately from the Quickpick for restart confirmation.

In addition I do not understand why the terminal needs to be restarted?
Setting an env var in the terminal should be possible without restarting, correct?

@weinand weinand added the debug Debug viewlet, configurations, breakpoints, adapter issues label Dec 4, 2020
@connor4312 connor4312 added terminal Integrated terminal issues debug Debug viewlet, configurations, breakpoints, adapter issues and removed debug Debug viewlet, configurations, breakpoints, adapter issues labels Dec 4, 2020
@connor4312 connor4312 added this to the On Deck milestone Dec 4, 2020
@Tyriar
Copy link
Member

Tyriar commented Jan 20, 2021

This is what we ended up landing on after a bit of back and forth around the UX in the syncs and tests #96300.

Initially the warning was going to be much more in your face but that was rejected as only sometimes is this something that we would want to interrupt the user on. There's a request to restart terminals automatically to help with this if they have not been interacted with #100193. I'm open to a different flow but going for something more obvious will also tick users off.

In addition I do not understand why the terminal needs to be restarted?
Setting an env var in the terminal should be possible without restarting, correct?

I considered adding another item in the hover's actions to send commands to update the environment but decided against it to simplify the already complex hover and I don't think I got good feedback in the UX sync. It's also difficult to get this right reliably (differing shells, shell levels, apps running in the shell taking the input), the only reliable way is to restart which is typically an inexpensive action to take.

@Tyriar Tyriar added the under-discussion Issue is under discussion for relevance, priority, approach label Jan 20, 2021
@Tyriar Tyriar modified the milestones: On Deck, Backlog Jan 20, 2021
@weinand
Copy link
Contributor Author

weinand commented Jan 20, 2021

The existing warning icon might be OK for cases where users don't want to be interrupted (but I still wonder what those case would be..)

But turning on "Auto Attach" is definitely not such a case!

If users explicitly enable Auto Attach, they expect that the feature works after executing the action.
Showing a modal dialog and asking them whether to restart the terminal is the only way to make this flow work.

@Tyriar
Copy link
Member

Tyriar commented Jan 20, 2021

This command?

image

You could have that command also show a notification saying that terminals need to be restarted if any terminals exist? workbench.action.terminal.relaunch is also available to relaunch the active terminal if needed, I could also create a relaunchAll command if desired.

This API is not just for debugging and we don't want it to be interrupting users in the general case.

@weinand
Copy link
Contributor Author

weinand commented Jan 20, 2021

Yes, this issue is only about the "Auto Attach" use case, not about setting env vars in the general case.

@connor4312 I like Daniel's suggestion to show a notification (or alert) saying that terminals need to be restarted.

@connor4312
Copy link
Member

I think a relaunchAll action would be useful here for a notification button

@weinand
Copy link
Contributor Author

weinand commented Feb 2, 2021

@connor4312 with "notification button" you mean an action in the alert or notification?

@connor4312
Copy link
Member

Yea

@Tyriar
Copy link
Member

Tyriar commented Feb 3, 2021

@connor4312 I'm not sure relaunch all action is needed anymore since I pushed the auto restarting yesterday (#100193)?

@connor4312
Copy link
Member

cool! One thing I ran into (adding an option to restart a single terminal) is that I don't know whether the terminal is about to restart on its own, so could show an extraneous notification.

I wonder if the auto restart you pushed solves the case in general... the ⚠️ is still easy to miss, but now it will always be caused by a user interaction and not just by opening a workspace.

@Tyriar
Copy link
Member

Tyriar commented Feb 5, 2021

@connor4312 don't you just need to wait a little bit before showing a notification?

@connor4312
Copy link
Member

That works. I have it about ready to go but currently this triggers a bad bug #115927

@Tyriar
Copy link
Member

Tyriar commented Feb 5, 2021

@connor4312 we're aiming to fix that for Feb, it's a long standing issue microsoft/node-pty#415

@Tyriar
Copy link
Member

Tyriar commented Oct 12, 2021

The color of the terminal tab is now yellow and there's another icon there now, it's much more discoverable.

@Tyriar Tyriar closed this as completed Oct 12, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues terminal Integrated terminal issues under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

3 participants