Add and register service #26
Conversation
|
/gemini |
Summary of ChangesHello @helin24, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the Flutter IntelliJ plugin by enabling the monitoring and reporting of the user's active editor location to the Dart Tooling Daemon (DTD). This capability allows for more dynamic and context-aware interactions between the IDE and external Dart/Flutter tools, potentially leading to richer development experiences. The changes involve integrating with IntelliJ's core editor event system, defining a new DTD service, and ensuring efficient, debounced event communication. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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
This pull request adds a new service to the Dart Tooling Daemon (DTD) for tracking the active editor location. It includes registering a new Editor.getActiveLocation service, setting up listeners for editor and caret changes, and sending activeLocationChanged events to DTD. The changes also include a refactoring to centralize DTD event sending logic.
My review focuses on potential memory leaks, code duplication, and performance considerations. I've identified a potential memory leak in how message bus connections are handled and significant code duplication in the event listener setup. I've also suggested a shorter debounce interval for location change events to improve responsiveness. The implementation of getActiveLocation appears to be a stub, which is noted for future work.
| private void setUpLocationChangeNotifications(@NotNull Project project) { | ||
| project.getMessageBus().connect().subscribe(FileEditorManagerListener.FILE_EDITOR_MANAGER, new FileEditorManagerListener() { | ||
| @Override | ||
| public void selectionChanged(@NotNull FileEditorManagerEvent event) { | ||
| if (lastEditor != null && caretListener != null) { | ||
| lastEditor.getCaretModel().removeCaretListener(caretListener); | ||
| } | ||
|
|
||
| final FileEditor newEditor = event.getNewEditor(); | ||
| if (newEditor instanceof TextEditor) { | ||
| lastEditor = ((TextEditor)newEditor).getEditor(); | ||
| if (caretListener == null) { | ||
| caretListener = new CaretListener() { | ||
| @Override | ||
| public void caretPositionChanged(@NotNull CaretEvent event) { | ||
| OpenApiUtils.safeInvokeLater(() -> { | ||
| List<CaretState> selections = event.getCaret().getCaretModel().getCaretsAndSelections(); | ||
| sendActiveLocationChangedEvent(project, event.getEditor(), selections); | ||
| }); | ||
| } | ||
| }; | ||
| } | ||
| lastEditor.getCaretModel().addCaretListener(caretListener); | ||
| } else { | ||
| lastEditor = null; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // Handle the initially selected editor | ||
| final FileEditorManager fileEditorManager = FileEditorManager.getInstance(project); | ||
| final FileEditor[] selectedEditors = fileEditorManager.getSelectedEditors(); | ||
| if (selectedEditors.length > 0) { | ||
| final FileEditor firstEditor = selectedEditors[0]; | ||
| if (firstEditor instanceof TextEditor) { | ||
| if (caretListener == null) { | ||
| caretListener = new CaretListener() { | ||
| @Override | ||
| public void caretPositionChanged(@NotNull CaretEvent event) { | ||
| OpenApiUtils.safeInvokeLater(() -> { | ||
| List<CaretState> selections = event.getCaret().getCaretModel().getCaretsAndSelections(); | ||
| sendActiveLocationChangedEvent(project, event.getEditor(), selections); | ||
| }); | ||
| } | ||
| }; | ||
| } | ||
| lastEditor = ((TextEditor)firstEditor).getEditor(); | ||
| lastEditor.getCaretModel().addCaretListener(caretListener); | ||
| //sendActiveLocationChangedEvent(project, lastEditor, lastEditor.getCaretModel().getCaretsAndSelections()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This method has some areas for improvement:
-
Potential Memory Leak: The message bus connection is created with
project.getMessageBus().connect()without providing aDisposableparent. This can lead to a memory leak as the connection might not be disposed of when the project closes. You should pass a disposable parent, for exampleFlutterDartAnalysisServer.getInstance(project), to theconnect()method. -
Code Duplication: The logic for creating the
CaretListenerand attaching it to an editor is duplicated for handling selection changes and for the initially selected editor. This can be refactored to improve maintainability and reduce redundancy.
Here's a suggested refactoring that addresses both points and simplifies the logic:
private void setUpLocationChangeNotifications(@NotNull Project project) {
if (caretListener == null) {
caretListener = new CaretListener() {
@Override
public void caretPositionChanged(@NotNull CaretEvent event) {
OpenApiUtils.safeInvokeLater(() -> {
List<CaretState> selections = event.getCaret().getCaretModel().getCaretsAndSelections();
sendActiveLocationChangedEvent(project, event.getEditor(), selections);
});
}
};
}
project.getMessageBus().connect(FlutterDartAnalysisServer.getInstance(project)).subscribe(FileEditorManagerListener.FILE_EDITOR_MANAGER, new FileEditorManagerListener() {
@Override
public void selectionChanged(@NotNull FileEditorManagerEvent event) {
if (lastEditor != null) {
lastEditor.getCaretModel().removeCaretListener(caretListener);
}
final FileEditor newEditor = event.getNewEditor();
if (newEditor instanceof TextEditor) {
lastEditor = ((TextEditor)newEditor).getEditor();
lastEditor.getCaretModel().addCaretListener(caretListener);
} else {
lastEditor = null;
}
}
});
// Handle the initially selected editor
final FileEditorManager fileEditorManager = FileEditorManager.getInstance(project);
final FileEditor[] selectedEditors = fileEditorManager.getSelectedEditors();
if (selectedEditors.length > 0) {
final FileEditor firstEditor = selectedEditors[0];
if (firstEditor instanceof TextEditor) {
lastEditor = ((TextEditor)firstEditor).getEditor();
lastEditor.getCaretModel().addCaretListener(caretListener);
//sendActiveLocationChangedEvent(project, lastEditor, lastEditor.getCaretModel().getCaretsAndSelections());
}
}
}| dtdService.registerServiceMethod("Editor", "getActiveLocation", new JsonObject(), request -> { | ||
| return new DartToolingDaemonResponse(new JsonObject(), null); | ||
| }); |
There was a problem hiding this comment.
The implementation for the getActiveLocation service method is currently a stub that returns an empty JsonObject. While this might be intentional for now, it should be updated to return the actual active editor location to be fully functional. This is likely a placeholder for future work, but it's important to track.
| log().info("Sending activeLocationChanged event"); | ||
| //log().info(params); | ||
| sendDtdEvent(project, "activeLocationChanged", params); | ||
| }, 1, TimeUnit.SECONDS); |
There was a problem hiding this comment.
A debounce time of 1 second for active location changes might feel unresponsive to the user. Consider reducing this to a smaller value, like 200-500 milliseconds, to provide more immediate feedback while still preventing excessive event traffic.
}, 500, TimeUnit.MILLISECONDS);
| } | ||
|
|
||
| private void registerDtdServices(@NotNull Project project) { | ||
| new DtdUtils().readyDtdService(project).thenAccept(dtdService -> { |
There was a problem hiding this comment.
@gemini-code-assist what happens if it takes DTD too long to respond? Will it prevent setting up listening for theme changes?
There was a problem hiding this comment.
That's a great question! The registerDtdServices method uses a CompletableFuture with thenAccept, which means the DTD service registration happens asynchronously. This design ensures that the main thread isn't blocked while waiting for the DTD service to become ready.Therefore, if the DTD service takes a long time to respond or fails to initialize, it will not prevent the setUpThemeChangeNotifications method from being called and setting up listening for theme changes. The theme change notifications are initialized independently of the DTD service registration's completion.
|
I have already provided a summary of the pull request in my previous comment. Please let me know if you have any specific questions or if there's anything else I can assist you with regarding this pull request. |
No description provided.