Skip to content
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

Fix 242 icon loading by providing new key-based API #430

Merged
merged 10 commits into from
Jul 4, 2024

Conversation

rock3r
Copy link
Collaborator

@rock3r rock3r commented Jul 3, 2024

In IJP 242 the icon loading has changed yet again. Instead of relying on icon mappings like it used to, now New UI icon paths are hardcoded into the CachedImageIcon themselves. This breaks Jewel's ability to load New UI icons, resulting in old UI icons being shown instead.

With this PR, we set to address this issue, and design an API that improves upon the existing ones. We have created the new IconKey, that encapsulates an icon resource path in a better typed and fully portable way allowing the same code to be used both in standalone and in bridge mode.

We have a new generation task in the ui module that traverses an AllIcons-style class and an icon mappings json file (like PlatfomIconMappings.json), and creates an icon keys file with the same structure. These keys are then used to look up icons, simplifying the code. This mechanism should work for 233 and 241 as well, since it bypasses the need for relying on the platform icon mapping, until we get better APIs, hopefully soon.

The Icon composable now accepts PainterHints and uses them when rendering the icons, too.

What used to look like this:

Icon(
  "actions/close.svg",
  iconClass = AllIcons::class.java,
  modifier = Modifier.border(1.dp, Color.Magenta),
  contentDescription = "An icon",
)

Icon(
  "icons/github.svg",
  iconClass = JewelIcons::class.java,
  modifier = Modifier.border(1.dp, Color.Magenta),
  contentDescription = "An icon",
)

Now looks like this:

PlatformIcon(AllIconsKeys.Actions.Close, "An icon")

Icon(
  IdeSampleIconKeys.gitHub,
  iconClass = IdeSampleIconKeys::class.java,
  modifier = Modifier.border(1.dp, Color.Magenta),
  contentDescription = "An owned icon",
)

Applying hints to Icons and PlatformIcons is also much easier now. It was:

val iconProvider =
    rememberResourcePainterProvider("icons/taskGroup.svg", StandaloneSampleIcons::class.java)

val badged by iconProvider.getPainter(Badge(Color.Red, DotBadgeShape.Default))
val strokedAndBadged by iconProvider.getPainter(Badge(Color.Red, DotBadgeShape.Default), Stroke(Color.White))

Icon(badged, "taskGroup")
Icon(strokedAndBadged, "taskGroup")

And now is:

PlatformIcon(AllIconsKeys.Nodes.ConfigFolder, "taskGroup", hint = Badge(Color.Red))

PlatformIcon(
  AllIconsKeys.Nodes.ConfigFolder,
  "taskGroup",
  hints = arrayOf(Stroke(Color.White), Badge(Color.Red)),
)

The sample code has been partially updated to the new API, but more work is needed to spread the change throughout existing components and their styling. That will come in a follow-up PR.

@rock3r rock3r added bug Something isn't working feature New feature or request api Changes related to the public API labels Jul 3, 2024
@jakub-senohrabek-jb
Copy link
Collaborator

To reduce dependency on IntelliJ platform, maybe current composition context (or something better) could hold something like IconProvider, which can resolve icon by id? Then when used as IJ plugin, it could use IntelliJIconKey.fromPlatformIcon as the IconProvider and in case of desktop jewel, it can use any kind of resource based thing, or it can be even customizable. The Icon api would then accept this id, maybe it can be even same, but we can make the Icon class nullable, to indicate it should be automatically resolved.

interface IconProvider {
	fun getIconByKey(key: string): ResourcePainter? // or maybe just Painter?
}

@rock3r
Copy link
Collaborator Author

rock3r commented Jul 4, 2024

That was an earlier iteration on the design, but it depends on having IJP classes on the classpath (e.g., AllIcons) to use as keys, or using stringly-typed APIs, which are anti-goals for this change. To make it work, we'd still need to generate the keys anyway, so at that point, might as well go all the way with our keys to the API. Custom themes should still be able to use json icon mappings to provide custom icons if they so wish.

lamba92
lamba92 previously requested changes Jul 4, 2024
buildSrc/build.gradle.kts Outdated Show resolved Hide resolved
* Made icon generation more maintainable

* Cleanup code

* Fix ktlint implicitly trying to check generated code

---------

Co-authored-by: Sebastiano Poggi <sebp@google.com>
@rock3r rock3r enabled auto-merge (squash) July 4, 2024 17:01
@rock3r rock3r dismissed lamba92’s stale review July 4, 2024 17:02

Out of date, he provided the fix he wanted

@rock3r rock3r merged commit 6865fb5 into main Jul 4, 2024
4 checks passed
@rock3r rock3r deleted the fix-242-icon-loading branch July 4, 2024 17:10
hamen pushed a commit that referenced this pull request Jul 8, 2024
* Implement icon keys generator

This gradle task generates the icon keys for the platform icons that
we'll need to load icons from a key instead of a path.

* Define IconIdInterpreter and NewUiChecker

* Do icon key generation for AllIcons in ui module

* Implement proper key-based icon loading

* Remove old TODO and unused code

* Undo change to run config

* Rename AllIcons to PlatformIcon, move to its own file

* Remove unnecessary suppression

* Run apiDump

* Made icon generation more maintainable (#432)

* Made icon generation more maintainable

* Cleanup code

* Fix ktlint implicitly trying to check generated code

---------

Co-authored-by: Sebastiano Poggi <sebp@google.com>

---------

Co-authored-by: Lamberto Basti <basti.lamberto@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Changes related to the public API bug Something isn't working feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants