feat: add kconfig editor support #1327
Conversation
- Add defensive error handling for EclipseUtil.getSelectedIDFProjectInExplorer() - Implement fallback to getSelectedProjectInExplorer() with IDF nature check - Prevent terminal opening crash when no project is selected - Gracefully handle bundle version conflicts in OSGi runtime
WalkthroughThis PR introduces KConfig editor support for the IDF Eclipse plugin, including syntax highlighting, content assist, and editor features. It also adds defensive error handling to project retrieval in the serial terminal settings page with a fallback mechanism. Changes
Sequence DiagramsequenceDiagram
participant User
participant Eclipse
participant KConfigEditor
participant ContentAssist
participant Grammar
User->>Eclipse: Open Kconfig file
Eclipse->>KConfigEditor: initializeKeyBindingScopes()
KConfigEditor->>KConfigEditor: setKeyBindingScopes(CONTEXT_ID)
User->>Eclipse: Type in editor / trigger assist
Eclipse->>ContentAssist: computeCompletionProposals(viewer, offset)
ContentAssist->>ContentAssist: Parse current line fragment
ContentAssist->>ContentAssist: Match against keywords/types/properties
ContentAssist-->>User: Return CompletionProposal[] suggestions
Eclipse->>Grammar: Apply syntax highlighting
Grammar-->>Eclipse: Color tokens (keywords, strings, operators)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (5)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialSettingsPage.java (1)
104-127: Revise approach: This NoSuchMethodError handling is intentional for OSGi bundle compatibility.This defensive error handling is a legitimate pattern for Eclipse/OSGi environments, as confirmed by the commit message: "Gracefully handle bundle version conflicts in OSGi runtime." The method may exist in source but not be available at runtime due to bundle loading order or version conflicts—a real issue that can crash the terminal.
However, two improvements remain valid:
Add logging for silent exception: Lines 122-124 catch
Exceptionwithout logging, losing diagnostic information. Add a logger call.Optional: Reduce nesting for readability: While the logic is sound, the three-level nesting could be extracted to a helper method (e.g.,
getSelectedIDFProjectWithFallback()) for better maintainability.The scope concern is also incorrect—this change directly relates to the serial terminal, addressing crashes when no project is selected.
bundles/com.espressif.idf.ui/syntaxes/kconfig.tmLanguage.json (1)
38-40: Minor duplication in keyword pattern.The keyword pattern at line 39 includes
endmenutwice in the match expression: once in the middle and once at the end. While this doesn't break functionality, it's redundant.Apply this diff to remove the duplication:
{ "name": "keyword.control.kconfig", - "match": "\\b(config|choice|endchoice|menuconfig|endmenu|if|endif|source|comment|menu|endmenu)\\b" + "match": "\\b(config|choice|endchoice|menuconfig|if|endif|source|comment|menu|endmenu)\\b" },bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/kconfig/KConfigContentAssistProcessor.java (2)
39-73: Content assist implementation is functional but basic.The implementation provides keyword-based content assist that works for simple use cases. However, it has some limitations:
No context awareness: The processor suggests all keywords regardless of context (e.g., it will suggest "config" even when inside a help text block where it's not valid).
Performance consideration: Retrieving the entire document with
viewer.getDocument().get()at line 44 could be inefficient for very large files, though this is likely acceptable for typical KConfig files.Simple line-based matching: The processor only considers the current line's prefix, which is appropriate for KConfig's line-oriented syntax.
These are design trade-offs rather than bugs. The current implementation provides a good starting point for basic content assist functionality.
Consider enhancing context awareness in a future iteration by checking the current scope (e.g., inside a config block, inside help text, etc.) to provide more relevant suggestions.
85-89: Auto-activation characters may trigger too frequently.The content assist is configured to auto-activate on space, tab, and newline characters. While this provides convenient access to proposals, it may be too aggressive and could interrupt normal typing flow, especially when typing help text or comments.
Consider whether auto-activation is necessary, or if manual activation via Ctrl+Space provides a better user experience. If auto-activation is desired, you might want to limit it to fewer characters or add context checking to avoid activating in comments or help text.
bundles/com.espressif.idf.ui/plugin.xml (1)
858-868: Potential redundancy in content type configuration.The content type definition at lines 858-864 includes
file-patterns="Kconfig,Kconfig.projbuild,Kconfig.in", and then lines 865-868 add a separate file association withfile-extensions="Kconfig,Kconfig.projbuild,Kconfig.in".In Eclipse, specifying
file-patternsin the content type definition is typically sufficient. The separatefile-associationentry may be redundant. However, this doesn't cause any functional issues and may be intentional for explicit clarity or compatibility.Consider verifying whether the separate file-association entry is necessary, or if the file-patterns attribute alone is sufficient for proper file type detection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialSettingsPage.java(1 hunks)bundles/com.espressif.idf.ui/META-INF/MANIFEST.MF(3 hunks)bundles/com.espressif.idf.ui/kconfig-language-configuration.json(1 hunks)bundles/com.espressif.idf.ui/plugin.xml(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/kconfig/KConfigContentAssistProcessor.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/kconfig/KConfigEditor.java(1 hunks)bundles/com.espressif.idf.ui/syntaxes/kconfig.tmLanguage.json(1 hunks)docs/en/additionalfeatures.rst(1 hunks)docs/en/additionalfeatures/kconfigeditor.rst(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T18:07:42.435Z
Learnt from: sigmaaa
PR: espressif/idf-eclipse-plugin#1339
File: bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java:27-34
Timestamp: 2025-10-27T18:07:42.435Z
Learning: In the file bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/handlers/NvsEditorHandler.java, the NvsEditorHandler is tied to a project explorer right-click command, so EclipseUtil.getSelectedProjectInExplorer() will never return null due to the command's enablement conditions.
Applied to files:
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialSettingsPage.java
🧬 Code graph analysis (1)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialSettingsPage.java (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/EclipseUtil.java (1)
EclipseUtil(36-186)bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFProjectNature.java (1)
IDFProjectNature(16-61)
🔇 Additional comments (7)
docs/en/additionalfeatures.rst (1)
8-8: LGTM!The KConfig Editor entry is correctly positioned in the documentation index and follows the established format.
bundles/com.espressif.idf.ui/kconfig-language-configuration.json (1)
1-29: LGTM!The language configuration is well-structured and appropriate for KConfig files. The indentation rules, bracket matching, and auto-closing pairs align with KConfig syntax conventions.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/kconfig/KConfigEditor.java (1)
1-28: LGTM!The editor class is cleanly implemented and follows Eclipse editor extension patterns. The use of
ExtensionBasedTextEditorfrom internal API is acceptable for this use case, and the constants align with the plugin.xml configuration.bundles/com.espressif.idf.ui/syntaxes/kconfig.tmLanguage.json (1)
1-133: LGTM!The TextMate grammar is comprehensive and well-structured for KConfig syntax highlighting. It covers all necessary KConfig language constructs including keywords, types, properties, values, operators, and comments. The scope names follow TextMate conventions appropriately.
bundles/com.espressif.idf.ui/META-INF/MANIFEST.MF (1)
16-16: LGTM!The manifest dependency additions are appropriate for the KConfig Editor feature. The new dependencies support text editing (
org.eclipse.jface.text,org.eclipse.ui.editors), generic editor functionality (org.eclipse.ui.genericeditor), and TM4E language support (org.eclipse.tm4e.registry,org.eclipse.tm4e.languageconfiguration).Also applies to: 37-39, 52-54
bundles/com.espressif.idf.ui/plugin.xml (1)
642-651: LGTM!The plugin.xml configuration properly integrates all components of the KConfig Editor:
- Editor registration with content type binding
- Content type definition with file patterns
- TM4E grammar and scope name binding
- Language configuration for editor behavior
- Content assist processor registration
The configuration follows Eclipse plugin extension patterns correctly and all IDs and references are consistent across the plugin definition.
Also applies to: 858-896
docs/en/additionalfeatures/kconfigeditor.rst (1)
138-143: All reference URLs are accessible and valid.The verification confirms that all three reference URLs in the documentation return HTTP 200 status codes and are functioning correctly. No further action is required for the references section.
| The KConfig Editor automatically recognizes and provides enhanced editing for the following file types: | ||
|
|
||
| - ``Kconfig`` - Main configuration files | ||
| - ``Kconfig.projbuild`` - Project-specific configuration files | ||
|
|
There was a problem hiding this comment.
Documentation incomplete: Missing Kconfig.in file type.
The supported file types section only lists Kconfig and Kconfig.projbuild, but the implementation in plugin.xml (lines 860, 867) and the TextMate grammar (line 4 of kconfig.tmLanguage.json) also support Kconfig.in files. Please add this file type to the documentation for completeness.
Apply this diff to include the missing file type:
The KConfig Editor automatically recognizes and provides enhanced editing for the following file types:
- ``Kconfig`` - Main configuration files
- ``Kconfig.projbuild`` - Project-specific configuration files
+- ``Kconfig.in`` - Include configuration files📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The KConfig Editor automatically recognizes and provides enhanced editing for the following file types: | |
| - ``Kconfig`` - Main configuration files | |
| - ``Kconfig.projbuild`` - Project-specific configuration files | |
| The KConfig Editor automatically recognizes and provides enhanced editing for the following file types: | |
| - ``Kconfig`` - Main configuration files | |
| - ``Kconfig.projbuild`` - Project-specific configuration files | |
| - ``Kconfig.in`` - Include configuration files |
🤖 Prompt for AI Agents
In docs/en/additionalfeatures/kconfigeditor.rst around lines 9 to 13, the
supported file types list omits "Kconfig.in"; update the list to include a third
bullet for ``Kconfig.in`` (Project or template configuration files) so the
documentation matches plugin.xml (lines ~860/867) and the TextMate grammar
(kconfig.tmLanguage.json line 4). Ensure formatting matches the existing bullet
style and run a quick spell/format check after editing.
alirana01
left a comment
alirana01
left a comment
There was a problem hiding this comment.
LGTM!
Just a few small questions and suggestions to see if we can improve this
| private static final String[] MAIN_KEYWORDS = { | ||
| "config", "menuconfig", "choice", "endchoice", "menu", "endmenu", | ||
| "if", "endif", "source", "comment", "mainmenu" | ||
| }; | ||
|
|
||
| private static final String[] TYPES = { | ||
| "bool", "tristate", "int", "hex", "string" | ||
| }; | ||
|
|
||
| private static final String[] PROPERTIES = { | ||
| "default", "depends", "select", "imply", "range", "help", "prompt", | ||
| "visible", "optional" | ||
| }; | ||
|
|
||
| private static final String[] VALUES = { | ||
| "y", "n", "m" | ||
| }; | ||
|
|
There was a problem hiding this comment.
What do you think about having it in a separate interface or enum. Seems like these values can even be in an enum to avoid any NLS tags as well since they seem to be pretty consistent
There was a problem hiding this comment.
enums or interfaces are useful if you plan to reuse them elsewhere. Since these keywords are only used within the content assist processor, I don't think they're a good fit here
@coderabbitai what do you think?
There was a problem hiding this comment.
@kolipakakondal I think your approach is reasonable here. The string arrays are simple, clear, and work well for the content assist processor. Since the logic is straightforward prefix matching and these constants aren't currently shared across multiple classes, keeping them colocated is pragmatic.
One minor consideration: the kconfig.tmLanguage.json file also defines similar keywords for syntax highlighting. If you find yourself needing to keep these in sync across multiple files in the future, that might be a signal to centralize them. But for now, the current implementation is clean and maintainable.
The main benefit of enums would be type safety, but since you're doing string prefix matching anyway, it would just add conversion overhead without much practical gain in this specific use case.
| List<ICompletionProposal> proposals = new ArrayList<>(); | ||
|
|
||
| String text = viewer.getDocument().get(); | ||
| int lineStart = text.lastIndexOf('\n', offset - 1) + 1; |
There was a problem hiding this comment.
System.lineSeparator() seems like a safe way to avoid using '\n' directly
|
@sigmaaa @AndriiFilippov please review this and will merge this to 4.0.0 as 3.7.0 was already tested for release. |
Description
The KConfig Editor provides enhanced editing capabilities for ESP-IDF configuration files, including syntax highlighting, content assist, and intelligent editing features. It supports all KConfig file types used in ESP-IDF projects.
Fixes # (IEP-1367)
Type of change
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes