feat(spx-gui): remake monaco editor completion menu. #687

molinla · 2024-07-30T09:54:40Z

fix content

fix duplicate completionItems from completionProvider when HMR triggered and not dispose all previous providers in development mode.

new features

new component CompletionMenu.vue and related types.
copy some code from vs code repo to safitisfy future completionProvider interface and CompletionMenu.vue component
almost original completion funciton and UI from monaco editor has been replaced by CompletionMenu.vue
some insert text in completion item may be visible in editor inline when completion item is actived
change completion menu UI position properly when showing mulit lines

fix: fix duplicate completionItems from completionProvider when HMR triggered and not dispose all previous providers in development mode.

qiniu-prow · 2024-07-30T09:54:58Z

Hi @molinla. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…automatically change completion menu position when preview multiple line codes. add JetBrains font for better user view.

…on_menu

qiniu-x · 2024-08-01T06:37:49Z

[Git-flow] Hi @molinla, There are some suggestions for your information:

Rebase suggestions

Following commits seems generated via git merge

Merge remote-tracking branch 'lijiabao/completion_menu' into completion_menu

Which seems insignificant, recommend to use git rebase command to reorganize your PR.

For other git-flow instructions, recommend refer to these examples.

If you have any questions about this comment, feel free to raise an issue here:

https://github.com/qiniu/reviewbot

…unction throw undefined error for select function's this context has accidentally changed by outer caller.

… self font size and line height to satisfy the editor font size changed.

…ant tag.

…to completion_menu # Conflicts: # spx-gui/src/components/editor/code-editor/ui/code-text-editor/CodeTextEditor.vue # spx-gui/src/components/editor/code-editor/ui/code-text-editor/CompletionMenu.vue # spx-gui/src/components/editor/code-editor/ui/code-text-editor/providers/CompletionMenuProvider.ts # spx-gui/src/components/editor/code-editor/ui/code-text-editor/tools/completion.ts # spx-gui/src/components/editor/code-editor/ui/code-text-editor/tools/monaco-editor-core.ts # spx-gui/src/components/editor/code-editor/ui/icons/ai-helper.svg

…terface design

nighca · 2024-08-12T06:26:29Z

spx-gui/src/components/editor/code-editor/ui/EditorMenu.vue

@@ -0,0 +1,108 @@
+<script setup lang="ts">
+import { NVirtualList, type VirtualListInst } from 'naive-ui'


这才多少项啊就需要 virtual list 了吗？

如果没有确实遇到性能问题的话，引入 virtual list 带来的只是复杂度

这里一开始确实没有使用 virtual list, 是因为之前有出现重复补全项 BUG 的时候发现如果使用传统 HTML 列表显示确实会在补全项目较多的感觉卡一下，同时也参考了 monaco 的实现，最后使用的是 virtual list, 使用 virtual list 也能直接在后面能够引入其它库增加补全项的情况下保持切换流畅，如果后续打算替换 Navie UI 的可以手动封装一个 virtual list，也可以保持简单直接一次性渲染全部展示

之前有出现重复补全项 BUG 的时候发现如果使用传统 HTML 列表显示确实会在补全项目较多的感觉卡一下

如果是 bug 导致补全项太多带来性能问题，那听上去不需要做什么处理，把 bug 修复就好了

在正常的使用场景中确实遇到性能问题的话，再去考虑 virtual list；几乎没有一个 virtual list 的实现是不要求业务代码做妥协的，引入这个机制本身就会引入成本，做得越早，成本越高，你一定听说过“过早优化是万恶之源”..

明白了，我会去掉这个 virtual list

nighca · 2024-08-12T06:38:18Z

spx-gui/src/components/editor/code-editor/ui/features/completion-menu/completion-menu.ts

