Merge pull request #2948 from square/when-i-open-up-a-tab-in-shark

Name a tab as it opens, let it grow in, and wrap the strip

Author: pyricau

shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/HeapDumpExplorer.kt

@@ -52,6 +52,7 @@ import shark.explorer.ObjectDominator
 import shark.explorer.ObjectList
 import shark.explorer.ObjectListFilter
 import shark.explorer.Place
+import shark.explorer.PresentedCell
 import shark.explorer.RadialLayout
 import shark.explorer.RadialPresentation
 import shark.explorer.RootPath
@@ -180,6 +181,24 @@ internal fun HeapDumpExplorer(
     )
   }
 
+  // What each tab is called, for the tabs the window couldn't name itself. A label is cheap enough to draw
+  // on every rectangle of the map, so naming a strip of them is one small read — but it is still a read,
+  // and reads queue on the heap dump's one thread.
+  //
+  // Which is why this goes in ahead of the effect that lays the view out: opening a tab asks for both, and
+  // the layout is the larger by orders of magnitude. Named after it, a tab would show its placeholder for
+  // as long as laying the tree out takes, which on a real dump is what someone sees as a flicker.
+  val unnamedPlaces = tabs.tabs.map { it.place }.filter { it.title == null && it !in placeTitles }
+  LaunchedEffect(session, unnamedPlaces) {
+    if (unnamedPlaces.isEmpty()) {
+      return@LaunchedEffect
+    }
+    val named = session.read("what to call ${unnamedPlaces.size} tabs") { explorer ->
+      unnamedPlaces.associateWith { explorer.tree.titleOf(it) }
+    }
+    placeTitles = placeTitles + named
+  }
+
   // Resizing, going to an object and switching shape all lay the tree out again, which reads the heap dump
   // for every visible label. All of it ends up here, on the heap dump's thread. Keyed on the request rather
   // than on the place, so that typing into a list doesn't lay the map out again and the map of the tab
@@ -287,20 +306,6 @@ internal fun HeapDumpExplorer(
     session.describing(hoveredPlace) { hoveredDetails = it }
   }
 
-  // What each tab is called. A label is cheap enough to draw on every rectangle of the map, so naming a
-  // strip of them is one small read — but it is still a read, which is why a tab opened in the background
-  // is named a beat after it appears rather than not at all.
-  val unnamedPlaces = tabs.tabs.map { it.place }.filter { it.title == null && it !in placeTitles }
-  LaunchedEffect(session, unnamedPlaces) {
-    if (unnamedPlaces.isEmpty()) {
-      return@LaunchedEffect
-    }
-    val named = session.read("what to call ${unnamedPlaces.size} tabs") { explorer ->
-      unnamedPlaces.associateWith { explorer.tree.titleOf(it) }
-    }
-    placeTitles = placeTitles + named
-  }
-
   // The panel shows the bitmap it describes as big as the panel is wide, so its pixels are read again at
   // that size: a treemap rectangle is a couple of hundred pixels across and this is four times that.
   var describedBitmap: ImageBitmap? by remember { mutableStateOf(null) }
