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

Improve the dynamic launch config UI #110009

Closed
weinand opened this issue Nov 5, 2020 · 19 comments
Closed

Improve the dynamic launch config UI #110009

weinand opened this issue Nov 5, 2020 · 19 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@weinand
Copy link
Contributor

weinand commented Nov 5, 2020

Currently the last used dynamic launch is remembered in the debug dropdown behind the debug type provider item.
This is not discoverable because the name of the provider item does not reflect what's behind it.

E.g. a provider item "Mock Debug..." on first execution will open a Quickpick with all dynamically provided launch configs. After one of them is selected and executed, it is remembered behind the "Mock Debug..." item.

Now the launch config can be used directly (but only for users that know what is behind the provider item).
And since the provider item's functionality is no longer available, there is no obvious way to select a different dynamic launch config from the same provider.

I suggest the following:
Exactly like "tasks" we will remember recently selected launch configs as separate items both in the debug dropdown and the "Select and Start" Quickpick. This makes even dynamic launch configs available for quick access based on typing their name.

/cc @connor4312

@isidorn
Copy link
Contributor

isidorn commented Nov 5, 2020

Makes sense and good points.

Some open questions:

  1. What is the order to show these configurations. I have chosen: launch.json configuration, seperator, currently chosen dynamic configuration, dynamic configuration providers, separator
  2. The label for the dynamic configuration I simply use the name. I do not want to use the provider label as a prefix since it would be too long.
  3. Should we keep the ... in the dynamic configuration names. I think yes since they require more input
  4. What happens if you select another configuration, for example from launch.json. Currently the dynamic one will simply dispear, since I do not store. And I think that is fine

All of this put together, it looks like this in the Dropdown. It will look similar in the quick pick

Screenshot 2020-11-05 at 13 09 16

fyi @alexr00 for potential input from tasks

@isidorn
Copy link
Contributor

isidorn commented Nov 5, 2020

Alternative sort order:

launch.json configruation, separator, providers (after the parent provider show the current dynamic one, so the user can sort of see who it belongs to)

Screenshot 2020-11-05 at 13 30 58

@alexr00
Copy link
Member

alexr00 commented Nov 5, 2020

Tasks maintains a history list of the most recently run tasks, and the size of the history can be configured using task.quickOpen.history. I think way you are showing these launch configurations is different enough that it isn't worth trying to align with tasks.

@weinand
Copy link
Contributor Author

weinand commented Nov 5, 2020

We have two UIs for launch configs:

  • the "debug drop down menu" maintains a current selected launch config
  • "Select and Start" Quickpick without a current selected launch config and no recent items

Let's start with the "Select and Start" Quickpick:

IMO this should behave like the "Run Task" QuickPick, which means that it needs a "recent section" and all recently started launch configs (both dynamic and static) should appear there.
The goal is to switch easily between a few launch configs independent from where they come from (static or dynamic).

An alternative to the "recent" section would be to add all dynamic launch configs that were started once to the list of static launch configs (and remember them across restarts).

I'm not really sure what approach I prefer but when considering the next suggestion, it would make sense to use the "alternative" approach for both UIs.

Now the "debug drop down menu":

In order to keep the menu short, we should not add a "recent" section.
But we should make dynamic launch configs for quick selection available once they have been used once. So we could use the same "alternative" approach suggested above for the "Select and Start" Quickpick: If a dynamic launch config has been started once it will be added to the list of static launch configs (because there is no real difference from an enduser perspective). And these dynamic configs should be remember across restarts.

And to avoid too many separators in the debug drop down, we could move the dynamic providers to the last section (the one with the "Add Configuration...") because these items are really very similar too.

@isidorn
Copy link
Contributor

isidorn commented Nov 5, 2020

I think the "alternative approach" here makes sense. However for that change we would have to store N dynamic configurations names in the storage for each workspace. That would be a bigger change which I can look into how feasable it is...

@weinand
Copy link
Contributor Author

weinand commented Nov 5, 2020

But today we are storing one, right? And with the proposal you would probably store 1-3?
If we would introduce a "recent" section (like tasks), then we would have the same problem, right?

@isidorn
Copy link
Contributor

isidorn commented Nov 5, 2020

Today we are not storing any dynamic configuration related data. We just store the name of the last used config. Whether it is dynamic or not.
But I will look into changing that. So we store all dynamic configurations names used in a workspace.

