Editing Accessibility Features #48
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 resolves an 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 effectively addresses the scheduling rule conflicts by replacing IResource.exists() with the lighter-weight isAccessible() method, which avoids acquiring workspace locks. The added null checks and accessibility checks in ImageHover, ImageHyperlinkDetector, IncludeHyperlinkDetector, and LinkHyperlinkDetector significantly improve the robustness of the code and prevent potential conflicts with Eclipse's auto-build process.
The changes are well-implemented and directly address the described issue. I've added a couple of minor suggestions to improve maintainability by using constants instead of hardcoded strings.
Additionally, I've identified a critical issue outside of the modified lines: the getParentFolder() method, which is duplicated across ImageHover.java, ImageHyperlinkDetector.java, and IncludeHyperlinkDetector.java, is susceptible to a NullPointerException if editorInput.getAdapter(IResource.class) returns null. The version of this method in LinkHyperlinkDetector.java correctly handles this case. I strongly recommend refactoring this duplicated method into a shared utility and applying the fix everywhere to prevent potential crashes.
| IContainer imgFolder = parent.getFolder(IPath.fromOSString("img")); | ||
| if (!imgFolder.isAccessible()) { | ||
| return "Image folder 'img' not found"; | ||
| } |
There was a problem hiding this comment.
To improve maintainability and avoid hardcoded strings, you should use the IMAGE_DIRECTORY constant, which is already defined in this class, for both the folder name and in the error message.
| IContainer imgFolder = parent.getFolder(IPath.fromOSString("img")); | |
| if (!imgFolder.isAccessible()) { | |
| return "Image folder 'img' not found"; | |
| } | |
| IContainer imgFolder = parent.getFolder(IPath.fromOSString(IMAGE_DIRECTORY)); | |
| if (!imgFolder.isAccessible()) { | |
| return "Image folder '" + IMAGE_DIRECTORY + "' not found"; | |
| } |
| return null; | ||
| } | ||
|
|
||
| IContainer imgFolder = parent.getFolder(IPath.fromOSString("img")); |
There was a problem hiding this comment.
The string "img" is hardcoded. It would be better to define it as a constant to improve maintainability, for example: private static final String IMAGE_DIRECTORY = "img";. This would also make it consistent with ImageHover.java, which already has such a constant.
Fixed scheduling rule conflicts that occurred when editing AsciiDoc files while Eclipse auto-build was running. The error manifested as: "IllegalArgumentException: Attempted to beginRule: MultiRule[], does not match outer scope rule: R/" Changes: - Replace IResource.exists() with isAccessible() to avoid acquiring locks - Add null checks before accessing parent containers - Add accessibility checks for image folders and include paths - Improve error handling in hover and hyperlink detectors Affected components: - IncludeHyperlinkDetector: Check parent accessibility before members() - ImageHyperlinkDetector: Validate parent and img folder accessibility - ImageHover: Use isAccessible() instead of exists() for image files - LinkHyperlinkDetector: Check file accessibility for internal links The isAccessible() method is a lighter-weight check that doesn't acquire workspace locks, preventing conflicts with the build manager's root scheduling rule.
…handlers Improved code maintainability by eliminating hardcoded "img" strings: - ImageHover: Use existing IMAGE_DIRECTORY constant for folder path and error message - ImageHyperlinkDetector: Add IMAGE_DIRECTORY constant and use it consistently This makes the code more maintainable and ensures consistency across both classes that handle image directory references. Addresses code review feedback from automated analysis.
vogella
force-pushed
the
claude/editing-as-018CXmj1ybQZn5fYJxrwDgNe
branch
from
fa2982f to
34a75ee
Compare
- Refactored the duplicated `getParentFolder()` method into a shared utility class `AsciiDocResourceUtil` to improve maintainability and prevent potential `NullPointerException`s. - Replaced hardcoded strings with constants from the new `AsciiDocConstants` class. - Updated `ImageHover`, `ImageHyperlinkDetector`, `IncludeHyperlinkDetector`, and `LinkHyperlinkDetector` to use the new utility class and constants.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the scheduling rule conflicts by replacing IResource.exists() with the non-locking isAccessible() method, which is the correct approach to prevent issues with Eclipse's auto-build process. The refactoring of duplicated code into the AsciiDocResourceUtil utility class is a good step towards better maintainability. I've identified a potential threading issue in the new utility class, inherited from the original implementation, and provided a suggestion to ensure it is thread-safe. Overall, these are solid improvements to the editor's stability and code quality.
| IEclipseContext context = PlatformUI.getWorkbench().getService(IEclipseContext.class); | ||
| Object object = context.get("activeEditor"); | ||
|
|
||
| if (object instanceof IEditorPart activeEditor) { | ||
| IEditorInput editorInput = activeEditor.getEditorInput(); | ||
| IResource adapter = editorInput.getAdapter(IResource.class); | ||
| if (adapter != null) { | ||
| return adapter.getParent(); | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
The implementation of getParentFolder uses PlatformUI.getWorkbench(), which is unsafe to call from a non-UI thread. Hyperlink detectors and hover providers are often executed on background threads to keep the UI responsive. Calling this method from a background thread will cause an IllegalStateException. To fix this, the UI-dependent code should be wrapped in Display.getDefault().syncExec() to ensure it runs on the UI thread, preventing potential crashes.
final java.util.concurrent.atomic.AtomicReference<IContainer> parent = new java.util.concurrent.atomic.AtomicReference<>();
org.eclipse.swt.widgets.Display.getDefault().syncExec(() -> {
if (!PlatformUI.isWorkbenchRunning()) {
return;
}
IEclipseContext context = PlatformUI.getWorkbench().getService(IEclipseContext.class);
if (context == null) {
return;
}
Object object = context.get("activeEditor");
if (object instanceof IEditorPart activeEditor) {
IEditorInput editorInput = activeEditor.getEditorInput();
IResource adapter = editorInput.getAdapter(IResource.class);
if (adapter != null) {
parent.set(adapter.getParent());
}
}
});
return parent.get();
|
I have addressed the feedback and updated the PR. |
Fixed scheduling rule conflicts that occurred when editing AsciiDoc files while Eclipse auto-build was running. The error manifested as: "IllegalArgumentException: Attempted to beginRule: MultiRule[], does not match outer scope rule: R/"
Changes:
Affected components:
The isAccessible() method is a lighter-weight check that doesn't acquire workspace locks, preventing conflicts with the build manager's root scheduling rule.