From aa3c88a6c4b129e71dd5338f44d5d5ef63b9ed60 Mon Sep 17 00:00:00 2001
From: Samuel Degueldre <sad@odoo.com>
Date: Fri, 23 Jun 2023 09:52:49 +0200
Subject: [PATCH] [IMP] devtools: present observed keys in a more compact way

Previously, the keys were displayed separately from the target, on top
of being displayed in bold inside the object when unfolded. This is
redundant, and in most cases you have to unfold the object anyway
because there are more keys observed than can be displayed.

The only "key" that cannot be displayed in bold inside the object is the
"key changes" magic key. This commit replaces it by a small +/- badge on
the right of the object's short content.

This commit also stops displaying observed object that are nested inside
other observed objects at the top level, as it can be confusing, and it
also heuristically gives a name to observed objects: if the observed
object is a property of the component or one of its props it will be
named accordingly, if not it will be named `[unknown]` (which may happen
when reobserving reactive objects inside a service or inside the env,
but is pretty rare in practice).
---
 .../details_window/details_window.js          |   3 +-
 .../details_window/details_window.xml         |   6 +-
 .../object_tree_element.js                    |  19 ++-
 .../object_tree_element.xml                   |   8 +-
 .../subscriptions/subscriptions.js            |  30 ----
 .../subscriptions/subscriptions.xml           |  24 ---
 tools/devtools/src/main.css                   |   4 +
 .../page_scripts/owl_devtools_global_hook.js  | 150 +++++++++++-------
 8 files changed, 112 insertions(+), 132 deletions(-)
 delete mode 100644 tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/subscriptions/subscriptions.js
 delete mode 100644 tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/subscriptions/subscriptions.xml

diff --git a/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/details_window.js b/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/details_window.js
index 9cca7882..1934c968 100644
--- a/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/details_window.js
+++ b/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/details_window.js
@@ -1,11 +1,10 @@
 const { Component, useRef, useEffect } = owl;
 import { useStore } from "../../../store/store";
 import { ObjectTreeElement } from "./object_tree_element/object_tree_element";
