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

Add code link support for hierarchical document symbols #303

Merged
merged 8 commits into from
Mar 16, 2022

Conversation

TheOneKevin
Copy link
Contributor

@TheOneKevin TheOneKevin commented Mar 5, 2022

This PR adds the ability to create code links to hierarchical document symbols. The main contribution of this PR is the addition of a new command:

hediet.vscode-drawio.linkSymbolWithSelectedNode

Some notes:

- Added linkFileWithSelectedNode command to package.json
- Changed command contributes in package.json to support category
- Removed linkFileWithSelectedNode from the command palette
(as it is a context menu only command and will error otherwise).
Copy link
Owner

@hediet hediet left a comment

Choose a reason for hiding this comment

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

Many thanks for implementing this cool feature!

I left some comments.
I think it would also be cool if this somehow worked together with the #Symbol syntax.

I just read this code, didn't try it out yet. Will do that soon.

package.json Outdated
},
{
"command": "hediet.vscode-drawio.linkFileWithSelectedNode",
"title": "Draw.io: Link File With Selected Node In Active Diagram"
"title": "Draw.io: Link File With Selected Node",
Copy link
Owner

Choose a reason for hiding this comment

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

I think the Draw.io: prefix is not necessary here.

"commandPalette": [
{
"when": "false",
"command": "hediet.vscode-drawio.linkFileWithSelectedNode"
Copy link
Owner

Choose a reason for hiding this comment

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

Why removing this command from the command palette?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Invoking this from the palette will fail as it is expecting an extra argument only present when calling it from the context menu. Removed to avoid accidentally using it from the palette.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, makes sense, thanks!

Comment on lines 386 to 388
path: path
.relative(relativeTo.fsPath, this.uri.fsPath)
.replace(/\\/g, "/"),
Copy link
Owner

Choose a reason for hiding this comment

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

In case of a symbol, is the path neccessary here?

Copy link
Owner

Choose a reason for hiding this comment

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

Ahh, the symbol is still relative to the document. Can we make it global? Otherwise, renaming a file will break the symbol reference.

Copy link
Contributor Author

@TheOneKevin TheOneKevin Mar 7, 2022

Choose a reason for hiding this comment

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

Sure, I'll add a way to link it to a top level workspace symbol in the hierarchy. However, we should still allow linking to files as some language servers/symbol providers don't expose workspace symbols in a hierarchy.

Comment on lines 200 to 205
items.push(<QuickPickItem>{
label: `$(${symbolList[x.kind]}) ${x.name}`,
description: x.detail,
detail: curpath
});
recurse(x.children, curpath);
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe it makes sense to consider the selection? I.e. only symbols that intersect with the current selection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about a context menu entry in the editor? We can show the symbols at the cursor or selection when the command is called from the context menu (and if the command is called from the command palette, we can show every symbol)

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, it would be great if we could show that context menu entry only if a node is selected in a draw.io diagram.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll add them as regular commands first then we'll see about the context menu.

Comment on lines 36 to 42
const symbolList = [
'symbol-file',
'symbol-module',
'symbol-namespace',
'symbol-package',
'symbol-class',
'symbol-method',
Copy link
Owner

Choose a reason for hiding this comment

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

Better documentation please or better name (e.g. symbolNameMap) ;)
Maybe a record Record<Symbol, string> is better here to indicate the purpose.

- Only shows symbols intersecting selections (if code is selected)
- Allowed symbols to be linked without URI.
-> This uses top-level workspace symbols to resolve the hierarchy
-> Added command linkWsSymbolWithSelectedNode
- Removed unneeded "Draw.io:" prefix
@TheOneKevin TheOneKevin requested a review from hediet March 10, 2022 04:38
@TheOneKevin
Copy link
Contributor Author

Updated with new changes 😄

@hediet
Copy link
Owner

hediet commented Mar 10, 2022

Awesome! Can you update the changelog file and set a prerelease version? (See history)
Then it will get released automatically after I merge this.

- Added 1.6.5-alpha.1
@hediet
Copy link
Owner

hediet commented Mar 11, 2022

Awesome! Did you test this feature?

Comment on lines 38 to 65
const symbolNameMap: Record<SymbolKind, string> = [
'symbol-file',
'symbol-module',
'symbol-namespace',
'symbol-package',
'symbol-class',
'symbol-method',
'symbol-property',
'symbol-field',
'symbol-constructor',
'symbol-enum',
'symbol-interface',
'symbol-function',
'symbol-variable',
'symbol-constant',
'symbol-string',
'symbol-number',
'symbol-boolean',
'symbol-array',
'symbol-object',
'symbol-key',
'symbol-null',
'symbol-enum-member',
'symbol-struct',
'symbol-event',
'symbol-operator',
'symbol-type-parameter'
];
Copy link
Owner

@hediet hediet Mar 11, 2022

Choose a reason for hiding this comment

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

Huh, I'm surprised this type-checks!
I think it would be better like this though:

const symbolNameMap: Record<SymbolKind, string> = {
    SymbolKind.file: 'symbol-file',
    SymbolKind.module: 'symbol-module',
    ...
};

Otherwise I would need to review that the order of the symbols here matches with the numeric values of the SymbolKinds (which could change btw.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, good idea. I assumed SymbolKinds enum would never change but that's a bad assumption.

@hediet
Copy link
Owner

hediet commented Mar 11, 2022

Please fix formatting, then I can merge this! Probably will do some refactoring later though.
Thanks a lot for this cool feature!

- Ran prettier on code. npm run lint passes :)
@TheOneKevin
Copy link
Contributor Author

The only testing I did was with the files under examples/linking. I added some nodes to cover the new cases. Other than that, I ran Prettier so it should conform to the style guides now.

@hediet hediet merged commit be6eded into hediet:master Mar 16, 2022
@hediet
Copy link
Owner

hediet commented Mar 16, 2022

Thank you! Will soon be available in Draw.io insiders!

@janklostermann
Copy link

Thanks so much for this incredible plugin!

If I understood this feature right, it allows to link a node in a draw.io-drawing to another file (e.g. a CAD-File, or another drawio-File) within the VScode controlled folder (and/or?) workspace. Click on the node would then open the other file (within VSCode if appropriate)
I was very hopeful to have it already available, as in 2022 it was "soon be available".

Maybe I am just to stupid to find it or understand how it works.

Could you please add this feature and usecase to your documentation and usage examples to allow me and others to easily understand and use it?
(Or did I just did not see it? And you could point me to it?)

@janklostermann janklostermann mentioned this pull request Mar 19, 2024
@TheOneKevin
Copy link
Contributor Author

@janklostermann: Hey! Check out docs/code-link.md for examples on this. The examples/linking/ directory also contains examples on how to use this. If I remember correctly, this linking feature allows you to link symbols inside a document -- I believe you need a language server support for this? Not sure about arbitrary files.

@janklostermann
Copy link

@TheOneKevin That's exactly where I tried to find it. But as you already mentioned, it seems to support only symbols.

Are files of a folder or workspace considered as symbols? (I don't actually know what symbols mean exactly in this context.)
Otherwise an function extention might be needed to allow relative paths or links to files within the workspace, when symbols within these files are not available.

By the way, for a more specific usecase: Linking to another *.drawio[.png/.svg]:
Do these drawio-files (languages?) provide symbols?

That would already resolve my current problem to expanding a box in a .drawio.png-file by clicking on it to open another .drawio.png file, showing the content of that box. (needed, as neither png nor svg support multi-pages, and I would like to use one of these more generally compatible formats in my repository. Great thanks they exist!!!)

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