search

JetBrains / jewel

An implementation of the IntelliJ look and feels in Compose for Desktop
Apache License 2.0
634 stars 30 forks source link

Don’t use `toInt(unit: kotlin.time.DurationUnit)` function #394

Closed morozkin closed 3 weeks ago

morozkin commented 4 weeks ago

Hi!

I noticed that HorizontalScrollbar uses toInt(unit:) function which requires kotlin.time package. Such requirements leads to the runtime linkage error.

loader constraint violation: when resolving method 
'int kotlin.time.Duration.toInt-impl(long, kotlin.time.DurationUnit)' 
the class loader com.intellij.ide.plugins.cl.PluginClassLoader of the current class,
org/jetbrains/jewel/ui/component/ScrollbarsKt, 
and the class loader com.intellij.util.lang.PathClassLoader for the method's defining class,
kotlin/time/Duration, have different Class objects for the type 
kotlin/time/DurationUnit used in the signature 
(org.jetbrains.jewel.ui.component.ScrollbarsKt is in unnamed module of
loader com.intellij.ide.plugins.cl.PluginClassLoader, parent loader 'bootstrap'; 
kotlin.time.Duration is in unnamed module of loader com.intellij.util.lang.PathClassLoader)

Other scroll bar components don't rely on it and their usage doesn't produce such error.

Some side questions:

rock3r commented 4 weeks ago

We've never had this error. Would you be able to provide more details in an issue, including the conditions under which it occurs?

morozkin commented 4 weeks ago

This error is only relevant for IntelliJ plugin development.

You can add HorizontalScrollbar(rememberScrollbarAdapter(scrollState)) to this function and run plugin after that.

UPD: The problem is that jewel is currently built using kotlin 1.8.21, while 2024.1 comes with bundled version 1.9.22 for the stdlib. That's why there is the error saying that different classes are used.

rock3r commented 3 weeks ago

Just to make sure, do you have this in your gradle.properties?

# Disable adding the Kotlin Stdlib by default to avoid issues with IJ plugins
# See https://jb.gg/intellij-platform-kotlin-stdlib
kotlin.stdlib.default.dependency=false

That error usually happens when you don't have that property

morozkin commented 3 weeks ago

This problem happens with this repository, with idea-plugin sample plugin for which this property equals to false. It is also false in my own project.

rock3r commented 3 weeks ago

This problem happens with this repository, with idea-plugin sample plugin

It doesn't happen to any of the contributors of this repo, otherwise we'd have addressed the issue. However, I am working on updating the dependencies to align with 241 (and preparing for 242, separately), and I think that would be the proper fix for issues caused by Kotlin version misalignment. What do you think?

morozkin commented 3 weeks ago

It sounds good.

However, I don't like that implementations of the same logic differ within the same file.

Also could you please answer these questions regarding scroll bars:

rock3r commented 3 weeks ago

However, I don't like that implementations of the same logic differ within the same file.

Agree there — we are planning a more comprehensive review of all the code and hopefully it should get rid of these inconsistencies. Would be great if you could file an issue for it, so we don't forget.

Why do shape and hoverDurationMillis store MutableState instead of plain values?

I suppose that so they don't get recreated on every recomposition, since they are derived from the style.

Why isn't there key for remeber calls that are used for shape and hoverDurationMillis?

Likely an oversight :) Please mention this in the aforementioned issue!

rock3r commented 3 weeks ago

FYI 0.19.6 is out, bumping Kotlin to 1.9.24

morozkin commented 3 weeks ago

I can confirm that there is no problem on version 0.19.6