Refactor: Update ComboBoxWithBrowseButton usage#267
Conversation
This commit updates the usage of ComboBoxWithBrowseButton to the new API. This fixes the compilation error that was occurring after the IntelliJ Platform SDK update.
|
/gemini please review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the usage of the deprecated ComboBoxWithBrowseButton to the modern IntelliJ Platform API using ComboBox with ExtendableTextField and browse extensions. While this addresses the compilation errors, the current implementation introduces several regressions and maintainability issues. Most notably, DartRemoteDebugConfigurationEditor lost its project discovery and selection dropdown, and DartSdkUtil lost its default SDK path suggestion logic. There is also significant code duplication between DartGeneratorPeer and DartConfigurable that should be addressed by extracting the common UI creation logic into a utility method. Additionally, a bug was identified in DartGeneratorPeer where the SDK path only updates on text insertion, ignoring deletions.
|
|
||
| private JPanel myMainPanel; | ||
| private ComboboxWithBrowseButton myDartProjectCombo; | ||
| private TextFieldWithBrowseButton myDartProjectCombo; |
There was a problem hiding this comment.
The replacement of ComboBoxWithBrowseButton with TextFieldWithBrowseButton has resulted in the removal of the project discovery logic and the dropdown list of projects. This is a regression in functionality, as users can no longer easily select from detected Dart projects within the workspace. Consider using a ComboBox with an ExtendableTextField and a browse extension (similar to the implementation in DartGeneratorPeer) to maintain the original behavior while adhering to the new API.
References
- Readability and Maintainability: The removal of useful features during refactoring reduces the overall quality and usability of the tool. (link)
| private static void addKnownPathsToCombo(final @NotNull JComboBox combo, | ||
| final @NotNull String propertyKey, | ||
| final @NotNull Predicate<? super String> pathChecker) { | ||
| public static void addKnownSDKPathsToCombo(final @NotNull JComboBox<String> combo) { |
There was a problem hiding this comment.
The removal of initDartSdkControls has eliminated the logic that suggests default SDK paths based on the operating system (e.g., Homebrew paths on macOS or standard paths on Windows/Linux). This makes the initial configuration more difficult for new users. Additionally, the logic that automatically appends /dart-sdk to a selected path if it contains the SDK has been lost. It is recommended to restore these helper features to maintain a good user experience.
| if(e.getType() == DocumentEvent.EventType.INSERT){ | ||
| final String sdkHomePath = getSdkPathText(); | ||
| final String sdkVersion = DartSdkUtil.getSdkVersion(sdkHomePath); | ||
| myVersionLabel.setText(sdkVersion == null ? "" : sdkVersion); | ||
| onSdkPathChanged(); | ||
| } |
There was a problem hiding this comment.
The DocumentListener here only reacts to INSERT events. This means if a user deletes characters from the SDK path (e.g., to correct a typo), the version label and the project templates list will not update. This leads to an inconsistent UI state. The check should be removed to handle all text changes, including deletions.
final String sdkHomePath = getSdkPathText();
final String sdkVersion = DartSdkUtil.getSdkVersion(sdkHomePath);
myVersionLabel.setText(sdkVersion == null ? "" : sdkVersion);
onSdkPathChanged();| final VirtualFile file = | ||
| FileChooser.chooseFile(descriptor, mySdkPathComboWithBrowse, null, null); | ||
| if (file != null) { | ||
| mySdkPathComboWithBrowse.getEditor().setItem(file.getPath()); |
There was a problem hiding this comment.
When a folder is selected via the browse button, file.getPath() returns a system-independent path (using forward slashes). For better consistency with the IntelliJ UI, especially on Windows, it is recommended to convert this to a system-dependent path before setting it in the editor.
mySdkPathComboWithBrowse.getEditor().setItem(FileUtil.toSystemDependentName(file.getPath()));References
- Consistency: Adhering to a consistent style across all projects improves collaboration and reduces errors. (link)
| final VirtualFile file = | ||
| FileChooser.chooseFile(descriptor, mySdkPathComboWithBrowse, myProject, null); | ||
| if (file != null) { | ||
| mySdkPathComboWithBrowse.getEditor().setItem(file.getPath()); |
There was a problem hiding this comment.
Similar to the project wizard, the path selected via the file chooser should be converted to a system-dependent format for UI consistency.
| mySdkPathComboWithBrowse.getEditor().setItem(file.getPath()); | |
| mySdkPathComboWithBrowse.getEditor().setItem(FileUtil.toSystemDependentName(file.getPath())); |
References
- Consistency: Adhering to a consistent style across all projects improves collaboration and reduces errors. (link)
| private void createUIComponents() { | ||
| mySdkPathComboWithBrowse = new ComboboxWithBrowseButton(new ComboBox<>()); | ||
| ExtendableTextComponent.Extension browseExtension = | ||
| ExtendableTextComponent.Extension.create(AllIcons.General.OpenDisk, AllIcons.General.OpenDiskHover, | ||
| DartBundle.message("dart.sdk.path.label"), | ||
| () -> { | ||
| final FileChooserDescriptor descriptor = | ||
| FileChooserDescriptorFactory.createSingleFolderDescriptor(); | ||
| final VirtualFile file = | ||
| FileChooser.chooseFile(descriptor, mySdkPathComboWithBrowse, null, null); | ||
| if (file != null) { | ||
| mySdkPathComboWithBrowse.getEditor().setItem(file.getPath()); | ||
| } | ||
| }); | ||
|
|
||
| mySdkPathComboWithBrowse = new ComboBox<>(); | ||
| mySdkPathComboWithBrowse.setEditable(true); | ||
| mySdkPathComboWithBrowse.setEditor(new BasicComboBoxEditor() { | ||
| @Override | ||
| protected JTextField createEditorComponent() { | ||
| final ExtendableTextField ecbEditor = new ExtendableTextField(); | ||
| ecbEditor.addExtension(browseExtension); | ||
| ecbEditor.setBorder(null); | ||
| return ecbEditor; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The logic for creating a ComboBox with a browse extension is duplicated here and in DartConfigurable.java. To improve maintainability and reduce duplication, this logic should be extracted into a reusable utility method, for example in DartSdkUtil.
References
- Maintainability: Code should be easy to modify and extend. Reducing duplication is key to maintainability. (link)
1 participant
This commit updates the usage of ComboBoxWithBrowseButton to the new API. This fixes the compilation error that was occurring after the IntelliJ Platform SDK update.
This was done in concert with Gemini Pro as a test. Please view it as a test and review the code very carefully.