search

EssentialGG / Vigilance

Configuration Utility using Elementa
GNU Lesser General Public License v3.0
54 stars 12 forks source link

Properties: Add support for custom property types. #43

Closed LlamaLad7 closed 2 years ago

LlamaLad7 commented 2 years ago

Specified using PropertyType.CUSTOM and by setting Property#customPropertyInfo

LlamaLad7 commented 2 years ago

Is it ok to use KClass as opposed to Java Class there? Are those usable in annotations from Java?

Yes, that's a weird Kotlin thing, the resultant bytecode only references java.lang.Class.

And iirc we deprecated some methods because Kotlin reflection is slow to initialize

In Essential's commands thing I think we initialize it on a background thread. We could possibly do the same here?

Johni0702 commented 2 years ago

In Essential's commands thing I think we initialize it on a background thread. We could possibly do the same here?

I don't think we can. The properties are initialized in the constructor (and subclasses rely on that). Or rather, it depends on whether simply having KClass instances is already enough to trigger that slow initialization. If it is, then we can't, and should probably switch to Class. If it isn't, and it's only the constructors call which initializes kotlin's reflection, then it's probably fine as is, cause that'll only be called when the GUI is opened, and by then kotlin's reflection will likely already have been initialized.

LlamaLad7 commented 2 years ago

it depends on whether simply having KClass instances is already enough to trigger that slow initialization. If it is, then we can't, and should probably switch to Class

From my testing, it seems having a KClass instance around triggers some initialization, though not as much as the constructors call, so I'll use Class instead, except in the DSL, where the frequent KMutableProperty use would initialize it anyway, and in the annotation itself where Kotlin requires KClass as a facade for Class. (Similarly to ::class.java literals, the intermediary KClass object is avoided when referencing annotation.kClassProperty.java.)

LlamaLad7 commented 2 years ago

adding a new argument to a method with optional arguments is not backwards compatible, just like it wouldn't be in Java; though even with @JvmOverloads, it's not backwards compatible because of how default arguments are actually implemented

Would you suggest adding an explicit overload instead? Or is there some more elegant solution.