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

API: add a preserveFocus boolean on `createTestRun #209491

Closed
connor4312 opened this issue Apr 3, 2024 · 8 comments · Fixed by #212177
Closed

API: add a preserveFocus boolean on `createTestRun #209491

connor4312 opened this issue Apr 3, 2024 · 8 comments · Fixed by #212177
Assignees
Labels
Milestone

Comments

@connor4312
Copy link
Member

connor4312 commented Apr 3, 2024

It's common that extensions trigger tests in various ways. Some of those may be user-initiated through UI actions outside of the built-in ones where focus should be transferred, some of them may be triggered in the background where focus should not be transferred. There have been a bunch of issues around this, on our tracker as well as those of community extensions.

I suggest adding an additional option to the TestRunRequest to allow extensions to hint whether focus should be preserved:

	export class TestRunRequest {
		/**
		 * Controls how test Test Results view is focused.  If true, the editor
		 * will keep the maintain the user's focus. If false, the editor will
		 * prefer to move focus into the Test Results view, although
		 * this may be configured by users.
		 */
		readonly preserveFocus: boolean;

Note this is on the TestRunRequest, not the individual TestRun. TestRunRequests can be created both by the the editor and extensions (if they initiate a test run), and this provides control for the latter case.

@DanTup
Copy link
Contributor

DanTup commented Apr 4, 2024

"If true, [...]. If false, [...]

I presume undefined is treated like false, but I think it's worth tweaking the text to make this explicit.

It's also not quite clear to me how the user overrides this - is this some tri-state setting like:

  • let extension decide whether to show results (default)
  • always show results
  • never show results

?

@connor4312
Copy link
Member Author

connor4312 commented Apr 4, 2024

Yea undefined would be the current behavior, which is false (most of the time.)

In the false case this is a 'hint': whether focus is actually moved is controlled by the user's testing.openTesting setting. I'm not planning on having a mechanism to force focus to move, although if an extension really wanted that they could run a command to that effect.

@DanTup
Copy link
Contributor

DanTup commented Apr 4, 2024

I think I'm still confused. From the above, it sounds like:

  • true - don't open testing view, always keep focus in the editor
  • undefined - do what happens today - don't open testing view for programmatic test runs triggered by extensions
  • false - does this differ from undefined?

But I'm not sure how I handle Dart-Code/Dart-Code#4951. Maybe the interaction with the user settings needs to be clearer - which different user settings influence the behaviour for each of these values?

@connor4312
Copy link
Member Author

connor4312 commented Apr 5, 2024

Ah, yea, sorry. It has been a while since I touched that area I thought there we cases where that was false for extension-triggered runs -- so there's no need for undefined, it'll be true/fase (defaulting to true for backwards compatibility.) I updated my initial proposal.

Maybe the interaction with the user settings needs to be clearer - which different user settings influence the behaviour for each of these values?

Whether focus actually moves when false will be controlled by testing.openTesting: the same way as ui-triggered runs do today.

@vogelsgesang
Copy link

vogelsgesang commented Apr 6, 2024

For my particular use case, I would need slightly different behavior. Essentially, I am offering a "Measure Code Coverage" command. This command is explicitly triggered by the user, i.e. I know the user wants to see the coverage results.

I think the most intuitive behavior would be the following:

  • If the user already has a file open, and the coverage run actually provided coverage for that file: Don't switch to the test results view. The user is probably interested in seeing the coverage for the file which he has already open, and the in-editor rendering of the coverage results will allow him to do exactly that.
  • If the user has no file open, or if the coverage run did not provide any coverage data for that file, switch focus to the test results / test coverage view

Not sure if my use case could be accommodated somehow by expanding on your proposed design. Maybe my use case is too much of a special case...

[...] if an extension really wanted that they could run a command to that effect.

How would I do that? I couldn't find an API or a command which would allow me to do so

@connor4312
Copy link
Member Author

How would I do that? I couldn't find an API or a command which would allow me to do so

You could run vscode.commands.executeCommand('workbench.panel.testResults.view.focus')

Your use case does seem like it's kind of special, I would say you can use the command for now, unless I get feedback from others asking for something similar for that.

@connor4312
Copy link
Member Author

connor4312 commented Apr 22, 2024

This is available in the latest Insiders, please try it out and let me know if it works for you

@DanTup
Copy link
Contributor

DanTup commented Apr 23, 2024

@connor4312 thanks, this seems to work for me :)

I'm currently setting it like this so I don't have to crank the min VS Code version to get the updated type though 🙂

const request = new vs.TestRunRequest();
(request as any).preserveFocus = false;

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants