ThFriedrich / lammps_vscode

VSCODE extension for language support of LAMMPS scripts
https://thfriedrich.github.io/lammps_vscode/
GNU General Public License v2.0
44 stars 14 forks source link

Autocompletion / quick access to docs #2

Closed arn-all closed 4 years ago

arn-all commented 4 years ago

Hi,

Thanks a lot for this helpful extension !

I think autocompletion and quick access to the docs would be much beneficial to new users (Lammps is hard to learn !). What I can think of is :

I am not familiar with the VS Code API, and have no experience with JS, but I'm keen to learn and try to work on this.

Do you think these features are on the scope of the extension ? Are you interested by developing these features ?

ThFriedrich commented 4 years ago

Hi, your suggestions are very good. I have been considering these things at some point and it is certainly possible to do. However, it is a lot more invested than simple syntax highlighting. So far I wasn't ready to invest the time to go down that road, but I created the keyword groups with these possibilities in mind already. Something relatively easy to do first would be to add snippets perhaps (instead of the Syntax reminder). Autocompletion should be feasible in principle, but I'd have to check how to do it. It's my first vscode extension, so I am learning these things myself in the process as well. Of course, it's a great motivator if other people join in and contribute to the project. :)

arn-all commented 4 years ago

Hi,

I'm new to this kind of things too. I did some tests with the VSCode tutorials, based on the "extension" template (it gives me a project structure pretty different than what you have, but I'm confident it can be easily transferred).

It's really not much, but I wanted to see if I could just create a dummy Hover behaviour.

The result looks pretty much like that when dump is hovered : image

As a first attempt, I didn't implement the get_documentation() and hard coded the docs for the "dump" command.

Adding the following code to the "src/extension.ts" template of the tutorial is pretty much all I had to do. The entire thing is here.

vscode.languages.registerHoverProvider("lmps", {
    provideHover(document, position) {
        const range = document.getWordRangeAtPosition(position)
        const word = document.getText(range)
        return createHover(word)
    }
})

function get_documentation(snippet){
    return {
        "syntax":"```dump ID group-ID style N file args```",
        "examples": "- dump myDump all atom 100 dump.atom  \n- dump myDump all atom/ mpiio 100  \n- dump.atom.mpiio  \n- dump myDump all atom / gz 100 dump.atom.gz  \n- dump 2 subgroup atom 50 dump.run.bin  \n- dump 2 subgroup atom 50 dump.run.mpiio.bin  \n- dump 4a all custom 100 dump.myforce.* id type x y vx fx  \n- dump 4b flow custom 100 dump.%.myforce id type c_myF[3] v_ke  \n- dump 4b flow custom 100 dump.%.myforce id type c_myF[\*] v_ke\n- dump 2 inner cfg 10 dump.snap.*.cfg mass type xs ys zs vx vy vz\n- dump snap all cfg 100 dump.config.*.cfg mass type xs ys zs id type c_Stress[2]\n- dump 1 all xtc 1000 file.xtc\n",
            }
}

function createHover(snippet) {
    const example =
        typeof snippet.example == 'undefined' ? '' : snippet.example
    const description =
        typeof snippet.description == 'undefined' ? '' : snippet.description
    const docs = get_documentation(snippet)
    const content = new vscode.MarkdownString("**Syntax** :  \n" + docs.syntax + "\n\n**Examples** :  \n" + docs.examples, true)
    return new vscode.Hover(content)
}

Regarding the Hover-for-docs feature, I think it'd be quite straightforward now to do two things :

For the autocompletion, I didn't really understand how it's done in VSCode. Maybe looking at a community extension would make it more clear.

Snippets are a nice feature too, but might be tedious to implement if it involves choosing one relevant example from the docs for each command ?

ThFriedrich commented 4 years ago

Nice work. It really doesn't seem all that difficult after all. :) I created a new branch where we can play with these functionalities here. I also merged your test example into the branch and adapted the package.json file and it seems to work already. If you could automate the generation of the js file from the documentation, that'd be awesome. I guess that'd be the most essential task for the time being. I'd volunteer to look into the autocompletion feature. :) Thanks a lot for your input!

arn-all commented 4 years ago

I managed to implement a quick POC. I'll have to test it more completely and implement it properly... but for now, it works pretty much, for min_spin and compute_dihedral_local ! I'll keep you posted

ThFriedrich commented 4 years ago

I've been playing with similar approaches. I pushed commits into the branch iss_2_hover, with some intermediate results. The idea was to:

That works in principle and allows for full automation of updating the documentation. Having the html files, we could also use those to provide an included documentation within vscode. I gave that a quick try as well. At the moment it looks like that: Screenshot at 2020-03-31 13-41-59

