-
Notifications
You must be signed in to change notification settings - Fork 5.6k
[JEWEL] IJPL-176416 Compose icon infrastructure #3217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
import org.jetbrains.annotations.NotNull; | ||
import org.jetbrains.annotations.Nullable; | ||
|
||
import javax.swing.Icon; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto |
||
import javax.swing.*; | ||
import java.awt.*; | ||
import java.util.Objects; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ import java.util.* | |
import java.util.concurrent.ConcurrentHashMap | ||
import java.util.function.Supplier | ||
import java.util.function.ToIntFunction | ||
import javax.swing.Icon | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto |
||
import javax.swing.* | ||
import kotlin.math.abs | ||
import kotlin.math.max | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. | ||
import org.jetbrains.compose.ComposeBuildConfig | ||
|
||
plugins { | ||
jewel | ||
`jewel-check-public-api` | ||
alias(libs.plugins.composeDesktop) | ||
alias(libs.plugins.compose.compiler) | ||
} | ||
|
||
private val composeVersion | ||
get() = ComposeBuildConfig.composeVersion | ||
|
||
dependencies { | ||
api("org.jetbrains.compose.foundation:foundation-desktop:$composeVersion") | ||
|
||
testImplementation(compose.desktop.currentOs) { exclude(group = "org.jetbrains.compose.material") } | ||
} | ||
Comment on lines
+14
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't look like we need these. The iml only uses coroutines and kotlin stdlib (which in Gradle you can probably mark as |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<module type="JAVA_MODULE" version="4"> | ||
<component name="FacetManager"> | ||
<facet type="kotlin-language" name="Kotlin"> | ||
<configuration version="5" platform="JVM 17" allPlatforms="JVM [17]" useProjectSettings="false"> | ||
<compilerSettings> | ||
<option name="additionalArguments" value="-Xjvm-default=all" /> | ||
</compilerSettings> | ||
<compilerArguments> | ||
<stringArguments> | ||
<stringArg name="jvmTarget" arg="17" /> | ||
<stringArg name="apiVersion" arg="2.2" /> | ||
<stringArg name="languageVersion" arg="2.2" /> | ||
</stringArguments> | ||
</compilerArguments> | ||
</configuration> | ||
</facet> | ||
</component> | ||
<component name="NewModuleRootManager" inherit-compiler-output="true"> | ||
<exclude-output /> | ||
<content url="file://$MODULE_DIR$"> | ||
<sourceFolder url="file://$MODULE_DIR$/src/main/resources" type="java-resource" /> | ||
<sourceFolder url="file://$MODULE_DIR$/src/main/kotlin" isTestSource="false" /> | ||
</content> | ||
<orderEntry type="library" name="kotlin-stdlib" level="project" /> | ||
<orderEntry type="library" name="kotlinx-coroutines-core" level="project" /> | ||
<orderEntry type="sourceFolder" forTests="false" /> | ||
<orderEntry type="inheritedJdk" /> | ||
</component> | ||
</module> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. | ||
package org.jetbrains.icons.api | ||
|
||
import kotlinx.coroutines.flow.StateFlow | ||
|
||
interface DynamicIcon : Icon { | ||
val onUpdate: StateFlow<IconState> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the current impl of |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: missing empty line at EOF — will not repeat for all files, but there are several |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. | ||
package org.jetbrains.icons.api | ||
|
||
interface Icon { | ||
val identifier: IconIdentifier | ||
val state: IconState | ||
|
||
fun render(api: PaintingApi) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need to have fully abstract painting api?
The problem with the current implementation is that the same interface |
||
} |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,9 @@ | ||||||||||
// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. | ||||||||||
package org.jetbrains.icons.api | ||||||||||
|
||||||||||
interface IconManager { | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like it's only extended in the IJP by another interface, and in turn has an empty implementation. Do we need it? |
||||||||||
fun loadIcon(id: IconIdentifier, path: String, aClass: Class<*>): Icon? | ||||||||||
fun registerIcon(id: IconIdentifier, icon: Icon) | ||||||||||
Comment on lines
+5
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: for consistency
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||
} | ||||||||||
|
||||||||||
interface IconIdentifier | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One of the things we can solve with new icons API is getting rid of dependency on javax.swing from *.psi modules. It seems that However, to do that we probably need to put |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. | ||
package org.jetbrains.icons.api | ||
|
||
interface IconState | ||
|
||
object DefaultIconState : IconState |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,103 @@ | ||||||
// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. | ||||||
package org.jetbrains.icons.api | ||||||
|
||||||
import kotlin.reflect.KClass | ||||||
|
||||||
interface ImageResourceProvider { | ||||||
companion object { | ||||||
@Volatile | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this to be volatile? It may become clearer later why we do, but it is suspicious that we need this for now |
||||||
private var instance: ImageResourceProvider? = null | ||||||
|
||||||
@JvmStatic | ||||||
fun getInstance(): ImageResourceProvider = instance ?: DummyImageResourceProvider | ||||||
|
||||||
fun activate(provider: ImageResourceProvider) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be |
||||||
instance = provider | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
object DummyImageResourceProvider : ImageResourceProvider | ||||||
|
||||||
interface ImageResource { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should expose an intrinsic size |
||||||
companion object | ||||||
} | ||||||
|
||||||
interface ImageResourceWithCrossApiCache { | ||||||
val crossApiCache: CrossApiImageBitmapCache | ||||||
} | ||||||
|
||||||
inline fun <reified TBitmap : Any> CrossApiImageBitmapCache.cachedBitmap(noinline generator: () -> TBitmap): TBitmap = | ||||||
cachedBitmap(TBitmap::class, generator) | ||||||
|
||||||
interface CrossApiImageBitmapCache { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Considering there are no actual cross-API implementations (there is a Compose one, and there is an AWT one, but neither is cross-API), I'm not sure we actually really need this... It feels like it could be replaced by a |
||||||
fun <TBitmap : Any> cachedBitmap(bitmapClass: KClass<TBitmap>, generator: () -> TBitmap): TBitmap | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This name has me really confused. Is it a |
||||||
} | ||||||
|
||||||
interface BitmapImageResource : ImageResource { | ||||||
fun getRGB(x: Int, y: Int): Int | ||||||
fun getRGBOrNull(x: Int, y: Int): Int? | ||||||
Comment on lines
+38
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do these return RGB? Or ARGB? Or RGBA? It's unclear from the name. Usages seem to suggest ARGB, but... can you clarify? Also, why would we need |
||||||
val width: Int | ||||||
val height: Int | ||||||
} | ||||||
|
||||||
object EmptyBitmapImageResource : BitmapImageResource { | ||||||
override fun getRGB(x: Int, y: Int): Int = 0 | ||||||
override fun getRGBOrNull(x: Int, y: Int): Int? = 0 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this return |
||||||
override val width: Int = 0 | ||||||
override val height: Int = 0 | ||||||
} | ||||||
|
||||||
interface RescalableImageResource : ImageResource { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this signify that an image can be scaled losslessly? And that by contrast other I'm asking because that's a major gripe I have with the current IJP Swing icon APIs — all images are inherently scalable, the question is whether that's a lossless or lossy scaling. I would prefer being more precise/explicit in this new API if we can. I would imagine that if my interpretation is correct, then vector-based and runtime-generated icons are inherently losslessly scalable, whereas bitmaps are not. |
||||||
fun scale(scale: ImageScale): BitmapImageResource | ||||||
fun calculateExpectedDimensions(scale: ImageScale): Bounds | ||||||
} | ||||||
|
||||||
sealed interface ImageScale { | ||||||
fun calculateScalingFactorByOriginalDimensions(width: Int, height: Int? = null): Float | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a fan of the nullable
Suggested change
I'm also not sure what |
||||||
} | ||||||
|
||||||
class FixedScale(val scale: Float) : ImageScale { | ||||||
override fun calculateScalingFactorByOriginalDimensions(width: Int, height: Int?): Float = scale | ||||||
|
||||||
override fun equals(other: Any?): Boolean { | ||||||
if (this === other) return true | ||||||
if (javaClass != other?.javaClass) return false | ||||||
|
||||||
other as FixedScale | ||||||
|
||||||
return scale == other.scale | ||||||
} | ||||||
|
||||||
override fun hashCode(): Int { | ||||||
return scale.hashCode() | ||||||
} | ||||||
} | ||||||
|
||||||
class FitAreaScale(val width: Int, val height: Int) : ImageScale { | ||||||
override fun calculateScalingFactorByOriginalDimensions(width: Int, height: Int?): Float { | ||||||
val scale = this.width / width.toFloat() | ||||||
if (height != null) { | ||||||
return minOf(scale, this.height / height.toFloat()) | ||||||
} | ||||||
return scale | ||||||
} | ||||||
|
||||||
override fun equals(other: Any?): Boolean { | ||||||
if (this === other) return true | ||||||
if (javaClass != other?.javaClass) return false | ||||||
|
||||||
other as FitAreaScale | ||||||
|
||||||
if (width != other.width) return false | ||||||
if (height != other.height) return false | ||||||
|
||||||
return true | ||||||
} | ||||||
|
||||||
override fun hashCode(): Int { | ||||||
var result = width | ||||||
result = 31 * result + height | ||||||
return result | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
// Copyright 2000-2025 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. | ||
package org.jetbrains.icons.api | ||
|
||
interface PaintingApi { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this supposed to be an abstraction over I'd also consider calling this something like |
||
val bounds: Bounds | ||
fun drawImage(image: BitmapImageResource, x: Int, y: Int, width: Int? = null, height: Int? = null) | ||
fun drawImage(image: RescalableImageResource, x: Int, y: Int, width: Int? = null, height: Int? = null) | ||
Comment on lines
+6
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto here — can we use non-nullable values for width and height? Using the resource's intrinsic size as defaults looks better to me. |
||
} | ||
|
||
class Bounds( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we not using an AWT |
||
val width: Int, | ||
val height: Int | ||
) { | ||
override fun equals(other: Any?): Boolean { | ||
if (this === other) return true | ||
if (javaClass != other?.javaClass) return false | ||
|
||
other as Bounds | ||
|
||
if (width != other.width) return false | ||
if (height != other.height) return false | ||
|
||
return true | ||
} | ||
|
||
override fun hashCode(): Int { | ||
var result = width | ||
result = 31 * result + height | ||
return result | ||
} | ||
|
||
override fun toString(): String { | ||
return "Bounds(width=$width, height=$height)" | ||
} | ||
|
||
fun canFit(other: Bounds): Boolean = other.width <= width && other.height <= height | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd revert changes to this file