Feat: implement webview to the api docs as go to definition #496

uxkjaer · 2022-09-12T10:49:24Z

lgtm-com · 2022-09-12T10:52:10Z

This pull request introduces 3 alerts when merging aaf056e into 9d0c89c - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

bd82 · 2022-09-29T13:15:24Z

Hi @uxkjaer

I am a bit conflicted about this feature:

Pros:

it is quite cool
and may be useful.

Cons:

It is not quite the regular "go to definition" behavior I am used to as it does not navigate to source code.
It feels more like it is "go to docs" which is already implemented when you hover and then click on "more-information".
So we have a bit of duplicate feature, (go-to-docs) which is implemented in two different ways (browser redirect vs webview inside vscode).

@petermuessig WDYT?

I think I should also consult with the product person responsible for this.

uxkjaer · 2022-09-29T20:12:26Z

My take on it.

youis often needs to read the api and check something.
The hover scroll down and click isn't really useful for me.
we currently don't have any other useful use case for the go to definition in an XML view
further possibilities are:
to pick up where there cursor is placed ie event or aggregation and handle deep navigation
implement a go to samples option as well. This could be maybe implementations.

I know we aren't navigating source code, but we don't have any other useful use case for these options. We could simply add our own as well and a different short cut. My idea was that you were already used to the f12 shortcut to go and read a definition.

bd82 · 2022-09-29T20:56:16Z

Good points I will setup a meeting with you and the relevant product person 👍

marianfoo · 2022-09-30T05:45:04Z

Hi Jacob,
I think this is a good solution.
There is already a similar plugin from Jacek:
https://marketplace.visualstudio.com/items?itemName=jacek-wozniczak.vscode-ui5-api-reference
I tried this plugin and personally I prefer to have a separate browser window.
But that could also have to do with the fact that it was formatted differently here.

But you are right, I didn't know the function of "more information".

I think the use case of Jakob is a good one and can help nicely, even if I probably won't use it.
But maybe it is well implemented and I will use it :)

vobu · 2022-09-30T07:29:15Z

just also wanted to bring up @wozjac's extension.
and there is also a .docset from @monavari-lebrecht at http://monavari.de/docset/ .
sooo...in best Ghostbuster's style 👻, why not combine the streams and ...

incorporate @wozjac's extension here, into the language assistant itself (and award him eternal fame)
provide an option to export to .docset, taking up @monavari-lebrecht's work
have @uxkjaer's API ref in the webview as a "read more"-link of sorts, being available from the incorporated @wozjac's extension

uxkjaer · 2022-10-01T09:57:02Z

We could also consider just using a custom menu option and command instead, if that makes more sense?

bd82 · 2022-10-01T23:21:17Z

We could also consider just using a custom menu option and command instead, if that makes more sense?

Perhaps:

Would it ever make sense to ever implement a go-to-definition that would open the source code of UI5?
So if we keep it as currently implemented would it conflict with such a future feature?

I am generally in favor of this (or similar) feature, But it a major feature and I'd like to demonstrate it to the
relevant product person, but with the holidays season in Israel now, this will add some delay...

uxkjaer · 2022-10-02T02:50:11Z

We have types for openui5 and Sapui5 so maybe navigate to those instead?

Happy to just add this as another menu option instead. Mainly I'm just keen to get a possibility not to leave vscode when needing to search the api.
We could also have another one for going to samples.

Happy to discuss this with relevant people.

uxkjaer · 2022-10-12T13:41:55Z

I've changed the shortcut and context menu to UI5-Language-Assistant: View API Reference. The go to definition no longer shows the api reference. I've also added a setting with options to either show API reference inside vscode or in browser.

lgtm-com · 2022-10-12T13:46:22Z

This pull request introduces 1 alert when merging 37f9155 into 90807cd - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

uxkjaer · 2022-10-19T12:37:08Z

closes #495

bd82 · 2022-10-27T16:24:07Z

