Parallel start example#64
Conversation
![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 upgrades the project to Java 25 and introduces two new plugins: com.vogella.ide.iconreplacer for dynamic icon patching and com.vogella.ide.parallelstart to optimize startup performance. It also refactors the AsciiDoc editor by removing legacy components and enhancing the Language Server Protocol (LSP) implementation with improved sorting and link resolution. Feedback highlights a critical bug in the icon patching logic where subsequent updates overwrite previous ones, and recommends using FileLocator for more robust bundle file access. Additionally, improvements are suggested for logging practices and broadening relative path support in document links.
| } | ||
|
|
||
| try (InputStream patched = rebuildJar(bundle, iconPath, replacement)) { | ||
| bundle.update(patched); |
There was a problem hiding this comment.
There is a critical bug in the update logic. bundle.update() is called for every single icon replacement. Because rebuildJar reads from bundle.getLocation() (which points to the original installation source), each subsequent update will overwrite the previous one. If a bundle has multiple icons mapped, only the last one processed will remain; all others will be reverted to their original state from the source JAR. You must group all replacements for a single bundle and apply them in a single rebuildJar and bundle.update() operation.
| throws Exception { | ||
| ByteArrayOutputStream buffer = new ByteArrayOutputStream(); | ||
| try (ZipOutputStream zout = new ZipOutputStream(buffer)) { | ||
| try (ZipInputStream zin = new ZipInputStream(new URL(bundle.getLocation()).openStream())) { |
There was a problem hiding this comment.
Using new URL(bundle.getLocation()) is unreliable for two reasons:
bundle.getLocation()often returns strings with OSGi-specific prefixes likereference:file:, which will causenew URL()to throw aMalformedURLExceptionif the protocol handler is not registered globally.- If the bundle is installed as a directory (unpacked),
openStream()will not provide a ZIP stream, causingZipInputStreamto fail.
A more robust approach in Eclipse is to useorg.eclipse.core.runtime.FileLocator.getBundleFile(bundle)to get the actual file or directory on disk.
|
|
||
| for (Map.Entry<String, List<String>> entry : mapping.entrySet()) { | ||
| String iconFileName = entry.getKey(); | ||
| URL replacement = iconFolderUrl.toURI().resolve(iconFileName).toURL(); |
There was a problem hiding this comment.
Using toURI() on OSGi-specific URL protocols (like bundleentry or bundleresource) can fail because they may not be strictly compatible with the java.net.URI specification. Since iconFolderUrl is obtained via Bundle.getEntry(), it is safer to use the URL(URL context, String spec) constructor to resolve the replacement icon path, even though it is deprecated in newer Java versions, as it correctly handles custom protocol resolution in OSGi.
| URL replacement = iconFolderUrl.toURI().resolve(iconFileName).toURL(); | |
| URL replacement = new URL(iconFolderUrl, iconFileName); |
| System.out.println("ParallelStartConfigurator: marked " + count | ||
| + " bundles for parallel activation"); |
There was a problem hiding this comment.
Avoid using System.out.println for logging in Eclipse plugins. It is better practice to use the platform logging facility (Platform.getLog(getClass())) or a dedicated logging framework to ensure messages are properly captured in the error log and can be managed by the user.
| if (path.startsWith("http://") || path.startsWith("https://")) { | ||
| // External link - use URL directly | ||
| targetUri = path; | ||
| } else if (path.startsWith("./") || path.startsWith("../")) { |
There was a problem hiding this comment.
The check path.startsWith("./") || path.startsWith("../") is too restrictive for internal file links. In AsciiDoc, a link can be a simple relative path without these prefixes (e.g., link:sub/file.adoc[Label]). Since resolveFileLocation already handles relative path resolution correctly, this explicit check should be removed to support all valid relative links.
| } else if (path.startsWith("./") || path.startsWith("../")) { | |
| } else { |
vogella
force-pushed
the
parallel-start-example
branch
from
b3ad251 to
ca38db8
Compare
- Introduced com.vogella.ide.iconreplacer module for dynamic icon replacement - Added iconreplacer to parent pom.xml and updatesite category.xml - Included documentation in icon-replacement-plugin.md - Cleaned up manual Asciidoc test files (Help.adoc, Testing.adoc) - Updated AsciidocCompletionTest.java to use assertNull where appropriate
BundlePatcher: - Group all icon replacements per bundle to apply them in a single rebuildJar/update cycle (fixes bug where only the last replacement survived per bundle) - Use FileLocator.getBundleFileLocation() instead of new URL(bundle.getLocation()) to handle OSGi-specific location prefixes (reference:file:, etc.) - Use URL(context, spec) constructor for resolving icon folder URLs to avoid issues with OSGi-specific URL protocols and toURI() ParallelStartConfigurator: - Replace System.out.println with java.util.logging.Logger AsciidocTextDocumentService: - Remove overly restrictive ./ and ../ prefix check for internal file links, allowing all relative paths to resolve Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
vogella
force-pushed
the
parallel-start-example
branch
from
ca38db8 to
63cc85a
Compare
1 participant
No description provided.