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

support cm* files #798

Merged
merged 21 commits into from
Jan 27, 2022
Merged

support cm* files #798

merged 21 commits into from
Jan 27, 2022

Conversation

tatchi
Copy link
Collaborator

@tatchi tatchi commented Dec 13, 2021

This PR adds a new custom editor that shows info about .cm(t, i, ti, o) and .bc files. It uses the ocamlobjinfo behind the scene.

I didn't test it with .bc files because I don't know how to generate these files 😅

Question: it looks like ocamlobjinfo also supports .cmx, .cmxa, .cma, .cmxs. Should we also add support for them?

Partially implements #505

src-bindings/vscode/vscode.ml Outdated Show resolved Hide resolved
src-bindings/vscode/vscode.ml Outdated Show resolved Hide resolved
@ulugbekna
Copy link
Collaborator

.bc are ocaml bytecode files, so you get those easily:

$ cat > test.ml
print_endline "Hi @tatchi!"
$ ocamlc test.ml -o test.bc
$ ocamlrun test.bc
Hi @tatchi!

Having a dune file such as

(executable
 (name hello_world)
 (modes byte exe))

and building your project should also produce hello_world.bc in _build.

A little more info: https://discuss.ocaml.org/t/ocamlc-bytecode-file-vs-dune-bytecode-bc-file/7395

@tatchi
Copy link
Collaborator Author

tatchi commented Dec 14, 2021

.bc are ocaml bytecode files, so you get those easily:

$ cat > test.ml
print_endline "Hi @tatchi!"
$ ocamlc test.ml -o test.bc
$ ocamlrun test.bc
Hi @tatchi!

Having a dune file such as

(executable
 (name hello_world)
 (modes byte exe))

and building your project should also produce hello_world.bc in _build.

A little more info: https://discuss.ocaml.org/t/ocamlc-bytecode-file-vs-dune-bytecode-bc-file/7395

Oh cool, thanks for the great explanations 🤗 I just tested it and it seems to work well with these files 👌

What about the other extensions: .cmx, .cmxa, .cma, .cmxs ? Should we support them too?

@mnxn
Copy link
Collaborator

mnxn commented Dec 15, 2021

What about the other extensions: .cmx, .cmxa, .cma, .cmxs ? Should we support them too?

If ocamlobjinfo works for those file types, we should definitely support them.

src-bindings/vscode/vscode.ml Outdated Show resolved Hide resolved
src/cm_editor.ml Outdated Show resolved Hide resolved
@tatchi
Copy link
Collaborator Author

tatchi commented Dec 20, 2021

Thanks for the review @mnxn 🤗 I should have addressed all your comments :)

src/cm_editor.ml Outdated Show resolved Hide resolved
src/cm_editor.ml Outdated Show resolved Hide resolved
Copy link
Collaborator

@ulugbekna ulugbekna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into TextDocumentContentProvider as a lighter alternative to using custom documents because custom documents seem quite heavy. According to its doc:

A text document content provider allows to add readonly documents to the editor, such as source from a dll or generated html from md.

Content providers are registered for a uri-scheme. When a uri with that scheme is to be loaded the content provider is asked.

It seems we could use that and avoid using a custom document, which seems a (heavy-weight) overkill for this feature. I also suppose that then we would get file-change watching for free. What do you think?

Here's an example of how to use that API and it seems the right fit for this task: https://github.com/microsoft/vscode-extension-samples/tree/main/contentprovider-sample

CHANGES.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@tatchi
Copy link
Collaborator Author

tatchi commented Dec 29, 2021

I looked into TextDocumentContentProvider as a lighter alternative to using custom documents because custom documents seem quite heavy. According to its doc:

A text document content provider allows to add readonly documents to the editor, such as source from a dll or generated html from md.
Content providers are registered for a uri-scheme. When a uri with that scheme is to be loaded the content provider is asked.

It seems we could use that and avoid using a custom document, which seems a (heavy-weight) overkill for this feature. I also suppose that then we would get file-change watching for free. What do you think?

Here's an example of how to use that API and it seems the right fit for this task: https://github.com/microsoft/vscode-extension-samples/tree/main/contentprovider-sample

Thanks for your review @ulugbekna 🤗 TextDocumentContentProvider is what I wanted to use in the first place for the reasons you mentioned above. According to my understanding, TextDocumentContentProvider can't be used when opening a file. It only works when we programmatically fire an action, like in your example when a user clicks on Show all references.

See this comment and the reply below: microsoft/vscode#53121 (comment)

src/cm_editor.ml Outdated
Comment on lines 81 to 87
let (_ : Disposable.t) =
WebviewPanel.onDidDispose webviewPanel
~listener:(fun () -> Disposable.dispose disposable)
()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure we'd like to ignore this disposable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I fixed it in 20c9896

I went for using a Stack since it's a mutable structure. I could also just use a regular List and mutate it if you prefer.

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

Successfully merging this pull request may close these issues.

3 participants