Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

post merge fixes and spaces tweaks #4475

Merged
merged 4 commits into from
Jun 11, 2024
Merged

post merge fixes and spaces tweaks #4475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
61bf6ee
fix executeOperator options
imanjra Jun 10, 2024
771f48a
scrollable add panel popout
imanjra Jun 10, 2024
043d3dd
fix python panel state persistence on app reload
imanjra Jun 11, 2024
3165db8
enhancements for ButtonView and IconButtonView
imanjra Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/packages/components/src/components/Popout/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@ export type PopoutProps = PropsWithChildren<{
style?: CSSProperties;
modal?: boolean;
onClose?: () => void;
popoutProps: React.HTMLAttributes<HTMLDivElement>;
}>;

function Popout(
{ children, style = {}, modal, onClose }: PopoutProps,
{ children, style = {}, modal, onClose, popoutProps = {} }: PopoutProps,
ref: ForwardedRef<HTMLDivElement>
) {
const show = useSpring({
Expand All @@ -32,6 +33,7 @@ function Popout(
}}
>
<PopoutDiv
{...popoutProps}
ref={ref}
style={{
...show,
Expand Down
2 changes: 2 additions & 0 deletions app/packages/core/src/plugins/SchemaIO/components/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ import React from "react";
import { ButtonProps, Button as MUIButton } from "@mui/material";

export default function Button(props: ButtonProps) {
const { variant } = props;
return (
<MUIButton
color={variant === "outlined" ? "secondary" : undefined}
{...props}
sx={{ textTransform: "none", ...(props?.sx || {}) }}
/>
Expand Down
14 changes: 6 additions & 8 deletions app/packages/core/src/plugins/SchemaIO/components/ButtonView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
import React from "react";
import usePanelEvent from "@fiftyone/operators/src/usePanelEvent";
import { usePanelId } from "@fiftyone/spaces";
import { Box } from "@mui/material";
import { useOperatorExecutor } from "@fiftyone/operators";
import Button from "./Button";
import React from "react";
import { getComponentProps } from "../utils";
import { useCustomPanelState, usePanelId } from "@fiftyone/spaces";
import { usePromptOperatorInput } from "@fiftyone/operators/src/state";
import usePanelEvent from "@fiftyone/operators/src/usePanelEvent";
import Button from "./Button";

export default function ButtonView(props) {
return props?.schema?.view?.operator ? (
Expand All @@ -18,11 +16,11 @@ export default function ButtonView(props) {
function BaseButtonView(props) {
const { schema, onClick } = props;
const { view = {} } = schema;
const { label, href } = view;
const { label, href, variant } = view;
return (
<Box {...getComponentProps(props, "container")}>
<Button
variant="contained"
variant={variant || "contained"}
href={href}
onClick={onClick}
{...getComponentProps(props, "button")}
Expand Down
64 changes: 49 additions & 15 deletions app/packages/core/src/plugins/SchemaIO/components/IconButtonView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,23 @@
import { MuiIconFont, PillButton } from "@fiftyone/components";
import usePanelEvent from "@fiftyone/operators/src/usePanelEvent";
import { usePanelId } from "@fiftyone/spaces";
import { IconButton, Link } from "@mui/material";
import { IconButton, Link, SxProps } from "@mui/material";
import React, { useCallback } from "react";
import styled from "styled-components";
import { getComponentProps } from "../utils";
import Button from "./Button";

export default function IconButtonView(props) {
const { schema } = props;
const { view = {} } = schema;
const { icon, operator, params = {}, prompt, variant, label } = view;
const { icon, operator, params = {}, prompt, variant, href, label } = view;
const panelId = usePanelId();
const handleClick = usePanelEvent();

const Icon = icon ? (
<MuiIconFont
name={icon}
sx={
variant === "round"
? {}
: { color: (theme) => theme.palette.primary.main }
}
sx={getVariantSx(variant)}
{...getComponentProps(props, "icon")}
/>
) : null;
Expand All @@ -30,14 +27,18 @@ export default function IconButtonView(props) {
}, [panelId, params, operator, prompt, handleClick]);

if (variant === "round") {
const Wrapper = href ? Link : React.Fragment;
return (
<PillButton
icon={Icon || label}
title={label}
highlight
onClick={onClick}
aria-label={`Button for ${icon}`}
/>
<Wrapper href={href}>
<PillButton
icon={Icon || label}
title={label}
highlight
onClick={onClick}
aria-label={`Button for ${icon}`}
{...getComponentProps(props, "button")}
/>
</Wrapper>
);
}

Expand All @@ -47,20 +48,35 @@ export default function IconButtonView(props) {
title={label}
aria-label={`Button for ${icon}`}
onClick={onClick}
href={href}
{...getComponentProps(props, "button")}
>
{Icon || label}
</SquareButton>
);
}

if (variant === "contained" || variant === "outlined") {
return (
<Button
variant={variant}
href={href}
onClick={onClick}
{...getComponentProps(props, "button")}
>
{Icon || label}
</Button>
);
}

return (
<IconButton
title={label}
aria-label={`Button for ${icon}`}
size="small"
{...getComponentProps(props, "button")}
href={href}
onClick={onClick}
{...getComponentProps(props, "button")}
>
{Icon || label}
</IconButton>
Expand All @@ -78,3 +94,21 @@ const SquareButton = styled(Link)`
border-top-right-radius: 3px;
padding: 0.25rem;
`;

function getVariantSx(variant: VariantType): SxProps {
if (variant === "round") return {};
if (variant === "contained") {
return {
color: (theme) => theme.palette.common.white,
};
}
if (variant === "outlined") {
return {
color: (theme) => theme.palette.secondary.main,
};
}
return {
color: (theme) => theme.palette.primary.main,
};
}
type VariantType = "round" | "square" | "contained" | "outlined" | "default";
19 changes: 13 additions & 6 deletions app/packages/operators/src/operators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,15 +573,22 @@ function resolveOperatorURIWithMethod(operatorURI, params) {
}

export async function executeOperator(
uri,
p: any = {},
callback?: ExecutionCallback
uri: string,
params: unknown = {},
options?: OperatorExecutorOptions
) {
const { operatorURI, params } = resolveOperatorURIWithMethod(uri, p);
const { operatorURI, params: computedParams } = resolveOperatorURIWithMethod(
uri,
params
);
const resolvedOperatorURI = resolveOperatorURI(operatorURI);
const queue = getInvocationRequestQueue();
const request = new InvocationRequest(resolvedOperatorURI, params);
queue.add(request, callback);
const request = new InvocationRequest(
resolvedOperatorURI,
computedParams,
options
);
queue.add(request);
Comment on lines +576 to +591
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The script output shows multiple instances of executeOperator being called across various files. However, not all calls include the optional options parameter, which is necessary to match the updated function signature.

Instances that need attention:

  • app/packages/operators/src/useCustomPanelHooks.ts (Lines: 80, 111, 125, 145, 153)
  • app/packages/operators/src/usePanelEvent.ts (Line: 32)
  • app/packages/operators/src/built-in-operators.ts (Lines: 728, 786)
  • app/packages/operators/src/operators.ts (Lines: 434, 537)

These lines should be reviewed and updated to ensure they include the optional options parameter as per the new function signature.

Analysis chain

Update to executeOperator Function Signature and Logic

The update to the executeOperator function to include an optional options parameter and the renaming of params to computedParams within the function body aligns with the PR objectives to make the third parameter explicitly for options. This change enhances the function's flexibility and clarity in handling operator execution options.

However, ensure that all calls to executeOperator throughout the codebase are updated to reflect this new signature to avoid runtime errors.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `executeOperator` match the new signature.

# Test: Search for the function usage. Expect: Only occurances of the new signature.
rg --type typescript $'executeOperator'

Length of output: 73

Script:

#!/bin/bash
# Description: Verify all function calls to `executeOperator` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type ts 'executeOperator'

Length of output: 3333

Script:

#!/bin/bash
# Description: Extract lines where `executeOperator` is called to verify the new function signature.

# Extract lines with `executeOperator` calls
rg --type ts 'executeOperator' -n | while read -r line; do
  file=$(echo "$line" | cut -d: -f1)
  lineno=$(echo "$line" | cut -d: -f2)
  echo "File: $file, Line: $lineno"
  sed -n "${lineno}p" "$file"
  echo
done

Length of output: 20060

}

export async function validateOperatorInputs(
Expand Down
60 changes: 38 additions & 22 deletions app/packages/operators/src/useCustomPanelHooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useState, useEffect, useRef } from "react";
import { useState, useEffect, useRef, useCallback, useMemo } from "react";
import { useRecoilValue } from "recoil";
import { merge, isEqual } from "lodash";

Expand All @@ -13,17 +13,17 @@ import {

export interface CustomPanelProps {
panelId: string;
onLoad?: Function;
onChange?: Function;
onUnLoad?: Function;
onChangeCtx?: Function;
onViewChange?: Function;
onChangeView?: Function;
onChangeDataset?: Function;
onChangeCurrentSample?: Function;
onChangeSelected?: Function;
onChangeSelectedLabels?: Function;
onChangeExtendedSelection?: Function;
onLoad?: string;
onChange?: string;
onUnLoad?: string;
onChangeCtx?: string;
onViewChange?: string;
onChangeView?: string;
onChangeDataset?: string;
onChangeCurrentSample?: string;
onChangeSelected?: string;
onChangeSelectedLabels?: string;
onChangeExtendedSelection?: string;
dimensions: {
bounds: {
height?: number;
Expand Down Expand Up @@ -54,7 +54,7 @@ function useCtxChangePanelEvent(panelId, value, operator) {

export function useCustomPanelHooks(props: CustomPanelProps): CustomPanelHooks {
const { panelId } = props;
const [panelState, setPanelState] = usePanelState(null, panelId);
const [panelState] = usePanelState(null, panelId);
const [panelStateLocal, setPanelStateLocal] = usePanelState(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper usage of executeOperator with the new function signature.

The executeOperator function is called multiple times in this file. Given the changes in its signature, ensure that all calls to this function pass the correct parameters as per the new signature. This includes the addition of the options parameter where necessary.

- executeOperator(props.onLoad, { panel_id: panelId, panel_state: panelState?.state });
+ executeOperator(props.onLoad, { panel_id: panelId, panel_state: panelState?.state }, {});

Also applies to: 78-85, 113-130, 138-151

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const [panelState] = usePanelState(null, panelId);
const [panelState] = usePanelState(null, panelId);
// Assuming the executeOperator function is called somewhere in this file, here is an example update:
executeOperator(props.onLoad, { panel_id: panelId, panel_state: panelState?.state }, {});

null,
panelId,
Expand All @@ -75,14 +75,14 @@ export function useCustomPanelHooks(props: CustomPanelProps): CustomPanelHooks {
});
const ctx = useGlobalExecutionContext();

function onLoad() {
const onLoad = useCallback(() => {
if (props.onLoad) {
executeOperator(props.onLoad, {
panel_id: panelId,
panel_state: panelState?.state,
});
}
}
}, [props.onLoad, panelId, panelState?.state]);
useCtxChangePanelEvent(panelId, ctx._currentContext, props.onChangeCtx);
useCtxChangePanelEvent(panelId, ctx.view, props.onChangeView);
useCtxChangePanelEvent(panelId, ctx.viewName, props.onChangeView);
Expand All @@ -105,34 +105,50 @@ export function useCustomPanelHooks(props: CustomPanelProps): CustomPanelHooks {
ctx.selectedLabels,
props.onChangeSelectedLabels
);
const isLoaded = useMemo(() => {
return panelStateLocal?.loaded;
}, [panelStateLocal?.loaded]);

useEffect(() => {
if (props.onLoad && !panelState?.loaded) {
executeOperator(props.onLoad, { panel_id: panelId }, (result) => {
const { error: onLoadError } = result;
setPanelState((s) => ({ ...s, onLoadError, loaded: true }));
});
if (props.onLoad && !isLoaded) {
executeOperator(
props.onLoad,
{ panel_id: panelId },
{
callback(result) {
const { error: onLoadError } = result;
setPanelStateLocal((s) => ({ ...s, onLoadError, loaded: true }));
},
}
);
}

return () => {
if (props.onUnLoad)
executeOperator(props.onUnLoad, { panel_id: panelId });
};
}, [panelId, props.onLoad, props.onUnLoad]);
}, [panelId, props.onLoad, props.onUnLoad, isLoaded, setPanelStateLocal]);

// Trigger panel "onLoad" operator when panel state changes externally
useEffect(() => {
if (
lastPanelLoadState.current?.count !== panelsStateUpdatesCount &&
!isEqual(lastPanelLoadState.current?.state, panelState)
) {
setPanelStateLocal({});
onLoad();
}
lastPanelLoadState.current = {
count: panelsStateUpdatesCount,
state: panelState,
};
}, [panelsStateUpdatesCount, panelState, panelId, props.onLoad]);
}, [
panelsStateUpdatesCount,
panelState,
panelId,
onLoad,
setPanelStateLocal,
]);

useEffect(() => {
if (props.onViewChange)
Expand Down
2 changes: 1 addition & 1 deletion app/packages/operators/src/usePanelEvent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export default function usePanelEvent() {
if (prompt) {
promptForOperator(operator, actualParams, { callback: options.callback });
} else {
executeOperator(operator, actualParams, options.callback);
executeOperator(operator, actualParams, { callback: options.callback });
}
});
}
13 changes: 11 additions & 2 deletions app/packages/spaces/src/components/AddPanelButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { IconButton, Popout } from "@fiftyone/components";
import { IconButton, Popout, scrollable } from "@fiftyone/components";
import { useOutsideClick } from "@fiftyone/state";
import { Add } from "@mui/icons-material";
import { useMemo, useRef, useState } from "react";
Expand Down Expand Up @@ -46,7 +46,16 @@ export default function AddPanelButton({ node, spaceId }: AddPanelButtonProps) {
<Add sx={{ fontSize: 16 }} />
</IconButton>
{open && (
<Popout style={{ top: "80%", left: "16%", padding: 0 }}>
<Popout
style={{
top: "80%",
left: "16%",
padding: 0,
maxHeight: "calc(90vh - 120px)",
overflow: "auto",
}}
popoutProps={{ className: scrollable }}
>
{availablePanels.map((panel) => (
<AddPanelItem
key={panel.name}
Expand Down
Loading
Loading