From 94a144f1b3193f5f4050ce03131286f483a2a3b7 Mon Sep 17 00:00:00 2001 From: Sebastiano Poggi Date: Thu, 19 Oct 2023 15:46:56 +0200 Subject: [PATCH] Handle SVG loading exceptions and path patching issues in IJ 233 EAP 4 (#191) * WIP * Bump IJ 233 version to 233.9802.14-EAP-SNAPSHOT (EAP 4) * Remove unused IconsPathPatching code * Update code after changes in main * Make loading fallback consistent for all resource types Also document the behaviour in the KDoc * Prevent icon loading from crashing Plus, some IDE sample code cleanup --------- Co-authored-by: Lamberto Basti --- buildSrc/src/main/kotlin/jewel.gradle.kts | 1 + .../jewel/painter/PainterProvider.kt | 15 +- .../jewel/painter/ResourcePainterProvider.kt | 141 ++++++++++++------ .../kotlin/org/jetbrains/jewel/util/Debug.kt | 4 + gradle/libs.versions.toml | 2 +- .../jewel/bridge/IconsPathPatching.kt | 29 ---- .../jetbrains/jewel/bridge/BridgeOverride.kt | 35 +++++ .../jewel/bridge/IconsPathPatching.kt | 29 ---- samples/ide-plugin/build.gradle.kts | 2 +- .../samples/ideplugin/ComponentShowcaseTab.kt | 18 ++- 10 files changed, 162 insertions(+), 114 deletions(-) delete mode 100644 ide-laf-bridge/ide-laf-bridge-232/src/main/kotlin/org/jetbrains/jewel/bridge/IconsPathPatching.kt delete mode 100644 ide-laf-bridge/ide-laf-bridge-233/src/main/kotlin/org/jetbrains/jewel/bridge/IconsPathPatching.kt diff --git a/buildSrc/src/main/kotlin/jewel.gradle.kts b/buildSrc/src/main/kotlin/jewel.gradle.kts index e6fc51148..9760b645f 100644 --- a/buildSrc/src/main/kotlin/jewel.gradle.kts +++ b/buildSrc/src/main/kotlin/jewel.gradle.kts @@ -37,6 +37,7 @@ kotlin { optIn("androidx.compose.ui.ExperimentalComposeUiApi") optIn("androidx.compose.foundation.ExperimentalFoundationApi") optIn("org.jetbrains.jewel.ExperimentalJewelApi") + optIn("org.jetbrains.jewel.InternalJewelApi") } } } diff --git a/core/src/main/kotlin/org/jetbrains/jewel/painter/PainterProvider.kt b/core/src/main/kotlin/org/jetbrains/jewel/painter/PainterProvider.kt index bcc62bf30..23d22b70f 100644 --- a/core/src/main/kotlin/org/jetbrains/jewel/painter/PainterProvider.kt +++ b/core/src/main/kotlin/org/jetbrains/jewel/painter/PainterProvider.kt @@ -6,9 +6,13 @@ import androidx.compose.runtime.State import androidx.compose.ui.graphics.painter.Painter /** - * Implementations of this interface should handle the passed [PainterHint]s correctly. - * For now, this means calling [PainterPathHint.patch] and [PainterSvgPatchHint.patch]. - * Most likely, a [PainterProvider] should also hold the resource path and [ClassLoader] + * Provides a [Painter] for an image, which may be transformed by the + * provided hints. + * + * Note: implementations of this interface should handle the passed + * [PainterHint]s correctly. For now, this means calling + * [PainterPathHint.patch] and [PainterSvgPatchHint.patch]. Most likely, a + * [PainterProvider] should also hold the resource path and [ClassLoader] * references. */ @Immutable @@ -16,6 +20,11 @@ interface PainterProvider { /** * Provides a [Painter] using the specified [PainterHint]s. + * The painters are [remember][androidx.compose.runtime.remember]ed + * and this function can be called multiple times for the same data. + * + * Depending on the implementation, errors may be suppressed and a + * replacement painter provided. */ @Composable fun getPainter(vararg hints: PainterHint): State diff --git a/core/src/main/kotlin/org/jetbrains/jewel/painter/ResourcePainterProvider.kt b/core/src/main/kotlin/org/jetbrains/jewel/painter/ResourcePainterProvider.kt index 0056fe4eb..dea2af98c 100644 --- a/core/src/main/kotlin/org/jetbrains/jewel/painter/ResourcePainterProvider.kt +++ b/core/src/main/kotlin/org/jetbrains/jewel/painter/ResourcePainterProvider.kt @@ -5,13 +5,16 @@ import androidx.compose.runtime.Immutable import androidx.compose.runtime.State import androidx.compose.runtime.remember import androidx.compose.runtime.rememberUpdatedState +import androidx.compose.ui.graphics.Color import androidx.compose.ui.graphics.painter.BitmapPainter +import androidx.compose.ui.graphics.painter.ColorPainter import androidx.compose.ui.graphics.painter.Painter import androidx.compose.ui.graphics.vector.rememberVectorPainter import androidx.compose.ui.platform.LocalDensity import androidx.compose.ui.res.loadImageBitmap import androidx.compose.ui.res.loadSvgPainter import androidx.compose.ui.res.loadXmlImageVector +import androidx.compose.ui.unit.Density import org.jetbrains.jewel.util.inDebugMode import org.w3c.dom.Document import org.xml.sax.InputSource @@ -29,10 +32,18 @@ import javax.xml.transform.TransformerFactory import javax.xml.transform.dom.DOMSource import javax.xml.transform.stream.StreamResult +private val errorPainter = ColorPainter(Color.Magenta) + /** - * Provide [Painter] by resources in the module and jars, it use the ResourceResolver to load resources. + * Provide [Painter] by resources in the module and jars, it use the + * ResourceResolver to load resources. + * + * It will cache the painter by [PainterHint]s, so it is safe to call + * [getPainter] multiple times. * - * It will cache the painter by [PainterHint]s, so it is safe to call [getPainter] multiple times. + * If a resource fails to load, it will be silently replaced by a + * magenta color painter, and the exception logged. If Jewel is in + * [debug mode][inDebugMode], however, exceptions will not be suppressed. */ @Immutable class ResourcePainterProvider( @@ -70,56 +81,40 @@ class ResourcePainterProvider( @Composable private fun loadPainter(hints: List): Painter { - val format = basePath.substringAfterLast(".").lowercase() - val pathStack = buildSet { var path = basePath add(path) - hints.forEach { - path = when (it) { - is PainterResourcePathHint -> it.patch(path, contextClassLoaders) - is PainterPathHint -> it.patch(path) - else -> return@forEach + for (hint in hints) { + path = when (hint) { + is PainterResourcePathHint -> hint.patch(path, contextClassLoaders) + is PainterPathHint -> hint.patch(path) + else -> continue } add(path) } }.reversed() - val url = pathStack.firstNotNullOfOrNull { - resolveResource(it) - } ?: error("Resource '$basePath(${hints.joinToString()})' not found") - - val density = LocalDensity.current + val url = pathStack.firstNotNullOfOrNull { resolveResource(it) } - return when (format) { - "svg" -> { - remember(url, density, hints) { - patchSvg(url.openStream(), hints).use { - if (inDebugMode) { - println("Load icon $basePath(${hints.joinToString()}) from $url") - } - loadSvgPainter(it, density) - } - } + @Suppress("UrlHashCode") // It's ok when comparing a URL to null + if (url == null) { + if (inDebugMode) { + error("Resource '$basePath(${hints.joinToString()})' not found") + } else { + return errorPainter } + } - "xml" -> { - val vector = url.openStream().use { - loadXmlImageVector(InputSource(it), density) - } - rememberVectorPainter(vector) - } + val density = LocalDensity.current - else -> { - remember(url, density) { - val bitmap = url.openStream().use { - loadImageBitmap(it) - } - BitmapPainter(bitmap) - } - } + val extension = basePath.substringAfterLast(".").lowercase() + + return when (extension) { + "svg" -> createSvgPainter(url, density, hints) + "xml" -> createVectorDrawablePainter(url, density) + else -> createBitmapPainter(url, density) } } @@ -137,6 +132,21 @@ class ResourcePainterProvider( return null } + @Composable + private fun createSvgPainter(url: URL, density: Density, hints: List): Painter = + tryLoadingResource( + url = url, + loadingAction = { resourceUrl -> + patchSvg(url.openStream(), hints).use { inputStream -> + if (inDebugMode) { + println("Loading icon $basePath(${hints.joinToString()}) from $resourceUrl") + } + loadSvgPainter(inputStream, density) + } + }, + rememberAction = { remember(url, density, hints) { it } }, + ) + private fun patchSvg(inputStream: InputStream, hints: List): InputStream { if (hints.all { it !is PainterSvgPatchHint }) { return inputStream @@ -155,6 +165,31 @@ class ResourcePainterProvider( } } + @Composable + private fun createVectorDrawablePainter(url: URL, density: Density): Painter = + tryLoadingResource( + url = url, + loadingAction = { resourceUrl -> + resourceUrl.openStream().use { + loadXmlImageVector(InputSource(it), density) + } + }, + rememberAction = { rememberVectorPainter(it) }, + ) + + @Composable + private fun createBitmapPainter(url: URL, density: Density) = + tryLoadingResource( + url = url, + loadingAction = { resourceUrl -> + val bitmap = resourceUrl.openStream().use { + loadImageBitmap(it) + } + BitmapPainter(bitmap) + }, + rememberAction = { remember(url, density) { it } }, + ) + private fun Document.writeToString(): String { val tf = TransformerFactory.newInstance() val transformer: Transformer @@ -172,14 +207,32 @@ class ResourcePainterProvider( error("Unable to render XML document to string: ${e.message}") } } + + @Composable + private fun tryLoadingResource( + url: URL, + loadingAction: (URL) -> T, + rememberAction: @Composable (T) -> Painter, + ): Painter { + @Suppress("TooGenericExceptionCaught") // This is a last-resort fallback when icons fail to load + val painter = try { + loadingAction(url) + } catch (e: RuntimeException) { + val message = "Unable to load SVG resource from $url\n${e.stackTraceToString()}" + if (inDebugMode) { + error(message) + } + + System.err.println(message) + return errorPainter + } + + return rememberAction(painter) + } } @Composable -fun rememberResourcePainterProvider( - path: String, - iconClass: Class<*>, -): PainterProvider { - return remember(path, iconClass.classLoader) { +fun rememberResourcePainterProvider(path: String, iconClass: Class<*>): PainterProvider = + remember(path, iconClass.classLoader) { ResourcePainterProvider(path, iconClass.classLoader) } -} diff --git a/core/src/main/kotlin/org/jetbrains/jewel/util/Debug.kt b/core/src/main/kotlin/org/jetbrains/jewel/util/Debug.kt index 80124508a..541821dbd 100644 --- a/core/src/main/kotlin/org/jetbrains/jewel/util/Debug.kt +++ b/core/src/main/kotlin/org/jetbrains/jewel/util/Debug.kt @@ -2,6 +2,10 @@ package org.jetbrains.jewel.util import org.jetbrains.jewel.InternalJewelApi +/** + * Determines whether we're in debug mode. This should not be used + * in the bridge for logging; instead, you should use the IDE logger. + */ @InternalJewelApi val inDebugMode by lazy { System.getProperty("org.jetbrains.jewel.debug")?.toBoolean() ?: false diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index b88ef3b08..b98c9ff9f 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -3,7 +3,7 @@ composeDesktop = "1.5.2" coroutines = "1.7.3" detekt = "1.23.1" idea232 = "232.8660.185" -idea233 = "233.9102.97-EAP-SNAPSHOT" +idea233 = "233.9802.14-EAP-SNAPSHOT" ideaGradlePlugin = "1.15.0" javaSarif = "2.0" kotlinSarif = "0.4.0" diff --git a/ide-laf-bridge/ide-laf-bridge-232/src/main/kotlin/org/jetbrains/jewel/bridge/IconsPathPatching.kt b/ide-laf-bridge/ide-laf-bridge-232/src/main/kotlin/org/jetbrains/jewel/bridge/IconsPathPatching.kt deleted file mode 100644 index dea03a894..000000000 --- a/ide-laf-bridge/ide-laf-bridge-232/src/main/kotlin/org/jetbrains/jewel/bridge/IconsPathPatching.kt +++ /dev/null @@ -1,29 +0,0 @@ -package org.jetbrains.jewel.bridge - -import com.intellij.util.ui.DirProvider -import org.jetbrains.jewel.InternalJewelApi - -@InternalJewelApi -fun getPatchedIconPath( - dirProvider: DirProvider, - originalPath: String, - classLoaders: List, -): String? { - val clazz = Class.forName("com.intellij.ui.icons.CachedImageIconKt") - val patchIconPath = clazz.getMethod("patchIconPath", String::class.java, ClassLoader::class.java) - patchIconPath.isAccessible = true - - // For all provided classloaders, we try to get the patched path, both using - // the original path, and an "abridged" path that has gotten the icon path prefix - // removed (the classloader is set up differently in prod IDEs and when running - // from Gradle, and the icon could be in either place depending on the environment) - val fallbackPath = originalPath.removePrefix(dirProvider.dir()) - val patchedPath = classLoaders.firstNotNullOfOrNull { classLoader -> - val patchedPathAndClassLoader = - patchIconPath.invoke(null, originalPath.removePrefix("/"), classLoader) - ?: patchIconPath.invoke(null, fallbackPath, classLoader) - patchedPathAndClassLoader as? Pair<*, *> - }?.first as? String - - return patchedPath -} diff --git a/ide-laf-bridge/ide-laf-bridge-233/src/main/kotlin/org/jetbrains/jewel/bridge/BridgeOverride.kt b/ide-laf-bridge/ide-laf-bridge-233/src/main/kotlin/org/jetbrains/jewel/bridge/BridgeOverride.kt index 6c1528ab0..24f76fa2d 100644 --- a/ide-laf-bridge/ide-laf-bridge-233/src/main/kotlin/org/jetbrains/jewel/bridge/BridgeOverride.kt +++ b/ide-laf-bridge/ide-laf-bridge-233/src/main/kotlin/org/jetbrains/jewel/bridge/BridgeOverride.kt @@ -1,5 +1,6 @@ package org.jetbrains.jewel.bridge +import com.intellij.openapi.diagnostic.Logger import com.intellij.ui.icons.patchIconPath import com.intellij.util.ui.DirProvider import org.jetbrains.jewel.painter.PainterResourcePathHint @@ -8,6 +9,7 @@ internal object BridgeOverride : PainterResourcePathHint { private val dirProvider = DirProvider() + @Suppress("UnstableApiUsage") // patchIconPath() is explicitly open to us override fun patch(path: String, classLoaders: List): String { // For all provided classloaders, we try to get the patched path, both using // the original path, and an "abridged" path that has gotten the icon path prefix @@ -19,6 +21,14 @@ internal object BridgeOverride : PainterResourcePathHint { val patchedPath = patchIconPath(path.removePrefix("/"), classLoader)?.first ?: patchIconPath(fallbackPath, classLoader)?.first + // 233 EAP 4 broke path patching horribly; now it can return a + // "reflective path", which is a FQN to an ExpUIIcons entry. + // As a (hopefully) temporary solution, we undo this transformation + // back into the original path. + if (patchedPath?.startsWith("com.intellij.icons.ExpUiIcons") == true) { + return inferActualPathFromReflectivePath(patchedPath) + } + if (patchedPath != null) { return patchedPath } @@ -26,5 +36,30 @@ internal object BridgeOverride : PainterResourcePathHint { return path } + private fun inferActualPathFromReflectivePath(patchedPath: String): String { + val iconPath = patchedPath.removePrefix("com.intellij.icons.ExpUiIcons.") + + return buildString { + append("expui/") + iconPath.split('.') + .map { it.trim() } + .filter { it.isNotEmpty() } + .forEach { + append(it.first().lowercaseChar()) + append(it.drop(1)) + append('/') + } + replace(length - 1, length, "") + if (iconPath.contains("_dark")) append("_dark") + append(".svg") + + Logger.getInstance("IconsPathPatching") + .warn( + "IntelliJ returned a reflective path: $patchedPath for $iconPath." + + " We reverted that to a plausible-looking resource path: ${toString()}", + ) + } + } + override fun toString(): String = "BridgeOverride" } diff --git a/ide-laf-bridge/ide-laf-bridge-233/src/main/kotlin/org/jetbrains/jewel/bridge/IconsPathPatching.kt b/ide-laf-bridge/ide-laf-bridge-233/src/main/kotlin/org/jetbrains/jewel/bridge/IconsPathPatching.kt deleted file mode 100644 index 52aba2360..000000000 --- a/ide-laf-bridge/ide-laf-bridge-233/src/main/kotlin/org/jetbrains/jewel/bridge/IconsPathPatching.kt +++ /dev/null @@ -1,29 +0,0 @@ -package org.jetbrains.jewel.bridge - -import com.intellij.ui.icons.patchIconPath -import com.intellij.util.ui.DirProvider -import org.jetbrains.jewel.InternalJewelApi - -@InternalJewelApi -@Suppress("UnstableApiUsage") -fun getPatchedIconPath( - dirProvider: DirProvider, - originalPath: String, - classLoaders: List, -): String? { - // For all provided classloaders, we try to get the patched path, both using - // the original path, and an "abridged" path that has gotten the icon path prefix - // removed (the classloader is set up differently in prod IDEs and when running - // from Gradle, and the icon could be in either place depending on the environment) - val fallbackPath = originalPath.removePrefix(dirProvider.dir()) - - for (classLoader in classLoaders) { - val patchedPath = patchIconPath(originalPath.removePrefix("/"), classLoader)?.first - ?: patchIconPath(fallbackPath, classLoader)?.first - - if (patchedPath != null) { - return patchedPath - } - } - return null -} diff --git a/samples/ide-plugin/build.gradle.kts b/samples/ide-plugin/build.gradle.kts index 81a57cb8c..b4b6ea0fa 100644 --- a/samples/ide-plugin/build.gradle.kts +++ b/samples/ide-plugin/build.gradle.kts @@ -45,6 +45,6 @@ tasks { } runIde { - this.systemProperties["org.jetbrains.jewel.debug"] = "true" + systemProperties["org.jetbrains.jewel.debug"] = "true" } } diff --git a/samples/ide-plugin/src/main/kotlin/org/jetbrains/jewel/samples/ideplugin/ComponentShowcaseTab.kt b/samples/ide-plugin/src/main/kotlin/org/jetbrains/jewel/samples/ideplugin/ComponentShowcaseTab.kt index ca3286e48..c1df6e06e 100644 --- a/samples/ide-plugin/src/main/kotlin/org/jetbrains/jewel/samples/ideplugin/ComponentShowcaseTab.kt +++ b/samples/ide-plugin/src/main/kotlin/org/jetbrains/jewel/samples/ideplugin/ComponentShowcaseTab.kt @@ -111,28 +111,32 @@ import org.jetbrains.jewel.intui.standalone.IntUiTheme } } - Row { + Row(horizontalArrangement = Arrangement.spacedBy(16.dp)) { Icon( "actions/close.svg", iconClass = AllIcons::class.java, modifier = Modifier.border(1.dp, Color.Magenta), contentDescription = "An icon", ) - } - Row { IconButton(onClick = { }) { Icon("actions/close.svg", contentDescription = "An icon", AllIcons::class.java) } } - Row { - Text("Circular progress small: ") + Row( + verticalAlignment = Alignment.CenterVertically, + horizontalArrangement = Arrangement.spacedBy(8.dp), + ) { + Text("Circular progress small:") CircularProgressIndicator() } - Row(verticalAlignment = Alignment.CenterVertically) { - Text("Circular progress big: ") + Row( + verticalAlignment = Alignment.CenterVertically, + horizontalArrangement = Arrangement.spacedBy(8.dp), + ) { + Text("Circular progress big:") CircularProgressIndicatorBig() }