@@ -409,6 +414,12 @@ internal fun HeapDumpExplorer(
    * does the same thing" has to mean to be true.
    */
   val open: (Place, OpenIn) -> Unit = { destination, openIn ->
+    // Named before the tab exists, from the view the click came from: the read that names tabs is a beat
+    // behind however small it is, and a tab that opens under a placeholder is one whose title, and width,
+    // change as you watch. Everything a view draws is named already, which is most of what is ever clicked.
+    view.presentation.cells.titleOf(destination)?.let { title ->
+      placeTitles = placeTitles + (destination to title)
+    }
     tabs = when (openIn) {
       OpenIn.CURRENT_TAB -> tabs.goTo(destination)
       OpenIn.NEW_TAB -> tabs.open(destination, inBackground = true)
@@ -1102,6 +1113,16 @@ internal sealed interface ViewPresentation {
   data class Radial(val presentation: RadialPresentation) : ViewPresentation
 
   data class Stack(val presentation: StackPresentation) : ViewPresentation
+
+  /**
+   * What was drawn and what it was named, whatever shape it came out as: every place a click on the view
+   * leads to, already named. See [titleOf].
+   */
+  val cells: List<PresentedCell<*>> get() = when (this) {
+    is Treemap -> presentation.cells
+    is Radial -> presentation.cells
+    is Stack -> presentation.cells
+  }
 }
 
 /** What a laid out view amounts to, for the log: one that drew nothing at all says so here. */

shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/TabStrip.kt

@@ -1,21 +1,27 @@
 package shark.explorer.app
 
+import androidx.compose.animation.AnimatedVisibility
+import androidx.compose.animation.core.MutableTransitionState
+import androidx.compose.animation.core.tween
+import androidx.compose.animation.expandHorizontally
+import androidx.compose.animation.fadeIn
 import androidx.compose.foundation.ExperimentalFoundationApi
 import androidx.compose.foundation.PointerMatcher
 import androidx.compose.foundation.background
 import androidx.compose.foundation.clickable
-import androidx.compose.foundation.horizontalScroll
 import androidx.compose.foundation.layout.Arrangement
+import androidx.compose.foundation.layout.FlowRow
 import androidx.compose.foundation.layout.Row
 import androidx.compose.foundation.layout.fillMaxWidth
 import androidx.compose.foundation.layout.padding
 import androidx.compose.foundation.layout.widthIn
 import androidx.compose.foundation.onClick
-import androidx.compose.foundation.rememberScrollState
 import androidx.compose.foundation.selection.selectable
 import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.Text
 import androidx.compose.runtime.Composable
+import androidx.compose.runtime.key
+import androidx.compose.runtime.remember
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.input.pointer.PointerButton
@@ -27,16 +33,18 @@ import shark.explorer.Place
 import shark.explorer.Tabs
 
 /**
- * The tabs open in this window, left to right, as a strip under the bar that opens them.
+ * The tabs open in this window, left to right and wrapping onto a line of its own once a line is full, as
+ * a strip under the bar that opens them.
  *
  * Every tab is closeable, the last one included: a window with no tab still holds the heap dump it spent
  * seconds reading, and the bar above is one click from a tab again. See [Tabs].
  *
- * The strip scrolls rather than shrinking its tabs to nothing, because what a tab is called — which class,
+ * Neither shrinking its tabs nor scrolling them off the edge, because what a tab is called — which class,
  * and which instance of it — is the whole of how a strip of a dozen instances of one class is one you can
- * pick out of.
+ * pick out of, and a tab you have to go looking for is one you may as well not have opened. So the strip
+ * grows down into the window instead: what it costs is the view's height, and only for someone who has
+ * opened enough tabs to be reading across them anyway.
  */
-@OptIn(ExperimentalFoundationApi::class)
 @Composable
 internal fun TabStrip(
   tabs: Tabs,
@@ -49,53 +57,99 @@ internal fun TabStrip(
   if (tabs.tabs.isEmpty()) {
     return
   }
-  Row(
-    modifier.fillMaxWidth().horizontalScroll(rememberScrollState()).padding(horizontal = 4.dp),
+  FlowRow(
+    modifier.fillMaxWidth().padding(horizontal = 4.dp),
     horizontalArrangement = Arrangement.spacedBy(2.dp),
-    verticalAlignment = Alignment.Bottom
+    verticalArrangement = Arrangement.spacedBy(2.dp),
+    itemVerticalAlignment = Alignment.Bottom
   ) {
     tabs.tabs.forEach { tab ->
-      val isSelected = tab.id == tabs.selectedId
-      Row(
-        Modifier
-          .background(
-            if (isSelected) {
-              MaterialTheme.colorScheme.surface
-            } else {
-              MaterialTheme.colorScheme.surfaceVariant
-            }
-          )
-          // Middle clicking a tab closes it, which is the other half of middle clicking opening one.
-          .onClick(matcher = PointerMatcher.mouse(PointerButton.Tertiary)) { onClose(tab.id) }
-          // Selectable rather than clickable, because a strip of these is a set with one of them on: it
-          // is also what tells a tab apart from the button and the chain row that lead to the same place.
-          .selectable(selected = isSelected, role = Role.Tab) { onSelect(tab.id) }
-          .padding(start = 8.dp, end = 4.dp, top = 4.dp, bottom = 4.dp)
-          .widthIn(max = MAX_TAB_WIDTH),
-        horizontalArrangement = Arrangement.spacedBy(4.dp),
-        verticalAlignment = Alignment.CenterVertically
-      ) {
-        Text(
-          titleOf(tab.place),
-          style = MaterialTheme.typography.bodySmall,
-          fontWeight = if (isSelected) FontWeight.Bold else FontWeight.Normal,
-          maxLines = 1,
-          overflow = TextOverflow.Ellipsis,
-          modifier = Modifier.weight(1f, fill = false)
-        )
-        // Its own click target rather than a modifier on the tab, so that closing a tab is never
-        // selecting it first: a strip where closing the fourth tab shows you the fourth tab is a strip
-        // that fights back.
-        Text(
-          CLOSE_TAB,
-          Modifier.clickable { onClose(tab.id) }.padding(horizontal = 2.dp),
-          style = MaterialTheme.typography.bodySmall
+      // Keyed on the tab rather than on where it is in the strip, because a tab opens *beside* the one it
+      // was opened from: without this, inserting one in the middle would hand its state to the tab that
+      // used to be there, and the one that grew in would be whichever ended up last.
+      key(tab.id) {
+        TabView(
+          title = titleOf(tab.place),
+          isSelected = tab.id == tabs.selectedId,
+          onSelect = { onSelect(tab.id) },
+          onClose = { onClose(tab.id) }
         )
       }
     }
   }
 }
 
+/**
+ * One tab, growing into the strip as it opens.
+ *
+ * Widening from nothing rather than appearing at full width, the way a browser does it, because a tab
+ * opened in the background is one nothing else on screen announces: what says a middle click did anything
+ * at all is the strip moving over to make room. It also puts the tab where it went — beside the one it was
+ * opened from, which is not where a strip that pops a tab in reads as having put it.
+ *
+ * Closing is not animated, deliberately: a strip that holds a tab open for a moment after it was closed is
+ * one where the tab under the pointer isn't the tab a second click closes.
+ */
+@OptIn(ExperimentalFoundationApi::class)
+@Composable
+private fun TabView(
+  title: String,
+  isSelected: Boolean,
+  onSelect: () -> Unit,
+  onClose: () -> Unit
+) {
+  // Starting closed and opening on the first composition, which is what makes this animate on the way in:
+  // a tab that was already open when the strip drew it has nothing to animate.
+  val opening = remember { MutableTransitionState(false) }
+  opening.targetState = true
+  AnimatedVisibility(
+    visibleState = opening,
+    // From the start edge, so the tab holds the spot it was inserted at and pushes the ones after it along,
+    // rather than sliding in from under its neighbour.
+    enter = expandHorizontally(
+      animationSpec = tween(TAB_OPEN_MILLIS),
+      expandFrom = Alignment.Start
+    ) + fadeIn(tween(TAB_OPEN_MILLIS))
+  ) {
+    Row(
+      Modifier
+        .background(
+          if (isSelected) {
+            MaterialTheme.colorScheme.surface
+          } else {
+            MaterialTheme.colorScheme.surfaceVariant
+          }
+        )
+        // Middle clicking a tab closes it, which is the other half of middle clicking opening one.
+        .onClick(matcher = PointerMatcher.mouse(PointerButton.Tertiary)) { onClose() }
+        // Selectable rather than clickable, because a strip of these is a set with one of them on: it
+        // is also what tells a tab apart from the button and the chain row that lead to the same place.
+        .selectable(selected = isSelected, role = Role.Tab) { onSelect() }
+        .padding(start = 8.dp, end = 4.dp, top = 4.dp, bottom = 4.dp)
+        .widthIn(max = MAX_TAB_WIDTH),
+      horizontalArrangement = Arrangement.spacedBy(4.dp),
+      verticalAlignment = Alignment.CenterVertically
+    ) {
+      Text(
+        title,
+        style = MaterialTheme.typography.bodySmall,
+        fontWeight = if (isSelected) FontWeight.Bold else FontWeight.Normal,
+        maxLines = 1,
+        overflow = TextOverflow.Ellipsis,
+        modifier = Modifier.weight(1f, fill = false)
+      )
+      // Its own click target rather than a modifier on the tab, so that closing a tab is never
+      // selecting it first: a strip where closing the fourth tab shows you the fourth tab is a strip
+      // that fights back.
+      Text(
+        CLOSE_TAB,
+        Modifier.clickable { onClose() }.padding(horizontal = 2.dp),
+        style = MaterialTheme.typography.bodySmall
+      )
+    }
+  }
+}
+
 /**
  * Shown where a tab would be once the last one has been closed.
  *
@@ -110,3 +164,11 @@ internal const val CLOSE_TAB = "✕"
 
 /** Wide enough for a class name and an address, short enough that ten tabs are all still on the strip. */
 private val MAX_TAB_WIDTH = 220.dp
+
+/**
+ * How long a tab takes to grow into the strip.
+ *
+ * Long enough to be read as the strip making room, short enough that a middle click and the tab being
+ * there are one thing: a tab you have to wait for is one you would rather had just appeared.
+ */
+private const val TAB_OPEN_MILLIS = 150

shark/shark-explorer/shark-explorer-app/src/test/java/shark/explorer/app/TabStripTest.kt

@@ -77,6 +77,25 @@ class TabStripTest {
     }
   }
 
+  @Test fun `tabs that no longer fit across the window go on a line of their own`() {
+    explorerUiTest {
+      openHeapDump()
+
+      repeat(TABS_PAST_ONE_LINE) { screenButton(Place.OBJECTS_LABEL).performClick() }
+
+      waitUntil(timeoutMillis = OPEN_TIMEOUT_MILLIS) {
+        tabs().fetchSemanticsNodes().size == TABS_PAST_ONE_LINE + 1
+      }
+      // Every tab still readable and still on screen, rather than squeezed to nothing or scrolled off the
+      // edge: which class and which instance of it is the whole of what a tab is for, and a tab you have
+      // to go looking for is one you may as well not have opened.
+      waitForIdle()
+      val bounds = tabs().fetchSemanticsNodes().map { it.boundsInRoot }
+      assertThat(bounds.map { it.top }.distinct()).hasSizeGreaterThan(1)
+      assertThat(bounds).allMatch { it.right <= WINDOW_WIDTH.value }
+    }
+  }
+
   @Test fun `clicking an object inside a tab moves that tab rather than opening one`() {
     explorerUiTest {
       openHeapDump()
@@ -103,6 +122,23 @@ class TabStripTest {
     }
   }
 
+  @Test fun `a tab opened from the view is named without reading the heap dump for it`() {
+    explorerUiTest {
+      openHeapDump()
+      // The window's first tab is named by a read of its own, there being no view yet to have drawn it.
+      val readsBefore = logged.count { it.startsWith(NAMING_READ) }
+
+      middleClickTheArray()
+
+      waitUntil(timeoutMillis = OPEN_TIMEOUT_MILLIS) { tabs().fetchSemanticsNodes().size == 2 }
+      tab(tabTitleOfTheArray()).assertIsDisplayed()
+      // Named from the rectangle that was clicked, which the view labelled when it laid the tree out. A
+      // read to name it would land a beat after the tab is on the strip, however small a read it is, and
+      // the placeholder it replaces is what someone watching sees as the tab flickering.
+      assertThat(logged.filter { it.startsWith(NAMING_READ) }).hasSize(readsBefore)
+    }
+  }
+
   @Test fun `middle clicking an object opens it in a tab behind the one being read`() {
     explorerUiTest {
       openHeapDump()
@@ -202,6 +238,16 @@ class TabStripTest {
     /** Opening a heap dump and laying the tree out both happen on another thread. */
     private const val OPEN_TIMEOUT_MILLIS = 10_000L
 
+    /** What the window logs when it has to read the heap dump to find out what to call a tab. */
+    private const val NAMING_READ = "Reading what to call"
+
+    /**
+     * Enough tabs off the bar that they can't all fit across a window, whose width a UI test fixes at
+     * [WINDOW_WIDTH]. A tab named after the object list is one of the narrower ones there is, so this is
+     * comfortably past what a line holds rather than exactly it.
+     */
+    private const val TABS_PAST_ONE_LINE = 20
+
     /** An `adb` that answers as if nothing were plugged in, so no test here reaches a real device. */
     private val NO_DEVICE_ADB = Adb { AdbOutput(exitCode = 0, text = "List of devices attached\n") }
   }

shark/shark-explorer/shark-explorer-core/src/main/java/shark/explorer/Place.kt

@@ -146,3 +146,36 @@ fun HeapDominatorTreemap.titleOf(place: Place): String = when (place) {
   is Place.Leaks -> place.title
   is Place.Starred -> place.title
 }
+
+/**
+ * What a tab opened at [place] is called, from the name a view already drew there, or null for a place
+ * none of these cells stands for.
+ *
+ * The same answer [titleOf] gives, and not a heap dump read: a cell was named when the view was laid out,
+ * so a tab opened by clicking one can be named as it opens instead of a beat later. A beat later is a tab
+ * whose title, and with it its width, change in front of whoever opened it — which is a flicker, and every
+ * cell of every view is one a click opens a tab at.
+ */
+fun List<PresentedCell<*>>.titleOf(place: Place): String? =
+  firstOrNull { Place.of(it.cell) == place }?.title()
+
+/**
+ * What a tab open where this cell leads is called.
+ *
+ * A cell is labelled with a class name and nothing else, because that is all a rectangle has room for. A
+ * tab has room for which instance of it this is, and needs it — see [titleOf].
+ */
+private fun PresentedCell<*>.title(): String {
+  val place = Place.of(cell)
+  // Only one object of the heap dump has an address to be named by: a pile of them is named by how many
+  // it holds, and the whole heap dump is no object at all.
+  return if (
+    place is Place.Object &&
+    content is CellContent.Object &&
+    place.objectId != HeapDominatorTreemap.ROOT_OBJECT_ID
+  ) {
+    "$label ${hexObjectId(place.objectId)}"
+  } else {
+    label
+  }
+}