-import { Subscriptions } from "./subscriptions/subscriptions";
 
 export class DetailsWindow extends Component {
   static template = "devtools.DetailsWindow";
-  static components = { ObjectTreeElement, Subscriptions };
+  static components = { ObjectTreeElement };
   setup() {
     this.store = useStore();
     this.contextMenu = useRef("contextmenu");
diff --git a/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/details_window.xml b/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/details_window.xml
index 5fff5d1e..ba477b71 100644
--- a/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/details_window.xml
+++ b/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/details_window.xml
@@ -53,7 +53,11 @@
           </div>
           <i title="Store observed states as global variable in the console" class="fa fa-bug utility-icon p-1" t-on-click.stop="() => this.store.logObjectInConsole([...this.store.activeComponent.path, {type: 'item', value: 'subscriptions'}])"></i>
         </div>
-        <Subscriptions t-if="store.activeComponent.subscriptions.toggled"/>
+        <div t-if="store.activeComponent.subscriptions.toggled" id="subscriptionsPanel">
+          <t t-foreach="store.activeComponent.subscriptions.children" t-as="subscription" t-key="subscription_index">
+            <ObjectTreeElement object="subscription.target"/>
+          </t>
+        </div>
       </div>
       <div t-if="store.activeComponent.instance.children.length > 0"  id="instance" class="details-panel ps-2 py-1">
         <div class="d-flex mb-2">
diff --git a/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/object_tree_element/object_tree_element.js b/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/object_tree_element/object_tree_element.js
index b38ec4ff..f659e45d 100644
--- a/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/object_tree_element/object_tree_element.js
+++ b/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/object_tree_element/object_tree_element.js
@@ -41,23 +41,22 @@ export class ObjectTreeElement extends Component {
     return JSON.stringify(this.props.object.path);
   }
 
-  get objectName() {
-    return this.props.object.name;
+  get keyChanges() {
+    return this.props.object.keys?.includes("Symbol(Key changes)");
   }
 
-  get objectLineClass() {
+  classFor(object) {
     // Prototype items will be dyed down to appear less important
-    if (this.pathAsString.includes('{"type":"prototype",')) {
-      return { attenuate: true };
+    if (object.path.some((item) => item?.type === "prototype")) {
+      return "attenuate";
     }
     // Same for subscription items which are not present in the keys while the keys will be bold
-    if (this.props.object.objectType === "subscription" && this.props.object.depth > 0) {
-      if (this.props.keys.includes(this.props.object.name.toString())) {
-        return { "fw-bolder": true };
+    if (object.objectType === "subscription" && object.depth > 0) {
+      if (this.props.object.keys?.includes(object.name.toString())) {
+        return "fw-bolder";
       }
-      return { attenuate: true };
+      return "attenuate";
     }
-    return {};
   }
 
   get objectPadding() {
diff --git a/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/object_tree_element/object_tree_element.xml b/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/object_tree_element/object_tree_element.xml
index ec84bd88..4e2121be 100644
--- a/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/object_tree_element/object_tree_element.xml
+++ b/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/object_tree_element/object_tree_element.xml
@@ -2,7 +2,7 @@
 <templates xml:space="preserve">
   <t t-name="devtools.ObjectTreeElement" owl="1">
     <div class="m-0 p-0 text-nowrap w-100 object-line"
-      t-att-class="objectLineClass" 
+      t-att-class="props.class"
       t-on-click.stop="() => this.store.toggleObjectTreeElementsDisplay(this.props.object)" 
       t-on-contextmenu.prevent="openMenu" 
     >
@@ -11,7 +11,7 @@
           t-att-class="{'fa-caret-right': !props.object.toggled, 'fa-caret-down': props.object.toggled}"
           t-attf-style="visibility: {{props.object.hasChildren ? '' : 'hidden'}};"
         />
-        <t t-esc="objectName"/>
+        <t t-esc="props.object.name"/>
         <t t-if="props.object.content.length > 0">: </t> 
         <t t-if="props.object.contentType == 'getter'">
           <span class="getter-content object-content" t-att-class="objectLineClass" t-on-click.stop="() => this.store.loadGetterContent(this.props.object)">
@@ -28,6 +28,7 @@
             </t>
           </span> 
         </t>
+        <span t-if="keyChanges" class="key-changes ms-1 badge p-1" title="Key additions/deletions are observed">+/-</span>
       </div>
     </div>
     <div t-if="store.contextMenu.activeMenu === contextMenuId" class="custom-menu" t-ref="contextmenu">
@@ -40,8 +41,7 @@
     </div>
     <t t-if="props.object.toggled" t-key="contextMenuId">
       <t t-foreach="props.object.children" t-as="child" t-key="child_index">
-        <ObjectTreeElement t-if="props.object.objectType === 'subscription'" object="child" keys="props.keys"/>
-        <ObjectTreeElement t-else="" object="child"/>
+        <ObjectTreeElement object="child" class="this.classFor(child)"/>
       </t>
     </t>
   </t>
diff --git a/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/subscriptions/subscriptions.js b/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/subscriptions/subscriptions.js
deleted file mode 100644
index bc8cb9ee..00000000
--- a/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/subscriptions/subscriptions.js
+++ /dev/null
@@ -1,30 +0,0 @@
-const { Component } = owl;
-import { useStore } from "../../../../store/store";
-import { ObjectTreeElement } from "../object_tree_element/object_tree_element";
-
-export class Subscriptions extends Component {
-  static template = "devtools.Subscriptions";
-
-  static components = { ObjectTreeElement };
-
-  setup() {
-    this.store = useStore();
-  }
-
-  // Used to display the keys in a compact way
-  keysContent(index) {
-    const keys = this.store.activeComponent.subscriptions.children[index].keys;
-    let content = JSON.stringify(keys);
-    const maxLength = 50;
-    content = content.replace(/,/g, ", ");
-    if (content.length > maxLength) {
-      content = content.slice(0, content.lastIndexOf(",", maxLength - 5)) + ", ...]";
-    }
-    return content;
-  }
-
-  expandKeys(event, index) {
-    this.store.activeComponent.subscriptions.children[index].keysExpanded =
-      !this.store.activeComponent.subscriptions.children[index].keysExpanded;
-  }
-}
diff --git a/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/subscriptions/subscriptions.xml b/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/subscriptions/subscriptions.xml
deleted file mode 100644
index 551f51c2..00000000
--- a/tools/devtools/src/devtools_app/devtools_window/components_tab/details_window/subscriptions/subscriptions.xml
+++ /dev/null
@@ -1,24 +0,0 @@
-<?xml version="1.0" encoding="UTF-8" ?>
-<templates xml:space="preserve">
-  <t t-name="devtools.Subscriptions" owl="1">
-    <div id="subscriptionsPanel">
-      <t t-foreach="store.activeComponent.subscriptions.children" t-as="subscription" t-key="subscription_index">
-        <div class="my-2">
-          <div class="my-0 p-0 object-line" t-on-click.stop="(ev) => this.expandKeys(ev, subscription_index)">
-            <span class="ps-1 text-nowrap">
-              <i class="fa fa-caret-right ms-1" t-attf-style="cursor: pointer;{{subscription.keysExpanded ? 'transform: rotate(90deg);' : ''}}"></i>
-              keys: <span class="key-name"><t t-esc="this.keysContent(subscription_index)"/></span>
-            </span>
-          </div>
-          <div t-foreach="subscription.keys" t-as="key" t-key="key_index" class="my-0 p-0 object-line" t-attf-style="display: {{subscription.keysExpanded ? 'flex' : 'none'}}">
-            <div style="transform: translateX(calc(1.1rem))" class="key-content">
-              <i class="fa fa-caret-right mx-1" t-attf-style="cursor: pointer; visibility: hidden;"></i>
-              <t t-esc="key"/>
-            </div>
-          </div>
-          <ObjectTreeElement object="subscription.target" keys="subscription.keys"/>
-        </div>
-      </t>
-    </div>
-  </t>
-</templates>
\ No newline at end of file
diff --git a/tools/devtools/src/main.css b/tools/devtools/src/main.css
index bb0c0748..5b8e6a5f 100644
--- a/tools/devtools/src/main.css
+++ b/tools/devtools/src/main.css
@@ -126,6 +126,10 @@
   color: var(--prototype-color);
 }
 
+.key-changes {
+  background-color: var(--version-bg);
+}
+
 .event-container {
   border-bottom: 1px solid rgb(240, 238, 238);
   padding-top: 2px!important;
diff --git a/tools/devtools/src/page_scripts/owl_devtools_global_hook.js b/tools/devtools/src/page_scripts/owl_devtools_global_hook.js
index 49d88484..519f3b24 100644
--- a/tools/devtools/src/page_scripts/owl_devtools_global_hook.js
+++ b/tools/devtools/src/page_scripts/owl_devtools_global_hook.js
@@ -785,57 +785,47 @@
           }
           break;
       }
-      if (child.contentType) {
-        if (child.toggled) {
-          child.children = this.loadObjectChildren(
-            child.path,
-            child.depth,
-            child.contentType,
-            child.objectType,
-            oldTree
-          );
-        }
-        return child;
-      }
-      if (obj === null) {
-        child.content = "null";
-        child.contentType = "object";
-        child.hasChildren = false;
-      } else if (obj === undefined) {
-        child.content = "undefined";
-        child.contentType = "undefined";
-        child.hasChildren = false;
-      } else {
-        obj = this.toRaw(obj);
-        switch (true) {
-          case obj instanceof Map:
-            child.contentType = "map";
-            child.hasChildren = true;
-            break;
-          case obj instanceof Set:
-            child.contentType = "set";
-            child.hasChildren = true;
-            break;
-          case obj instanceof Array:
-            child.contentType = "array";
-            child.hasChildren = obj.length > 0;
-            break;
-          case typeof obj === "function":
-            child.contentType = "function";
-            child.hasChildren = true;
-            break;
-          case obj instanceof Object:
-            child.contentType = "object";
-            child.hasChildren = Object.keys(obj).length > 0;
-            break;
-          default:
-            child.contentType = typeof obj;
-            child.hasChildren = false;
-        }
-        if (key.type === "set entry") {
-          child.content = this.serializer.serializeItem(obj, true);
+      if (!child.contentType) {
+        if (obj === null) {
+          child.content = "null";
+          child.contentType = "object";
+          child.hasChildren = false;
+        } else if (obj === undefined) {
+          child.content = "undefined";
+          child.contentType = "undefined";
+          child.hasChildren = false;
         } else {
-          child.content = this.serializer.serializeContent(obj, child.contentType);
+          obj = this.toRaw(obj);
+          switch (true) {
+            case obj instanceof Map:
+              child.contentType = "map";
+              child.hasChildren = true;
+              break;
+            case obj instanceof Set:
+              child.contentType = "set";
+              child.hasChildren = true;
+              break;
+            case obj instanceof Array:
+              child.contentType = "array";
+              child.hasChildren = obj.length > 0;
+              break;
+            case typeof obj === "function":
+              child.contentType = "function";
+              child.hasChildren = true;
+              break;
+            case obj instanceof Object:
+              child.contentType = "object";
+              child.hasChildren = Object.keys(obj).length > 0;
+              break;
+            default:
+              child.contentType = typeof obj;
+              child.hasChildren = false;
+          }
+          if (key.type === "set entry") {
+            child.content = this.serializer.serializeItem(obj, true);
+          } else {
+            child.content = this.serializer.serializeContent(obj, child.contentType);
+          }
         }
       }
       if (child.toggled) {
@@ -847,6 +837,7 @@
           oldTree
         );
       }
+      this.addHighlightedKeys(child);
       return child;
     }
 
@@ -1269,16 +1260,15 @@
           children: [],
         };
       } else {
-        const rawSubscriptions = node.subscriptions;
+        const rawSubscriptions = this.topLevelSubscriptions(node);
         component.subscriptions = {
           toggled: oldTree ? oldTree.subscriptions.toggled : true,
           children: [],
         };
         rawSubscriptions.forEach((rawSubscription, index) => {
           let subscription = {
-            keys: [],
             target: {
-              name: "target",
+              name: this.targetName(rawSubscription.target, node),
               contentType:
                 typeof rawSubscription.target === "object"
                   ? Array.isArray(rawSubscription.target)
@@ -1295,7 +1285,6 @@
               toggled: false,
               objectType: "subscription",
             },
-            keysExpanded: false,
           };
           if (
             oldTree &&
@@ -1304,13 +1293,6 @@
           ) {
             subscription.target.toggled = true;
           }
-          rawSubscription.keys.forEach((key) => {
-            if (typeof key === "symbol") {
-              subscription.keys.push(key.toString());
-            } else {
-              subscription.keys.push(key);
-            }
-          });
           if (rawSubscription.target == null) {
             if (subscription.target.contentType === "undefined") {
               subscription.target.content = "undefined";
@@ -1340,6 +1322,7 @@
               oldTree
             );
           }
+          this.addHighlightedKeys(subscription.target);
           component.subscriptions.children.push(subscription);
         });
       }
@@ -1672,6 +1655,51 @@
         inspect(obj);
       }
     }
+
+    targetName(target, node) {
+      // check on component
+      const { component } = node;
+      for (const [key, value] of Object.entries(component)) {
+        if (target === this.toRaw(value)) {
+          return key;
+        }
+      }
+      // check on props
+      for (const [key, value] of Object.entries(component.props)) {
+        if (target === this.toRaw(value)) {
+          return `props.${key}`;
+        }
+      }
+      return "[unknown]";
+    }
+
+    /**
+     * Removes subscriptions that are a direct child of another subscription:
+     * they will be reachable from the top level by expanding observed keys.
+     *
+     * @param {ComponentNode} node
+     * @returns {{ keys: PropertyKey[], target: unknown}[]} the top level
+     *  subscriptions of the node
+     */
+    topLevelSubscriptions(node) {
+      const subscriptions = node.subscriptions;
+      const toOmit = new Set(
+        subscriptions.flatMap(({ keys, target }) => keys.map((k) => this.toRaw(target[k])))
+      );
+      return subscriptions.filter(({ target }) => !toOmit.has(target));
+    }
+
+    addHighlightedKeys(child) {
+      const { path } = child;
+      const subscriptionIndex = path.findIndex((item) => typeof item !== "string");
+      if (path[subscriptionIndex]?.value === "subscriptions") {
+        const node = this.getComponentNode(path.slice(0, subscriptionIndex));
+        // Add observed keys
+        const targetToKeys = new Map(node.subscriptions.map(({ keys, target }) => [target, keys]));
+        const target = this.getObjectProperty(child.path);
+        child.keys = targetToKeys.get(target)?.map((k) => String(k));
+      }
+    }
   }
 
   function checkOwlStatus() {