Skip to content

Commit

Permalink
Use val/var consistently in schemes, rewrite trees
Browse files Browse the repository at this point in the history
The goal was actually only to fix the usages of `val`s and `var`s across the `Scheme`s (cf. #476), but this ended up in being a major overhaul of how the various `Tree`-related classes work together. I found quite a few pieces of code that were, by themselves, neat, but for which much better solutions existed. With these changes, I feel more confident in the stability of the code, and plenty of docs and tests have been added as well. I am aware that #473 will be another overhaul of the `Tree`s, but that won't be until v3.1.x, and I don't want to wait for that. Also, depending on the solution for that issue, the code here might be retained completely anyway.
  • Loading branch information
FWDekker committed Oct 19, 2023
1 parent 1ddaf86 commit e960a9d
Show file tree
Hide file tree
Showing 26 changed files with 1,166 additions and 1,060 deletions.
13 changes: 12 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,22 @@ See [Plugin Signing](https://plugins.jetbrains.com/docs/intellij/plugin-signing.
```bash
$ gradlew test # Run tests
$ gradlew test --tests X # Run tests in class X (package name optional)
$ gradlew test --kotest.tags="X" # Run tests with `NamedTag` X (also supports not (!), and (&), or (|))
$ gradlew test -Pkotest.tags="X" # Run tests with `NamedTag` X (also supports not (!), and (&), or (|))
$ gradlew check # Run tests and static analysis
$ gradlew runPluginVerifier # Check for compatibility issues
```

#### 🏷️ Filtering tests
[Kotest tests can be tagged](https://kotest.io/docs/framework/tags.html) to allow selectively running tests.
Tag an entire test class by adding `tags(...)` to the class definition, or tag an individual test `context` by
writing `context("foo").config(tags = setOf(...)) {`.
It is not possible to tag an individual `test` due to limitations in Kotest.

To run only one `context` in some test class `X`, prefix the `context`'s name with `f:` and run with `--tests X`.
The prefix `f:` filters out other `context`s in that test class, and `--tests X` filters out other test classes.
Alternatively, tag the desired `context` with the `Focus` tag and run with `-Pkotest.tags="Focus"` to filter by that
tag.

### 📚 Documentation
```bash
$ gradlew dokkaHtml # Generate documentation
Expand Down
5 changes: 4 additions & 1 deletion build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,10 @@ tasks {
if (project.hasProperty("kotest.tags")) systemProperty("kotest.tags", project.findProperty("kotest.tags")!!)

useJUnitPlatform {
includeEngines("junit-vintage", "kotest")
if (!project.hasProperty("kotest.tags"))
includeEngines("junit-vintage")

includeEngines("kotest")
}

testLogging {
Expand Down
2 changes: 1 addition & 1 deletion src/main/kotlin/com/fwdekker/randomness/Bundle.kt
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ object Bundle {
* @throws java.util.MissingFormatArgumentException if [args] has fewer arguments than required for [format]
*/
fun String.matchesFormat(format: String, vararg args: String) =
Regex("%[0-9]+\\\$[Ss]").findAll(format)
Regex("%[0-9]+\\\$[Ssd]").findAll(format)
.toList()
.reversed()
.fold(format) { acc, match ->
Expand Down
2 changes: 1 addition & 1 deletion src/main/kotlin/com/fwdekker/randomness/SchemeEditor.kt
Original file line number Diff line number Diff line change
Expand Up @@ -76,5 +76,5 @@ abstract class SchemeEditor<S : Scheme>(val scheme: S) : Disposable {
/**
* Disposes this editor's resources.
*/
override fun dispose() = Disposer.dispose(this)
override fun dispose() = decoratorEditors.forEach { Disposer.dispose(it) }
}
42 changes: 25 additions & 17 deletions src/main/kotlin/com/fwdekker/randomness/Settings.kt
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import com.intellij.openapi.components.SettingsCategory
import com.intellij.openapi.components.Storage
import com.intellij.openapi.components.service
import com.intellij.util.xmlb.XmlSerializer
import com.intellij.util.xmlb.annotations.OptionTag
import com.intellij.util.xmlb.annotations.Transient
import org.jdom.Element
import java.lang.module.ModuleDescriptor
Expand All @@ -22,7 +23,8 @@ import com.intellij.openapi.components.State as JBState
*/
data class Settings(
var version: String = CURRENT_VERSION,
var templateList: TemplateList = TemplateList(),
@OptionTag
val templateList: TemplateList = TemplateList(),
) : State() {
/**
* @see TemplateList.templates
Expand All @@ -31,27 +33,34 @@ data class Settings(
val templates: MutableList<Template> get() = templateList.templates


override fun doValidate() = templateList.doValidate()

override fun deepCopy(retainUuid: Boolean) =
copy(templateList = templateList.deepCopy(retainUuid = retainUuid)).deepCopyTransient(retainUuid)
init {
applyContext(this)
}

override fun copyFrom(other: State) {
require(other is Settings) { "Cannot copy from different type." }

this.templateList.copyFrom(other.templateList)
copyFromTransient(other)
override fun applyContext(context: Box<Settings>) {
super.applyContext(context)
templateList.applyContext(context)
}


override fun doValidate() = templateList.doValidate()

override fun deepCopy(retainUuid: Boolean) =
copy(templateList = templateList.deepCopy(retainUuid = retainUuid))
.deepCopyTransient(retainUuid)
.also { it.applyContext(it) }


/**
* Holds constants.
*/
companion object {
/**
* The persistent [Settings] instance.
*/
val DEFAULT: Settings by lazy { service<PersistentSettings>().settings }
val DEFAULT: Settings
get() = service<PersistentSettings>().settings
}
}

Expand All @@ -70,11 +79,11 @@ data class Settings(
)
class PersistentSettings : PersistentStateComponent<Element> {
/**
* The persistent settings instance.
* The [Settings] that should be persisted.
*
* @see Settings.DEFAULT Preferred method of accessing the persistent settings instance.
*/
val settings = Settings()
var settings = Settings()


/**
Expand All @@ -83,12 +92,11 @@ class PersistentSettings : PersistentStateComponent<Element> {
override fun getState(): Element = XmlSerializer.serialize(settings)

/**
* Deserializes [element] into a [Settings] instance, which is then copied into the [settings] instance.
*
* @see TemplateList.copyFrom
* Deserializes [element] into a [Settings] instance, which is then stored in [settings].
*/
override fun loadState(element: Element) =
settings.copyFrom(XmlSerializer.deserialize(upgrade(element), Settings::class.java))
override fun loadState(element: Element) {
settings = XmlSerializer.deserialize(upgrade(element), Settings::class.java)
}


/**
Expand Down
40 changes: 8 additions & 32 deletions src/main/kotlin/com/fwdekker/randomness/State.kt
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.fwdekker.randomness

import com.fasterxml.uuid.Generators
import com.intellij.util.xmlb.XmlSerializerUtil
import com.intellij.util.xmlb.annotations.Transient
import kotlin.random.Random
import kotlin.random.asJavaRandom
Expand All @@ -10,9 +9,12 @@ import kotlin.random.asJavaRandom
/**
* A state holds variables that can be configured, validated, copied, and loaded.
*
* All non-transient non-primitive fields of a state should be immutable references. For example, if a state has a field
* `list: List<String>`, then `list` should be a `val`, not a `var`, so that references to `state.list` remain valid
* even after `copyFrom`.
* In a [State], fields are typically mutable, because the user can change the field. However, fields that themselves
* also contain data (e.g. a [Collection] or a [DecoratorScheme]) should be stored as immutable references (but may
* themselves be mutable). For example, instead of having the field `var list: List<String>`, a [State] should have the
* field `val list: MutableList<String>`. This reflects the typical case in which the user does not change the reference
* to the object, but changes the properties inside the object. And, more importantly, means that nested a
* [SchemeEditor] can share a single unchanging reference to a [State] with its nested [SchemeEditor]s.
*/
abstract class State {
/**
Expand Down Expand Up @@ -58,7 +60,7 @@ abstract class State {
*
* Fields annotated with [Transient] are shallow-copied.
*
* @see deepCopyTransient utility function for subclasses that want to implement `deepCopy`
* @see deepCopyTransient utility function for subclasses that want to implement [deepCopy]
*/
abstract fun deepCopy(retainUuid: Boolean = false): State

Expand All @@ -73,34 +75,8 @@ abstract class State {
val thiz: State = this@State

if (retainUuid) self.uuid = thiz.uuid
self.applyContext(thiz.context.copy())
self.applyContext(thiz.context.copy()) // Copies the [Box], not the context itself

return self
}

/**
* Copies [other] into `this`.
*
* Works by shallow-copying a [deepCopy] of [other] into `this`. [Transient] fields are shallow-copied directly from
* [other] instead.
*
* Implementations may choose to shallow-copy additional fields directly from [other].
*
* @param other the state to copy into `this`; should be a (sub)class of this state
* @see copyFromTransient utility function for subclasses that want to implement `copyFrom`
*/
open fun copyFrom(other: State) {
XmlSerializerUtil.copyBean(other.deepCopy(retainUuid = true), this)
copyFromTransient(other)
}

/**
* Copies basic [Transient] fields from [other] into `this`.
*
* Typically used by subclasses to help implement [copyFrom].
*/
protected fun copyFromTransient(other: State) {
this.uuid = other.uuid
this.applyContext(other.context.copy())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ data class Template(
WordScheme::class,
]
)
var schemes: MutableList<Scheme> = DEFAULT_SCHEMES,
val schemes: MutableList<Scheme> = DEFAULT_SCHEMES,
val arrayDecorator: ArrayDecorator = DEFAULT_ARRAY_DECORATOR,
) : Scheme() {
override val typeIcon
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ open class TemplateActionLoader(
newList: List<Template>,
actionManager: ActionManager = ActionManager.getInstance(),
) {
oldList.filterNot { it in newList }.forEach { unregisterAction(actionManager, it) }
val newUuidList = newList.map { it.uuid }
oldList.filterNot { it.uuid in newUuidList }.forEach { unregisterAction(actionManager, it) }
newList.forEach { registerAction(actionManager, it) }
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@ import com.fwdekker.randomness.Timely.generateTimely
import com.fwdekker.randomness.array.ArrayDecorator
import com.fwdekker.randomness.array.ArrayDecoratorEditor
import com.fwdekker.randomness.ui.PreviewPanel
import com.intellij.openapi.Disposable
import com.intellij.openapi.actionSystem.ActionGroup
import com.intellij.openapi.actionSystem.ActionUpdateThread
import com.intellij.openapi.actionSystem.AnAction
import com.intellij.openapi.actionSystem.AnActionEvent
import com.intellij.openapi.options.Configurable
import com.intellij.openapi.options.ShowSettingsUtil
import com.intellij.openapi.util.Disposer
import java.awt.BorderLayout
import javax.swing.JPanel

Expand Down Expand Up @@ -128,9 +130,13 @@ class TemplateInsertAction(
*
* @param arrayDecorator the decorator being edited in this configurable
*/
inner class ArrayDecoratorConfigurable(arrayDecorator: ArrayDecorator) : Configurable {
private val editor = ArrayDecoratorEditor(arrayDecorator, embedded = true)
private val previewPanel = PreviewPanel { template.deepCopy().also { it.arrayDecorator.enabled = true } }
inner class ArrayDecoratorConfigurable(arrayDecorator: ArrayDecorator) : Configurable, Disposable {
private val editor =
ArrayDecoratorEditor(arrayDecorator, embedded = true)
.also { Disposer.register(this, it) }
private val previewPanel =
PreviewPanel { template.deepCopy().also { it.arrayDecorator.enabled = true } }
.also { Disposer.register(this, it) }


init {
Expand Down Expand Up @@ -170,12 +176,14 @@ class TemplateInsertAction(
override fun getDisplayName() = text

/**
* Disposes the [editor] and [previewPanel].
* Recursively disposes this configurable's resources.
*/
override fun disposeUIResources() {
editor.dispose()
previewPanel.dispose()
}
override fun disposeUIResources() = Disposer.dispose(this)

/**
* Non-recursively disposes this configurable's resources.
*/
override fun dispose() = Unit
}
}

Expand Down
Loading

0 comments on commit e960a9d

Please sign in to comment.