-
Notifications
You must be signed in to change notification settings - Fork 442
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 refactoring documentation #1334
Conversation
2fde90c
to
8f2cc21
Compare
Since the API is not ready in stable version yet, so we can first start working on the document structure. @fbricon @snjeza @akaroml @testforstephen @Eskibear Please take a look at the structure and give your feedbacks. Once the structure is determined and checked-in. All the teammates can start filling the content into it. I'll raise another PR to add the implementation part once the document is finished. |
Question is: how will you map refactoring ids to markdown anchors, such as #extract-to-constant |
That a good question! Currently, the API will categorize the document according to the export class CodeActionKind {
static readonly Empty: CodeActionKind;
static readonly QuickFix: CodeActionKind;
// --- refactor start
static readonly Refactor: CodeActionKind;
static readonly RefactorExtract: CodeActionKind;
static readonly RefactorInline: CodeActionKind;
static readonly RefactorRewrite: CodeActionKind;
// --- refactor end
static readonly Source: CodeActionKind;
static readonly SourceOrganizeImports: CodeActionKind;
static readonly SourceFixAll: CodeActionKind;
} This is somehow different from our |
I guess a mapping mechanism will be needed in the follow-up PR. |
Update: the API has been finalized and should be available in the next VS Code stable version, I'll update the PR after the new version released. |
1a29ef0
to
d3412a4
Compare
PR updated... To navigate to the specific section in the refactoring document, you need to add a keyboard shortcut for a certain refactoring, for example:
|
Can you please fix the conflict? |
d3412a4
to
8a9c0e6
Compare
@fbricon Done. Travis linux image failed, seems caused by java 8 runtime. Should I fix it in this PR?
|
@jdneo please open a different PR |
Signed-off-by: Sheng Chen <[email protected]>
Signed-off-by: Sheng Chen <[email protected]>
8a9c0e6
to
1abc1f8
Compare
context.subscriptions.push(markdownPreviewProvider); | ||
context.subscriptions.push(languages.registerCodeActionsProvider({ scheme: 'file', language: 'java' }, new RefactorDocumentProvider(), RefactorDocumentProvider.metadata)); | ||
context.subscriptions.push(commands.registerCommand(Commands.LEARN_MORE_ABOUT_REFACTORING, async (kind: CodeActionKind) => { | ||
const sectionId: string = javaRefactorKinds.get(kind) || ''; | ||
markdownPreviewProvider.show(context.asAbsolutePath(path.join('document', `${Commands.LEARN_MORE_ABOUT_REFACTORING}.md`)), 'Java Refactoring', sectionId, context); | ||
})); |
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.
Can we check the new API is available (think Theia/Che) before registering the new provider?
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.
Is there any guidance that we could follow to do such checks?
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 assume something along the lines of if (CodeActionProviderMetadata !== null)
. But I'm no typescript expert
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.
CodeActionProviderMetadata
is only used as a prarm when register the code action provider, so probably it's not able to be used for API checking. Using env.appName.includes('VS Code')
?
// BTW, will theia/che honor the engines
field in package.json
?
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.
env.appName.includes('VS Code') is bad, since we'd need to rerelease vscode-java once Theia/Che support the feature.
// BTW, will theia/che honor the engines field in package.json?
That's a question for @benoitf
I don't understand why we can't do a check on CodeActionProviderMetadata (&& CodeActionProviderMetadata.documentation) like we do for https://github.com/redhat-developer/vscode-java/blob/master/src/semanticTokenProvider.ts#L8
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.
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.
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.
here is a link to try this vsix with che online: http://che.openshift.io/f/?url=https://gist.githubusercontent.com/benoitf/dcc2fa363b37161f5c8105a8e439a389/raw/55c6c7b117614c45d12a576d8159437c3afd923e/devfile.yaml
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.
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 the cool feature @jdneo! |
#1335
Note that the two css files
highlight.css
&markdown.css
are all copied from https://github.com/microsoft/vscode/tree/master/extensions/markdown-language-features/media