+  $container?: HTMLElement
+}
+
+export class CompletionMenu implements IDisposable {


这个 class 的实现风格有点奇怪，是因为大部分代码是从 monaco 拷的吗？

这里的 ....implements IDisposable 确实是参考 monaco 的类，我发现项目里面实现了 Disposable 类是可以直接监听dispose 事件，主要是想跟 monaco 对齐所以就直接实现的 dispose 接口。

$container 用的命名规则可能比较老了，我将会改用更直观的命名，这里用 $ 前缀就表示这个是原生的元素能够直接操作 DOM，代码里面涉及到需要计算补全菜单弹出位置的代码所以需要直接计算原生元素的坐标。

除了你提到的，还有比如

_onHide vs onHide 这种设计

resolveSuggestMatches2Highlight 通过 jsdoc 来描述类型信息

一个看上去没用的 providerId 为什么会存在

等等

主要是想跟 monaco 对齐所以就直接实现的 dispose 接口。

“对齐”有什么好处吗？

这里用 $ 前缀就表示这个是原生的元素能够直接操作 DOM

我们用 TS，根据类型就能知道某个变量/字段对应的值是否 DOM 节点，并不容易犯错

如果这些“奇怪的风格”是从 monaco 拷的代码带过来的，那问题不大；如果是你自己写代码有这些习惯，我就可能要挨个跟你对一下，看是否必要了，否则回头也会带到其他代码中的

平时的风格都是从各种 npm 库里面看的，这里的：

_onHide vs onHide 是从monaco 参考的

jsdoc 是看一些库会使用这个风格，我想着标准一点，就用了这种 jsdoc

providerId 这里的作用是后面在代码里面统一显示文字如：行内形参、行内高亮文字、代码预览做统一管理，所以需要每个类保留一个ID，防止某个类的全部行内文字刷新也把其它类的行文字也刷新掉，这里是疏忽了名字命名，我会改为更贴切的名字

jsdoc 是看一些库会使用这个风格，我想着标准一点，就用了这种 jsdoc

关于 jsdoc，参考 #131 (comment)

providerId 这里的作用是后面在代码里面统一显示文字如：行内形参、行内高亮文字、代码预览做统一管理，所以需要每个类保留一个ID，防止某个类的全部行内文字刷新也把其它类的行文字也刷新掉，这里是疏忽了名字命名，我会改为更贴切的名字

这样的逻辑依赖一个字符串的 providerID 来做标识好像不太可靠啊

好的，我会更改注释的格式。如果认为不太可靠的话，我会把 textDeciration 这样的类公开一个专门创建 ID 的函数，通过这样的方式应该会可靠很多

如果认为不太可靠的话，我会把 textDeciration 这样的类公开一个专门创建 ID 的函数，通过这样的方式应该会可靠很多

“行内文字刷新”的细节我还没不太了解，可以等做到那里的时候我们再去对具体的方案

nighca · 2024-08-12T06:42:06Z

spx-gui/src/components/editor/code-editor/ui/features/completion-menu/completion-menu.ts

+  private monacoCompletionModelItems: MonacoCompletionModelItem[] = []
+  private completionMenuItemPreviewDecorationsCollection: IEditor.IEditorDecorationsCollection
+
+  constructor(editor: IEditor.IStandaloneCodeEditor) {


这里似乎还没有跟外部传入的 CompletionProvider 联系起来；这个 PR 只是把 monaco 默认的 completion menu 渲染逻辑给替换了？

是的这里是先把默认的渲染逻辑给替换了，后续使用将会对接外部传入的 Provider。想尽量减少每次 PR 的代码量，审查起来轻松点，这次的稍微有点大了，后续都会少代码多 PR

好，不过后边还是需要注意，一般来说，我们“先接起来，再实现细节”；而不是“先实现细节，再去做对接”；因为后者更容易造成返工

…nt for better type intelligence, update standard function jsdoc to satisfy project standard

nighca

没什么大的问题，先 merge；一些细节的疑问可以确认后在后续的 PR 中处理

nighca · 2024-08-13T07:36:08Z

spx-gui/src/components/editor/code-editor/ui/features/completion-menu/completion.ts

-import type { FuzzyScore, IMatch } from '../common/monaco-editor-core'
-import type { Icon } from '@/components/editor/code-editor/common'
+import type { FuzzyScore, IMatch } from '../../common/monaco-editor-core'
+import type { Icon } from 'src/components/editor/code-editor/ui/common'


@ 改成 src 应该不是有意的？

这里是更改了文件 webstorm 会自动更新引用路径，有时候会使用 src/.. 有时候会使用 @/..，后续我再重新检查这些路径引用

nighca · 2024-08-13T07:36:48Z

...ui/src/components/editor/code-editor/ui/features/completion-menu/CompletionMenuComponent.vue

-import { computed, ref, watch, watchEffect } from 'vue'
-import { type VirtualListInst, NVirtualList } from 'naive-ui'
+import { computed, ref, watchEffect } from 'vue'
+import { NScrollbar } from 'naive-ui'


引入 NScrollbar 是什么目的？

原生的滚动条不够好看，这里引用它的滚动条好看一点，也可以使用原生的 css 修改滚动条，但是效果会生硬一些

这里的 NScrollbar 是因为要使用它的 scrollTo 方法，为了 TS 的类型补全所以引用了这个

原生的滚动条不够好看，这里引用它的滚动条好看一点

如果是外观的目的，是需要比较慎重地做决策的；因为局部引入 NScrollbar 往往会导致局部的视觉风格跟整体不一致；如果二者只能取其一，一致很多时候要比“好看”重要一点

这里的 NScrollbar 是因为要使用它的 scrollTo 方法，为了 TS 的类型补全所以引用了这个

如果是为了控制滚动行为，不引入 NScrollbar 直接通过 DOM 的方法也可以达到目的的，而且不会更麻烦

“引入第三方依赖”本身也是一个需要慎重做的决策，因为它常常在当下看上去“没有代价”，但代价会潜在地在将来发生。越是低质量的第三方库，给将来带来的代价越大，而大部分（甚至很流行的）第三方库的实现质量是低的。naive-ui 也不是一个质量很高的库，它有很多设计是欠考虑的，如果不是因为第一版的代码依赖了它，我们是不会选择它的。你会看到代码中有一个 @/components/ui/ 的目录，这个目录的存在一方面是以代码的形式定义我们的设计规范，另外一方面也是避免我们的业务代码直接依赖 naive-ui 的 API，从而我们可以逐步地干掉对 naive-ui 的依赖，而不影响业务代码（只要对 @/components/ui/ 中的内容依次替换实现即可）

而至于 NScrollbar，naive-ui 的作者自己也很清楚它不太可靠，参考 Scrollbar 的文档：

It looks better but I'm sure it's not as reliable as native scrollbar.

关于“代价会潜在地在将来发生”，举一个具体的例子：naive-ui 的 popover 实现的时候依赖了同一个作者的另外一个库 vueuc，我们之前遇到 popover 的一个问题：不可见浮层中的 svg 仍然会遮挡点击事件，导致界面上普通的元素部分不可点击，详情可以看这里。排查这个问题本身就会花不少时间，因为这个问题很不常规。如果引入某个第三方依赖在当下带来的工作量的节省，不比在将来带来的排查 & 修复问题的工作量更多，那么引入这个第三方依赖就是不划算的，而后者往往很难在当下评估，所以建议总是谨慎一点，确实必要才去引入依赖。

明白了，后面针对 UI 部分我会保持统一性，也会尽量减少对第三方 UI 库的使用，我看了下项目目前使用的是原生滚动条，这里我将会改为原生滚动条，后面针对 markdown 渲染和代码高亮引入了2个常用的库，markdownit, highlightjs 从项目 Star 上是 OK的，在代码高亮发现一个新库是 vitepress 中使用的代码高亮库 shiki 粗略浏览自由度和功能会比 highlightjs 高很多，但是目前的 Star 数会少很多

nighca · 2024-08-13T07:37:18Z

spx-gui/src/components/editor/code-editor/ui/common/index.ts

+export type Icon = IconEnum
+
+/** transform icon enum to raw svg html content */
+export function Icon2SVG(icon: Icon): string {


一般函数/方法的命名用小驼峰

好的，这里确实疏忽了

feat: remake monaco editor completion menu.

d4189f6

fix: fix duplicate completionItems from completionProvider when HMR triggered and not dispose all previous providers in development mode.

vercel bot deployed to Preview July 30, 2024 09:56 View deployment

molinla marked this pull request as draft July 31, 2024 02:51

molinla changed the title ~~feat(spx-gui): remake monaco editor completion menu.~~ [WIP]feat(spx-gui): remake monaco editor completion menu. Jul 31, 2024

molinla changed the title ~~[WIP]feat(spx-gui): remake monaco editor completion menu.~~ [WIP] feat(spx-gui): remake monaco editor completion menu. Jul 31, 2024

feat: code preview in the editor when completion menu has activated. …

4363890

…automatically change completion menu position when preview multiple line codes. add JetBrains font for better user view.

vercel bot deployed to Preview August 1, 2024 04:54 View deployment

molinla changed the title ~~[WIP] feat(spx-gui): remake monaco editor completion menu.~~ feat(spx-gui): remake monaco editor completion menu. Aug 1, 2024

molinla marked this pull request as ready for review August 1, 2024 04:55

molinla added 3 commits August 1, 2024 14:35

chore: remove editor font updates to new pr.

8d200a2

chore: remove editor font updates to new pr(goplus#693).

0bc228d

Merge remote-tracking branch 'lijiabao/completion_menu' into completi…

aa06215

…on_menu

vercel bot deployed to Preview August 1, 2024 06:37 View deployment

vercel bot deployed to Preview August 1, 2024 06:39 View deployment

fix: when mouse click completion item CompletionMenuProvider select f…

7f76966

…unction throw undefined error for select function's this context has accidentally changed by outer caller.

vercel bot deployed to Preview August 2, 2024 06:55 View deployment

feat: now completion menu and inline code preview will dynamic update…

db20c6b

… self font size and line height to satisfy the editor font size changed.

vercel bot deployed to Preview August 2, 2024 08:08 View deployment

chore: change CompletionMenu.vue css to remove unnecessary css import…

c6eab68

…ant tag.

vercel bot deployed to Preview August 2, 2024 08:21 View deployment

docs: update some comment to avoid misunderstanding

e313d41

vercel bot deployed to Preview August 2, 2024 08:34 View deployment

molinla marked this pull request as draft August 7, 2024 09:42

molinla changed the title ~~feat(spx-gui): remake monaco editor completion menu.~~ [WIP] feat(spx-gui): remake monaco editor completion menu. Aug 7, 2024

molinla added 2 commits August 9, 2024 10:50

chore: refactor completionMenu features to satisfy new UI module in…

f852919

…terface design

molinla changed the title ~~[WIP] feat(spx-gui): remake monaco editor completion menu.~~ feat(spx-gui): remake monaco editor completion menu. Aug 9, 2024

molinla marked this pull request as ready for review August 9, 2024 09:41

vercel bot deployed to Preview August 9, 2024 09:43 View deployment

nighca reviewed Aug 12, 2024

View reviewed changes

feat: remove virtual list, update EditorMenu.vue to generic compone…

532b1c2

…nt for better type intelligence, update standard function jsdoc to satisfy project standard

vercel bot deployed to Preview August 13, 2024 07:27 View deployment

nighca approved these changes Aug 13, 2024

View reviewed changes

nighca merged commit 262d762 into goplus:trailhead_code Aug 13, 2024
3 checks passed

molinla deleted the completion_menu branch August 13, 2024 07:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(spx-gui): remake monaco editor completion menu. #687

feat(spx-gui): remake monaco editor completion menu. #687

molinla commented Jul 30, 2024 •

edited

Loading

qiniu-prow bot commented Jul 30, 2024

qiniu-x bot commented Aug 1, 2024

nighca Aug 12, 2024

molinla Aug 13, 2024

nighca Aug 13, 2024

molinla Aug 13, 2024

nighca Aug 12, 2024

molinla Aug 13, 2024

nighca Aug 13, 2024

molinla Aug 13, 2024

nighca Aug 13, 2024 •

edited

Loading

molinla Aug 13, 2024

nighca Aug 13, 2024 •

edited

Loading

molinla Aug 13, 2024

nighca Aug 12, 2024

molinla Aug 13, 2024

nighca Aug 13, 2024

nighca left a comment

nighca Aug 13, 2024

molinla Aug 13, 2024

nighca Aug 13, 2024

molinla Aug 13, 2024

molinla Aug 13, 2024

nighca Aug 13, 2024

molinla Aug 13, 2024 •

edited

Loading

nighca Aug 13, 2024

molinla Aug 13, 2024

		@@ -0,0 +1,108 @@
		<script setup lang="ts">
		import { NVirtualList, type VirtualListInst } from 'naive-ui'

feat(spx-gui): remake monaco editor completion menu. #687

feat(spx-gui): remake monaco editor completion menu. #687

Conversation

molinla commented Jul 30, 2024 • edited Loading

fix content

new features

qiniu-prow bot commented Jul 30, 2024

qiniu-x bot commented Aug 1, 2024

Rebase suggestions

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

nighca Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nighca Aug 13, 2024 • edited Loading

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

nighca left a 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

molinla Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molinla commented Jul 30, 2024 •

edited

Loading

nighca Aug 13, 2024 •

edited

Loading

nighca Aug 13, 2024 •

edited

Loading

molinla Aug 13, 2024 •

edited

Loading