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
WalkthroughAdds a KConfig editor (grammar, language configuration, content assist, editor integration, packaging, and docs) and makes SerialSettingsPage project-selection retrieval defensive with a fallback and exception handling. Changes
Sequence Diagram(s)%%{init: {"themeVariables":{"actorBackground":"#f3f4f6","actorBorder":"#d1d5db","noteBackground":"#fff7ed","noteBorder":"#f59e0b"}}}%%
sequenceDiagram
participant User
participant Eclipse
participant Editor as KConfigEditor
participant Assist as KConfigContentAssistProcessor
participant TM as TM4E/Grammar
User->>Eclipse: Open Kconfig file
Eclipse->>Editor: instantiate editor, initializeKeyBindingScopes()
Editor->>Editor: setKeyBindingScopes(CONTEXT_ID)
User->>Editor: Type / trigger completion
Editor->>Assist: computeCompletionProposals(viewer, offset)
Assist->>Assist: analyze current line fragment
Assist-->>Editor: CompletionProposal[] (suggestions)
Editor-->>User: show proposals
Eclipse->>TM: apply grammar & language config
TM-->>Editor: tokenization, folding, indentation metadata
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)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. |
|
Hi @AndriiFilippov please verify so that we can merge also this to the 4.0.0 release. This shouldn't impact any other functionality. |
|
@kolipakakondal hi ! Tested under: was not able to open |
|
My bad, I forgot to add the language configuration files to the build. They're added now-please check once the builds are available. |
|
@kolipakakondal syntax highlighting / content assist / and intelligent editing features work fine ✅ |
Hi @AndriiFilippov Thanks for identifying this! Issue got fixed with the recent commit. Now Kconfig editor also support standard generic editor command actions. Content Assist: Press Ctrl+Space (or Cmd+Space on Mac) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/plugin.xml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (4)
bundles/com.espressif.idf.ui/plugin.xml (4)
642-651: Editor registration structure is sound.The KConfigEditor extension is properly wired with:
- Correct content-type binding (line 649)
- Appropriate text editor action contributor (line 644)
- Default editor flag enabled for KConfig files (line 645)
870-896: TM4E grammar, language config, and content assist integrations are correctly configured.The grammar registration, language configuration, and content assist processor are all properly bound to the KConfig content-type with consistent IDs and appropriate trigger characters.
916-920: Editor context definition is correct.The new KConfig editor context properly inherits from
org.eclipse.ui.textEditorScopefor standard text editor key bindings and behaviors.
642-896: All referenced resources are present and properly included in the plugin build.Verification confirms that:
syntaxes/kconfig.tmLanguage.jsonexists andsyntaxes/is included inbin.includeskconfig-language-configuration.jsonexists and is explicitly included inbin.includesKConfigEditorandKConfigContentAssistProcessorclasses exist as Java source files and will be compiled (viasource.. = src/andoutput.. = bin/configuration)The resources referenced in the plugin.xml extensions (lines 643, 874, 886, 892) are all present and properly configured for the editor to function.
| <file-association | ||
| content-type="com.espressif.idf.ui.kconfig.contentType" | ||
| file-extensions="Kconfig,Kconfig.projbuild,Kconfig.in"> | ||
| </file-association> |
There was a problem hiding this comment.
Fix file-extensions attribute: should contain only extension names, not full filename patterns.
The file-extensions attribute on line 867 contains full filename patterns ("Kconfig", "Kconfig.projbuild", "Kconfig.in"), but this attribute expects only extension names without dots or path separators. The file-patterns attribute on line 860 already correctly specifies these filenames.
Remove the redundant file-association block (lines 865–868), as the content-type's file-patterns already establishes the file matching.
- <file-association
- content-type="com.espressif.idf.ui.kconfig.contentType"
- file-extensions="Kconfig,Kconfig.projbuild,Kconfig.in">
- </file-association>📝 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.
| <file-association | |
| content-type="com.espressif.idf.ui.kconfig.contentType" | |
| file-extensions="Kconfig,Kconfig.projbuild,Kconfig.in"> | |
| </file-association> |
🤖 Prompt for AI Agents
In bundles/com.espressif.idf.ui/plugin.xml around lines 865 to 868, the
file-association block uses the file-extensions attribute incorrectly by listing
full filename patterns; since file-patterns already covers these names, remove
the redundant file-association block (lines 865–868) entirely so only the
existing file-patterns entry defines the mappings.
|
Tested under: everythibg works fine including standard generic editor command actions ✔️ |
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
Documentation
Chores