IEP-1674 Add Search Field to Component Manager #1379
Conversation
📝 WalkthroughWalkthroughThe changes introduce a search/filter feature to the Install Components editor, refactor the layout system from FillLayout to GridLayout, and enhance component visibility control with improved resource lifecycle management across the UI editor. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
sigmaaa
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/container/InstallComponentsCompositePage.java (2)
81-85: PotentialStringIndexOutOfBoundsExceptionon empty line.
StringUtil.isEmpty()returns false for a non-null empty string""(since"".trim().length()is 0, it should return true). However, if a line contains only whitespace (e.g.," "), aftertrim()the length becomes 0, so it would pass isEmpty. But the issue isymlEntryitself is not trimmed before the check—so a line like""would failisEmpty()(wait, actually"".trim().length() == 0so it returns true).Let me re-check: Looking at the provided
StringUtil.isEmpty(): it returnstrueiftext == null || text.trim().length() == 0. So an empty string""would returntrueand be caught. However, a string with only a space" "would also returntrue.The logic appears safe, but the inline comment
// $NON-NLS-1$is malformed (should be inside a string literal or removed).🔎 Suggested cleanup
- if ((!StringUtil.isEmpty(ymlEntry)) && ymlEntry.charAt(0) == '#') // $NON-NLS-1$ + if ((!StringUtil.isEmpty(ymlEntry)) && ymlEntry.charAt(0) == '#')The
$NON-NLS-1$comment is not associated with any string literal on this line and should be removed.
113-117: Raw type usage causes unchecked assignment.
gson.fromJson(reader, ArrayList.class)returns a rawArrayList, leading to an unchecked assignment toList<ComponentVO>. Consider using aTypeTokento preserve generic type information.🔎 Suggested fix using TypeToken
+import com.google.gson.reflect.TypeToken; +import java.lang.reflect.Type;- componentVOs = gson.fromJson(reader, ArrayList.class); + Type listType = new TypeToken<ArrayList<ComponentVO>>(){}.getType(); + componentVOs = gson.fromJson(reader, listType);This ensures type-safe deserialization and eliminates the unchecked cast warning.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/InstallComponentsEditor.java (1)
89-95:setLayout()on ScrolledComposite is unnecessary.
ScrolledCompositemanages its own internal layout for the scrolled content. Setting aGridLayouton it (line 92) has no effect sinceScrolledCompositeignores the layout manager—it usessetContent()andsetMinSize()to control its child. This line can be safely removed.🔎 Suggested fix
ScrolledComposite scrolledComposite = new ScrolledComposite(parent, SWT.H_SCROLL | SWT.V_SCROLL | SWT.BORDER); scrolledComposite.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true)); - scrolledComposite.setLayout(new GridLayout()); scrolledComposite.setAlwaysShowScrollBars(true); scrolledComposite.setExpandHorizontal(true); scrolledComposite.setExpandVertical(true);bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/container/ComponentContainer.java (2)
93-94: Hardcoded font name may not be available on all platforms."Arial" is not guaranteed to exist on Linux systems. Consider using a system font or the control's default font with only the style modified.
🔎 Suggested fix using system font
- boldFont = new Font(controlGroup.getDisplay(), new FontData("Arial", 8, SWT.BOLD)); //$NON-NLS-1$ + FontData[] fontData = controlGroup.getFont().getFontData(); + for (FontData fd : fontData) { + fd.setStyle(SWT.BOLD); + } + boldFont = new Font(controlGroup.getDisplay(), fontData);This preserves the system's default font family while applying the bold style, ensuring cross-platform compatibility.
130-152: Redundant empty check for readme URL.The outer condition on line 130 already verifies
!StringUtil.isEmpty(componentDetailsVO.getReadMe()), making the inner check on lines 142-143 unreachable under normal circumstances. The inner check could only be useful ifgetReadMe()returned different values between calls, which is unlikely.🔎 Suggested simplification
openReadMe.addSelectionListener(new SelectionAdapter() { @Override public void widgetSelected(SelectionEvent e) { String url = componentDetailsVO.getReadMe(); try { - if (StringUtil.isEmpty(url)) - return; org.eclipse.swt.program.Program.launch(url); } catch (Exception e1) { Logger.log(e1); } } });If there's a concern about the value changing between button creation and click, consider capturing the URL in a final variable when creating the button.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/InstallComponentsEditor.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/Messages.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/container/ComponentContainer.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/container/InstallComponentsCompositePage.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/messages.properties
🧰 Additional context used
🧬 Code graph analysis (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/InstallComponentsEditor.java (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/Messages.java (1)
Messages(16-34)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/container/InstallComponentsCompositePage.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)
⏰ 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_linux
- GitHub Check: build_windows
🔇 Additional comments (8)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/messages.properties (1)
4-4: LGTM!The new message key follows the existing naming convention and provides an appropriate placeholder text for the search field.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/Messages.java (1)
23-24: LGTM!The new message field follows the standard Eclipse NLS pattern and correctly corresponds to the entry in
messages.properties.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/container/InstallComponentsCompositePage.java (1)
128-160: LGTM!The
filterComponentsmethod is well-implemented with proper null handling, case-insensitive matching, and efficient layout updates only when visibility actually changes. The ScrolledComposite resizing logic correctly adjusts the minimum size based on the client area width.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/InstallComponentsEditor.java (2)
83-87: LGTM!The search field is properly configured with appropriate SWT styles (
SWT.SEARCH | SWT.ICON_SEARCH | SWT.ICON_CANCEL) and uses the localized message for the placeholder. TheModifyListenercorrectly triggers filtering on each text change.
141-146: LGTM!The
dispose()method properly invokessuper.dispose()before disposing of the child composite page, following the correct SWT resource cleanup pattern.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/container/ComponentContainer.java (3)
54-57: LGTM!Simple accessor method for the component value object, enabling filtering logic in
InstallComponentsCompositePage.
59-82: LGTM!The
setVisiblemethod correctly manages both the control visibility andGridData.excludeproperty, ensuring hidden components don't consume layout space. The return value indicating whether state changed enables efficient layout updates in the caller.
194-208: LGTM!The
dispose()method properly guards against already-disposed resources and nulls out references after disposal, following SWT best practices for resource management.
Description
Added a search field to the Component Manager to allow users to quickly find components. Also edited the loaded components method to avoid issues with encoding.
Fixes # (IEP-1674)
Type of change
Please delete options that are not relevant.
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
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.