Run menu does not remember dynamic launch configurations

[connor4312]

Refs: #95837

Version: 1.45.0-insider (user setup)
Commit: 0778354f3a6a2c21b7f738a5e6b02e2f1c765e73
Date: 2020-04-27T05:42:11.567Z
Electron: 7.2.2
Chrome: 78.0.3904.130
Node.js: 12.8.1
V8: 7.8.279.23-electron.0
OS: Windows_NT x64 10.0.18363

Steps to Reproduce:

1. Select a dynamic launch configuration
2. Reload the window
3. The dynamic launch configuration is no longer the selected item in the Run menu

I assume this is because the dynamic configurations are provided asynchronously/aren't present when the view is first loaded.

Switching out the selected configuration after extension activation would be awkward. Maybe the run view should have a 'stub' item and assume that the previously selected dynamic configuration will be provided once activation completes (and only reset the selected item if it doesn't exist after activation).

[isidorn]

Yes, this is the limitation of the current approach. Since they come async they are not present when view loaded.
Your idea of having a stub item makes sense. Though moving this out to may when we finalise the debug dropdown work with regards to dynamic configurations.

[isidorn]

Will have to reopen this one due to the way I fixed #100265
More details can be found in that issue. In short we are no longer storing whole configurations, thus the best we can do is store origin and name, which would lead us to ask for configuration as soon as vscode starts as when user launches. This is not the optimal experience. Thus I am open for ideas and PRs.

[weinand]

I think @connor4312 already suggested the best solution in his initial comment:

Maybe the run view should have a 'stub' item and assume that the previously selected dynamic configuration will be provided once activation completes (and only reset the selected item if it doesn't exist after activation).

[isidorn]

I have pushed a "controversial" fix for this. I am storing dynamic configurations in the local storage across reloads. Regular configurations I do not store.

I did not see an alternative solution that would make this experience nice. Your suggestion to simply preserve stub objects would have a strange experience:

• User selects NodeJS dynamic for the first time. We ask him immediatly what config he wants to use. He chooses.
• Once user presses play we just run that configuration
  Now when he reloads if we were to just show the NodeJS stub, the question is when to ask the user what config from NodeJS to use(there can be multiple). Immediatly on startup does not make sense, and once he wants to start debugging makes sense, but then it would be inconsistent iwth the original experience, where we do not ask before F5. And we would not ask after every F5 after the first.

My previous attempt to store launch configuratoins produced some side effects but all of the problems were due to the fact that I was storing all configurations. Now I handle dynamic configurations as a corner case and only store them

@weinand @connor4312 please try it out and let me know what you think. I am fine with reverting this or taking another approach, this just made the most sense to me from the user perspective. User chooses just once, and every consecutive F5 he hits it just starts debugging. To "clear" the choosen configuration he has to unselect the provider and select it again.

Alternative approach: is to ask the user every time he hits F5 what configuration he wants.
Which is a safer approach, but will lead to less smooth experience.

[weinand]

@isidorn you said:

Now when she reloads if we were to just show the NodeJS stub, the question is when to ask the user what config from NodeJS to use(there can be multiple).

I don't understand why there is a need to for asking the user anything after a reload.
If the user selects the stub, we would need to activate the (remembered) extension, get all dynamic launch config from it and then run the one that has the same name as the stub. Only if no dynamic config matches the stub we would have to show an error or just show the Quickpick.

"controversial" fix

Did you make sure that the issues with the persisted "noDebug" property are not again occurring with your fix?
If I understand your fix correctly, then you are doing basically the same, but only on a smaller subset of the configs.
This makes problems to occur less likely, but it is not a fundamentally different approach.

[isidorn]

@weinand I have verified that the perisisted "noDebug" property is not again occuring. However I tried this with JS Debug which is now using a new noDebug API. So there might be some provider which provides dynamic configurations with the noDebug field for which this might get broken - I did not try them all.

Also you are correct that we can not ask the user, that we could just pick the right launch config. I missed this idea in my thought process. However for this we need to store the type of the dynamic config provider (so we know which one to ask) and we would have to re-wire a bit how getting dynamic configurations is done (since currently it is coded in a way that it asumes a user would make the choice). So it is a medium size change.

Due to the above I will revert the change and push this out to debt week.

[isidorn]

As we have agreed I am now:

• Storing the type of a used debug configuration. We already stored the name
• On startup if I can not find the last used configuration among regular configs I will activate all dynamic providers for the last used type and ask them for dynamic configurations. If they provide a confiuration which has the last used name we will automatically select it

I have tried this and it works good. Please try it out and let me know how it goes for you.

[bersbersbers]

While I don't know what a dynamic launch configuration is (and Google is not helpful: https://www.google.com/search?q=%22dynamic+launch+configuration%22+%22vs+code%22), I would like to point out that two very similar issues that I expected to be fixed by this are not fixed. This includes #100451 (which is related to reloads) and issue 2 of #90216 (which is related to changes in launch configs). I can repro both issues in today's insiders:

Version: 1.50.0-insider (user setup)
Commit: 635cfbc
Date: 2020-09-24T05:36:05.681Z
Electron: 9.3.0
Chrome: 83.0.4103.122
Node.js: 12.14.1
V8: 8.3.110.13-electron.0
OS: Windows_NT x64 10.0.19041

[connor4312]

I tried this today and things were not working too well for me. I observed two issues:

• When I selected a dynamic configuration, the config shown in the dropdown was actually my first static launch config, not the dynamic one. However, clicking run on it did the right thing.
• After reloading the window, it did not remember my dynamic configuration.

Here's a video -- the dynamic config opens a terminal and runs npm run start, the static config runs it in the debug console: https://memes.peet.io/img/20-09-590732b6-2c12-492a-9855-94bfc84cbe63.mp4

[isidorn]

@connor4312 thanks. I am sure this was working when I closed this 23 days ago.
Something broke in the meantime as I can reproduce this. Investigating...

[isidorn]

@connor4312 good catch. The issue was that the type of the Javascript debug terminal dynamic config was pwa-terminal which did not macth the provider type. Which I needed to select the right element. Anyways pushed a fix for this and it should now be good. Thanks

[bpasero]

Unclear to me how to verify this, I suggest that @connor4312 verifies this today.

[bersbersbers]

I have to say that #100451 is not fixed in today's insider:

Version: 1.50.0-insider (user setup)
Commit: 5052a7bc2ec35879b4ef5c835de78c9620bc1d9e
Date: 2020-10-02T05:30:35.675Z
Electron: 9.2.1
Chrome: 83.0.4103.122
Node.js: 12.14.1
V8: 8.3.110.13-electron.0
OS: Windows_NT x64 10.0.19041

[isidorn]

@bersbersbers In your previous comment you mentioned that you do not know what dynamic launch configurations are.
I suggest to wait for @connor4312 to verify this.

Thanks

[bersbersbers]

@bersbersbers In your previous comment you mentioned that you do not know what dynamic launch configurations are.

Correct - I dropped that as a hint for someone to maybe point out the difference between "launch configurations" and "dynamic launch configurations" ;) and also, between this bug and the bug I reported. Since the one I reported is exactly identical to this one except that it also applies to "non-dynamic launch configurations" (concluding from the fact that it's still reproducible).

Furthermore, from the fast that the concept of "dynamic launch configurations" is hardly documented anywhere, I had concluded that non-dynamic launch configurations are the much more frequent use case, and so I thought that bug I reported might have at least the same relevance as this once. But apparently that's not the case, and I'll patiently wait now until it rises from the backlog ;)

[connor4312]

Works well now, thanks!

[weinand]

The UI for remembered dynamic launch configurations is not discoverable and needs to fixed.
I've created #110009 for this.