packages/language-server/api.d.ts

@@ -25,6 +25,13 @@ export type ServerInitializationOptions = {
  logLevel?: LogLevel;
 };

+export function getNodeName(


why does the language server exposes a logical utility function?
It generally runs in a different process and communicates via JSON_RPC

Generally the language server should use logical utilities from other packages.
Maybe getNodeName should be extracted out of the language server sub-package.

bd82 · 2022-10-27T16:24:54Z

packages/language-server/api.d.ts

@@ -25,6 +25,13 @@ export type ServerInitializationOptions = {
  logLevel?: LogLevel;
 };

+export function getNodeName(
+  text: string,


with so many arguments, some of which are optional you should probably use a config object in this case.

bd82 · 2022-10-27T16:32:08Z

packages/language-server/src/api.ts

+    framework,
+    ui5Version
+  );
+  return ui5Node


I think too much is done here in a single expression.
Can you split this up into multiple statements for readability?

bd82 · 2022-10-27T16:37:05Z

packages/language-server/src/documentation.ts

@@ -108,3 +113,30 @@ export function getNodeDetail(node: BaseUI5Node): string {
      return node.name;
  }
 }
+
+export async function getUI5NodeName(


I am not sure this function belongs in this sub-package

bd82 · 2022-10-27T16:39:25Z

packages/language-server/src/documentation.ts

+  cachePath?: string,
+  framework?: string,
+  ui5Version?: string
+): Promise<BaseUI5Node | undefined> {


with so many params and optional params a config object may be warranted

bd82 · 2022-10-27T16:41:31Z

packages/language-server/src/documentation.ts

+}
+
+async function fetchModel(cachePath, framework, ui5Version) {
+  const ui5Model = await getSemanticModel(cachePath, framework, ui5Version);


I do not recall:
Does every functional flow invoke getSemanticModel ?
Or does it happen once on language server startup?

Is this different or the same as other functional flows?

bd82 · 2022-10-27T16:42:22Z

packages/language-server/src/documentation.ts

+  ui5Version?: string
+): Promise<BaseUI5Node | undefined> {
+  const documentText = text;
+  const { cst, tokenVector } = parse(documentText);


Again I do not recall.
Does every flow functional flow re-parses the text each time?
Or is there a cache of documents / CST?

bd82 · 2022-10-27T16:43:59Z

packages/language-server/src/hover.ts

 import { track } from "./swa";

-export function getHoverResponse(
+export async function getHoverResponse(


could changing this to async break anything?

bd82 · 2022-10-27T16:46:27Z

packages/language-server/src/server.ts

@@ -128,6 +130,7 @@ connection.onCompletion(
        minUI5Version
      );
      connection.sendNotification("UI5LanguageAssistant/ui5Model", {
+        cachePath: initializationOptions?.modelCachePath,


why does this feature require adding the cache path to the notifications?
Perhaps I am rusty in regards to this code base, but why was it not needed for other features, but needed here?

bd82 · 2022-10-27T16:47:34Z

yarn.lock

@@ -8841,11 +8841,6 @@ tr46@^1.0.1:
  dependencies:


why did the lock file change if no changes were made to the package.json?

bd82 · 2022-10-27T16:49:24Z

packages/settings/api.d.ts

+export type Settings = CodeAssistSettings &
+  TraceSettings &
+  LoggingSettings &
+  API_Reference;


The naming of API_Reference type does not seem consistent
with the existing pattern in this code base / file

bd82 · 2022-10-27T16:51:35Z

packages/settings/api.d.ts

+  browser: true;
+}
+
+export const validViewApiReferenceValues: IValidApiReferenceValues;


remove view from the name to have consistent type and const names?

bd82 · 2022-10-27T17:29:11Z

packages/vscode-ui5-language-assistant/README.md

@@ -124,6 +124,16 @@ The feature is available in the following:

 ![](https://raw.githubusercontent.com/SAP/ui5-language-assistant/master/packages/vscode-ui5-language-assistant/resources/readme/preview-manifest-json.gif)

+### Quick navigation to API Reference
+
+Right click a tag in the XML file and use the View API reference shortcut to navigate directly to the API reference. Use the setting


Creating a demo GIF for this feature may be a good idea to "publicize" it.

bd82 · 2022-10-27T17:31:14Z

packages/vscode-ui5-language-assistant/README.md

+"UI5LanguageAssistant.view.API_Reference": "browser" | "editor"
+```
+
+The default setting is editor which opens the API reference in a new editor to the side.


clearer explanation needed.

editor settings is described by the same word "editor"

"browser" flow is not described.

bd82 · 2022-10-27T17:32:36Z

packages/vscode-ui5-language-assistant/README.md

@@ -124,6 +124,16 @@ The feature is available in the following:

 ![](https://raw.githubusercontent.com/SAP/ui5-language-assistant/master/packages/vscode-ui5-language-assistant/resources/readme/preview-manifest-json.gif)

+### Quick navigation to API Reference
+
+Right click a tag in the XML file and use the View API reference shortcut to navigate directly to the API reference. Use the setting


View API reference --> View API Reference

bd82 · 2022-10-27T17:35:11Z

packages/vscode-ui5-language-assistant/package.json

@@ -20,6 +20,29 @@
    "*"
  ],
  "contributes": {
+    "commands": [
+      {
+        "command": "UI5LanguageAssistant.command.webView",


is webView the correct term for the command?
Or is this an implementation detail of one of the two possible implementation?

bd82 · 2022-10-27T17:38:07Z

Does the ui5 logo belong in this PR?

bd82 · 2022-10-27T17:49:23Z

packages/language-server/test/documentation-spec.ts

  });
+
+  function getXmlSnippet(


are these two utility test functions copy / pasted from another sub-package?

bd82 · 2022-10-27T17:50:23Z

packages/language-server/test/documentation-spec.ts

+        "SAPUI5",
+        ui5Model.version
+      );
+      if (result) expect(getNodeDetail(result)).to.equal("sap.m.Input");


why is the test none deterministic ?
why is an if / else needed?

bd82 · 2022-10-27T17:50:29Z

packages/language-server/test/documentation-spec.ts

+        document.getText(),
+        ui5SemanticModel
+      );
+      if (result) {


why is the test none deterministic ?
why is an if / else needed?

bd82 · 2022-10-27T17:52:36Z

packages/language-server/test/documentation-spec.ts

+    });
+
+    it("get the UI5 node name without a model", async () => {
+      const xmlSnippet = `<mvc:View displayBlock="true"


these snippets normally need a "⇶" symbol to mark the cursor position.
Why is this not needed here?

packages/vscode-ui5-language-assistant/src/constants.ts

bd82 · 2022-10-27T17:56:48Z

packages/vscode-ui5-language-assistant/src/extension.ts

@@ -125,6 +150,101 @@ function updateCurrentModel(model: UI5Model | undefined) {
  }
 }

+async function showWebView() {


this logic should be in a separate file.
The "extension.ts" responsibility is for initializing the language server.
new concerns should be in a separate file.

bd82 · 2022-10-27T17:57:41Z

packages/vscode-ui5-language-assistant/src/extension.ts

+  }
+}
+
+function getWebviewContent(ui5Url: string) {


missing explicit return type

bd82 · 2022-10-27T18:01:52Z

packages/vscode-ui5-language-assistant/src/extension.ts

+      return undefined;
+    });
+  }
+  const ui5Url = `${new URL(apiRefUrl).href}${name ? "#/api/" + name : ""}`;


should not there be re-use in build the API ref link?
This is also done in the "more details" in hover feature...

bd82 · 2022-10-27T18:03:04Z

packages/vscode-ui5-language-assistant/src/extension.ts

@@ -125,6 +150,101 @@ function updateCurrentModel(model: UI5Model | undefined) {
  }
 }

