Feature: don't show error peek overlay when navigating over errors (next/prev error by F8 hotkey)

[n-gist]

Adds ZoneWidget showOverlay (true) option to skip insertion of overlayWidget if set to false

Adds problems.gotoError.showOverlay setting to make use of it when navigating over errors (next/prev error by F8 hotkey)

Fixes #51787 Fixes #63591

[n-gist]

Made error overlay still shown on clicking View Problem from hover widget, by adding forceShowOverlay parameter to showAtMarker method

Hid the hotkey hint in brackets on View Problem (Alt + F8) button if setting is set to false

Open question. I tried to retrieve setting inside MarkerController constructor to cache it for further use, but it seems that MarkerController doesn't get recreated on settings edits. So I made a getter which retrieves it every time. I don't feel it right, but I don't know how to do it properly. Maybe it should be boxed into something adaptive to settings updates?

[n-gist]

I looked at how it was done in other places and added a disposable callback for settings updates. Now it looks correct to me

[n-gist]

@sandy081 check please, it looks complete to me, but maybe I should change it or do it other way?

Regarding of adding forceShowOverlay to showAtMarker - actually this may be not necessary, since showAtMarker is used only in one place, from hover widget View Problem button click, and thus it can be made to always force to show. I have added it to make it more verbose if it is going to be called from somewhere else in the future. But I'm not sure if it is needed still, since showAtMarker can be read as 'just show it at the specified marker'. So, it can be kept or removed

[n-gist]

@jrieken sorry, I don't know who to ping for, but probably it is your area? Seems like @sandy081 is busy

[jrieken]

Thanks for the effort here. Tho, we should discussed first if this contribution will be taken. There are not many upvotes on either feature requests and the gist of the F8 feature is to peek into errors, not to navigate across them.

Taking a PR always comes with the maintenance cost and the feature you are asking for can also be done as extension (vscode.languages.getDiagnostics and a command) or many with some keybinding configuration from the problems view

[n-gist]

@jrieken I see that my implementation is wrong, it should be done as a different command, instead of as configuration setting. I didn't really think about implementing this as an extension. I guess I can do that. If all the API needed for this is available to extensions, that's a good solution. Thanks

But since we're already here. If I can add my two cents to why this might be wanted to be built into editor

Why not many upvotes

One of the reasons is that command named not accurate, because it is named as Go to, but does Go to and Peek. It's a bit strange to ask about a command that formally already exists, and even more so about cutting its functionality. Maybe if it was named correctly there would be more votes for the raw Go to command. But now, you just look for the setting to disable the widget on jumping, find nothing, give up and live with what is there, just hit Esc every time. For example, it took me a couple of years before I decided to do something about it

Why someone might not want information about a Problem. Cases

a) For overall usage. It would be actually interesting to gather statistics about the percentage ratio of immediately closing peek overlay to keeping it open for more than two seconds. But subjectively the reading frequency will be much lower than 50 percent, since most often the problem is clear because code segment is underlined, or even decorated in an additional way (dimmed, etc.) which gives understanding of the issue faster, than reading error description. But everyone can check himself - how often you hit Esc immediately? It's just not noticeable until you pay attention to this fact b) An extension can be used that prints error messages inlined, such as Error Lens c) You don't intend to fix the problems right now, you just want to check the areas affected by the problems-producing change and jump back to it d) You know the problems in advance, because you just did a change that produced them. Actually this is the main case in this list, especially when you do refactoring in a stable, free of errors project

Why it is essential, from UX view

Even if peek is useful in less than 50% of cases, it is still good to have it by default, this makes it user-friendly, I understand that. Not a big deal to just close it. But the option to opt out of it is really missing. Since this is not information somewhere on the side that can be ignored. It is a feature that requires action to be taken to reject it. Physical movements to hit Esc and back to initial position of a hand, this takes time. Peek overlay breaks the code where it is inserted and collapses it back when closed. Jumping code is a strain on the eyes and attention. It delays the moment when the code begins to be perceived clearly. This is a cognitive interruption. The ability to skip the peek is a workflow optimization, which is one of the main goals of an editor as I see it. Reducing cognitive noise to maintain attention hygiene. Code, as a first-class editor, imo should provide such options out of the box

If this is reasonable enough, I can try to work on it. My proposal is:

1. Rename Go to Problem to Peek Problem/Peek to Problem/Go to and Peek Problem. Keep default F8 hotkeys to it
2. Add Go to Problem commands without assigning default hotkeys

If not, I'm fine to create an extension

[n-gist]

@jrieken since decision is taking some time, I have tried to write an extension. Done it, but when I tried to jump to closed file, it failed. Seems like vscode.languages.getDiagnostics provides data only for currently open files. It can't be used to get list of problems from, for example, a tsc watcher used as typechecker service. This is crucial. Built-in commands use MarkerService for this. Is there a way to access it? Or get the list from problems view pane? I have found this your issue, looks like related to this https://github.com/microsoft/vscode/issues/47292

I have also tried to use original Go to commands and execute closeMarkersNavigation right after. It works, but blinks for a moment

[jrieken]

Seems like vscode.languages.getDiagnostics provides data only for currently open files. It can't be used to get list of problems from, for example, a tsc watcher used as typechecker service. This is crucial. Built-in commands use MarkerService for this.

