Count attached sliders instead of flagging them

A boolean goes wrong as soon as two sliders of the same kind share a
controller: the first one to leave the composition clears it while the
other is still on screen, and the surviving slider stops reaching the
color. The controller counts them now, and the last one out is what
hands the component back to the palette color.

Adds regression tests for both, four of which fail when a slider that
leaves stays attached, and two when one leaving speaks for the other.

5 files changed, 274 insertions, 53 deletions

diff --git a/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/AlphaSlider.kt b/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/AlphaSlider.kt
index 7c2a768..216172a 100644
--- a/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/AlphaSlider.kt
+++ b/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/AlphaSlider.kt
@@ -80,11 +80,8 @@ public fun AlphaSlider(
   )
 
   DisposableEffect(controller) {
-    controller.isAttachedAlphaSlider = true
-
-    onDispose {
-      controller.isAttachedAlphaSlider = false
-    }
+    controller.attachAlphaSlider()
+    onDispose { controller.detachAlphaSlider() }
   }
 
   Slider(
diff --git a/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/BrightnessSlider.kt b/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/BrightnessSlider.kt
index c640c0c..a95c4dc 100644
--- a/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/BrightnessSlider.kt
+++ b/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/BrightnessSlider.kt
@@ -66,11 +66,8 @@ public fun BrightnessSlider(
   onFinish: () -> Unit = {},
 ) {
   DisposableEffect(controller) {
-    controller.isAttachedBrightnessSlider = true
-
-    onDispose {
-      controller.isAttachedBrightnessSlider = false
-    }
+    controller.attachBrightnessSlider()
+    onDispose { controller.detachBrightnessSlider() }
   }
 
   Slider(
diff --git a/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/ColorPickerController.kt b/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/ColorPickerController.kt
index 464543e..dd4fe95 100644
--- a/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/ColorPickerController.kt
+++ b/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/ColorPickerController.kt
@@ -162,50 +162,49 @@ public class ColorPickerController {
       _enabled.value = value
     }
 
-  /** Indicates if the alpha slider has been attached. */
-  internal var isAttachedAlphaSlider: Boolean = false
-    set(value) {
-      if (field != value) {
-        field = value
-        if (!value) {
-          alpha.value = 1.0f
-        }
-        recalculateColorDueToAttachmentChange()
-      }
-    }
+  /** An alpha slider that leaves hands the alpha channel back to the palette color. */
+  private val alphaSliders = SliderAttachment { alpha.value = 1.0f }
 
-  /** Indicates if the brightness slider has been attached. */
-  internal var isAttachedBrightnessSlider: Boolean = false
-    set(value) {
-      if (field != value) {
-        field = value
-        if (!value) {
-          val (_, _, v) = pureSelectedColor.value.toHSV()
-          brightness.value = v
-        }
-        recalculateColorDueToAttachmentChange()
-      }
-    }
+  /** A brightness slider that leaves hands the value back to the palette color. */
+  private val brightnessSliders = SliderAttachment {
+    brightness.value = pureSelectedColor.value.toHSV().third
+  }
+
+  /** A saturation slider that leaves hands the saturation back to the palette color. */
+  private val saturationSliders = SliderAttachment {
+    saturation.value = pureSelectedColor.value.toHSV().second
+  }
+
+  /** Indicates if an alpha slider has been attached. */
+  internal val isAttachedAlphaSlider: Boolean
+    get() = alphaSliders.isAttached
+
+  /** Indicates if a brightness slider has been attached. */
+  internal val isAttachedBrightnessSlider: Boolean
+    get() = brightnessSliders.isAttached
 
   /** Whether a SaturationSlider is attached. */
-  internal var isAttachedSaturationSlider: Boolean = false
-    set(value) {
-      if (field != value) {
-        field = value
-        if (!value) {
-          val (_, s, _) = pureSelectedColor.value.toHSV()
-          saturation.value = s
-        }
-        recalculateColorDueToAttachmentChange()
-      }
-    }
+  internal val isAttachedSaturationSlider: Boolean
+    get() = saturationSliders.isAttached
+
+  internal fun attachAlphaSlider(): Unit = onAttachmentChanged(alphaSliders.attach())
+
+  internal fun detachAlphaSlider(): Unit = onAttachmentChanged(alphaSliders.detach())
+
+  internal fun attachBrightnessSlider(): Unit = onAttachmentChanged(brightnessSliders.attach())
+
+  internal fun detachBrightnessSlider(): Unit = onAttachmentChanged(brightnessSliders.detach())
+
+  internal fun attachSaturationSlider(): Unit = onAttachmentChanged(saturationSliders.attach())
+
+  internal fun detachSaturationSlider(): Unit = onAttachmentChanged(saturationSliders.detach())
 
   /**
-   * Forcibly recalculates the current color when dynamically added
-   * or removing sliders from the composition tree.
+   * Recalculates the selected color after a slider was added to, or removed from, the composition.
+   * Which factors apply depends on what is attached, so the color has to be built again.
    */
-  private fun recalculateColorDueToAttachmentChange() {
-    if (!enabled) return
+  private fun onAttachmentChanged(changed: Boolean) {
+    if (!changed || !enabled) return
     val newColor = applyHSVFactors(pureSelectedColor.value)
     if (_selectedColor.value != newColor) {
       _selectedColor.value = newColor
@@ -490,3 +489,28 @@ public class ColorPickerController {
     _paletteBitmap.value = null
   }
 }
+
+/**
+ * Counts the sliders of one kind composed against a controller.
+ *
+ * A plain flag goes wrong the moment two sliders of the same kind share a controller: the first one
+ * to leave the composition clears it while the other is still on screen, and the surviving slider
+ * stops reaching the color.
+ */
+private class SliderAttachment(private val onLastDetached: () -> Unit) {
+
+  private var count: Int = 0
+
+  val isAttached: Boolean
+    get() = count > 0
+
+  /** Returns true when this is the first slider of its kind to arrive. */
+  fun attach(): Boolean = count++ == 0
+
+  /** Returns true when this was the last slider of its kind to leave. */
+  fun detach(): Boolean {
+    if (count == 0 || --count > 0) return false
+    onLastDetached()
+    return true
+  }
+}
diff --git a/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/SaturationSlider.kt b/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/SaturationSlider.kt
index cfd3cb0..87bfbce 100644
--- a/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/SaturationSlider.kt
+++ b/colorpicker-compose/src/commonMain/kotlin/com/github/skydoves/colorpicker/compose/SaturationSlider.kt
@@ -66,11 +66,8 @@ public fun SaturationSlider(
   onFinish: () -> Unit = {},
 ) {
   DisposableEffect(controller) {
-    controller.isAttachedSaturationSlider = true
-
-    onDispose {
-      controller.isAttachedSaturationSlider = false
-    }
+    controller.attachSaturationSlider()
+    onDispose { controller.detachSaturationSlider() }
   }
 
   Slider(
diff --git a/colorpicker-compose/src/desktopTest/kotlin/com/github/skydoves/colorpicker/compose/SliderAttachmentTest.kt b/colorpicker-compose/src/desktopTest/kotlin/com/github/skydoves/colorpicker/compose/SliderAttachmentTest.kt
new file mode 100644
index 0000000..230f1b6
--- /dev/null
+++ b/colorpicker-compose/src/desktopTest/kotlin/com/github/skydoves/colorpicker/compose/SliderAttachmentTest.kt
@@ -0,0 +1,206 @@
+/*
+ * Designed and developed by 2022 skydoves (Jaewoong Eum)
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.github.skydoves.colorpicker.compose
+
+import androidx.compose.foundation.layout.Column
+import androidx.compose.foundation.layout.height
+import androidx.compose.foundation.layout.size
+import androidx.compose.foundation.layout.width
+import androidx.compose.runtime.Composable
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.setValue
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.geometry.Offset
+import androidx.compose.ui.graphics.Color
+import androidx.compose.ui.platform.testTag
+import androidx.compose.ui.test.ComposeUiTest
+import androidx.compose.ui.test.click
+import androidx.compose.ui.test.onNodeWithTag
+import androidx.compose.ui.test.performTouchInput
+import androidx.compose.ui.unit.dp
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertFalse
+import kotlin.test.assertTrue
+
+private const val PICKER = 200
+private const val LENGTH = 200
+private const val THICKNESS = 40
+private val NEAR_START = Offset(20f, THICKNESS / 2f)
+private val RIGHT_OF_WHEEL = Offset(PICKER - 1f, PICKER / 2f)
+
+/**
+ * A slider used to tell the controller it was there and never that it had gone, so a slider hidden
+ * behind a switch kept its last value folded into every color the picker reported afterwards.
+ */
+class SliderAttachmentTest {
+
+  @Test
+  fun aBrightnessSliderThatLeavesReleasesItsHoldOnTheColor() = runColorPickerUiTest {
+    var showSlider by mutableStateOf(true)
+    val controller = pickerWithOptionalSlider({ showSlider }) { c ->
+      BrightnessSlider(sliderModifier(), c)
+    }
+    onNodeWithTag("slider").performTouchInput { click(NEAR_START) }
+    waitForIdle()
+    assertTrue(controller.selectedColor.value.red < 0.2f, "the slider never darkened the color")
+
+    showSlider = false
+    waitForIdle()
+
+    assertFalse(controller.isAttachedBrightnessSlider)
+    assertColorEquals(Color.White, controller.selectedColor.value)
+  }
+
+  @Test
+  fun aPickerKeepsItsOwnBrightnessOnceTheSliderIsGone() = runColorPickerUiTest {
+    var showSlider by mutableStateOf(true)
+    val controller = pickerWithOptionalSlider({ showSlider }) { c ->
+      BrightnessSlider(sliderModifier(), c)
+    }
+    onNodeWithTag("slider").performTouchInput { click(NEAR_START) }
+    waitForIdle()
+    showSlider = false
+    waitForIdle()
+
+    onNodeWithTag("picker").performTouchInput { click(RIGHT_OF_WHEEL) }
+
+    assertColorEquals(Color.Red, controller.selectedColor.value, tolerance = 0.02f)
+  }
+
+  @Test
+  fun anAlphaSliderThatLeavesGivesTheAlphaBack() = runColorPickerUiTest {
+    var showSlider by mutableStateOf(true)
+    val controller = pickerWithOptionalSlider({ showSlider }) { c ->
+      AlphaSlider(sliderModifier(), c)
+    }
+    onNodeWithTag("slider").performTouchInput { click(NEAR_START) }
+    waitForIdle()
+    assertTrue(controller.selectedColor.value.alpha < 0.2f, "the slider never cleared the alpha")
+
+    showSlider = false
+    waitForIdle()
+
+    assertFalse(controller.isAttachedAlphaSlider)
+    assertEquals(1f, controller.alpha.value)
+    assertEquals(1f, controller.selectedColor.value.alpha)
+  }
+
+  @Test
+  fun aSaturationSliderThatLeavesReleasesItsHoldOnTheColor() = runColorPickerUiTest {
+    var showSlider by mutableStateOf(true)
+    val controller = pickerWithOptionalSlider({ showSlider }) { c ->
+      SaturationSlider(sliderModifier(), c)
+    }
+    onNodeWithTag("picker").performTouchInput { click(RIGHT_OF_WHEEL) }
+    onNodeWithTag("slider").performTouchInput { click(NEAR_START) }
+    waitForIdle()
+    assertTrue(controller.selectedColor.value.green > 0.7f, "the slider never washed out the color")
+
+    showSlider = false
+    waitForIdle()
+
+    assertFalse(controller.isAttachedSaturationSlider)
+    assertColorEquals(Color.Red, controller.selectedColor.value, tolerance = 0.02f)
+  }
+
+  @Test
+  fun oneSliderLeavingDoesNotSpeakForTheOtherOfItsKind() = runColorPickerUiTest {
+    var showSecond by mutableStateOf(true)
+    lateinit var controller: ColorPickerController
+    setContent {
+      controller = rememberColorPickerController()
+      Column {
+        HsvColorPicker(Modifier.size(PICKER.dp).testTag("picker"), controller)
+        BrightnessSlider(sliderModifier("first"), controller)
+        if (showSecond) BrightnessSlider(sliderModifier("second"), controller)
+      }
+    }
+
+    showSecond = false
+    waitForIdle()
+
+    assertTrue(
+      controller.isAttachedBrightnessSlider,
+      "the surviving slider was cut off from the controller",
+    )
+  }
+
+  @Test
+  fun theSurvivingSliderStillReachesTheColor() = runColorPickerUiTest {
+    var showSecond by mutableStateOf(true)
+    lateinit var controller: ColorPickerController
+    setContent {
+      controller = rememberColorPickerController()
+      Column {
+        HsvColorPicker(Modifier.size(PICKER.dp).testTag("picker"), controller)
+        BrightnessSlider(sliderModifier("first"), controller)
+        if (showSecond) BrightnessSlider(sliderModifier("second"), controller)
+      }
+    }
+    showSecond = false
+    waitForIdle()
+    onNodeWithTag("first").performTouchInput { click(NEAR_START) }
+    waitForIdle()
+
+    // Picking again is what asks the controller which factors still apply.
+    onNodeWithTag("picker").performTouchInput { click(RIGHT_OF_WHEEL) }
+    waitForIdle()
+
+    assertTrue(
+      controller.selectedColor.value.red < 0.2f,
+      "the surviving slider stopped reaching the color, which came out ${controller.selectedColor.value}",
+    )
+  }
+
+  @Test
+  fun bringingASliderBackDoesNotJumpTheColor() = runColorPickerUiTest {
+    var showSlider by mutableStateOf(true)
+    val controller = pickerWithOptionalSlider({ showSlider }) { c ->
+      BrightnessSlider(sliderModifier(), c)
+    }
+    onNodeWithTag("slider").performTouchInput { click(NEAR_START) }
+    waitForIdle()
+    showSlider = false
+    waitForIdle()
+    val afterRemoval = controller.selectedColor.value
+
+    showSlider = true
+    waitForIdle()
+
+    assertEquals(afterRemoval, controller.selectedColor.value)
+  }
+
+  /** A picker with a slider under it that a switch can take away. */
+  private fun ComposeUiTest.pickerWithOptionalSlider(
+    visible: () -> Boolean,
+    slider: @Composable (ColorPickerController) -> Unit,
+  ): ColorPickerController {
+    lateinit var controller: ColorPickerController
+    setContent {
+      controller = rememberColorPickerController()
+      Column {
+        HsvColorPicker(Modifier.size(PICKER.dp).testTag("picker"), controller)
+        if (visible()) slider(controller)
+      }
+    }
+    return controller
+  }
+}
+
+private fun sliderModifier(tag: String = "slider") =
+  Modifier.width(LENGTH.dp).height(THICKNESS.dp).testTag(tag)