Correct for recent, we would have to store even more, since we would need to know in what order dynamic and regular configs were used.

@weinand
Copy link
Contributor Author

weinand commented Nov 5, 2020

If I remember correctly, then storing a dynamic config means remembering the name and the providers debug type.
That was the fix for #96293, right? Please read #96293 (comment)

For this proposal the only difference is that we would potentially store more than one.
And we must be careful not to activate any debug extension when restoring the remembered dynamic configs on startup. Only when starting a dynamic launch config we need to activate the corresponding debug extension.

@isidorn
Copy link
Contributor

isidorn commented Nov 5, 2020

Correct. However we do not distinguish between dynamic and regular ones. So for the active one we simply store the name and the type.
For this purpose we would store all the dynamic configs ever used - their name and type. This should be fine, I have to check.

@weinand
Copy link
Contributor Author

weinand commented Nov 5, 2020

We'll have to decide whether we want to cap the number of dyn. entries and we should consider to introduce a command to clear the dyn. entries.

@isidorn
Copy link
Contributor

isidorn commented Nov 5, 2020

Yeah beacuse of that showing just one is the pragmatic approach. If users have to manage this via a command than it is a pain to use.
Caping it at 3 or 5 should do the job imho.

@connor4312
Copy link
Member

Another approach is to make the dropdown a context menu instead, which would allow you to have nested menus and a "recent" section without worry of clutter.

@isidorn
Copy link
Contributor

isidorn commented Nov 6, 2020

Thanks a lot for feedback. Here's how it looks now

Screenshot 2020-11-06 at 13 03 19
Screenshot 2020-11-06 at 13 03 27

  1. We store all recently used dynamic config stubs in storage. However in Dropdown we show only last 3. In quick pick we show all since that UI scales pretty well
  2. Decided in the quick pick to show dynamic configs with Providers since they are in the section "Contributed"
  3. As Andre suggested in the debug drop down I show them in the section with static configurations, just at the end. Also Provider labels and "Add Configuration" are now in the same group
  4. Activation of debug extension did not change. So we try to activate as late as possible

Try it out and let me know what you think. Thanks

@connor4312 if we used the context menu in the debug dropdown that would make sense, but imho context menu does not look nice on all platforms and the whole thing no longer feels like a native dropdown. Anyways I am open for changing that in the future

@isidorn isidorn closed this as completed in 4b0855f Nov 6, 2020
@isidorn isidorn added the verification-needed Verification of issue is requested label Nov 6, 2020
@weinand
Copy link
Contributor Author

weinand commented Nov 10, 2020

@isidorn I've tried the new UI and it works great!

One problem I see: when running the latest Mock-Debug out of source, it activates on startup.
I have two remembered dynamic launch configs and since there is no UI to clean them from session state I cannot try what happens when they are gone.

@isidorn
Copy link
Contributor

isidorn commented Nov 10, 2020

@weinand thanks for trying it out.
Yes that is the reason for activation. If there is a selected mock debug configuration we have to activate mock debug on startup so we get all the dynamic configurations so we could match against them and verify if the configuraiton is still valid.
So this is expected behavior.

What should not happen and what would be a bug:

  • You have 2 remembered mock dynamic launch configurations, but you select some node configuration in launch.json. You reload -> mock debug should not be activated

@weinand
Copy link
Contributor Author

weinand commented Nov 10, 2020

@isidorn there is no need to validate selected configurations on startup - validation only needs to occur if user tries to launch the configuration. If the configuration is no longer valid, the corresponding dyn. provider should open.

@isidorn
Copy link
Contributor

isidorn commented Nov 10, 2020

@weinand makes sense, I have filed a new feature request for that #110318

@alexr00 alexr00 added the verified Verification succeeded label Dec 4, 2020
@alexr00
Copy link
Member

alexr00 commented Dec 4, 2020

Looks very similar to tasks. I like it 😁

@isidorn
Copy link
Contributor

isidorn commented Dec 4, 2020

Thanks a lot for verifying!

@isidorn isidorn added the on-release-notes Issue/pull request mentioned in release notes label Dec 4, 2020
@JacksonKearl JacksonKearl added verified Verification succeeded and removed verified Verification succeeded labels Dec 7, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2020
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 feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

6 participants
@weinand @isidorn @connor4312 @JacksonKearl @alexr00 and others