Both, the marker service and getDiagnostics API, use the same data. This forwards all markers from the service into the API. This includes diagnostics from the API as well as from tasks. Tho, there is no guarantee that diagnostics are computed on a project-level, e.g tsserver doesn't do that by default.

[n-gist]

@jrieken thanks for the info, but it doesn't helps. Am I doing something wrong?

1. I open one file which cointains several problems
2. vscode.languages.getDiagnostics() returns entry for this file with a list of problems, as it should
3. Go to Next Problem in Files (editor.action.marker.nextInFiles) command loops over problems inside this file, and don't jumps to other files, as it should
4. I launch tsc watcher task, which adds problems from other files to Problems view pane
5. vscode.languages.getDiagnostics() returns same, unchanged data, cointaining entry for initially opened file only
6. But Go to Next Problem in Files (editor.action.marker.nextInFiles) now see new problems and jumps to previously not opened files

Is it working as intended? If yes, how to get full list, the same as Go to Problem commands have access to? I tried to follow the flow of code to find differences from there, but I'm losing the track

[jrieken]

Mystery. The problems pane is also just fed from the marker service, afaik there is no other way to feed problems into the system and from there everything is pushed back into the extension host

[n-gist]

@jrieken, I just thought that it could be because of monoproject (I'm using tsconfig refs), thought maybe it just discards data coming from 'another' project. But no, it is reproducible in very simple project. Here is repo for testing https://github.com/n-gist/getDiagnostics-test There is vsix which simply outputs getDiagnostics() result to output channel named 'getDiagnostics() test' (Ctrl+F8), as:

const diagnostics = vscode.languages.getDiagnostics()
for (const d of diagnostics) {
	channel.appendLine(`${d[0].toString()}`)
	channel.appendLine(`${d[1].length} entries`)
}

Reproduce steps:

1. Open main.ts
2. Ctrl+F8 outputs entry for main.ts only
3. Start tsc watch task, it adds another.ts to the problems list
4. Ctrl+F8 still outputs main.ts only
5. F8 correctly jumps to another.ts
6. When both files are open, Ctrl+F8 outputs correctly
7. If the files are closed after that, it still outputs entries for them, but without Diagnostics entries

You can point me to where I can investigate for a chance of finding the source, but I assume my vs code codebase knowledge is not enough to find a fix acceptably fast. Is it there?

Or will you take a look at it yourself sometime?

[n-gist]

@jrieken, I just opened the repo I created (in previous comment) through Codespaces extension, installed typescript for watch task, this vsix I made, and it works as intended. Step 4 outputs data for both files. And still outputs as it should after closing files (Step 7). So, there is some sort of difference in behaviour when I run it locally on my Windows desktop machine

Another difference is that locally there is a tsconfig.json URI entry in getDiagnostics() results, without problems entries (In my other project there is also similar entry for package.json URI)

Step 2 (one file is open) local/codepaces

Step 4 (tsc watch is running)

Step 6 (both files are open)

Step 7 (files closed)

I tried disabling all extensions and emptying my settings file. Didn't helped

[n-gist]

@jrieken, I was trying to find the source of issue, so here is what I found

First, I found that DiagnosticCollection.set is not called when kicking watch task However, it is called from ExtHostDiagnostics.$acceptMarkersChange without entries (clearing call) when task is terminated, for not opened files URIs

So, I checked what happens in MainThreadDiagnostics._forwardMarkers and it turns out that all markers always get filtered out there (even for opened files) because typescript exists in _activeOwners, so data array doesn't get filled, and this._proxy.$acceptMarkersChange(data); is not called

However, when a file is opened, DiagnosticCollection.set is called from somewhere else (not from ExtHostDiagnostics.$acceptMarkersChange and I didn't found from where), before the MainThreadDiagnostics._forwardMarkers call

So, in conclusion, collection is feeded from somewhere by DiagnosticCollection.set for opened files, and MainThreadDiagnostics._forwardMarkers also called after that, but never transmits markers further

I wanted to check how it behaves through codespaces extension for comparison, but it doesn't connects from OSS build

[n-gist]

Here is one more test I did - if watch task is kicked before opening any file, _forwardMarkers passes markers normally because there is no 'typescript' in _activeOwners yet

[jrieken]

This is a great find. There is something going on with task- and extension-markers. I need to double check but I believe they can share owners so that extension-markers overwrite task-markers but not the other way around.

[n-gist]

@jrieken, @dbaeumer, I did some more testing of vscode.languages.getDiagnostics() after #253368 landed in 1.102.0, and found another issue:

1. When a file is open, and a task is started after, getDiagnostics() result looks ok
2. When a task is running, and a file is opened during, getDiagnostics() gives twice more entries for that file

The question is, does getDiagnostics() consumers should filter duplicates themselves? Entries are identical for tsc watch + ts extension, but they could differ if another watch instument or problem matcher is used, which means that we may need entries collected from all origins? However, still, no way to distinguish task entries from extension ones atm. If yes, the case 1 need a fix, so that duplicates are always there. If not, then need to fix case 2. Didn't checked how it is implemented for problems view panel, but it doesn't show dupes in any case

[dbaeumer]

@n-gist IMO they shouldn't be included twice. Can you please open an issue against VS Code with steps on how to reproduce. Then we can take it from there.

[n-gist]

@dbaeumer #256118