Antora support prompt always appears unexpectedly in a Git workspace

[nc7s]

More than once have I run into the "We detected Antora do you want to enable Antora support" prompt, in projects with but a README.adoc. If I click "No", .vscode/settings.json is created with a single line of "asciidoc.antora.enableAntoraSupport": false. For people who don't like stuffing editor-specific configurations in projects, like me, this is rather unfortunate.

Curiously, "Enable Antora support" isn't checked in extension settings, and that seems to be the default - checking it would add "asciidoc.antora.enableAntoraSupport": true in user settings.json. IMO that means full off - no support at all, no related code paths ran, not to mention prompt to ask to enable in individual projects.

[ggrossetie]

More than once have I run into the "We detected Antora do you want to enable Antora support" prompt, in projects with but a README.adoc.

Are you saying that you get the prompt when it shouldn't? If that's case please provide a sample reproduction case.

If I click "No", .vscode/settings.json is created with a single line of "asciidoc.antora.enableAntoraSupport": false. For people who don't like stuffing editor-specific configurations in projects, like me, this is rather unfortunate.

As far I know that's the only way to save a state in a workspace.

Curiously, "Enable Antora support" isn't checked in extension settings

That sounds consistent since the prompt will set asciidoc.antora.enableAntoraSupport on the workspace.

and that seems to be the default - checking it would add "asciidoc.antora.enableAntoraSupport": true in user settings.json

Extension settings are, by default, defined on the user scope. Please note that workspace settings have higher precendence:

Configurations can be overridden at multiple levels by the different setting scopes. In the following list, later scopes override earlier scopes: Default settings - This scope represents the default unconfigured setting values. User settings - Apply globally to all VS Code instances. Remote settings - Apply to a remote machine opened by a user. Workspace settings - Apply to the open folder or workspace. (...)

https://code.visualstudio.com/docs/getstarted/settings#_settings-precedence

IMO that means full off - no support at all, no related code paths ran, not to mention prompt to ask to enable in individual projects.

I guess we could provide a new setting to completely disable Antora support in case you are not using it. Please open a new issue with this feature request.

[nc7s]

Thanks for the detailed reply @ggrossetie!

More than once have I run into the "We detected Antora do you want to enable Antora support" prompt, in projects with but a README.adoc.

Are you saying that you get the prompt when it shouldn't? If that's case please provide a sample reproduction case.

Here, a freshly created project and a freshly created .adoc:

and that seems to be the default - checking it would add "asciidoc.antora.enableAntoraSupport": true in user settings.json

Extension settings are, by default, defined on the user scope.

That's more like a confirmation of the default state ;)

I guess we could provide a new setting to completely disable Antora support in case you are not using it. Please open a new issue with this feature request.

Thanks for considering it. I'll go check if that can be a simple PR.

[rmannibucau]

Same there, not a single antora.yaml in the workspace but the popup to request to enable antora, https://github.com/asciidoctor/asciidoctor-vscode/blob/a34a34e438109ce47865d29175252a0032f09c97/src/features/antora/antoraSupport.ts#L197 looks wrong and to always return not undefined if there is at least a folder.

[ggrossetie]

looks wrong and to always return not undefined if there is at least a folder.

Could you please expand on that? antoraConfig is undefined and will be assign a value only if await exists(antoraConfigUri)

[ggrossetie]

I cannot reproduce this issue:

Maybe there's something in your settings.xml and/or workspace state?

[rmannibucau]

@ggrossetie well the issue requires some more setup and ultimately means you cannot use vscode.workspace.fs in the function i pointed out. Can you give it a try using a "sample" git repo to ensure your uri looks like f {scheme: 'git', authority: '', path: '/opt/dev/sample/antora.yml', query: '{"path":"/opt/dev/sample/my.adoc","ref":""}', fragment: '', …} or something like that? Nothing special but having a valid .git. You end up in https://github.com/microsoft/vscode/blob/0d22ba354b5e9482d3953759dd817150e15c181f/extensions/git/src/fileSystemProvider.ts#L128 which never fails when the file does not exist (because it is remote I guess and probably "better" to assume than using the network?) so basically exists is not reliable in current form. Probably something like while (current is in workspace folder) { if (localExists(current)) break; } in the spirit.

[nc7s]

Can confirm. A plain directory with a README.adoc does not trigger the prompt. As soon as I run git init there, it shows.

[ggrossetie]

Interesting, I will give it a try!

[ggrossetie]

I can reproduce this issue. As you've mentioned as soon as I run git init, the Antora support prompt appears 😬

[ggrossetie]

In my opinion, we should improve the FileSystemProvider implementation in the Git extension. When using the workspace.fs abstraction I cannot tell if a file exists or not. Upstream issue: https://github.com/microsoft/vscode/issues/204231

[rmannibucau]

Probably but not sure it will solve the issue, the adoc extension wants to work locally not on remote fs as vscode so issue can pop up with any other fs as well and as mentionned impl makes sense in several cases so maybe more a "forceLocal" toggle?

