From 79827c535bda8ada21a2dca8138b4a218788419e Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 9 Apr 2019 18:10:17 -0700 Subject: [PATCH 01/12] Added isToggled boolean to tree nodes and toggleIsCollapsed() method to Store --- src/devtools/store.js | 49 +++++++++++++++++++++++--- src/devtools/views/Components/types.js | 3 ++ 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/devtools/store.js b/src/devtools/store.js index 225f9a8ac8..424ec333eb 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -287,16 +287,18 @@ export default class Store extends EventEmitter { let currentElement = ((this._idToElement.get(firstChildID): any): Element); let currentWeight = rootWeight; while (index !== currentWeight) { - for (let i = 0; i < currentElement.children.length; i++) { + const numChildren = currentElement.children.length; + for (let i = 0; i < numChildren; i++) { const childID = currentElement.children[i]; const child = ((this._idToElement.get(childID): any): Element); - const { weight } = child; - if (index <= currentWeight + weight) { + const childWeight = child.isCollapsed ? 1 : child.weight; + + if (index <= currentWeight + childWeight) { currentWeight++; currentElement = child; break; } else { - currentWeight += weight; + currentWeight += childWeight; } } } @@ -420,6 +422,28 @@ export default class Store extends EventEmitter { this.emit('isProfiling'); } + toggleIsCollapsed(id: number, isCollapsed: boolean): void { + const element = this.getElementByID(id); + if (element !== null) { + element.isCollapsed = isCollapsed; + + const weightDelta = isCollapsed ? 1 - element.weight : element.weight - 1; + + this._numElements += weightDelta; + + let parentElement = this._idToElement.get(element.parentID); + while (parentElement != null) { + parentElement.weight += weightDelta; + parentElement = this._idToElement.get(parentElement.parentID); + } + + // The Tree context's search reducer expects an explicit list of ids for nodes that were added or removed. + // In this case, we can pass it empty arrays since nodes in a collapsed tree are still there (just hidden). + // Updating the selected search index later may require auto-expanding a collapsed subtree though. + this.emit('mutated', [[], []]); + } + } + _captureScreenshot = throttle( memoize((commitIndex: number) => { this._bridge.send('captureScreenshot', { commitIndex }); @@ -518,6 +542,7 @@ export default class Store extends EventEmitter { depth: -1, displayName: null, id, + isCollapsed: false, key: null, ownerID: 0, parentID: 0, @@ -566,6 +591,7 @@ export default class Store extends EventEmitter { depth: parentElement.depth + 1, displayName, id, + isCollapsed: false, key, ownerID, parentID: parentElement.id, @@ -715,14 +741,27 @@ export default class Store extends EventEmitter { throw Error(`Unsupported Bridge operation ${operation}`); } - this._numElements += weightDelta; + let isInsideCollapsedSubTree = false; while (parentElement != null) { parentElement.weight += weightDelta; + + // Additions and deletions within a collapsed subtree should not bubble beyond the collapsed parent. + // Their weight will bubble up when the parent is expanded. + if (parentElement.isCollapsed) { + isInsideCollapsedSubTree = true; + break; + } + parentElement = ((this._idToElement.get( parentElement.parentID ): any): Element); } + + // Additions and deletions within a collapsed subtree should not affect the overall number of elements. + if (!isInsideCollapsedSubTree) { + this._numElements += weightDelta; + } } this._revision++; diff --git a/src/devtools/views/Components/types.js b/src/devtools/views/Components/types.js index fc71d5e9bb..ca3c7e3df0 100644 --- a/src/devtools/views/Components/types.js +++ b/src/devtools/views/Components/types.js @@ -14,6 +14,9 @@ export type Element = {| displayName: string | null, key: number | string | null, + // Should the elements children be visible in the tree? + isCollapsed: boolean, + // Owner (if available) ownerID: number, From b6df86c90c43a8346013a2d3b5a4ecde28c2eef8 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 9 Apr 2019 18:10:57 -0700 Subject: [PATCH 02/12] Added toggle button to Tree > Element views --- src/devtools/views/ButtonIcon.js | 12 ++++++ src/devtools/views/Components/Element.css | 10 +++++ src/devtools/views/Components/Element.js | 46 +++++++++++++++++++++-- 3 files changed, 64 insertions(+), 4 deletions(-) diff --git a/src/devtools/views/ButtonIcon.js b/src/devtools/views/ButtonIcon.js index 7aa08b3c6c..c27e63183c 100644 --- a/src/devtools/views/ButtonIcon.js +++ b/src/devtools/views/ButtonIcon.js @@ -7,8 +7,10 @@ export type IconType = | 'back' | 'cancel' | 'close' + | 'collapsed' | 'copy' | 'down' + | 'expanded' | 'export' | 'filter' | 'import' @@ -40,12 +42,18 @@ export default function ButtonIcon({ type }: Props) { case 'close': pathData = PATH_CLOSE; break; + case 'collapsed': + pathData = PATH_COLLAPSED; + break; case 'copy': pathData = PATH_COPY; break; case 'down': pathData = PATH_DOWN; break; + case 'expanded': + pathData = PATH_EXPANDED; + break; case 'export': pathData = PATH_EXPORT; break; @@ -121,6 +129,8 @@ const PATH_CANCEL = ` const PATH_CLOSE = 'M19 6.41L17.59 5 12 10.59 6.41 5 5 6.41 10.59 12 5 17.59 6.41 19 12 13.41 17.59 19 19 17.59 13.41 12z'; +const PATH_COLLAPSED = 'M10 17l5-5-5-5v10z'; + const PATH_COPY = ` M3 13h2v-2H3v2zm0 4h2v-2H3v2zm2 4v-2H3a2 2 0 0 0 2 2zM3 9h2V7H3v2zm12 12h2v-2h-2v2zm4-18H9a2 2 0 0 0-2 2v10a2 2 0 0 0 2 2h10c1.1 0 2-.9 2-2V5c0-1.1-.9-2-2-2zm0 12H9V5h10v10zm-8 6h2v-2h-2v2zm-4 0h2v-2H7v2z @@ -128,6 +138,8 @@ const PATH_COPY = ` const PATH_DOWN = 'M7.41 8.59L12 13.17l4.59-4.58L18 10l-6 6-6-6 1.41-1.41z'; +const PATH_EXPANDED = 'M7 10l5 5 5-5z'; + const PATH_EXPORT = 'M15.82,2.14v7H21l-9,9L3,9.18H8.18v-7ZM3,20.13H21v1.73H3Z'; const PATH_FILTER = 'M10 18h4v-2h-4v2zM3 6v2h18V6H3zm3 7h12v-2H6v2z'; diff --git a/src/devtools/views/Components/Element.css b/src/devtools/views/Components/Element.css index 4f1748c9c1..72f6d68a2f 100644 --- a/src/devtools/views/Components/Element.css +++ b/src/devtools/views/Components/Element.css @@ -7,6 +7,8 @@ align-items: center; cursor: default; user-select: none; + + --color-expand-collapse-toggle: var(--color-dim); } .Element:hover { background-color: var(--color-hover-background); @@ -21,6 +23,7 @@ --color-jsx-arrow-brackets: var(--color-jsx-arrow-brackets-inverted); --color-attribute-name: var(--color-hover-background); --color-attribute-value: var(--color-component-name-inverted); + --color-expand-collapse-toggle: var(--color-component-name-inverted); } .DollarR { @@ -55,3 +58,10 @@ .CurrentHighlight { background-color: var(--color-search-match-current); } + +.ExpandCollapseToggle { + display: inline-flex; + width: 1rem; + height: 1rem; + color: var(--color-expand-collapse-toggle); +} diff --git a/src/devtools/views/Components/Element.js b/src/devtools/views/Components/Element.js index 01f983c217..c3cc65c0cf 100644 --- a/src/devtools/views/Components/Element.js +++ b/src/devtools/views/Components/Element.js @@ -9,6 +9,8 @@ import React, { useRef, } from 'react'; import { ElementTypeClass, ElementTypeFunction } from 'src/devtools/types'; +import Store from 'src/devtools/store'; +import ButtonIcon from '../ButtonIcon'; import { createRegExp } from '../utils'; import { TreeContext } from './TreeContext'; import { BridgeContext, StoreContext } from '../context'; @@ -75,8 +77,6 @@ export default function ElementView({ data, index, style }: Props) { } }, [id, isSelected, lastScrolledIDRef]); - // TODO Add click and key handlers for toggling element open/close state. - const handleMouseDown = useCallback( ({ metaKey }) => { if (id !== null) { @@ -114,8 +114,6 @@ export default function ElementView({ data, index, style }: Props) { const showDollarR = isSelected && (type === ElementTypeClass || type === ElementTypeFunction); - // TODO styles.SelectedElement is 100% width but it doesn't take horizontal overflow into account. - return (
+ {key && ( @@ -152,6 +151,45 @@ export default function ElementView({ data, index, style }: Props) { ); } +// Prevent double clicks on toggle from drilling into the owner list. +const swallowDoubleClick = event => { + event.preventDefault(); + event.stopPropagation(); +}; + +type ExpandCollapseToggleProps = {| + element: Element, + store: Store, +|}; + +function ExpandCollapseToggle({ element, store }: ExpandCollapseToggleProps) { + const { children, id, isCollapsed } = element; + + const toggleCollapsed = useCallback( + event => { + event.preventDefault(); + event.stopPropagation(); + + store.toggleIsCollapsed(id, !isCollapsed); + }, + [id, isCollapsed, store] + ); + + if (children.length === 0) { + return
; + } + + return ( +
+ +
+ ); +} + type DisplayNameProps = {| displayName: string | null, id: number, From 4c522c4c3845aac5442fd5f12957da6167b609b4 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 9 Apr 2019 18:11:40 -0700 Subject: [PATCH 03/12] Selected search result auto-opens collapsed nodes when necessary. Also fixed an unrelated bug about when we reset search index when text changes. --- src/devtools/views/Components/TreeContext.js | 50 ++++++++++++++++++-- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/src/devtools/views/Components/TreeContext.js b/src/devtools/views/Components/TreeContext.js index 8e264f826f..d9d9233c80 100644 --- a/src/devtools/views/Components/TreeContext.js +++ b/src/devtools/views/Components/TreeContext.js @@ -24,6 +24,7 @@ import React, { useEffect, useMemo, useReducer, + useRef, } from 'react'; import { createRegExp } from '../utils'; import { BridgeContext, StoreContext } from '../context'; @@ -198,7 +199,7 @@ function reduceSearchState(store: Store, state: State, action: Action): State { selectedElementIndex, } = state; - const prevSearchIndex = searchIndex; + let prevSearchIndex = searchIndex; const prevSearchText = searchText; const numPrevSearchResults = searchResults.length; @@ -306,10 +307,25 @@ function reduceSearchState(store: Store, state: State, action: Action): State { if (prevSearchIndex === null) { searchIndex = 0; } else { - searchIndex = Math.min( - ((prevSearchIndex: any): number), - searchResults.length - 1 - ); + // Changes in search index or typing should override the selected element. + // The one exception is when a search is broadened (e.g. "Cat" -> "Ca"). + // In this case, it's probably desirable to maintain a stable selection. + // Replacements of text( e.g. "cat" -> "dog") likely require an index change. + const didRelaxSearchText = + prevSearchText.length > searchText.length && + prevSearchText.startsWith(searchText); + if (didRelaxSearchText) { + searchIndex = prevSearchIndex; + } else { + searchIndex = Math.min( + ((prevSearchIndex: any): number), + searchResults.length - 1 + ); + + // Force selected element ID to be re-evaluated below, even if the search index didn't change, + // because new search text means the same index may now point to a new element. + prevSearchIndex = null; + } } } } @@ -686,6 +702,30 @@ function TreeContextController({ children, viewElementSource }: Props) { return () => bridge.removeListener('selectFiber', handleSelectFiber); }, [bridge, dispatch]); + // If a newly-selected search result is inside of a collapsed subtree, auto expand it. + // We also need to handle when the search text changed (selecting a new element) without changing the index. + const prevSearchIndex = useRef(null); + const prevSearchText = useRef(''); + useEffect(() => { + if ( + state.searchIndex !== prevSearchIndex.current || + state.searchText !== prevSearchText.current + ) { + prevSearchIndex.current = state.searchIndex; + prevSearchText.current = state.searchText; + + if (state.searchIndex !== null && state.selectedElementID !== null) { + let element = store.getElementByID(state.selectedElementID); + while (element !== null && element.parentID > 0) { + element = ((store.getElementByID(element.parentID): any): Element); + if (element.isCollapsed) { + store.toggleIsCollapsed(element.id, false); + } + } + } + } + }, [state.searchIndex, state.searchText, state.selectedElementID, store]); + // Mutations to the underlying tree may impact this context (e.g. search results, selection state). useEffect(() => { const handleStoreMutated = ([ From f7212d8035cb8fadfdefda6728fce269ed2a4c87 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 9 Apr 2019 18:11:57 -0700 Subject: [PATCH 04/12] Upgrade react-window to fix a scroll-to bug --- package.json | 2 +- yarn.lock | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 970f1f99d5..aa58a10370 100644 --- a/package.json +++ b/package.json @@ -104,7 +104,7 @@ "react-dom": "^16.8.4", "react-is": "^16.8.4", "react-virtualized-auto-sizer": "^1.0.2", - "react-window": "^1.5.1", + "react-window": "^1.7.2", "scheduler": "^0.13", "semver": "^5.5.1", "style-loader": "^0.23.1", diff --git a/yarn.lock b/yarn.lock index 25a54310d8..4b0e19920e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7323,6 +7323,11 @@ mem@^4.0.0: mimic-fn "^1.0.0" p-is-promise "^2.0.0" +"memoize-one@>=3.1.1 <6": + version "5.0.4" + resolved "https://registry.yarnpkg.com/memoize-one/-/memoize-one-5.0.4.tgz#005928aced5c43d890a4dfab18ca908b0ec92cbc" + integrity sha512-P0z5IeAH6qHHGkJIXWw0xC2HNEgkx/9uWWBQw64FJj3/ol14VYdfVGWWr0fXfjhhv3TKVIqUq65os6O4GUNksA== + memoize-one@^3.1.1: version "3.1.1" resolved "https://registry.yarnpkg.com/memoize-one/-/memoize-one-3.1.1.tgz#ef609811e3bc28970eac2884eece64d167830d17" @@ -8799,13 +8804,13 @@ react-virtualized-auto-sizer@^1.0.2: resolved "https://registry.yarnpkg.com/react-virtualized-auto-sizer/-/react-virtualized-auto-sizer-1.0.2.tgz#a61dd4f756458bbf63bd895a92379f9b70f803bd" integrity sha512-MYXhTY1BZpdJFjUovvYHVBmkq79szK/k7V3MO+36gJkWGkrXKtyr4vCPtpphaTLRAdDNoYEYFZWE8LjN+PIHNg== -react-window@^1.5.1: - version "1.5.2" - resolved "https://registry.yarnpkg.com/react-window/-/react-window-1.5.2.tgz#39dbfd7aa47c1d80b37928f3269f7112a5900294" - integrity sha512-xGGKS9bR2y/XbkyQBk0qRO3y1mdENXVfksjAIn1fcbZ9qwiML52HryKfCaK50c1bIX3f0xqPgu8Q6FIxHKKbag== +react-window@^1.7.2: + version "1.7.2" + resolved "https://registry.yarnpkg.com/react-window/-/react-window-1.7.2.tgz#2e1528d5b9991e863302bfe74cb52d0ff7082e78" + integrity sha512-GK1gxSeGPLBDSQhPmYhCYrtf5MkGK8rwVjeyPgxZLvLRw0wvyzKZPMc/jfemiGNGfuJyW3kx1z6QR9uK7r2XdA== dependencies: "@babel/runtime" "^7.0.0" - memoize-one "^3.1.1" + memoize-one ">=3.1.1 <6" react@^16.8.4: version "16.8.4" From 55b3635f2837b7bf1fc2e4f6d9bffcb7259b9d12 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 9 Apr 2019 18:27:10 -0700 Subject: [PATCH 05/12] Left/right arrow toggles node collapsed state as well --- src/devtools/views/Components/Tree.js | 36 ++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/devtools/views/Components/Tree.js b/src/devtools/views/Components/Tree.js index b97ff5a9cf..d5a2b31826 100644 --- a/src/devtools/views/Components/Tree.js +++ b/src/devtools/views/Components/Tree.js @@ -12,7 +12,7 @@ import AutoSizer from 'react-virtualized-auto-sizer'; import { FixedSizeList } from 'react-window'; import { TreeContext } from './TreeContext'; import { SettingsContext } from '../Settings/SettingsContext'; -import { BridgeContext } from '../context'; +import { BridgeContext, StoreContext } from '../context'; import ElementView from './Element'; import InspectHostNodesToggle from './InspectHostNodesToggle'; import OwnersStack from './OwnersStack'; @@ -37,12 +37,14 @@ export default function Tree(props: Props) { getElementAtIndex, numElements, ownerStack, + selectedElementID, selectedElementIndex, selectNextElementInTree, selectParentElementInTree, selectPreviousElementInTree, } = useContext(TreeContext); const bridge = useContext(BridgeContext); + const store = useContext(StoreContext); // $FlowFixMe https://github.com/facebook/flow/issues/7341 const listRef = useRef | null>(null); const treeRef = useRef(null); @@ -77,6 +79,8 @@ export default function Tree(props: Props) { return; } + let element; + // eslint-disable-next-line default-case switch (event.key) { case 'ArrowDown': @@ -84,10 +88,34 @@ export default function Tree(props: Props) { event.preventDefault(); break; case 'ArrowLeft': - selectParentElementInTree(); + element = + selectedElementID !== null + ? store.getElementByID(selectedElementID) + : null; + if ( + element !== null && + element.children.length > 0 && + !element.isCollapsed + ) { + store.toggleIsCollapsed(element.id, true); + } else { + selectParentElementInTree(); + } break; case 'ArrowRight': - selectNextElementInTree(); + element = + selectedElementID !== null + ? store.getElementByID(selectedElementID) + : null; + if ( + element !== null && + element.children.length > 0 && + element.isCollapsed + ) { + store.toggleIsCollapsed(element.id, false); + } else { + selectNextElementInTree(); + } event.preventDefault(); break; case 'ArrowUp': @@ -107,9 +135,11 @@ export default function Tree(props: Props) { ownerDocument.removeEventListener('keydown', handleKeyDown); }; }, [ + selectedElementID, selectNextElementInTree, selectParentElementInTree, selectPreviousElementInTree, + store, ]); // Let react-window know to re-render any time the underlying tree data changes. From 61203f77cf850b07a6da57ea4257bcfeea200ae5 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 9 Apr 2019 21:35:09 -0700 Subject: [PATCH 06/12] Hide toggle arrows in owners list mode --- src/devtools/views/Components/Element.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/devtools/views/Components/Element.js b/src/devtools/views/Components/Element.js index c3cc65c0cf..f696a1e3cd 100644 --- a/src/devtools/views/Components/Element.js +++ b/src/devtools/views/Components/Element.js @@ -30,6 +30,7 @@ export default function ElementView({ data, index, style }: Props) { const { baseDepth, getElementAtIndex, + ownerStack, selectOwner, selectedElementID, selectElementByID, @@ -136,7 +137,9 @@ export default function ElementView({ data, index, style }: Props) { marginBottom: `-${style.height}px`, }} > - + {ownerStack.length === 0 ? ( + + ) : null} {key && ( From 102267b1d61b4362eb6cfeaebe226548d318027d Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 10 Apr 2019 07:31:34 -0700 Subject: [PATCH 07/12] Renamed _numElements attribute based on PR feedback --- src/devtools/store.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/devtools/store.js b/src/devtools/store.js index 424ec333eb..844d7e9b07 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -71,10 +71,6 @@ export default class Store extends EventEmitter { // When profiling is in progress, operations are stored so that we can later reconstruct past commit trees. _isProfiling: boolean = false; - // Total number of visible elements (within all roots). - // Used for windowing purposes. - _numElements: number = 0; - // Suspense cache for reading profilign data. _profilingCache: ProfilingCache; @@ -112,6 +108,10 @@ export default class Store extends EventEmitter { _supportsProfiling: boolean = false; _supportsReloadAndProfile: boolean = false; + // Total number of visible elements (within all roots). + // Used for windowing purposes. + _weightAcrossRoots: number = 0; + constructor(bridge: Bridge, config?: Config) { super(); @@ -198,7 +198,7 @@ export default class Store extends EventEmitter { } get numElements(): number { - return this._numElements; + return this._weightAcrossRoots; } get profilingCache(): ProfilingCache { @@ -429,7 +429,7 @@ export default class Store extends EventEmitter { const weightDelta = isCollapsed ? 1 - element.weight : element.weight - 1; - this._numElements += weightDelta; + this._weightAcrossRoots += weightDelta; let parentElement = this._idToElement.get(element.parentID); while (parentElement != null) { @@ -760,7 +760,7 @@ export default class Store extends EventEmitter { // Additions and deletions within a collapsed subtree should not affect the overall number of elements. if (!isInsideCollapsedSubTree) { - this._numElements += weightDelta; + this._weightAcrossRoots += weightDelta; } } From b004a794152df4051e02661c04dcd718f7b60c7e Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 10 Apr 2019 07:45:45 -0700 Subject: [PATCH 08/12] Reverted some unnecessary changes to TreeContext after Dan's PR 101 was merged --- src/devtools/views/Components/TreeContext.js | 25 ++++---------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/src/devtools/views/Components/TreeContext.js b/src/devtools/views/Components/TreeContext.js index d9d9233c80..26a925a717 100644 --- a/src/devtools/views/Components/TreeContext.js +++ b/src/devtools/views/Components/TreeContext.js @@ -199,7 +199,7 @@ function reduceSearchState(store: Store, state: State, action: Action): State { selectedElementIndex, } = state; - let prevSearchIndex = searchIndex; + const prevSearchIndex = searchIndex; const prevSearchText = searchText; const numPrevSearchResults = searchResults.length; @@ -307,25 +307,10 @@ function reduceSearchState(store: Store, state: State, action: Action): State { if (prevSearchIndex === null) { searchIndex = 0; } else { - // Changes in search index or typing should override the selected element. - // The one exception is when a search is broadened (e.g. "Cat" -> "Ca"). - // In this case, it's probably desirable to maintain a stable selection. - // Replacements of text( e.g. "cat" -> "dog") likely require an index change. - const didRelaxSearchText = - prevSearchText.length > searchText.length && - prevSearchText.startsWith(searchText); - if (didRelaxSearchText) { - searchIndex = prevSearchIndex; - } else { - searchIndex = Math.min( - ((prevSearchIndex: any): number), - searchResults.length - 1 - ); - - // Force selected element ID to be re-evaluated below, even if the search index didn't change, - // because new search text means the same index may now point to a new element. - prevSearchIndex = null; - } + searchIndex = Math.min( + ((prevSearchIndex: any): number), + searchResults.length - 1 + ); } } } From 1f7f0ea6913f261878d5e70cb4e538e141ea6109 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 10 Apr 2019 07:53:06 -0700 Subject: [PATCH 09/12] Update Store.getIndexOfElementID to take isCollapsed into account --- src/devtools/store.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/devtools/store.js b/src/devtools/store.js index 844d7e9b07..a764394184 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -353,7 +353,7 @@ export default class Store extends EventEmitter { break; } const child = ((this._idToElement.get(childID): any): Element); - index += child.weight; + index += child.isCollapsed ? 1 : child.weight; } previousID = current.id; From dd84e8ff2bbb8193f585ec23d870716a0e2151c0 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 10 Apr 2019 08:57:42 -0700 Subject: [PATCH 10/12] Newly selected components always auto-expand their ancestors --- src/devtools/store.js | 7 ++++- src/devtools/views/Components/TreeContext.js | 31 ++++++++++--------- .../views/Profiler/ProfilerContext.js | 11 ++++--- 3 files changed, 29 insertions(+), 20 deletions(-) diff --git a/src/devtools/store.js b/src/devtools/store.js index a764394184..66e84b578c 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -353,7 +353,12 @@ export default class Store extends EventEmitter { break; } const child = ((this._idToElement.get(childID): any): Element); - index += child.isCollapsed ? 1 : child.weight; + + // We intentionally ignore collapsed state when determining an item's index. + // We only do this for the direct path to an element. + // That's because the index of the element is meaningless if it's inside of a collapsed tree. + // If this index is used to display the element, the caller should also un-collapse its ancestors. + index += child.weight; } previousID = current.id; diff --git a/src/devtools/views/Components/TreeContext.js b/src/devtools/views/Components/TreeContext.js index 26a925a717..915312d521 100644 --- a/src/devtools/views/Components/TreeContext.js +++ b/src/devtools/views/Components/TreeContext.js @@ -22,6 +22,7 @@ import React, { useCallback, useContext, useEffect, + useLayoutEffect, useMemo, useReducer, useRef, @@ -110,6 +111,8 @@ function reduceTreeState(store: Store, state: State, action: Action): State { selectedElementID, } = state; + let lookupIDForIndex = true; + // Base tree should ignore selected element changes when the owner's tree is active. if (ownerStack.length === 0) { switch (type) { @@ -128,6 +131,11 @@ function reduceTreeState(store: Store, state: State, action: Action): State { selectedElementIndex = ((payload: any): number | null); break; case 'SELECT_ELEMENT_BY_ID': + // Skip lookup in this case; it would be redundant. + // It might also cause problems if the specified element was inside of a (not yet expanded) subtree. + lookupIDForIndex = false; + + selectedElementID = payload; selectedElementIndex = payload === null ? null @@ -168,7 +176,7 @@ function reduceTreeState(store: Store, state: State, action: Action): State { } // Keep selected item ID and index in sync. - if (selectedElementIndex !== state.selectedElementIndex) { + if (lookupIDForIndex && selectedElementIndex !== state.selectedElementIndex) { if (selectedElementIndex === null) { selectedElementID = null; } else { @@ -687,19 +695,14 @@ function TreeContextController({ children, viewElementSource }: Props) { return () => bridge.removeListener('selectFiber', handleSelectFiber); }, [bridge, dispatch]); - // If a newly-selected search result is inside of a collapsed subtree, auto expand it. - // We also need to handle when the search text changed (selecting a new element) without changing the index. - const prevSearchIndex = useRef(null); - const prevSearchText = useRef(''); - useEffect(() => { - if ( - state.searchIndex !== prevSearchIndex.current || - state.searchText !== prevSearchText.current - ) { - prevSearchIndex.current = state.searchIndex; - prevSearchText.current = state.searchText; + // If a newly-selected search result or inspection selection is inside of a collapsed subtree, auto expand it. + // This needs to be a layout effect to avoid temporarily flashing an incorrect selection. + const prevSelectedElementID = useRef(null); + useLayoutEffect(() => { + if (state.selectedElementID !== prevSelectedElementID.current) { + prevSelectedElementID.current = state.selectedElementID; - if (state.searchIndex !== null && state.selectedElementID !== null) { + if (state.selectedElementID !== null) { let element = store.getElementByID(state.selectedElementID); while (element !== null && element.parentID > 0) { element = ((store.getElementByID(element.parentID): any): Element); @@ -709,7 +712,7 @@ function TreeContextController({ children, viewElementSource }: Props) { } } } - }, [state.searchIndex, state.searchText, state.selectedElementID, store]); + }, [state.selectedElementID, store]); // Mutations to the underlying tree may impact this context (e.g. search results, selection state). useEffect(() => { diff --git a/src/devtools/views/Profiler/ProfilerContext.js b/src/devtools/views/Profiler/ProfilerContext.js index d06b5dab73..c8a89c0d9e 100644 --- a/src/devtools/views/Profiler/ProfilerContext.js +++ b/src/devtools/views/Profiler/ProfilerContext.js @@ -79,7 +79,7 @@ type Props = {| function ProfilerContextController({ children }: Props) { const store = useContext(StoreContext); - const { selectElementAtIndex, selectedElementID } = useContext(TreeContext); + const { selectElementByID, selectedElementID } = useContext(TreeContext); const subscription = useMemo( () => ({ @@ -152,13 +152,14 @@ function ProfilerContextController({ children }: Props) { selectFiberID(id); selectFiberName(name); if (id !== null) { - const index = store.getIndexOfElementID(id); - if (index !== null) { - selectElementAtIndex(index); + // If this element is still in the store, then select it in the Components tab as well. + const element = store.getElementByID(id); + if (element !== null) { + selectElementByID(id); } } }, - [selectElementAtIndex, selectFiberID, selectFiberName, store] + [selectElementByID, selectFiberID, selectFiberName, store] ); if (isProfiling) { From 74c9904e5d520662cc9c29f8b760818c77b537a3 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 10 Apr 2019 08:59:43 -0700 Subject: [PATCH 11/12] Rewrote weightDelta calculation to be more readable based on PR feedback --- src/devtools/store.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/devtools/store.js b/src/devtools/store.js index 66e84b578c..d5f3a45b25 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -430,9 +430,10 @@ export default class Store extends EventEmitter { toggleIsCollapsed(id: number, isCollapsed: boolean): void { const element = this.getElementByID(id); if (element !== null) { + const oldWeight = element.isCollapsed ? 1 : element.weight; element.isCollapsed = isCollapsed; - - const weightDelta = isCollapsed ? 1 - element.weight : element.weight - 1; + const newWeight = element.isCollapsed ? 1 : element.weight; + const weightDelta = newWeight - oldWeight; this._weightAcrossRoots += weightDelta; From f89f2b2146fa806c6bcabd23bb3a2af328f31f6b Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 10 Apr 2019 09:23:24 -0700 Subject: [PATCH 12/12] Reverted misguided index/collapsed behavior --- src/devtools/store.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/devtools/store.js b/src/devtools/store.js index d5f3a45b25..d81b6e89c1 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -353,12 +353,7 @@ export default class Store extends EventEmitter { break; } const child = ((this._idToElement.get(childID): any): Element); - - // We intentionally ignore collapsed state when determining an item's index. - // We only do this for the direct path to an element. - // That's because the index of the element is meaningless if it's inside of a collapsed tree. - // If this index is used to display the element, the caller should also un-collapse its ancestors. - index += child.weight; + index += child.isCollapsed ? 1 : child.weight; } previousID = current.id;