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

group built-in document attributes #619

Open
eiswind opened this issue Aug 14, 2022 · 11 comments
Open

group built-in document attributes #619

eiswind opened this issue Aug 14, 2022 · 11 comments

Comments

@eiswind
Copy link
Contributor

eiswind commented Aug 14, 2022

I would like to suggest to show only relevant suggestions on include/image/video.
Preferably the file suggestions should appear before the attributes that may be relevant.

@Mogztter suggested

https://docs.asciidoctor.org/asciidoc/latest/attributes/document-attributes-ref/

then we can decide which group(s) we want to enable on which context
for instance: compliance, localization and numbering attributes don't make much in the context of an include, image, video, link macro.

As far as I understand the workings, those items are provided by AttributeReferenceProvider

I do not have an idea how to check the context there, as the includes check that on their own in AsciidocProvider

I have seen prefixes beeing used to achieve sortOrder, but I don't have an idea on filtering by context. But maybe that is not even necessary when we have a meaningful order.

@ggrossetie
Copy link
Member

Built-in attributes are provided by: https://github.com/asciidoctor/asciidoctor-vscode/blob/master/src/features/builtinDocumentAttributeProvider.ts with relies on https://github.com/asciidoctor/asciidoctor-vscode/blob/master/src/features/builtinDocumentAttribute.json

AttributeReferenceProvider provides auto-completion when you want to reference an attribute (i.e. {my-attribute}).

I do not have an idea how to check the context there, as the includes check that on their own in AsciidocProvider

VS code gives you the document (vscode.TextDocument) and the position (vscode.Position) when auto-completion is requested (i.e., when provideCompletionItems is called).
So you can get the line and check if the line starts by include:: or image:: or video:: and use that information to change the sortOrder and/or return only meaningful completions items. Does it make sense?

I have seen prefixes beeing used to achieve sortOrder, but I don't have an idea on filtering by context. But maybe that is not even necessary when we have a meaningful order.

I think we should use numeric values: 10, 20, 30.... if two items have the same sortOrder then I think it will use the label. So if we want to show files before attributes then files will be assigned 10 as sortOrder and attributes will be assigned 20 as sortOrder.

@eiswind
Copy link
Contributor Author

eiswind commented Aug 15, 2022

When the built-in attributes come from builtinDocumentAttributeProvider and the file suggestions come from AsciidocProvider the context check is (current state) made in AsciidocProvider. Should we set the sort prefix on the attributes always (does that do any harm?) or do we need to have the context check in all the places (what I think would make things even harder to reason about)?

@ggrossetie
Copy link
Member

I think that's fine, we can set an arbitrary value on built-in attributes and use a lower (or greater) value on other completion items.

@eiswind
Copy link
Contributor Author

eiswind commented Aug 15, 2022

I'll try to set up a PR in the next few days.

@eiswind
Copy link
Contributor Author

eiswind commented Aug 18, 2022

When trying to set this up, I found that in the context on an include the attributes are provided by AttributeReferenceProvider which uses document.getAttributes() . That completely makes sense, but it would be of no help to group the attributes from BuiltinDocumentAttributeProvider as they are not shown here anyway.

If it is of interest to have the grouping for other reasons I could open up thePR, as I made the changes to buildinDocumentAttribute.jsonalready, but I don't see how to make any sense of it in the current setup.

@ggrossetie
Copy link
Member

If it is of interest to have the grouping for other reasons I could open up the PR

I haven't tested it recently but I think you can show all the auto-completion items (from all the providers) if you explicitly type Ctrl+Space.

@eiswind
Copy link
Contributor Author

eiswind commented Aug 19, 2022

async provideCompletionItems (textDocument: vscode.TextDocument, position: vscode.Position): Promise<vscode.CompletionItem[]> {
const linePrefix = textDocument.lineAt(position).text.substr(0, position.character)
if (linePrefix !== ':') {
return undefined
}
return this.completionItems
}
}

The attributes from the json file are only shown, when the line starts with a colon.

Actually there are also duplicates then coming from the AttributeReferenceProvider.

@ggrossetie
Copy link
Member

The attributes from the json file are only shown, when the line starts with a colon.

Oh that's right!

Actually there are also duplicates then coming from the AttributeReferenceProvider.

Do you have an example? We might need to use a Set to remove duplicates.
Also, I think we need to check the context when providing document attributes as completion items.

@eiswind
Copy link
Contributor Author

eiswind commented Aug 21, 2022

Bildschirmfoto vom 2022-08-21 10-41-53

Scroll down a bit

Bildschirmfoto vom 2022-08-21 10-42-09

@ggrossetie
Copy link
Member

In this case, built-in attributes must come first. Having said that, it does not make sense to display attribute references unless you want to use the value of an attribute in an attribute value:

:url-asciidoctor-docs: https://docs.asciidoctor.org
:url-asciidoctorjs-doc: {url-asciidoctor-docs}/asciidoctor.js/latest/

Anyway, we should hide attribute references if the line only contains :.
Similarly, it does not make sense to display built-in attribute when defining an attribute value, so if the line is not : then we should not display them (but I think that's already the case)

@ggrossetie
Copy link
Member

To clarify, the name is identical but the purpose is not the same so they are not duplicates.

// Define a built-in attribute
:app-name: foo

// Reference the app-name attribute 
Install {app-name} using `npm`...

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

No branches or pull requests

2 participants