+async function showWebView() {


try to refactor this 70 lines function by extracting logical sub-parts
so the high level logical flow would be easily visible

bd82 · 2022-10-27T18:05:42Z

I think we need to review this in a "pair programing" session.
I get the feeling this PR has broken some assumptions on the code structure in this mono-repo.
But I am a bit rusty in regards to this project...

cla-assistant · 2023-10-18T14:31:26Z

All committers have signed the CLA.

cla-assistant · 2023-10-18T14:31:36Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

cla-assistant · 2023-10-18T14:31:50Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

uxkjaer added 4 commits September 9, 2022 20:00

feat: add webview command

79fc650

feat: handle go to definition to create webview

e3d82a2

refactor: moved code to language server

d37f620

refactor: updated tests

aaf056e

refactor: updated tests, linting and logo

6837eaa

uxkjaer mentioned this pull request Sep 13, 2022

feat: added turborepo for speed #498

Closed

uxkjaer added 4 commits October 12, 2022 22:57

chore: merge master

af383c1

feat: added settings for open in browser

5e0c21e

feat: added context menu

ad36c12

fix: remove go to definition functionality

37f9155

uxkjaer added 5 commits October 13, 2022 07:21

refactor: changed wording from vscode to editor

b700af1

refactor: remove // @ts-ignore from local eslint

4b0a8bc

refactor: remove unused dependency

3179ebd

refactor: remove unused param

ce2251f

refactor: changed tests for new settings

d09feb1

uxkjaer requested a review from petermuessig October 13, 2022 04:04

uxkjaer linked an issue Oct 19, 2022 that may be closed by this pull request

Implement go to definition #495

Open

chore: updated readme with api reference settings

edd0ae7

bd82 reviewed Oct 27, 2022

View reviewed changes

yarn.lock

@@ -8841,11 +8841,6 @@ tr46@^1.0.1:

dependencies:

Copy link

Member

bd82 Oct 27, 2022

Choose a reason for hiding this comment

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

why did the lock file change if no changes were made to the package.json?

bd82 reviewed Oct 27, 2022

View reviewed changes

packages/vscode-ui5-language-assistant/src/constants.ts Show resolved Hide resolved

bd82 reviewed Oct 27, 2022

View reviewed changes

packages/vscode-ui5-language-assistant/src/extension.ts

}

}

