Integrating the Modelica LSP in OMEdit#15925
Conversation
Adds a lightweight LSP client (stdin/stdout JSON-RPC) that connects to an external modelica-language-server process and wires hover, go-to-definition, and document symbol support into the Modelica text editor. Feature is disabled by default; enabled via Options > Language Server. Also adds AGENTS.md with OMEdit coding standards. Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a "Language Server Options" section to the omedit.rst user guide covering the Options page, omedit.ini keys, and installation steps. Adds OMEditLIB/LSP/README.md with feature overview and architecture notes. Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ships the pre-built modelica-language-server (server.js + WASM files) alongside OMEdit. On startup, LSPClient resolves the bundled server automatically before falling back to PATH search. When the server is a .js file and Node.js is missing, a setup dialog (LSPSetupDialog) guides the user through installation with platform-specific commands and a "Check Again" button. CMakeLists installs the bundle to share/omedit/languageserver/ (Linux/macOS) or bin/languageserver/ (Windows). Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The setup dialog now fires only when the user explicitly enables the language server in Options and Node.js is not found — not on every startup. On startup, a missing Node.js causes the LSP to be skipped silently (the user was already informed via the dialog at enable-time). Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Renamed local QJsonObject variable from 'initialized' to 'initializedNotif' to avoid shadowing the initialized() signal, which caused a compilation error. Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
32x32 SVG icon: bold Modelica M, bidirectional arrow, rack-server box with green status LEDs. Registered in the Qt resource file. Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace placeholder general.svg with the dedicated language-server.svg icon on the Language Server options page entry. Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Pass --stdio to the language server process (required by vscode-languageserver) - findBundledServer() now checks for modelica-language-server binary first (Node.js SEA standalone, no Node.js required for end users) and falls back to server.js only if no binary is found Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pport - Start/stop the language server live when toggled in Options, so a restart is no longer required. Factored the startup logic into MainWindow::startLanguageServer()/stopLanguageServer(). - ModelicaEditor connects to the server lazily (per client, via QPointer) and opens its document on first use, so editors already open when the server starts also get hover and navigation. - Remove LSP go-to-definition from Ctrl+Click; Ctrl+Click keeps native OMEdit class navigation. Expose LSP navigation through the context menu as "LSP: Go to Definition" and "LSP: Go to Declaration", shown only when the server is running. - Add textDocument/declaration support to LSPClient. Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…wser - LSPClient forwards window/logMessage and serverError as a logMessage signal. - MainWindow writes them to the Messages Browser, prefixed with "LSP", mapping the LSP message type to the OMEdit message level. Gated on a new setting. - Add a "Log language server messages to the Messages Browser" checkbox to the Language Server options page (languageServer/logging), toggleable at runtime. Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…paths - Parse LocationLink (targetUri/targetSelectionRange) in addition to plain Location in definition/declaration results. The server returns LocationLink, so navigation results were previously discarded. - Add a "Library Paths" setting (languageServer/libraries, semicolon-separated) passed to the server as initializationOptions.modelicaPath so it can resolve go-to-definition/declaration and richer hover across files. Default empty; loading large libraries such as the MSL costs a few seconds at startup. - Restart the running server from the Options dialog only when the executable or library set actually changed. Verified end-to-end against the bundled server: opening a real MSL file with the MSL configured resolves definition (LocationLink) and hover. Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…vigation Co-Authored-By: JKRT <jtinnerholm@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collapse the near-identical hover/definition/declaration/documentSymbol request functions into thin wrappers over a shared sendRequest helper. No behavior change; public API unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Putting @adeas31 and @AnHeuermann as a reviewers. We should iterate a bit on the design of this I think |
|
What is the difference between definition and declaration in Modelica? I think the LSP doesn't differentiate. |
Address review: Modelica/LSP does not meaningfully distinguish definition from declaration (both were routed to the same handler), so drop the redundant "Go to Declaration" menu item and the declaration request path. Mark the remaining "Go to Definition" entry with the language-server icon. Also add the LSP sources/headers to OMEditLIB.pro; they were only in the CMake build, so the qmake-based Jenkins build failed to link (undefined references to LSPClient::*). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Updated. New look on right click |
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughAdds a complete opt-in Language Server Protocol (LSP) client subsystem to OMEdit. This includes new ChangesLSP Client Integration
Sequence Diagram(s)sequenceDiagram
participant OMEditApplication
participant MainWindow
participant LSPClient
participant LanguageServer as modelica-language-server
OMEditApplication->>MainWindow: startLanguageServer()
MainWindow->>LSPClient: start(executable, rootUri, libraries)
LSPClient->>LanguageServer: launch process with --stdio
LSPClient->>LanguageServer: initialize request
LanguageServer-->>LSPClient: initialize response
LSPClient->>LanguageServer: initialized notification
note over MainWindow,LSPClient: Editor opens a file
MainWindow-->>LSPClient: openDocument(uri, text) via ModelicaEditor
LSPClient->>LanguageServer: textDocument/didOpen
note over MainWindow,LSPClient: User hovers over symbol
MainWindow-->>LSPClient: requestHover(uri, line, char) via ModelicaEditor
LSPClient->>LanguageServer: textDocument/hover
LanguageServer-->>LSPClient: hover result
LSPClient-->>MainWindow: hoverResult(id, markdown) → ModelicaEditor shows tooltip
note over MainWindow,LSPClient: User invokes Go to Definition
MainWindow-->>LSPClient: requestDefinition(uri, line, char)
LSPClient->>LanguageServer: textDocument/definition
LanguageServer-->>LSPClient: definition location
LSPClient-->>MainWindow: definitionResult(id, location) → ModelicaEditor navigates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
OMEdit/OMEditLIB/LSP/README.md (1)
40-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd Node.js minimum version requirement and improve Linux instructions.
The user guide specifies Node.js "version 16 or later," but the README omits this. Also,
sudo apt install nodejson Debian/Ubuntu typically installs an outdated version (10.x or 12.x in older distributions) that may not support the bundled server.Add the version requirement and consider noting that Ubuntu/Debian users may need to install from NodeSource or use
nvmif the distribution package is too old.OMEdit/OMEditLIB/Editors/ModelicaEditor.cpp (1)
235-245: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCoalesce full-text LSP updates before sending them.
contentsHasChanged()fires per edit, andLSPClient::changeDocument()pushes the entire buffer each time. On larger models this turns typing into full-document JSON-RPC writes on every keystroke. A short single-shot timer aroundnotifyLanguageServerContentChanged()would keep the editor responsive without changing behavior.Also applies to: 854-875
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 612579c6-2e67-4a6c-87a3-54696d882a88
⛔ Files ignored due to path filters (1)
OMEdit/OMEditLIB/Resources/icons/language-server.svgis excluded by!**/*.svg
📒 Files selected for processing (17)
OMEdit/OMEditLIB/CMakeLists.txtOMEdit/OMEditLIB/Editors/ModelicaEditor.cppOMEdit/OMEditLIB/Editors/ModelicaEditor.hOMEdit/OMEditLIB/LSP/LSPClient.cppOMEdit/OMEditLIB/LSP/LSPClient.hOMEdit/OMEditLIB/LSP/LSPProtocol.hOMEdit/OMEditLIB/LSP/LSPSetupDialog.cppOMEdit/OMEditLIB/LSP/LSPSetupDialog.hOMEdit/OMEditLIB/LSP/README.mdOMEdit/OMEditLIB/MainWindow.cppOMEdit/OMEditLIB/MainWindow.hOMEdit/OMEditLIB/OMEditApplication.cppOMEdit/OMEditLIB/OMEditLIB.proOMEdit/OMEditLIB/Options/OptionsDialog.cppOMEdit/OMEditLIB/Options/OptionsDialog.hOMEdit/OMEditLIB/resource_omedit.qrcdoc/UsersGuide/source/omedit.rst
AnHeuermann
left a comment
AnHeuermann
left a comment
There was a problem hiding this comment.
Something basic seems to be off. I can't use go to declaration or it searching for the wrong stuff.
Going to declaration on spring (Line 59 of Modelica.Blocks.Examples.PID_Controller for MSL 3.2.3) is looking for startTime:
[227] 12:18:43 Scripting Notification
LSP: 10:18:43.445 DEBUG Didn't find UnresolvedReference { symbols: startTime, kind: class, position: 46:7, document: "/home/USER/.openmodelica/libraries/Modelica 3.2.3+maint.om/Blocks/package.mo" }
[228] 12:18:43 Scripting Notification
LSP: 10:18:43.445 DEBUG Didn't find declaration of startTime
| QStringLiteral("sudo apt install nodejs # Debian / Ubuntu\n" | ||
| "sudo dnf install nodejs # Fedora / RHEL"); |
There was a problem hiding this comment.
I don't think we should install stuff for users, especialy not as root. Instead just tell them to install Node.js themself.
There was a problem hiding this comment.
True, this part is a part of the setup dialog if the user opted to use the language server. I agree 👀
There was a problem hiding this comment.
There seems to be no crash detection or recovery logic implemented.
Claude: The current LSP implementation in OMEdit is minimalist and stateless regarding crash recovery:
- No crash detection beyond basic process error signals
- No retry mechanism or automatic restart
- No tracking of crash history or attempt counts
- No backoff or exponential delay strategies
- Relies entirely on user manual intervention to re-enable LSP via the Options dialog
This design assumes the language server is stable and properly managed. If the server crashes, the user must manually restart it through the Options dialog or restart OMEdit.
The server could crash multiple times. To improve it it would be important to see why.
VS Code tries to re-start the LSP server a few times (3 or 5, not sure) and then gives up. We should do something similar. Also some logging mechanic that produces a file that can be added to GitHub issues would be great.
There was a problem hiding this comment.
This should be addressed now
|
|
||
| # Install the bundled Modelica language server (optional — skipped when not built) | ||
| set(MODELICA_LS_DIR | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/../../../modelica-language-server/server/out" | ||
| CACHE PATH "Directory containing the built modelica-language-server (server.js and WASM files)") | ||
| if(EXISTS "${MODELICA_LS_DIR}/server.js") | ||
| set(MODELICA_LS_FILES | ||
| "${MODELICA_LS_DIR}/server.js" | ||
| "${MODELICA_LS_DIR}/tree-sitter-modelica.wasm" | ||
| "${MODELICA_LS_DIR}/web-tree-sitter.wasm") | ||
| if(WIN32) | ||
| # On Windows, place next to OMEdit.exe so findBundledServer() finds it | ||
| install(FILES ${MODELICA_LS_FILES} | ||
| DESTINATION ${CMAKE_INSTALL_BINDIR}/languageserver) | ||
| else() | ||
| install(FILES ${MODELICA_LS_FILES} | ||
| DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/omedit/languageserver) | ||
| endif() | ||
| message(STATUS "Bundled Modelica language server will be installed from ${MODELICA_LS_DIR}") | ||
| else() | ||
| message(STATUS "Modelica language server not found at ${MODELICA_LS_DIR} — skipping bundled install") | ||
| endif() |
There was a problem hiding this comment.
Is this a local file from your machine? It works for me as well because I have a similar layout on my machine 😆
Create a release of https://github.com/OpenModelica/modelica-language-server and add a download step here. E.g. for v0.3.1. Maybe compare the SHA to something known for a basic layer of protection.
file(DOWNLOAD ...)AI generated:
# Install the bundled Modelica language server (optional)
set(MODELICA_LS_VERSION "v0.3.1"
CACHE STRING "GitHub release tag of the Modelica language server to download")
set(_mls_dl_dir "${CMAKE_BINARY_DIR}/modelica-language-server")
set(_mls_base_url
"https://github.com/OpenModelica/modelica-language-server/releases/download/${MODELICA_LS_VERSION}")
# The npm pack tarball is named without the leading 'v' (e.g. v0.3.1 → 0.3.1)
string(REGEX REPLACE "^v" "" _mls_npm_ver "${MODELICA_LS_VERSION}")
set(_mls_tgz "${_mls_dl_dir}/openmodelica-modelica-language-server-${_mls_npm_ver}.tgz")
# Download and extract server.js from the npm tarball (package/out/server.js inside)
if(NOT EXISTS "${_mls_dl_dir}/server.js")
if(NOT EXISTS "${_mls_tgz}")
message(STATUS "Modelica language server: downloading ${MODELICA_LS_VERSION} tarball")
file(DOWNLOAD
"${_mls_base_url}/openmodelica-modelica-language-server-${_mls_npm_ver}.tgz"
"${_mls_tgz}"
STATUS _mls_status SHOW_PROGRESS)
list(GET _mls_status 0 _mls_code)
if(NOT _mls_code EQUAL 0)
message(WARNING "Modelica language server: tarball download failed: ${_mls_status}")
file(REMOVE "${_mls_tgz}")
endif()
endif()
if(EXISTS "${_mls_tgz}")
file(ARCHIVE_EXTRACT INPUT "${_mls_tgz}"
DESTINATION "${_mls_dl_dir}/extracted"
PATTERNS "package/out/server.js")
file(COPY "${_mls_dl_dir}/extracted/package/out/server.js"
DESTINATION "${_mls_dl_dir}")
endif()
endif()
# Download the WASM files (published as individual release assets)
foreach(_mls_wasm tree-sitter-modelica.wasm web-tree-sitter.wasm)
if(NOT EXISTS "${_mls_dl_dir}/${_mls_wasm}")
message(STATUS "Modelica language server: downloading ${_mls_wasm}")
file(DOWNLOAD
"${_mls_base_url}/${_mls_wasm}"
"${_mls_dl_dir}/${_mls_wasm}"
STATUS _mls_status SHOW_PROGRESS)
list(GET _mls_status 0 _mls_code)
if(NOT _mls_code EQUAL 0)
message(WARNING "Modelica language server: failed to download ${_mls_wasm}: ${_mls_status}")
file(REMOVE "${_mls_dl_dir}/${_mls_wasm}")
endif()
endif()
endforeach()
if(DEFINED _mls_source_dir)
set(_mls_files
"${_mls_source_dir}/server.js"
"${_mls_source_dir}/tree-sitter-modelica.wasm"
"${_mls_source_dir}/web-tree-sitter.wasm")
if(WIN32)
# On Windows, place next to OMEdit.exe so findBundledServer() finds it
install(FILES ${_mls_files} DESTINATION ${CMAKE_INSTALL_BINDIR}/languageserver)
else()
install(FILES ${_mls_files} DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/omedit/languageserver)
endif()
endif()
adeas31
left a comment
adeas31
left a comment
There was a problem hiding this comment.
Thanks for starting with this.
I tested this successfully e.g., on ChuaCircuit example but it doesn't work on PID_Controller example as @AnHeuermann mentioned. I will see what is wrong.
As a general note, I would like that we use ModelicaLSP as naming convention. Who knows if we add more LSPs in the future. Perhaps LSPProtocol, LSPClient can be abstract base classes and ModelicaLSPClient has the actual implementation.
OMEdit crashes everytime on the exit. It crashes inside ModelicaEditor::~ModelicaEditor()
| mpOptionsDialog = pOptionsDialog; | ||
| mpLanguageServerGroupBox = new QGroupBox(tr("Language Server Protocol (LSP)")); | ||
| // Enable LSP checkbox | ||
| mpEnableLSPCheckBox = new QCheckBox(tr("Enable Language Server")); |
There was a problem hiding this comment.
Instead of having separate checkbox make the above groupbox checkable.
| } | ||
| } | ||
| } | ||
| pLSPClient->start(executable, rootUri, libraries); |
There was a problem hiding this comment.
Why do we need to maintain a separate list of loaded libraries for LSP. I think we should sync this with OMC. Currently LSP is initialized during startup when nothing is loaded. So as soon as something is loaded in OMC that model/library should be loaded in LSP as well. In this way we can skip this separate handling of libraries for LSP and can remove it from LSP settings as well.
There was a problem hiding this comment.
This is a Claude Code agent acting on behalf of @JKRT.
Looked into whether the bundled modelica-language-server could pick up newly loaded OMC libraries live, since that's what a real sync would need.
Checked the server itself (v0.3.0): it advertises the workspace/didChangeConfiguration capability, but never actually subscribes a handler to it anywhere in the built server — no .onDidChangeConfiguration(...) call in the bundle. modelicaPath is only ever read once, from initializationOptions at initialize time.
So today, syncing with OMC's loaded libraries as they load would mean restarting/reinitializing the LSP client each time a new library is loaded (dropping in-flight requests, closing and reopening documents), not a lightweight live update. @JKRT wants to think through the right approach here (e.g. debounced restart on library load vs. keeping the explicit setting) before implementing, so leaving this open for now.
@JKRT you need to fix the line numbers that you pass to LSP. You can't just rely on ``. See for example int line = block.blockNumber() + 1;
if (mpBaseEditor->isModelicaModelInPackageOneFile()) {
line = block.blockNumber() + mpBaseEditor->getModelWidget()->getLibraryTreeItem()->mClassInformation.lineNumberStart;
}When the model is inside the package then you need to fix the line number start. In case of |
|
There is a basic navigation implementation in OMEdit e.g., do a ctrl+click on a path OR right click |
| pLibraryTreeItem->getModelWidget()->getEditor()->getPlainTextEdit()->goToLineNumber(lineNumber); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks a bit weird. If we can't find the file then we shouldn't try to load it as external model. The external model argument seems to be not doing anything special and should be removed.
If we keep the LSP sync with OMC (as I suggested earlier) then we definitely don't need to open any new model.
There was a problem hiding this comment.
Yea, this is a hack I did so this is partially on purpose
If I recall I did it like this because in some cases it opened files that could not be parsed by OMEdit (some information was missing) leading to annoying popup errors for instance if the file started with a within clause. I'll try to see if I can do this better
|
Great comments! I'll try to adress the bullets above + the comments from the rabbit |
Do you think a check box option in the LSP settings would suffice for this. That is if it checked we override the old behavior? |
Noted, I'll make the LSP itself an abstract class (LSPProtocol and LSPClient) abstract classes and do the ModelicaLSP variant as an extension from that |
Definitely. When LSP is enabled prefer it and use old behavior as fallback, |
…time, docs) Revise the Modelica LSP integration per reviewer feedback: - Fix the line-number bug: map editor (block,column) to full-file LSP coordinates, restoring the one-file-package lineNumberStart offset, the per-line stripped leading spaces and the first-line columnNumberStart trim. Sync the full file (exact on-disk text when unmodified) on didOpen/didChange so the server's view matches the loaded class. - Unify navigation: ctrl+click and a single right-click "Go to Definition" use the language server first and fall back to class-tree navigation on failure/timeout/when the server is off. The editor reuses the existing "Open Class" action (relabelled), so the base editor is unchanged. - Split LSPClient into a generic abstract base and a ModelicaLSPClient implementation. - Fix the exit crash path in ~ModelicaEditor by using a QPointer and a cached uri instead of MainWindow::instance() during teardown. - Do not cache a failed startup; always surface server errors; reference-count shared documents; debounce didChange and flush it before position requests; handle in-memory modelica:/// definition targets; remove the unused documentSymbol scaffolding (it had no consumer). - Docs: fix the RST anchor, remove restart contradictions and the false bundled-binary claim, correct the checkbox name and setup-dialog timing, state Node.js 16+, and drop the unexposed document-symbols claim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # OMEdit/OMEditLIB/CMakeLists.txt # OMEdit/OMEditLIB/MainWindow.cpp # OMEdit/OMEditLIB/OMEditLIB.pro
… CMake Make the Options page's Language Server Protocol groupbox itself checkable instead of a separate checkbox, matching the sibling groupboxes elsewhere on the page. CMake now downloads a pinned modelica-language-server release and verifies it against a known SHA256 before bundling it, instead of only working off a local development checkout.
…, crash recovery Skip starting the language server when running the testsuite, matching the policy OptionsDialog already applies to persisted settings. Centralize executable resolution (configured path -> bundled server -> PATH) into ModelicaLSPClient::resolveExecutable() so the Options page and MainWindow::startLanguageServer() agree; the Options page previously skipped the PATH fallback before deciding whether to show the Node.js setup dialog. Add crash recovery to LSPClient: an unexpected exit is now logged to languageserver_crash.log and the server is restarted automatically, capped at 5 crashes within 3 minutes (matching vscode-languageclient's default policy) before giving up and surfacing a clear error.
|
New iteration. Regarding, opening as an external model I am not sure I can get that to work cleanly without restarting the language server itself several times. Maybe that can be resolved at the language server level, but I think it is a current limitation. The response from Claude concerning that |
@JKRT did you create a release for the LSP so the CMake installer simply can download it?
Add an issue on https://github.com/OpenModelica/modelica-language-server/issues. Shouldn't be easy to implement. |
Not yet, I'll do it asap
I can do that as well |
|
Related issue created here: OpenModelica/modelica-language-server#62 Attempting to solve it in: |
4 participants
Purpose
The purpose of this PR is to integrate the modern LSP workflow in OMEdit. That is supporting features such as documentation upon hovering and interactive navigation.
Approach
This is solved by integrating a new option for OMEdit navigation which integrates the Language Server Protocol together with OMEdit.
A new setting is available as shown in the screenshot below:
Currently the language server is opt in and the user will need to download and configure it. This can be improved by automation or bundling the binary with OMEdit.
Showcase
Below is a showcase of the language server as it works right now
Hover over objects to see documentation as formatted markdown
Navigation
Currently go to declaration and go to definition works via right click and then selecting the LSP option from a context menu
Below are a few gifs showcasing the navigation etc in action