[ggrossetie]

Probably but not sure it will solve the issue, the adoc extension wants to work locally not on remote fs as vscode

Yes and no.
The VS Code extension is also available in a web environment (for instance https://github.dev/). Previously, we were using node:fs but it was not ideal.

As mentioned in the issue upstream, the Git FileSystemProvider return an empty buffer when using readFile on a non-existing file. Also, stat does not indicate if the file exist in the worktree or not.

so issue can pop up with any other fs as well and as mentionned impl makes sense in several cases so maybe more a "forceLocal" toggle?

If a remote FileSystemProvider (correctly) implements both stat and readFile most features should work fine.

[nc7s]

Thanks for upstreaming. It feels weird to have data for nonexistent files, indeed. The try and no-op catch block was added in https://github.com/microsoft/vscode/commit/fb2eca62c21d9112126835acf0c1b868f45019b2. Let's see what they say.

[ggrossetie]

According to the PR title, it could make sense to move the logic in the diff view: "Show empty image instead of error in git diff view for newly staged images". Or add a special logic for newly staged files. Anyway, let's see how it goes.

[rmannibucau]

@ggrossetie ok, the browser case makes sense but as mentionned I don't think remote FS will be implemented as you expect since it is a common optimization from my experience and anyway you don't control the fs impl since it is pluggable, what about checking it exists and has some actual content - at least name and version keys?

[ggrossetie]

what about checking it exists and has some actual content

That's the file system provider purpose, it should check that the file exists or at least return an error when you try to read a non existing file. I cannot fallback to a "local" file system implementation because I don't know how to read a file or how to check if the file exists. Not all remote file systems have a local copy on the disk.

since it is a common optimization from my experience

In my opinion, that's not an optimization but a poor implementation because it breaks the API's expectations.

[rmannibucau]

That's the file system provider purpose, it should check that the file exists or at least return an error when you try to read a non existing file.

Well yes and no, most abstractions don't care about stat cause of the remote point, a better option would be to preload the tree but it is not always possible and you would note that in the reported case the local filesystem would perfectly work in 100% of the case so worse case a test can be done on the env too to make it working all the time (agree it is a workaround but both ways have pros/cons IMHO).

I cannot fallback to a "local" file system implementation because I don't know how to read a file or how to check if the file exists.

As a matter of fact if you run "not in the browser" you perfectly know and it would work in 100% of the case cause in standalone vscode you are always local in terms of desired test - ultimately using git meta without the local file is not what you want to test to give an example where local test is saner than abstracted one. Side note there: why does vscode fallsback on git for the whole repo, this looks either like a bug or api misuse - not sure to be honest.

In my opinion, that's not an optimization but a poor implementation because it breaks the API's expectations.

Most important thing in any UI: feedback speed, way more than consistency or accuracy for such FS operations so it is one, for vscode I'm not 100% sure but I saw it in multiple editors so I assumed it was intended (maybe I'm wrong but was not shocked at least).

Anyway, in any case the extension shouldnt test the presence but the presence of a valid file (with the 2 required attributes I mentionned) so just require the file, there it will fail if not there then you can check its content, no?

[nc7s]

@rmannibucau: don't know why you insist on a "local filesystem" but it shouldn't be concern of this extension, or any "domain business" extension thereof. Their job is to focus on their domain logic, not to patch some particular filesystem provider's leaky impl. If they have to, what is the use of these FS providers?

Most important thing in any UI: feedback speed, way more than consistency or accuracy for such FS operations so it is one, for vscode I'm not 100% sure but I saw it in multiple editors so I assumed it was intended (maybe I'm wrong but was not shocked at least).

The classic, speed over correctness. Apple does that too, they don't sync disk writes so write speed looks amazing. One power outage and you are doomed.

And the most important thing in any UI, is not speed, but to not let anything block the UI, aka Responsiveness. You can definitely let the loading icon spin forever, as long as the user can tap Cancel any time they like and you cancel and goes back.

If the underlying system can only go that far, so be it. Show a spinning wheel if you see fit, but god forbid don't return false data just because "it's fast".

[rmannibucau]

Well your answer goes against most UX rules (I assume you do a dev rule - not block - and not an UX rule - answer fast even if not yet accurate and refine later, too long spinner = no answer there). Anyway nlt my choice, was just trying to say the extension is responsible to choose what it uses as view, the project (scm) view or the dev (local) view. For standalone case - this issue - there is no technical blocker to fix it. Guess we all agree on these statements, rest is project will so I'll let you judge it.

[ggrossetie]

I took a stab at it but using the Git FileSystemProvider there's no way to tell the difference between a file that doesn't exist and an empty file. In both cases, the provider returns empty.

Arguably, an empty antora.yml is not valid since it must have a name and version key. I will update the logic to not show the prompt if antora.yml is empty.