arn-all commented 4 years ago

Good job, that looks brilliant !

I tried to pull it to test, but the extension was unable to start. When I open a .lmp file, I get :

Activating extension 'ThFriedrich.lammps' failed: Cannot find module './get_doc'
Require stack:
- /home/arnaud/test/dev-code-extension/lammps_vscode/out/extension.js
- /usr/share/code/resources/app/out/vs/loader.js
- /usr/share/code/resources/app/out/bootstrap-amd.js
- /usr/share/code/resources/app/out/bootstrap-fork.js.

Is it normal ?

ThFriedrich commented 4 years ago

Maybe it is because I have my "out" folder in my .gitignore. I just uploaded that folder but I am not entirely sure that that's going to fix the problem. Did you run npm run compile?

arn-all commented 4 years ago

It fixed it, thanks. The behaviour of the hover is great.

It will just need a bit of styling on bullet lists, etc. It seems that VSCode MarkdownString can't render inline math, which can be found in the Description section of some commands.

Anyway, I guess most people won't read this section directly in the hover widget. One possibility would be printing just the first few lines of the description (for a quick answer to "what's that command about again ?"), ending by a "... read more" hyperlink to the online (or offline ?) docs. Having the hyperlink right under the command name can also be a nice time saver...

I also found a pitfall that I'm not sure about how to fix; multi-words commands like "fix box/relax" have a dedicated entry in the ts array, but hovering it either triggers the docs for "fix" or an "undefined" (when hovering boxrelax). Changing the ts entry for "boxrelax" would work for this case, but would probably cause collisions for some keywords...

ThFriedrich commented 4 years ago

The possibilities with markdown may be better used if we construct the content in that fashion:

function createHover(snippet: string) {
    const docs = get_documentation(snippet)
    if (docs?.command) {
        const content = new vscode.MarkdownString()
        content.appendMarkdown("# "+docs?.command+" \n" + "--- " +" \n")
        if (docs?.syntax) {
            content.appendMarkdown("## Syntax: \n")
            content.appendCodeblock(docs?.syntax,"lmps")
            content.appendMarkdown(docs?.parameters + "\n\n")
        }
        if (docs?.examples) {
            content.appendMarkdown("## Examples: \n")
            content.appendCodeblock(docs?.examples,"lmps")
        }
        if (docs?.description) {
            content.appendMarkdown("## Description: \n")
            content.appendMarkdown(docs?.description + "\n")
        }
        if (docs?.restrictions) {
            content.appendMarkdown("## Restrictions: \n")
            content.appendMarkdown(docs?.restrictions)
        }
            return new vscode.Hover(content)
    }
}

With the appendCodeblock functionality we can properly display examples with keyword highlighting, etc. The if-statements also avoid the "undefined"-Hover popups. Looks like that now: image

It doesn't fix all the styling issues but I think its a great improvement. I also think that the entire description may be a bit much for the pop-up. I like the idea of showing only the first bit and providing a hyperlink.

The problem with the "fix box/relax" - type of commands can probably be fixed by using regular expressions in the range selection in some way: const range = document.getWordRangeAtPosition(position)

ThFriedrich commented 4 years ago

I implemented the RegExp Selector and I think the Styling is okay now too. I pushed the changes for testing and I'd be interested in @arn-all's feedback if you find the time to give it a go. :)
The internal HTML documentation still doesn't work yet though. That needs some more work. Until we have that running, I'd leave the full description in the hover...

arn-all commented 4 years ago

That looks great, I'm very happy with how the extension behaves. I'll use it every time I have to deal with lammps files from now ! For "fix box/relax", it doesn't seem to really work as expected : what I get is the docs of "fix" whether I hover fix or box/relax. Another detail is that the hover window opens at the scroll position of any previous hover. I hover keyword1, scroll, hover keyword2 ; I'm at the position I scrolled down to in the hover of keyword1, instead of being at the top of keyword2 docs...

(Perhaps, some users might not want to use the hover docs - one improvement for the future would be an option to disable it in the extension parameters.)

ThFriedrich commented 4 years ago

Thanks for the feedback. :) The "fix box/relax" behaves correctly in my case... fix_box_relax

Are you running on Windows? Are there differences in the interpretation of regexp among the different OS? :thinking:

I also had a look into the autocompletion feature, which is not too difficult to provide using the typescript array:

autocomplete

However, a short description of the commands would be very useful here as well. But I am not sure how to go about this. Just cutting the descriptions isn't ideal I suppose. There's also still Styling issues remaining, especially with Formulas:

styling

Perhaps we should consider better ways to generate the ts-array?

The issue with the scrolling position is a little bit annoying but I don't know how to fix it. It seems there's not much one can do with the Hover-Object. :man_shrugging:

