Fix layer order inside aboveLayer compose blocks (#2942)

Author: jush

CHANGELOG.md

@@ -3,6 +3,8 @@
 Mapbox welcomes participation and contributions from everyone.
 
 # main
+## Bug fixes 🐞
+* [compose] Fix wrong layer order when using `aboveLayer(..)` composable function.
 
 
 # 11.10.0-rc.1 January 31, 2025

extension-compose/src/main/java/com/mapbox/maps/extension/compose/annotation/internal/BaseAnnotationNode.kt

@@ -14,7 +14,7 @@ internal abstract class BaseAnnotationNode(private val mapboxStyleManager: Mapbo
   }
 
   @CallSuper
-  override fun onMoved(parent: MapNode, from: Int, to: Int) {
+  override fun onMoved(parent: MapNode) {
     mapboxStyleManager.repositionCurrentNode(parent)
   }
 

extension-compose/src/main/java/com/mapbox/maps/extension/compose/internal/LayerPositionAwareNode.kt

@@ -2,6 +2,7 @@ package com.mapbox.maps.extension.compose.internal
 
 import com.mapbox.maps.LayerPosition
 import com.mapbox.maps.MapboxStyleManager
+import com.mapbox.maps.extension.compose.style.LayerPositionedContent
 import com.mapbox.maps.extension.compose.style.internal.StyleLayerPositionNode
 import com.mapbox.maps.logW
 
@@ -18,7 +19,8 @@ internal interface LayerPositionAwareNode {
    * Reposition all the layers within the current node according to [getRelativePositionInfo].
    */
   fun MapboxStyleManager.repositionCurrentNode(parent: MapNode) {
-    repositionLayersForCurrentNode(getRelativePositionInfo(parent))
+    val layerPosition = (parent as? StyleLayerPositionNode)?.layerPosition
+    repositionLayersForCurrentNode(getRelativePositionInfo(parent, layerPosition))
   }
 
   /**
@@ -52,27 +54,61 @@ internal interface LayerPositionAwareNode {
 
   /**
    * Search the children of the parent node, and see if there's any other [LayerPositionAwareNode]
-   * which is on top of the current node, construct the [LayerPosition] with below layer id, so the
+   * which is rendered on top of this one (for [LayerPositionedContent.belowLayer]) or below (for [LayerPositionedContent.aboveLayer])
+   * of the current node, construct the [LayerPosition] with the found layer id, so the
    * current layer is inserted at the correct position and consistent with the node tree.
    *
-   * If there's nothing above current node, take the [targetLayerPosition] as it's position.
+   * If there's nothing above/below current node, take the [targetLayerPosition] as its insertion position.
+   *
+   * For example, the following compose block:
+   * ```
+   * aboveLayer("buildings") {
+   *  layerPositionAwareNode_1
+   *  layerPositionAwareNode_2
+   *  layerPositionAwareNode_3
+   * }
+   * ```
+   * Should be rendered in the map in the following order (top visible layer to bottom layer):
+   * ```
+   *  layerPositionAwareNode_3
+   *  layerPositionAwareNode_2
+   *  layerPositionAwareNode_1
+   *  "buildings"
+   * ```
+   *
+   * Meaning that the relative position of the layers should be:
+   * ```
+   * layerPositionAwareNode_3 above layerPositionAwareNode_2
+   * layerPositionAwareNode_2 above layerPositionAwareNode_1
+   * layerPositionAwareNode_1 above "buildings"
+   * ```
+   *
    *
    * @param parent the parent node of the node to be positioned.
    * @param targetLayerPosition the optional target layer position, useful in case of [StyleLayerPositionNode].
    */
   fun getRelativePositionInfo(
     parent: MapNode,
     targetLayerPosition: LayerPosition? = null
-  ): LayerPosition? {
-    return with(parent.children.filterIsInstance<LayerPositionAwareNode>()) {
-      elementAtOrNull(indexOf(this@LayerPositionAwareNode) + 1)?.getLayerIds()?.firstOrNull()
-        ?.let { belowLayerId ->
-          LayerPosition(
-            /* above = */ null,
-            /* below = */ belowLayerId,
-            /* at = */ null
-          )
-        } ?: targetLayerPosition
+  ): LayerPosition? =
+    parent.children.filterIsInstance<LayerPositionAwareNode>().let { children ->
+      val me = this@LayerPositionAwareNode
+      val isAbove = targetLayerPosition?.above != null
+      val anchorLayerIdx = children.indexOf(me) + if (isAbove) {
+        -1
+      } else {
+        1
+      }
+      val anchorLayerNode = children.elementAtOrNull(anchorLayerIdx)
+      return if (anchorLayerNode == null) {
+        targetLayerPosition
+      } else {
+        val anchorLayerId = anchorLayerNode.getLayerIds().first()
+        LayerPosition(
+          /* above = */ if (isAbove) anchorLayerId else null,
+          /* below = */ if (isAbove) null else anchorLayerId,
+          /* at = */ null
+        )
+      }
     }
-  }
 }

extension-compose/src/main/java/com/mapbox/maps/extension/compose/internal/MapApplier.kt

@@ -1,6 +1,8 @@
 package com.mapbox.maps.extension.compose.internal
 
 import androidx.compose.runtime.AbstractApplier
+import com.mapbox.common.LogConfiguration
+import com.mapbox.common.LoggingLevel
 import com.mapbox.maps.MapView
 import com.mapbox.maps.logD
 
@@ -12,20 +14,17 @@ internal abstract class MapNode {
   val children = mutableListOf<MapNode>()
 
   /**
-   * Invoked when the [MapNode] is attached to the node tree.
+   * Invoked when this [MapNode] is attached to the node tree.
    */
   open fun onAttached(parent: MapNode) {}
 
   /**
-   * Invoked when the [MapNode] is moved inside the node tree.
-   *
-   * @param from original position
-   * @param to the target position
+   * Invoked when this [MapNode] is moved inside the node tree.
    */
-  open fun onMoved(parent: MapNode, from: Int, to: Int) {}
+  open fun onMoved(parent: MapNode) {}
 
   /**
-   * Invoked when the [MapNode] is removed from the node tree.
+   * Invoked when this [MapNode] is removed from the node tree.
    */
   open fun onRemoved(parent: MapNode) {}
 
@@ -90,12 +89,10 @@ internal class MapApplier(
   override fun move(from: Int, to: Int, count: Int) {
     logD(TAG, "move: $from, $to, $count")
     current.children.move(from, to, count)
-    repeat(count) { offset ->
-      current.children[to + offset - if (to > from) count else 0].onMoved(
-        parent = current,
-        from = from + offset,
-        to = to + offset - if (to > from) count else 0
-      )
+
+    // Let's move the children nodes in the tree
+    current.children.forEach { child ->
+      child.onMoved(current)
     }
     printNodesTree("MapNodeMoved")
   }
@@ -110,7 +107,13 @@ internal class MapApplier(
   }
 
   private fun printNodesTree(tag: String = TAG) {
-    walkChildren(tag = tag, node = root)
+    // We don't know how many nodes are in the tree, so we only walk the tree when the logging
+    // level is DEBUG.
+    LogConfiguration.getLoggingLevel(tag)?.let {
+      if (it.ordinal <= LoggingLevel.DEBUG.ordinal) {
+        walkChildren(tag = tag, node = root)
+      }
+    }
   }
 
   private fun walkChildren(tag: String = TAG, prefix: String = "\t", node: MapNode) {

extension-compose/src/main/java/com/mapbox/maps/extension/compose/style/layers/internal/LayerNode.kt

@@ -77,8 +77,8 @@ internal class LayerNode(
     }
   }
 
-  override fun onMoved(parent: MapNode, from: Int, to: Int) {
-    logD(TAG, "onMoved: from $from to $to")
+  override fun onMoved(parent: MapNode) {
+    logD(TAG, "onMoved")
     dispatchWhenStyleDataLoaded {
       it.repositionCurrentNode(parent)
     }