From deaf08c6f9fccf10a997afaa41b4b8ed49cd45b5 Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Tue, 16 Apr 2024 14:29:26 -0400 Subject: [PATCH] Remove suspend from 3D viewer during modal navigation (#4253) * add none fields filter in modal to sidebar entries * reuse sample parsing * add sidebar sample parsing tests * add hiddenNoneGroups tests * add multi-slice cases --- .../core/src/components/Modal/Modal.tsx | 11 +- .../Sidebar/Entries/PathValueEntry.tsx | 94 ++------ .../src/components/Sidebar/Entries/index.tsx | 2 +- .../core/src/components/Sidebar/Sidebar.tsx | 19 +- app/packages/state/src/recoil/schema.ts | 18 +- app/packages/state/src/recoil/sidebar.test.ts | 212 +++++++++++++++++- app/packages/state/src/recoil/sidebar.ts | 99 +++++++- 7 files changed, 353 insertions(+), 102 deletions(-) diff --git a/app/packages/core/src/components/Modal/Modal.tsx b/app/packages/core/src/components/Modal/Modal.tsx index f113068306..7000db2e49 100644 --- a/app/packages/core/src/components/Modal/Modal.tsx +++ b/app/packages/core/src/components/Modal/Modal.tsx @@ -66,8 +66,6 @@ const SampleModal = () => { const isGroup = useRecoilValue(fos.isGroup); const isPcd = useRecoilValue(fos.isPointcloudDataset); const is3D = useRecoilValue(fos.is3DDataset); - const sampleId = useRecoilValue(fos.currentSampleId); - const clearModal = fos.useClearModal(); const { jsonPanel, helpPanel, onNavigate } = usePanels(); const tooltip = fos.useTooltip(); @@ -111,9 +109,6 @@ const SampleModal = () => { [tooltipEventHandler] ); - const noneValuedPaths = useRecoilValue(fos.noneValuedPaths)?.[sampleId]; - const hideNoneFields = useRecoilValue(fos.hideNoneValuedFields); - const renderEntry = useCallback( ( key: string, @@ -135,10 +130,6 @@ const SampleModal = () => { const isFieldPrimitive = !isLabelTag && !isLabel && !isOther && !(isTag && mode === "group"); - if (hideNoneFields && noneValuedPaths?.has(entry?.path)) { - return { children: null }; - } - return { children: ( <> @@ -208,7 +199,7 @@ const SampleModal = () => { throw new Error("invalid entry"); } }, - [disabled, hideNoneFields, labelPaths, mode, noneValuedPaths] + [disabled, labelPaths, mode] ); useEffect(() => { diff --git a/app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx b/app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx index f57bfa0267..53cf523ad4 100644 --- a/app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx +++ b/app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx @@ -3,8 +3,6 @@ import * as fos from "@fiftyone/state"; import { DATE_FIELD, DATE_TIME_FIELD, - DYNAMIC_EMBEDDED_DOCUMENT_FIELD, - EMBEDDED_DOCUMENT_FIELD, formatDate, formatDateTime, FRAME_SUPPORT_FIELD, @@ -12,12 +10,7 @@ import { import { KeyboardArrowDown, KeyboardArrowUp } from "@mui/icons-material"; import { useSpring } from "@react-spring/core"; import React, { Suspense, useMemo, useState } from "react"; -import { - selectorFamily, - useRecoilState, - useRecoilValue, - useRecoilValueLoadable, -} from "recoil"; +import { selectorFamily, useRecoilValue, useRecoilValueLoadable } from "recoil"; import styled from "styled-components"; import LoadingDots from "../../../../../components/src/components/Loading/LoadingDots"; import { prettify } from "../../../utils/generic"; @@ -314,7 +307,7 @@ const SlicesLoadable = ({ path }: { path: string }) => { ); }; -const useSlicesData = (path: string) => { +const useSlicesData = (path: string) => { const keys = path.split("."); const loadable = useRecoilValueLoadable(fos.activePcdSlicesToSampleMap); const slices = Array.from(useRecoilValue(fos.activePcdSlices) || []).sort(); @@ -328,28 +321,20 @@ const useSlicesData = (path: string) => { } if (!slices.every((slice) => loadable.contents[slice])) { - throw new Promise(() => {}); + throw new Promise(() => null); } - const data = { ...loadable.contents }; + const data = { ...loadable.contents } as object; - const target = useRecoilValue(fos.field(keys[0])); + const target = fos.useAssertedRecoilValue(fos.field(keys[0])); + const isList = useRecoilValue(fos.isOfDocumentFieldList(path)); slices.forEach((slice) => { - let sliceData = data[slice].sample; - let field = target; - - for (let index = 0; index < keys.length; index++) { - if (!sliceData) { - break; - } - const key = keys[index]; - sliceData = sliceData[field?.dbField || key]; - - if (keys[index + 1]) { - field = field?.fields[keys[index + 1]]; - } - } - data[slice] = sliceData; + data[slice] = fos.pullSidebarValue( + target, + keys, + data[slice].sample, + isList + ); }); return data as { [slice: string]: T }; @@ -374,25 +359,9 @@ const Loadable = ({ path }: { path: string }) => { ); }; -const isOfDocumentFieldList = selectorFamily({ - key: "isOfDocumentField", - get: - (path: string) => - ({ get }) => { - const field = get(fos.field(path.split(".")[0])); - - return [ - DYNAMIC_EMBEDDED_DOCUMENT_FIELD, - EMBEDDED_DOCUMENT_FIELD, - ].includes(field?.subfield || ""); - }, -}); - const useData = (path: string): T => { const keys = path.split("."); const loadable = useRecoilValueLoadable(fos.activeModalSidebarSample); - const [noneValuedPaths, setNoneValued] = useRecoilState(fos.noneValuedPaths); - const sampleId = useRecoilValue(fos.currentSampleId); if (loadable.state === "loading") { throw loadable.contents; @@ -400,47 +369,16 @@ const useData = (path: string): T => { if (loadable.state === "hasError") { if (loadable.contents instanceof fos.SampleNotFound) { - throw new Promise(() => {}); + throw new Promise(() => null); } throw loadable.contents; } - let data = loadable.contents; - let field = useRecoilValue(fos.field(keys[0])); - - if (useRecoilValue(isOfDocumentFieldList(path))) { - data = data?.[field?.dbField || keys[0]]?.map((d) => d[keys[1]]); - } else { - for (let index = 0; index < keys.length; index++) { - if (!data) { - break; - } - const key = keys[index]; - data = data[field?.dbField || key]; - - if (keys[index + 1]) { - field = field?.fields?.[keys[index + 1]] || null; - } - } - } - - const noneValued = noneValuedPaths?.[sampleId]; - if ( - (data === undefined || data === null) && - path && - !noneValued?.has(path) && - sampleId - ) { - const newNoneValued = noneValued - ? new Set([...noneValued]).add(path) - : new Set([path]); - if (newNoneValued) { - setNoneValued({ [sampleId]: new Set([...newNoneValued, path]) }); - } - } + const field = fos.useAssertedRecoilValue(fos.field(keys[0])); + const isList = useRecoilValue(fos.isOfDocumentFieldList(path)); - return data as T; + return fos.pullSidebarValue(field, keys, loadable.contents, isList) as T; }; const isScalarValue = selectorFamily({ diff --git a/app/packages/core/src/components/Sidebar/Entries/index.tsx b/app/packages/core/src/components/Sidebar/Entries/index.tsx index 5ab2443760..185454b3dd 100644 --- a/app/packages/core/src/components/Sidebar/Entries/index.tsx +++ b/app/packages/core/src/components/Sidebar/Entries/index.tsx @@ -5,4 +5,4 @@ import Filter from "./FilterEntry"; import { PathGroupEntry as PathGroup } from "./GroupEntries"; import PathValue from "./PathValueEntry"; -export { AddGroup, Empty, FilterablePath, Filter, PathGroup, PathValue }; +export { AddGroup, Empty, Filter, FilterablePath, PathGroup, PathValue }; diff --git a/app/packages/core/src/components/Sidebar/Sidebar.tsx b/app/packages/core/src/components/Sidebar/Sidebar.tsx index ed6170d9f3..bec3c545eb 100644 --- a/app/packages/core/src/components/Sidebar/Sidebar.tsx +++ b/app/packages/core/src/components/Sidebar/Sidebar.tsx @@ -199,12 +199,12 @@ const measureGroups = ( data: { top: number; height: number; key: string }[]; activeHeight: number; } => { - const data = []; let current = { top: -MARGIN, height: 0, key: getEntryKey(items[order[0]].entry), }; + const data: typeof current[] = []; let activeHeight = -MARGIN; for (let i = 0; i < order.length; i++) { @@ -264,24 +264,25 @@ const isDisabledEntry = ( }; const getAfterKey = ( - activeKey: string, + activeKey: string | null, items: InteractiveItems, order: string[], direction: Direction, disabled: Set ): string | null => { - if (!items[activeKey]) { - return; + if (activeKey === null || !items[activeKey]) { + return null; } const up = direction === Direction.UP; - const baseTop = items[order[0]].el.parentElement.getBoundingClientRect().y; + const baseTop = + items[order[0]].el.parentElement?.getBoundingClientRect().y || 0; const isGroup = items[activeKey].entry.kind === fos.EntryKind.GROUP; - let { data, activeHeight } = isGroup + const measurement = isGroup ? measureGroups(activeKey, items, order) : measureEntries(activeKey, items, order); - data = data.filter( + const data = measurement.data.filter( ({ key }) => !isDisabledEntry(items[key].entry, disabled, !isGroup) ); @@ -289,7 +290,7 @@ const getAfterKey = ( let y = top - baseTop; if (!up) { - y += activeHeight; + y += measurement.activeHeight; } let filtered = data @@ -435,7 +436,7 @@ const InteractiveSidebar = ({ throw entries; } - let group = null; + let group: string | null = null; order.current = [...entries].map((entry) => getEntryKey(entry)); for (const entry of entries) { const key = getEntryKey(entry); diff --git a/app/packages/state/src/recoil/schema.ts b/app/packages/state/src/recoil/schema.ts index 444efeef6a..715562ba86 100644 --- a/app/packages/state/src/recoil/schema.ts +++ b/app/packages/state/src/recoil/schema.ts @@ -17,13 +17,13 @@ import { LABEL_LISTS, LABEL_LISTS_MAP, LIST_FIELD, + meetsFieldType, OBJECT_ID_FIELD, - STRING_FIELD, Schema, StrictField, + STRING_FIELD, VALID_NUMERIC_TYPES, VALID_PRIMITIVE_TYPES, - meetsFieldType, withPath, } from "@fiftyone/utilities"; import { RecoilState, selector, selectorFamily } from "recoil"; @@ -780,3 +780,17 @@ export const parentField = selectorFamily({ return get(field(parent)); }, }); + +export const isOfDocumentFieldList = selectorFamily({ + key: "isOfDocumentFieldList", + get: + (path: string) => + ({ get }) => { + const f = get(field(path.split(".")[0])); + + return [ + DYNAMIC_EMBEDDED_DOCUMENT_FIELD, + EMBEDDED_DOCUMENT_FIELD, + ].includes(f.subfield || ""); + }, +}); diff --git a/app/packages/state/src/recoil/sidebar.test.ts b/app/packages/state/src/recoil/sidebar.test.ts index 7410777cd6..209b0146fc 100644 --- a/app/packages/state/src/recoil/sidebar.test.ts +++ b/app/packages/state/src/recoil/sidebar.test.ts @@ -1,4 +1,9 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; +vi.mock("recoil"); +vi.mock("recoil-relay"); + +import { Field } from "@fiftyone/utilities"; +import { setMockAtoms, TestSelector } from "../../../../__mocks__/recoil"; import * as sidebar from "./sidebar"; const mockFields = { @@ -356,3 +361,208 @@ describe("test sidebar groups resolution", () => { expect(labels.expanded).toBe(true); }); }); + +const TEST_FIELD_DATA: Field = { + name: "unused", + path: "unused", + embeddedDocType: null, + dbField: null, + description: null, + ftype: null, + info: {}, + subfield: null, +}; + +describe("test pullSidebarValue", () => { + it("handles dbField values", () => { + expect( + sidebar.pullSidebarValue( + { ...TEST_FIELD_DATA, dbField: "_id" }, + ["id"], + { _id: "idValue" }, + false + ) + ).toBe("idValue"); + }); + + it("handles embedded list values", () => { + expect( + sidebar.pullSidebarValue( + TEST_FIELD_DATA, + ["nested", "field"], + { nested: [{ field: "nestedListValue" }] }, + true + ) + ).toStrictEqual(["nestedListValue"]); + }); + + it("handles nested values", () => { + expect( + sidebar.pullSidebarValue( + TEST_FIELD_DATA, + ["nested", "field"], + { nested: { field: "nestedValue" } }, + false + ) + ).toBe("nestedValue"); + }); + + it("handles undefined values", () => { + expect( + sidebar.pullSidebarValue( + TEST_FIELD_DATA, + ["undefined", "value"], + {}, + false + ) + ).toBe(undefined); + }); +}); + +describe("hiddenNoneGroups selector", () => { + const assertSidebarGroupsRequest = ( + filtered: boolean, + loading: boolean, + modal: boolean + ) => { + if (!filtered) { + throw new Error("'filtered' should be 'true'"); + } + + if (loading) { + throw new Error("'loading' should be 'false'"); + } + + if (!modal) { + throw new Error("'modal' should be 'true'"); + } + }; + + const testHiddenNoneGroups = >( + (sidebar.hiddenNoneGroups) + ); + + const present = { + paths: new Set(), + groups: new Set(), + }; + + const hidden = { + paths: new Set(["my_field", "my_list_field.subfield"]), + groups: new Set(["field", "list_field"]), + }; + + const base = { + hideNoneValuedFields: true, + isOfDocumentFieldList: (p) => p === "my_list_field.subfield", + sidebarGroups: ({ filtered, loading, modal }) => { + assertSidebarGroupsRequest(filtered, loading, modal); + + return [ + { + name: "tags", + paths: [], + }, + { + name: "field", + paths: ["my_field"], + }, + { + name: "list_field", + paths: ["my_list_field.subfield"], + }, + ]; + }, + field: () => TEST_FIELD_DATA, + }; + + it("tests disabled", () => { + setMockAtoms({ + hideNoneValuedFields: false, + }); + + expect(testHiddenNoneGroups()).toStrictEqual(present); + }); + + it("tests single slice enabled", () => { + setMockAtoms(base); + // test null + setMockAtoms({ + activeModalSidebarSample: { + field: null, + my_list_field: null, + }, + }); + + expect(testHiddenNoneGroups()).toStrictEqual(hidden); + + // test empty list + setMockAtoms({ + activeModalSidebarSample: { + my_field: null, + my_list_field: [], + }, + }); + + expect(testHiddenNoneGroups()).toStrictEqual(hidden); + + // test null list value + setMockAtoms({ + activeModalSidebarSample: { + my_field: null, + my_list_field: [{ subfield: null }], + }, + }); + + expect(testHiddenNoneGroups()).toStrictEqual(hidden); + + // test values + setMockAtoms({ + activeModalSidebarSample: { + my_field: "value", + my_list_field: [{ subfield: "value" }], + }, + }); + + expect(testHiddenNoneGroups()).toStrictEqual(present); + }); + + it("tests multiple slices enabled", () => { + setMockAtoms(base); + + // test null + setMockAtoms({ + activePcdSlices: ["one", "two"], + activePcdSlicesToSampleMap: { + one: { + sample: {}, + }, + two: { + sample: {}, + }, + }, + pinned3DSampleSlice: "one", + }); + + expect(testHiddenNoneGroups()).toStrictEqual(hidden); + + // test null and present + setMockAtoms({ + activePcdSlices: ["one", "two"], + activePcdSlicesToSampleMap: { + one: { + sample: {}, + }, + two: { + sample: { + my_field: "value", + my_list_field: [{ subfield: "value" }], + }, + }, + }, + pinned3DSampleSlice: "one", + }); + + expect(testHiddenNoneGroups()).toStrictEqual(present); + }); +}); diff --git a/app/packages/state/src/recoil/sidebar.ts b/app/packages/state/src/recoil/sidebar.ts index 7782627f29..65c68485d2 100644 --- a/app/packages/state/src/recoil/sidebar.ts +++ b/app/packages/state/src/recoil/sidebar.ts @@ -14,6 +14,7 @@ import { import { DICT_FIELD, EMBEDDED_DOCUMENT_FIELD, + Field, LABELS_PATH, LABEL_DOC_TYPES, LIST_FIELD, @@ -34,13 +35,21 @@ import { } from "recoil"; import { collapseFields, getCurrentEnvironment } from "../utils"; import * as atoms from "./atoms"; +import { + activeModalSidebarSample, + activePcdSlices, + activePcdSlicesToSampleMap, + pinned3DSampleSlice, +} from "./groups"; import { isLargeVideo } from "./options"; import { cumulativeValues, values } from "./pathData"; import { buildSchema, + field, fieldPaths, fields, filterPaths, + isOfDocumentFieldList, pathIsShown, } from "./schema"; import { isFieldVisibilityActive as isFieldVisibilityActiveState } from "./schemaSettings.atoms"; @@ -505,6 +514,10 @@ export const sidebarEntries = selectorFamily< (params) => ({ get }) => { const isFieldVisibilityActive = get(isFieldVisibilityActiveState); + const hidden = + params.modal && !params.loading + ? get(hiddenNoneGroups) + : { groups: new Set(), paths: new Set() }; const entries = [ ...get(sidebarGroups(params)) .map(({ name, paths }) => { @@ -539,7 +552,7 @@ export const sidebarEntries = selectorFamily< ...paths.map((path) => ({ path, kind: EntryKind.PATH, - shown, + shown: shown && !hidden.paths.has(path), })), ]; }) @@ -826,3 +839,87 @@ export const sidebarWidth = atomFamily({ key: "sidebarWidth", default: 300, }); + +export const hiddenNoneGroups = selector({ + key: "hiddenNoneGroups", + get: ({ get }) => { + if (!get(atoms.hideNoneValuedFields)) { + return { paths: new Set(), groups: new Set() }; + } + + const groups = get( + sidebarGroups({ modal: true, loading: false, filtered: true }) + ).filter((group) => group.name !== "tags"); // always show tags + + let samples: { [key: string]: { sample: object } } = { + default: { sample: get(activeModalSidebarSample) }, + }; + let slices = ["default"]; + + const multipleSlices = + Boolean(get(pinned3DSampleSlice)) && + (get(activePcdSlices)?.length || 1) > 1; + if (multipleSlices) { + samples = get(activePcdSlicesToSampleMap); + slices = Array.from(get(activePcdSlices) || []).sort(); + } + + const items = groups + .map(({ name: group, paths }) => paths.map((path) => ({ group, path }))) + .flat(); + + const result = { + groups: new Set(groups.map(({ name }) => name)), + paths: new Set(items.map(({ path }) => path)), + }; + + for (const { group, path } of items) { + const isList = get(isOfDocumentFieldList(path)); + for (const slice of slices) { + const keys = path.split("."); + const data = pullSidebarValue( + get(field(keys[0])), + keys, + samples[slice]?.sample, + isList + ); + + if (data !== null && data !== undefined) { + result.groups.delete(group); + result.paths.delete(path); + } + } + } + + return result; + }, +}); + +export const pullSidebarValue = ( + field: Pick, + keys: string[], + data: null | object | undefined, + isList: boolean +) => { + if (isList) { + data = data?.[field?.dbField || keys[0]]?.map((d) => d[keys[1]]); + } else { + for (let index = 0; index < keys.length; index++) { + if (data === null || data === undefined) { + break; + } + const key = keys[index]; + data = data[field?.dbField || key]; + + if (keys[index + 1]) { + field = field?.fields?.[keys[index + 1]] || null; + } + } + } + + if (Array.isArray(data)) { + return data.some((d) => d !== null && d !== undefined) ? data : null; + } + + return data; +};