The embedded documentation still needs some more work. This may take a while. For now, I let the "Open Lammps Documentation"-Command open the online documentation in the browser.

arn-all commented 4 years ago

Whoa, I'm litteraly blown away by the autocompletion module. I wish I had that before !

Concerning fix box/relax, I found the trick : the pattern is "fix 1 all box/relax iso 0.0 vmax 0.001", whilst the regex you implemented would match "fix box/relax"... there's the ID and Group_ID in between. It'd require some more testing to avoid false positives, but something like (\w+)(?:[\t\s]+[\w\d]+){2}[\t\s]+([\w\/]+)? seems to capture efficiently :

Note that I used [\s\t]+ for whitespaces as it allows users to separate arguments by several whitespaces or tabs for indentation. This way, the regex gives 1 or 2 keywords corresponding to the capture groups. If there's documentation for keword1+" "+keyword2, we can display it, if not, we can fall back to keyword1. What do you think ?

Concerning the description of the command, I remarked that this section always starts with a one-sentence description of what the command does (please tell me if I'm wrong !). I think this sentence is just enough context for users to know if it's the command they're looking for, at least in the autocompletion module. It might be captured from the documentation section with a regex as simple as ^[^.]+\.\s+ (line start ^, any series of chars that are not ., then a mandatory \.\s), and added to the ts-array in a separate field for convenience. I tested the regex and obtained this : matches.txt. It's not perfect though: out of 421 description entries of the ts-array, there are 34 that contain a formula in the 1st sentence, and that won't be well formatted for the moment...

I'd advocate for giving up rendering correctly the full Description section, as maths rendering, correct lists display, etc. seem to be tricky to achieve, whilst not giving a comfortable reading experience in the end. I'd propose the following structure if you agree :

one-sentence description of the command. [Read more in browser hyperlink]
## Syntax
-- syntax pattern --
## Examples
-- examples as shown already --

It also shortens considerably the text to display, and makes the scrolling thing much less annoying. In the "docs-hover", I'd remove the huge T1 title (command name) that doesn't bring information.

(I wrote all of this assuming formulae can't be rendered in VSCode hovers - I might be wrong of course)

arn-all commented 4 years ago

Some more thoughts :

1/ There's another issue with "fix box/relax"-like commands : typing fix suggests fix box/relax. If I press enter, I get this fix box/relax command written to my file, which is an invalid Lammps command, as there should be an ID and Group ID between fix and "box/relax"...

2/ For some reason, lammps keyword highlighting has been partially broken in my VSCode for a few days, whilst the hovers you implemented work correctly : image

As you can see, keywords are not highlighted as they are in your screenshots. I have no idea what I did wrong, [deleting + git cloning + npm install + npm run compile] the repo didn't fix it, neither did getting the official extension from the market place... And now I just saw that when the new autocomplete module appears, the docs are not displayed on the side anymore... :facepalm: Do you have an idea of how I can fix that, or any relevant documentation I could rely on ? I'm sorry, computers aren't being friendly to me today

ThFriedrich commented 4 years ago

I couldn't get your proposed regex to work properly, but if you did, feel free to push the changes. :) I agree it'd be more useful to capture the commands as they are written out in working syntax for the hover feature. Concerning the structure of the hover I also agree with you the heading is not useful. It should go. I'd put the Syntax first though. Especially because we cannot yet provide a short description for every command, because of the rendering issues of math symbols. I don't think it is easy to implement. Perhaps we should just filter them out for the time being and just refer to the documentation instead for these cases. I would suggest adding the short description as a key to the ts-array. It can easily be done in the python-script. There we can also insert the link in markdown format. I'll try to implement that.

We could let the automcompletion feature insert the syntax string instead of the command. It is as easy as adding compl_it.insertText = c.syntax in the get_completion_list function:

export function get_completion_list() {

    const completion_List = new CompletionList();

    for (let c of command_docs.values()) {
        var compl_it = new CompletionItem(c.command);
        compl_it.documentation = new MarkdownString();
        compl_it.documentation.appendCodeblock(c.syntax)
        compl_it.documentation.appendMarkdown(c.parameters)
        compl_it.documentation.appendMarkdown(" \n" + "--- " +" \n")
        compl_it.documentation.appendText(c.description)
        compl_it.insertText = c.syntax
        completion_List.items.push(compl_it)
    }
    return completion_List
}

Why the highlighting doesn't work for you anymore, I have no idea... :frowning_face:

arn-all commented 4 years ago

I implemented a much simpler regex, the PR is #3. Now I get the expected behaviour for the following various patterns I tested :

quit
units metal 
atom_style atomic
boundary p p p
change_box all triclinic
mass 1 55.845
pair_coeff * * lib/potentials/2007--Becquart--Fe-C--LAMMPS--ipr1.eam Fe
# min_modify dmax 0.05
timestep 0.0001
min_style fire
minimize 0 1e-06 10000 10000

# and some multi-words commands :
fix boxrelax all box/relax x 0.0 y 0.0 z 0.0 xy 0.0 xz 0.0 yz 0.0
compute peatom all pe/atom # hello world

Does it work for you ?

arn-all commented 4 years ago

I tested the completion insertText idea. It can be convenient, but it's a lot of manual intervention to move the cursor around to replace the generic content with something relevant to the context. Plus, I'm not sure the user who sees a command autocomplete would expect getting a bunch of words when pressing enter. I guess that 'd be better handled by snippets you mentioned earlier, as they move the cursor around for you -but it's my personal view, and snippets would require more thinking and more effort to implement.

I also tried to add compl_it.detail = c.syntax, which interestingly prints a tiny little reminder of the syntax only in case the side docs panel is closed. I think it's a good idea to change compl_it.documentation.appendCodeblock(c.syntax) to compl_it.detail = c.syntax, so the behaviour is the same with the docs panel, but a more minimalist auto-completion experience is possible by closing it :

image

By the way, closing the docs panel was one of my mysteries of yesterday :roll_eyes: ; there's actually a little "close" and an little " i " symbols that allow to close and re-open the docs panel (the (i) won't show up on my capture above because of my capture tool, it should be at the end of line).

ThFriedrich commented 4 years ago

I pushed a couple of changes. I implemented the function to check for incomplete syntax in the get_documentation function here. In the process, I stumbled over the group of "fix_modify AtC"-Commands which are somewhat different. They have the AtC-Tag in the Command name in the documentation but these Tags are not actually part of the command. They can be followed by up to two single words, as in this example:fix_modify AtC mesh read where the syntax is actually fix_modify <AtC fixID> mesh read <f|p> <f|p> <f|p>. The function I have now seems to work for all these cases, but it's kind of ugly code with a bunch of nested if-loops. Do you have an idea how to do this more elegant?

Further, I implemented the short description and the possibility to customize the style of the Hover: hover and also the behaviour of the Autocomplete feature can be customised: autocomplete As you can see, the Extension now has a dedicated menu in the settings.

I dropped the idea with the embedded html documentation for now. Though I've been playing with the webview api and iframes and managed to display html content and images in a vscode-column/tab, it lacks html-functionality, like links or math rendering. I'm fairly confident it can be done but it'd need more expertise in html and css I reckon.

arn-all commented 4 years ago

That is great !

I have no idea of a refactor of the nested "if" off the top of my head, as the problem lies in the syntax used by LAMMPS in the first place... We can think of a more efficient approach if it proves to slow down the editor, though I don't fell like it's much the case for now.

I guess the last thing to do before closing the issue is the hyperlink to online docs ? It seems that the path is trivially : https://lammps.sandia.gov/doc/ + the name of the HTML file parsed by getPar.py. If so, I can do the modification in getPar.py to add it to the ts array, and add the hyperlink to the markdown hover. I'm just not sure yet that vscode will fire up a browser tab when an hyperlink is clicked or if I need to add something more, but I'll figure out.

arn-all commented 4 years ago

I had trouble with git but now PR #6 is submitted.

I put the documentation hyperlink at the top of the hover, because in case the arguments list is long, it gets annoying to scroll to reach the link, and somehow takes off the idea of a quick access to docs. Plus, there's the unaddressed scrolling memory problem that would make it worse.
If you can think of a better option please modify !

Is there more to do on this issue ?

Edit : also feel free to reformulate the "open documentation" and "read more..." if needed

ThFriedrich commented 4 years ago

Thanks a lot for your contributions and inspirations on these topics! You really pushed this project a big step further. :) I think at this point it is good enough to close the issue. The remaining minor problems should probably be tracked in separate issues. Anyway, I'd actually like to go ahead and publish an update to the marketplace if everything works. It would be nice if a few more people could actually test and confirm that. I think this work is a great improvement that many users will appreciate.

Did you find out why your keyword highlighting didn't work? Is that fixed?

arn-all commented 4 years ago

I'm very happy with what we have now, and very thankful as I couldn't do all of that alone. I'd be keen to keep contributing if you have other features in mind. I'd be happy to ask my colleagues for feedback as soon as it's pushed to the marketplace !

No, I just opened a separate issue for my highlighting issues #7. I guess I can figure out myself what's wrong or at least how to fix.

ThFriedrich commented 4 years ago

I updated the documentation and published Version 1.2.0 to the marketplace.