-
Notifications
You must be signed in to change notification settings - Fork 80
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
Autocompletion and Syntax Highlight. #2171
Autocompletion and Syntax Highlight. #2171
Conversation
Thx for your time. Now worries about typescript :) I could review it only on tomorrow. |
No need to hurry! |
Also, this should be the list of built-in themes (may be strongly out of date), although I have no idea on how to activate them except for the 15-minute rebuild. (That can be done on line 36 of
(The above one is custom-coded.) |
Now, I do. :) @ert78gb Please send me the full list of alternative themes and instructions for activating them. (If this issue is relatively time-consuming, the module configuration UI issue takes precedence over this one.) |
@mondalaci we used the I think have to extend the |
@ert78gb Great idea! If there are multiple light/dark Monaco editor theme options, I'd like to try them. |
@kareltucek I started to review PR, but I haven't spent too much time on it, because when I start running the browser version of the Agent then it did not start because the I did a quick insight into the |
You can try the built in themes on the monaco editor playground set the |
Ah, I see. Fixed.
Yes. Pulling reference-manual.md should be solved analogous to the way the Agent and firmware handle the smart macro docs. (That's what the todo is about anyways.) I didn't realize that we need to extend the builds for that too though. |
Actually, only 'vs' and 'vs-dark' work :-(. |
Hmmm, in a new (non-saved) macro action, completions are listed in many duplicities. In a saved action, everything works right. This could be caused by duplicit registration of the completion provider, except that I make sure I register the provider only once (via a static variable). Any ideas? |
Do you mean in Agent? Because in Agent maybe yes I have to investigate, but the playground supports all.
I am investigating it |
Hm, I believe I tried them in the playground. |
@ert78gb Don't want to press you, but could you give me some pointers on how to:
(I already added the reference-manual.md to the firmware build.) |
@kareltucek No worries and sorry I don't have enough time to work on it.
The smart macro document contains the link to the user guide. https://github.com/UltimateHackingKeyboard/firmware/blob/f794a563b2053144c2305c84fa5d3bd70c026f69/doc/dist/index.html#L595C48-L595C80 What is the goal of this md files? Would the same linking enough? |
Ah, The autocompletion parser uses the grammar listed in the reference-manual.md, so I just need to get the file (as a string) and hand it over to the parser. Most handling (like retrieving the grammar from the file) is hidden in the naive-autocompletion-parser package. Now I am doing it here which leads into the parser code. I guess we consider that to be just a temporary hack and want to replace it with whatever system you are using for the smart macro pane. (Also, current code does not respect current firmware tag. Which is probably not a big deal at the moment, but given that we have the system in place, it would be nice to be consistent just on general principles.) |
The smart macro doc servered via a webserver. The web server has been started when Agents start on a random port. Angular mainly follow OOP, so you have to inject everything. The I could give more guidance if you need. Or I could finish this PR on this week. I wanted to review it on Sunday but I had a unexpected program. |
More guidance would be appreciated. (Maybe just a few links to where these things are defined and used?) (I find it desirable to learn the ropes when it comes to both Agent codebase in specific, and typescript ways of doing things in general.) (I am still utterly lost, so somewhere between "give me links to all relevant code sites" and "complete coding-monkey instructions" is the right granularity.) |
I am reviewing this PR on the weekend (Saturday is my current plan). I am trying to write really detailed review that maybe helps to understand Agent. The key framework here is Angular and not really the typescript. Agent is a bit different from a simple Angular single page application because it running in electron and browser too. |
I would love to. I will be available in the evenings (sat, sun), if that works for you. Otherwise, I am afraid that written review will have to do. |
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.
To be continue tomorrow. Until I submit my recommendations
packages/uhk-web/src/app/components/macro/action-editor/tab/command/completion-provider.ts
Outdated
Show resolved
Hide resolved
packages/uhk-web/src/app/components/macro/action-editor/tab/command/completion-provider.ts
Outdated
Show resolved
Hide resolved
packages/uhk-web/src/app/components/macro/action-editor/tab/command/highlight-provider.ts
Outdated
Show resolved
Hide resolved
packages/uhk-web/src/app/components/macro/action-editor/tab/command/highlight-provider.ts
Outdated
Show resolved
Hide resolved
packages/uhk-web/src/app/components/macro/action-editor/tab/command/completion-provider.ts
Outdated
Show resolved
Hide resolved
monaco.languages.register({ id: languageId }); | ||
|
||
// Register the custom completion provider | ||
const completionProvider = new CustomCompletionProvider(this.logService); |
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.
We can use Angular built-in dependency injection. https://angular.io/guide/architecture-services
private setLanguageProperties(editor: MonacoStandaloneCodeEditor) { | ||
const languageId = 'uhkScript'; | ||
|
||
if (!MacroCommandEditorComponent.completionRegistered) { |
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.
This static variable is not really good for manage new language service registration, because if someone upgrade the firmware we have to apply the new language service.
We have to registerCompletionItemProvider
and setMonarchTokensProvider
.
We could register these new providers as we register the new themes in the MonacoEditorCustomThemeService
. The difference is we have to re-register the providers every time when the configurations changed.
We can create a new MonacoEditorUhkLanguageService
this service has to subscribe the monacoLoaderService.isMonacoLoaded$
and when the new getLanguageServiceConfig
. To combine 2 observers we could use combineLatest function.
How to create the getLanguageServiceConfig
observerable?
We can extend the packages/uhk-web/src/app/store/reducers/smart-macro-doc.reducer.ts
or create a new reducer. to be continue tomorrow
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.
The Agent has 2 main "process" the electron thread and the browser thread. The 2 world communicate with each other via electron built in inter-process communication
The packages/uhk-common/src/util/ipcEvents.ts
file contains all of the ipc events that Agent uses. I tried to organise them. When the main module reply to an event in async mode I used the Reply
suffix.
Every time when the user opens the smart macro document panel.
- The renderer process dispatches the
SmartMacroDoc.downloadDocumentation
event. - The main process checks the documentation for the firmware is available or not. if not then download it from the server.
This moment is too late to load the auto-complete md file, because maybe the user start using the smart macro editor before opens the smart macro doc panel.
I think everytime when Agents starts or updates the firmware then have to check the smart macro document is available or not. If it not available, then have to download it and after provide the auto complete markdown file to the renderer process.
If we use the web version of the Agent then when Agents start we have to download the auto complete markdown file from the Agent repo.
To achieve this functionality we have to refactor many small things. I don't know should I able to write every step in high level. My assumption is to provide a really step-by-step guide I have to implement the whole code in my mind. I am afraid if I forget something maybe I create a bigger problem for you than guidance.
I think faster if I finish the PR. Unfortunately, my availability will very limited in the next 3 weeks.
I am happy to help you to learn Agent coding, but I think we have to start with a smaller issue if you don't mind.
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 think faster if I finish the PR.
Allright, it is yours then.
Unfortunately, my availability will very limited in the next 3 weeks.
No worries ;)
Could this get a bit higher priority now? If no, then I would vote for merging this as it is now, as I believe that even this version brings a lot of value. Respecting current firmware version is definitely nice to have as well as the clean way of doing things, but at the end of the day, it is a rather minor improvement. |
I think there are 2 issue before it:
I hope both will solve on the next week and I can work on it |
Ok sure! |
superseded by #2270 |
Here is some proof of concept of grammar-based autocompletion for Agent.
@ert78gb could you please look at the TODO and either implement it, or advice me on how to do that?
Also, if you have any nitpicks about my code, they are welcome. I am completely unacquainted with typescript customs 🙃.