Cycle visualization #52
Conversation
Summary of ChangesHello @vogella, 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 significantly upgrades the cyclic dependency detection feature by introducing a visual representation of cycles. Instead of merely listing detected cycles in a pop-up dialog, the tool now leverages the Eclipse Console view to display these cycles as clear, vertical ASCII art flow diagrams. This enhancement aims to provide developers with a more intuitive and actionable understanding of complex dependency structures, with the console automatically appearing to present these detailed insights. 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 introduces a new visualization for cyclic dependencies, displaying them in the Eclipse console with ASCII art. The core changes are within DetectCyclicDependenciesHandler.java to implement this new output format. My review focuses on a logical issue in the ASCII art generation that causes redundant output, a potential NullPointerException when trying to display the console, and several areas where code readability and maintainability have been reduced. I've provided suggestions to fix the bugs and improve the code's clarity and robustness.
| IWorkbenchPage page = PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage(); | ||
| String id = IConsoleConstants.ID_CONSOLE_VIEW; | ||
| IConsoleView view = (IConsoleView) page.showView(id); | ||
| view.display(myConsole); |
There was a problem hiding this comment.
There is a potential NullPointerException here. PlatformUI.getWorkbench().getActiveWorkbenchWindow() can return null if no workbench window is active, and subsequently getActivePage() can also be called on null or return null. This would crash the handler inside the try-catch block. It's important to add null checks to handle this case gracefully.
org.eclipse.ui.IWorkbenchWindow window = PlatformUI.getWorkbench().getActiveWorkbenchWindow();
if (window == null) {
Platform.getLog(getClass()).log(new Status(Status.WARNING, "com.vogella.ide.debugtools", "Could not open console view: no active window."));
return;
}
IWorkbenchPage page = window.getActivePage();
if (page == null) {
Platform.getLog(getClass()).log(new Status(Status.WARNING, "com.vogella.ide.debugtools", "Could not open console view: no active page."));
return;
}
String id = IConsoleConstants.ID_CONSOLE_VIEW;
IConsoleView view = (IConsoleView) page.showView(id);
view.display(myConsole);| for (int i = 0; i < cycle.size() - 1; i++) { | ||
| String current = cycle.get(i); | ||
| String next = cycle.get(i + 1); | ||
| String type = cycleInfo.getEdgeType(current, next); | ||
|
|
||
| sb.append(horizontalBorder).append("\n"); | ||
| sb.append(String.format(" | %-" + (boxWidth - 4) + "s |\n", current)); | ||
| sb.append(horizontalBorder).append("\n"); | ||
|
|
||
| sb.append(" |\n"); | ||
| sb.append(" | [").append(type).append("]\n"); | ||
| sb.append(" v\n"); | ||
| } | ||
|
|
||
| String lastNode = cycle.get(cycle.size() - 1); | ||
| sb.append(horizontalBorder).append("\n"); | ||
| sb.append(String.format(" | %-" + (boxWidth - 4) + "s |\n", lastNode)); | ||
| sb.append(horizontalBorder).append("\n"); | ||
|
|
||
| sb.append(" ^ (Loops back to start)\n"); | ||
| sb.append(" |______________________|\n"); | ||
|
|
There was a problem hiding this comment.
The current implementation for generating the ASCII art visualization has a flaw. For a cycle of N nodes, it prints N+1 boxes, with the first node being printed twice (once at the beginning and once at the end). This makes the visualization redundant and slightly confusing. The logic should be adjusted to print each unique node in the cycle only once, with the final arrow indicating the loop back to the start.
| for (int i = 0; i < cycle.size() - 1; i++) { | |
| String current = cycle.get(i); | |
| String next = cycle.get(i + 1); | |
| String type = cycleInfo.getEdgeType(current, next); | |
| sb.append(horizontalBorder).append("\n"); | |
| sb.append(String.format(" | %-" + (boxWidth - 4) + "s |\n", current)); | |
| sb.append(horizontalBorder).append("\n"); | |
| sb.append(" |\n"); | |
| sb.append(" | [").append(type).append("]\n"); | |
| sb.append(" v\n"); | |
| } | |
| String lastNode = cycle.get(cycle.size() - 1); | |
| sb.append(horizontalBorder).append("\n"); | |
| sb.append(String.format(" | %-" + (boxWidth - 4) + "s |\n", lastNode)); | |
| sb.append(horizontalBorder).append("\n"); | |
| sb.append(" ^ (Loops back to start)\n"); | |
| sb.append(" |______________________|\n"); | |
| for (int i = 0; i < cycle.size() - 1; i++) { | |
| String current = cycle.get(i); | |
| String next = cycle.get(i + 1); | |
| String type = cycleInfo.getEdgeType(current, next); | |
| sb.append(horizontalBorder).append("\n"); | |
| sb.append(String.format(" | %-" + (boxWidth - 4) + "s |\n", current)); | |
| sb.append(horizontalBorder).append("\n"); | |
| if (i < cycle.size() - 2) { | |
| sb.append(" |\n"); | |
| sb.append(" | [").append(type).append("]\n"); | |
| sb.append(" v\n"); | |
| } else { | |
| sb.append(" |\n"); | |
| sb.append(" | [").append(type).append("]\n"); | |
| sb.append(" ^ (Loops back to start)\n"); | |
| sb.append(" |______________________|\n"); | |
| } | |
| } |
| for (int i = 0; i < existing.length; i++) | ||
| if (name.equals(existing[i].getName())) | ||
| return (MessageConsole) existing[i]; |
There was a problem hiding this comment.
The traditional for loop with an index can be replaced with an enhanced for-each loop. This is more idiomatic in modern Java and improves readability by removing the boilerplate of index management.
| for (int i = 0; i < existing.length; i++) | |
| if (name.equals(existing[i].getName())) | |
| return (MessageConsole) existing[i]; | |
| for (IConsole console : existing) { | |
| if (name.equals(console.getName())) { | |
| return (MessageConsole) console; | |
| } | |
| } |
| private CycleInfo extractCycle(String current, String cycleStart, DependencyEdge finalEdge) { | ||
| LinkedList<String> path = new LinkedList<>(); | ||
| path.addFirst(current); // Start with the node where recursion found cycle | ||
|
|
||
| // Reconstruct path from 'current' back to 'cycleStart' | ||
| path.addFirst(current); | ||
| String node = current; | ||
| while (!node.equals(cycleStart)) { | ||
| node = parent.get(node); | ||
| path.addFirst(node); | ||
| } | ||
| // Now 'path' is [cycleStart, ..., current] | ||
|
|
||
| // Create cycle list for CycleInfo: [cycleStart, ..., current, cycleStart] | ||
| List<String> cycleList = new ArrayList<>(path); | ||
| cycleList.add(cycleStart); // Close the cycle | ||
|
|
||
| cycleList.add(cycleStart); | ||
| CycleInfo cycleInfo = new CycleInfo(cycleList); | ||
|
|
||
| // Populate edge types for the cycle | ||
| // Edges from cycleStart to current | ||
| for (int i = 0; i < path.size() - 1; i++) { | ||
| String from = path.get(i); | ||
| String to = path.get(i + 1); | ||
| // The edge that leads to 'to' from 'from' | ||
| DependencyEdge edge = parentEdge.get(to); | ||
| cycleInfo.addEdge(from, to, edge.type); | ||
| } | ||
|
|
||
| // The final edge from 'current' back to 'cycleStart' | ||
| cycleInfo.addEdge(current, cycleStart, finalEdge.type); | ||
|
|
||
| return cycleInfo; |
There was a problem hiding this comment.
This method was refactored to be more compact, but in the process, several helpful comments explaining the logic of cycle path reconstruction were removed. While the code is shorter, it's now less self-documenting, making it harder for other developers to understand the algorithm at a glance. Please consider re-adding some of the key comments to improve maintainability. For example, explaining how the path is reconstructed backwards from the current node to the cycleStart node.
| private List<String> normalizeCycle(List<String> cycle) { | ||
| if (cycle.size() <= 1) return new ArrayList<>(cycle); | ||
|
|
||
| // Remove the duplicate last element for comparison | ||
| List<String> temp = new ArrayList<>(cycle.subList(0, cycle.size() - 1)); | ||
|
|
||
| // Find the minimum element | ||
| int minIndex = 0; | ||
| for (int i = 1; i < temp.size(); i++) { | ||
| if (temp.get(i).compareTo(temp.get(minIndex)) < 0) { | ||
| minIndex = i; | ||
| } | ||
| if (temp.get(i).compareTo(temp.get(minIndex)) < 0) minIndex = i; | ||
| } | ||
|
|
||
| // Rotate to start with minimum element | ||
| List<String> normalized = new ArrayList<>(); | ||
| for (int i = 0; i < temp.size(); i++) { | ||
| normalized.add(temp.get((minIndex + i) % temp.size())); | ||
| } | ||
| normalized.add(normalized.get(0)); // Add back the duplicate to complete the cycle | ||
|
|
||
| normalized.add(normalized.get(0)); | ||
| return normalized; |
There was a problem hiding this comment.
Similar to extractCycle, the explanatory comments in this method have been removed. The logic for normalizing a cycle (finding the lexicographically smallest representation by rotation to avoid duplicate cycle reporting) is not trivial. The removed comments were valuable for understanding the algorithm. Please consider restoring them to aid future maintenance.
- Add null checks for active workbench window/page - Optimize ASCII art generation to remove redundant node - Use enhanced for-loop in findConsole
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a great new feature for visualizing cyclic dependencies using the Eclipse console with ASCII art, which is a significant improvement over the previous dialog-based output. The changes also include refactoring of the cycle detection logic. My review focuses on improving the robustness and appearance of the new visualization, ensuring the UI interaction is thread-safe, and suggesting the restoration of some comments in the refactored code to maintain its clarity.
| List<String> cycle = cycleInfo.cycle; | ||
|
|
||
| int maxLen = 0; | ||
| for (String node : cycle) maxLen = Math.max(maxLen, node.length()); |
There was a problem hiding this comment.
This loop to find the maximum length can be expressed more concisely and efficiently using a Java Stream. This would also naturally handle the case of an empty list, although that's unlikely here.
| for (String node : cycle) maxLen = Math.max(maxLen, node.length()); | |
| int maxLen = cycle.stream().mapToInt(String::length).max().orElse(0); |
| sb.append(" |\n"); | ||
| sb.append(" | [").append(type).append("]\n"); | ||
| sb.append(" ^ (Loops back to start)\n"); | ||
| sb.append(" |______________________|\n"); |
There was a problem hiding this comment.
The ASCII art for the loop-back connection uses a hardcoded string (|______________________|). This will not scale visually if the plug-in names are very long, causing the box width to increase significantly. To make the visualization more robust and look better with varying name lengths, consider making this part of the drawing dynamic based on boxWidth.
| private void showConsoleView(IConsole myConsole) { | ||
| try { | ||
| org.eclipse.ui.IWorkbenchWindow window = PlatformUI.getWorkbench().getActiveWorkbenchWindow(); | ||
| if (window == null) { | ||
| Platform.getLog(getClass()).log(new Status(Status.WARNING, "com.vogella.ide.debugtools", "Could not open console view: no active window.")); | ||
| return; | ||
| } | ||
| IWorkbenchPage page = window.getActivePage(); | ||
| if (page == null) { | ||
| Platform.getLog(getClass()).log(new Status(Status.WARNING, "com.vogella.ide.debugtools", "Could not open console view: no active page.")); | ||
| return; | ||
| } | ||
| String id = IConsoleConstants.ID_CONSOLE_VIEW; | ||
| IConsoleView view = (IConsoleView) page.showView(id); | ||
| view.display(myConsole); | ||
| } catch (PartInitException e) { | ||
| // Log error if view cannot be opened, but don't fail the whole operation | ||
| Platform.getLog(getClass()).log(new Status(Status.WARNING, | ||
| "com.vogella.ide.debugtools", "Could not open console view", e)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This method interacts with the UI and must be called on the UI thread. While the current usage from the @Execute handler is safe, it's a good practice to make UI-interacting methods robust by wrapping their logic in PlatformUI.getWorkbench().getDisplay().asyncExec(). This prevents potential SWTExceptions if the method is ever called from a background thread in the future.
private void showConsoleView(IConsole myConsole) {
PlatformUI.getWorkbench().getDisplay().asyncExec(() -> {
try {
org.eclipse.ui.IWorkbenchWindow window = PlatformUI.getWorkbench().getActiveWorkbenchWindow();
if (window == null) {
Platform.getLog(getClass()).log(new Status(Status.WARNING, "com.vogella.ide.debugtools", "Could not open console view: no active window."));
return;
}
IWorkbenchPage page = window.getActivePage();
if (page == null) {
Platform.getLog(getClass()).log(new Status(Status.WARNING, "com.vogella.ide.debugtools", "Could not open console view: no active page."));
return;
}
String id = IConsoleConstants.ID_CONSOLE_VIEW;
IConsoleView view = (IConsoleView) page.showView(id);
view.display(myConsole);
} catch (PartInitException e) {
// Log error if view cannot be opened, but don't fail the whole operation
Platform.getLog(getClass()).log(new Status(Status.WARNING,
"com.vogella.ide.debugtools", "Could not open console view", e));
}
});
}| CycleInfo cycle = extractCycle(node, dep, edge); | ||
| if (!isDuplicateCycle(cycle)) { | ||
| cycles.add(cycle); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| recursionStack.remove(node); | ||
| } | ||
|
|
||
| private CycleInfo extractCycle(String current, String cycleStart, DependencyEdge finalEdge) { | ||
| LinkedList<String> path = new LinkedList<>(); | ||
| path.addFirst(current); // Start with the node where recursion found cycle | ||
|
|
||
| // Reconstruct path from 'current' back to 'cycleStart' | ||
| path.addFirst(current); | ||
| String node = current; | ||
| while (!node.equals(cycleStart)) { | ||
| node = parent.get(node); | ||
| path.addFirst(node); | ||
| } | ||
| // Now 'path' is [cycleStart, ..., current] | ||
|
|
||
| // Create cycle list for CycleInfo: [cycleStart, ..., current, cycleStart] | ||
| List<String> cycleList = new ArrayList<>(path); | ||
| cycleList.add(cycleStart); // Close the cycle | ||
|
|
||
| cycleList.add(cycleStart); | ||
| CycleInfo cycleInfo = new CycleInfo(cycleList); | ||
|
|
||
| // Populate edge types for the cycle | ||
| // Edges from cycleStart to current | ||
| for (int i = 0; i < path.size() - 1; i++) { | ||
| String from = path.get(i); | ||
| String to = path.get(i + 1); | ||
| // The edge that leads to 'to' from 'from' | ||
| DependencyEdge edge = parentEdge.get(to); | ||
| cycleInfo.addEdge(from, to, edge.type); | ||
| } | ||
|
|
||
| // The final edge from 'current' back to 'cycleStart' | ||
| cycleInfo.addEdge(current, cycleStart, finalEdge.type); | ||
|
|
||
| return cycleInfo; | ||
| } | ||
|
|
||
| private boolean isDuplicateCycle(CycleInfo newCycleInfo) { | ||
| List<String> normalized = normalizeCycle(newCycleInfo.cycle); | ||
|
|
||
| for (CycleInfo existingCycleInfo : cycles) { | ||
| List<String> normalizedExisting = normalizeCycle(existingCycleInfo.cycle); | ||
| if (normalized.equals(normalizedExisting)) { | ||
| return true; | ||
| } | ||
| if (normalized.equals(normalizedExisting)) return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| private List<String> normalizeCycle(List<String> cycle) { | ||
| if (cycle.size() <= 1) return new ArrayList<>(cycle); | ||
|
|
||
| // Remove the duplicate last element for comparison | ||
| List<String> temp = new ArrayList<>(cycle.subList(0, cycle.size() - 1)); | ||
|
|
||
| // Find the minimum element | ||
| int minIndex = 0; | ||
| for (int i = 1; i < temp.size(); i++) { | ||
| if (temp.get(i).compareTo(temp.get(minIndex)) < 0) { | ||
| minIndex = i; | ||
| } | ||
| if (temp.get(i).compareTo(temp.get(minIndex)) < 0) minIndex = i; | ||
| } | ||
|
|
||
| // Rotate to start with minimum element | ||
| List<String> normalized = new ArrayList<>(); | ||
| for (int i = 0; i < temp.size(); i++) { | ||
| normalized.add(temp.get((minIndex + i) % temp.size())); | ||
| } | ||
| normalized.add(normalized.get(0)); // Add back the duplicate to complete the cycle | ||
|
|
||
| normalized.add(normalized.get(0)); | ||
| return normalized; | ||
| } |
There was a problem hiding this comment.
In several methods within CyclicDependencyDetector, such as extractCycle and normalizeCycle, helpful explanatory comments have been removed. This code performs complex graph traversal and data manipulation, and the comments were important for understanding the algorithms used for cycle detection, extraction, and normalization. Their removal reduces the maintainability of the code. Please consider restoring them to help other developers (and your future self) understand the logic more easily.
No description provided.