-
Notifications
You must be signed in to change notification settings - Fork 74
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
fix: make VsCoq activity bar logo appear only when Coq files present #897
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this !
client/src/extension.ts
Outdated
@@ -51,6 +51,27 @@ import { QUICKFIX_COMMAND, CoqWarningQuickFix } from './QuickFixProvider'; | |||
let client: Client; | |||
|
|||
export function activate(context: ExtensionContext) { | |||
|
|||
// Function to check for Coq files present in the workspace | |||
function checkCoqFilesPresent() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can replace this by the "workspaceContains" activation event in the package.json (see here). I am not sure if it requires workspace capabilities from the server that we still need to implement but I don't see why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be cool to use the activation event, the only downsides I can think of are:
-
The
workspaceContains
is only emitted on folder open (not file creation or deletion). For example, this would make it so if we open an new/empty folder, then create a_CoqProject
it won't make the activity bar entry appear
(this could maybe mitigated by adding_CoqProject
to the recognized extensions for the coq language, although I think this would involve more separate work) -
Similarly to above, since no deletion events are tracked the activity bar entry wont disappear if all
.v
and_CoqProject
files disappear. This does seem like a corner case though, if someone ends up deleting all their Coq related files they can maybe deal with the activity bar entry sticking around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay ! I see your point ! I think it is fine as is then ! Could you just add _CoqProject
as part of the checkCoqFilesPresent()
routine ? I'll then merge !
client/src/extension.ts
Outdated
} | ||
|
||
// Set initially on extension activation | ||
checkCoqFilesPresent(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use the activation event this simply can be replaced by a
commands.executeCommand('setContext', 'coqFilesPresent', true);
Don't forget to "deset" it in the deactivate function, i.e. add
commands.executeCommand('setContext', 'coqFilesPresent', undefined);
in the deactivate function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose on top of that, it could activate if other files are present such as a _CoqProject file ?
So maybe call the context 'inCoqProject'
?
@rtetley Pushed two commits: Both include adding a language entry for _CoqProjects. This allows for proper comment toggling in CoqProjects via the VsCode built in commenting command ( Let me know your thoughts on the CoqProject language entry, and which of the two commit methods seem more reasonable to you. |
I like this very much ! I was worried about _CoqProject comments being parsed anyways because of coq-community/vscoq-legacy#49 but it turns out after checking that we do ignore comments ! |
Sounds good, pushed changes to make it appear/disappear on file create/delete if it should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks !
Ah I would much rather you rebase onto main. Do you mind resetting to 455b5f1 then rebasing on main ? |
Yes sorry, I didn't know that using the GitHub built in conflict resolution made a merge |
9761462
to
455b5f1
Compare
455b5f1
to
24fb030
Compare
Unrelated to this PR but I didn't know where else to ask; is it intended that |
I am waiting for ocaml/opam-repository#26506 (comment) In general I like to sync the release of the language server and the extension. In this case I suppose it's not blocking but I could be confusing. |
That makes perfect sense and is a good practice. I was mainly just curious |
Thanks for implementing this! So what will the behavior of the icon be then? The LaTeX workshop icon only appears when I actually open a tex file. That's quite nice as it means if a workspace happens to contain a tex file I don't care about, I don't have the icon clutter the sidebar. But the discussion above makes it sound like the Coq icon will appear whenever a Coq file exists, even if no Coq file is ever opened? |
@RalfJung you are correct, it would function based on if the active workspace is related to Coq (existence of a Coq related file), not the specific focused or opened file(s). I can definitely see the use case you are describing (and it would be programmatically easier to implement). I think maybe we should think of the following possible cases and spec out what would be optimal behavior for the activity bar: Here is its current behavior (feel free to add any more conditions that might be relevant):
|
Yeah that's fair. I think I have exactly one workspace that contains .v files I don't want it to open (since I have a separate sub-workspace for that folder)... so not worth a lot of effort, I just figured I would bring this up. |
We could add a config option along the lines of With With
|
Do people actually use the sidebar though? One of the first things I did when configuring VSCode was getting rid of it given how useless it is, am I missing on anything? |
Like in general or specifically for VsCoq? I like the vscode provided workspace search and source control built into the sidebar and use it fairly frequently. The VsCoq one I do not use because I'd rather type my Search, Check, etc. commands in a file and have them print in the proof view. |
I see, thanks. I do use those features but using commands. It's good to have it at least for new users I guess. :) |
This adds a context to watch and track the presence of Coq files in the current workspace, and will dynamically display the activity bar logo only if Coq files are present.
Should fix #886