function getWebviewContent(ui5Url: string) {

Copy link

Member

bd82 Oct 27, 2022

Choose a reason for hiding this comment

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

missing explicit return type

bd82 reviewed Oct 27, 2022

View reviewed changes

Feat: implement webview to the api docs as go to definition #496

Are you sure you want to change the base?

Feat: implement webview to the api docs as go to definition #496

Conversation

uxkjaer commented Sep 12, 2022

lgtm-com bot commented Sep 12, 2022

bd82 commented Sep 29, 2022

uxkjaer commented Sep 29, 2022

bd82 commented Sep 29, 2022

marianfoo commented Sep 30, 2022

vobu commented Sep 30, 2022

uxkjaer commented Oct 1, 2022

bd82 commented Oct 1, 2022

uxkjaer commented Oct 2, 2022

uxkjaer commented Oct 12, 2022

lgtm-com bot commented Oct 12, 2022

uxkjaer commented Oct 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bd82 Oct 27, 2022 • edited

Choose a reason for hiding this comment

bd82 commented Oct 27, 2022

bd82 Oct 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bd82 commented Oct 27, 2022

cla-assistant bot commented Oct 18, 2023 • edited

cla-assistant bot commented Oct 18, 2023

cla-assistant bot commented Oct 18, 2023

bd82 Oct 27, 2022 •

edited

bd82 Oct 27, 2022 •

edited

cla-assistant bot commented Oct 18, 2023 •

edited