Skip to content

Add ability for MethodRow to render multiple modelings of the same method #2910

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

Merged
robertbrignull merged 10 commits into from
Oct 9, 2023
Merged

Add ability for MethodRow to render multiple modelings of the same method #2910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e75eccb
Change MethodRow to accept ModelEditorViewState object instead of jus…
robertbrignull Oct 4, 2023
a704cd7
Convert getModelingStatus to take ModeledMethod[]
robertbrignull Oct 4, 2023
86d7d83
Convert ApiOrMethodCell to ApiOrMethodRow
robertbrignull Oct 4, 2023
252c7a2
Convert MethodRow to display multiple modelings
robertbrignull Oct 4, 2023
8eef4eb
Add story with multiple modelings
robertbrignull Oct 4, 2023
f87b1c4
Add some simple tests of rendering multiple models
robertbrignull Oct 4, 2023
5ba64a1
Add test for when there's no modeled method
robertbrignull Oct 4, 2023
8b0825a
Use better name for variable
robertbrignull Oct 9, 2023
0265353
Use index as react key
robertbrignull Oct 9, 2023
edd2aa5
Merge branch 'main' into robertbrignull/multiple-models-method-row
robertbrignull Oct 9, 2023
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
5 changes: 4 additions & 1 deletion extensions/ql-vscode/src/model-editor/methods-usage/methods-usage-data-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,10 @@ export class MethodsUsageDataProvider
const modeledMethod = this.modeledMethods[method.signature];
const modifiedMethod = this.modifiedMethodSignatures.has(method.signature);

const status = getModelingStatus(modeledMethod, modifiedMethod);
const status = getModelingStatus(
modeledMethod ? [modeledMethod] : [],
modifiedMethod,
);
switch (status) {
case "unmodeled":
return new ThemeIcon("error", new ThemeColor("errorForeground"));
Expand Down
6 changes: 3 additions & 3 deletions extensions/ql-vscode/src/model-editor/shared/modeling-status.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ import { ModeledMethod } from "../modeled-method";
export type ModelingStatus = "unmodeled" | "unsaved" | "saved";

export function getModelingStatus(
modeledMethod: ModeledMethod | undefined,
modeledMethods: ModeledMethod[],
methodIsUnsaved: boolean,
): ModelingStatus {
if (modeledMethod) {
if (modeledMethods.length > 0) {
if (methodIsUnsaved) {
return "unsaved";
} else if (modeledMethod.type !== "none") {
} else if (modeledMethods.some((m) => m.type !== "none")) {
return "saved";
}
}
Expand Down
44 changes: 37 additions & 7 deletions extensions/ql-vscode/src/stories/model-editor/MethodRow.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import { CallClassification, Method } from "../../model-editor/method";
import { ModeledMethod } from "../../model-editor/modeled-method";
import { VSCodeDataGrid } from "@vscode/webview-ui-toolkit/react";
import { GRID_TEMPLATE_COLUMNS } from "../../view/model-editor/ModeledMethodDataGrid";
import { ModelEditorViewState } from "../../model-editor/shared/view-state";
import { createMockExtensionPack } from "../../../test/factories/model-editor/extension-pack";
import { Mode } from "../../model-editor/shared/mode";

export default {
title: "CodeQL Model Editor/Method Row",
Expand Down Expand Up @@ -66,51 +69,78 @@ const modeledMethod: ModeledMethod = {
methodParameters: "()",
};

const viewState: ModelEditorViewState = {
extensionPack: createMockExtensionPack(),
showFlowGeneration: true,
showLlmButton: true,
showMultipleModels: true,
mode: Mode.Application,
};

export const Unmodeled = Template.bind({});
Unmodeled.args = {
method,
modeledMethod: undefined,
modeledMethods: [],
methodCanBeModeled: true,
viewState,
};

export const Source = Template.bind({});
Source.args = {
method,
modeledMethod: { ...modeledMethod, type: "source" },
modeledMethods: [{ ...modeledMethod, type: "source" }],
methodCanBeModeled: true,
viewState,
};

export const Sink = Template.bind({});
Sink.args = {
method,
modeledMethod: { ...modeledMethod, type: "sink" },
modeledMethods: [{ ...modeledMethod, type: "sink" }],
methodCanBeModeled: true,
viewState,
};

export const Summary = Template.bind({});
Summary.args = {
method,
modeledMethod: { ...modeledMethod, type: "summary" },
modeledMethods: [{ ...modeledMethod, type: "summary" }],
methodCanBeModeled: true,
viewState,
};

export const Neutral = Template.bind({});
Neutral.args = {
method,
modeledMethod: { ...modeledMethod, type: "neutral" },
modeledMethods: [{ ...modeledMethod, type: "neutral" }],
methodCanBeModeled: true,
viewState,
};

export const AlreadyModeled = Template.bind({});
AlreadyModeled.args = {
method: { ...method, supported: true },
modeledMethod: undefined,
modeledMethods: [],
viewState,
};

export const ModelingInProgress = Template.bind({});
ModelingInProgress.args = {
method,
modeledMethod,
modeledMethods: [modeledMethod],
modelingInProgress: true,
methodCanBeModeled: true,
viewState,
};

export const MultipleModelings = Template.bind({});
MultipleModelings.args = {
method,
modeledMethods: [
{ ...modeledMethod, type: "source" },
{ ...modeledMethod, type: "sink" },
{ ...modeledMethod },
],
methodCanBeModeled: true,
viewState,
};
3 changes: 2 additions & 1 deletion extensions/ql-vscode/src/view/method-modeling/MethodModelingView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ export function MethodModelingView({ initialViewState }: Props): JSX.Element {
const [isMethodModified, setIsMethodModified] = useState<boolean>(false);

const modelingStatus = useMemo(
() => getModelingStatus(modeledMethod, isMethodModified),
() =>
getModelingStatus(modeledMethod ? [modeledMethod] : [], isMethodModified),
[modeledMethod, isMethodModified],
);

Expand Down
2 changes: 1 addition & 1 deletion extensions/ql-vscode/src/view/model-editor/LibraryRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ export const LibraryRow = ({
modeledMethods={modeledMethods}
modifiedSignatures={modifiedSignatures}
inProgressMethods={inProgressMethods}
mode={viewState.mode}
viewState={viewState}
hideModeledMethods={hideModeledMethods}
revealedMethodSignature={revealedMethodSignature}
onChange={onChange}
Expand Down
158 changes: 100 additions & 58 deletions extensions/ql-vscode/src/view/model-editor/MethodRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,16 @@ import { MethodName } from "./MethodName";
import { ModelTypeDropdown } from "./ModelTypeDropdown";
import { ModelInputDropdown } from "./ModelInputDropdown";
import { ModelOutputDropdown } from "./ModelOutputDropdown";
import { ModelEditorViewState } from "../../model-editor/shared/view-state";

const ApiOrMethodCell = styled(VSCodeDataGridCell)`
const MultiModelColumn = styled(VSCodeDataGridCell)`
display: flex;
flex-direction: column;
gap: 0.5em;
`;

const ApiOrMethodRow = styled.div`
min-height: calc(var(--input-height) * 1px);
display: flex;
flex-direction: row;
align-items: center;
Expand Down Expand Up @@ -55,10 +63,10 @@ const DataGridRow = styled(VSCodeDataGridRow)<{ focused?: boolean }>`
export type MethodRowProps = {
method: Method;
methodCanBeModeled: boolean;
modeledMethod: ModeledMethod | undefined;
modeledMethods: ModeledMethod[];
methodIsUnsaved: boolean;
modelingInProgress: boolean;
mode: Mode;
viewState: ModelEditorViewState;
revealedMethodSignature: string | null;
onChange: (modeledMethod: ModeledMethod) => void;
Copy link
Copy Markdown
Contributor Author

@robertbrignull robertbrignull Oct 5, 2023

Choose a reason for hiding this comment

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

I'm not yet sure what the signature for onChange should be. Perhaps it should not be (modeledMethods: ModeledMethod[]) => void so we send over all the modelings for this method whenever anything changes.

I think it'll have to be that, as otherwise is something changes on one modeling, the receiver of the callback wouldn't know which modeling it was. So we have to send all of them at once so it knows if the number of modelings has changed and the exact state of all of them.

I could do that in this PR or the next one. This PR isn't intending to actually make this component work yet and be able to edit multiple modelings. I just want to get the UI looking right and then we can hook up all the implementation later. So long as it still works correctly when the feature flag is disabled that's ok.

Copy link
Copy Markdown
Member

@koesie10 koesie10 Oct 6, 2023

Choose a reason for hiding this comment

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

I think sending all of them even if only 1 has changed makes the most sense. My suggestion would be something like onChange: (signature: string, modeledMethods: ModeledMethod[]). I would suggest adding the signature so we don't need to find the signature of the method that is being modeled from the modeledMethods, but can simply use it as-is.

Copy link
Copy Markdown
Contributor Author

@robertbrignull robertbrignull Oct 9, 2023

Choose a reason for hiding this comment

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

Making the method signature onChange: (signature: string, modeledMethods: ModeledMethod[]) sounds good to me. I was also considering the same thing. I'll save that change for a separate PR though.

};
Expand Down Expand Up @@ -88,38 +96,44 @@ const ModelableMethodRow = forwardRef<HTMLElement | undefined, MethodRowProps>(
(props, ref) => {
const {
method,
modeledMethod,
modeledMethods: modeledMethodsProp,
methodIsUnsaved,
mode,
viewState,
revealedMethodSignature,
onChange,
} = props;

const modeledMethods = viewState.showMultipleModels
? modeledMethodsProp
: modeledMethodsProp.slice(0, 1);

const jumpToUsage = useCallback(
() => sendJumpToUsageMessage(method),
[method],
);

const modelingStatus = getModelingStatus(modeledMethod, methodIsUnsaved);
const modelingStatus = getModelingStatus(modeledMethods, methodIsUnsaved);

return (
<DataGridRow
data-testid="modelable-method-row"
ref={ref}
focused={revealedMethodSignature === method.signature}
>
<ApiOrMethodCell gridColumn={1}>
<ModelingStatusIndicator status={modelingStatus} />
<MethodClassifications method={method} />
<MethodName {...props.method} />
{mode === Mode.Application && (
<UsagesButton onClick={jumpToUsage}>
{method.usages.length}
</UsagesButton>
)}
<ViewLink onClick={jumpToUsage}>View</ViewLink>
{props.modelingInProgress && <ProgressRing />}
</ApiOrMethodCell>
<VSCodeDataGridCell gridColumn={1}>
<ApiOrMethodRow>
<ModelingStatusIndicator status={modelingStatus} />
<MethodClassifications method={method} />
<MethodName {...props.method} />
{viewState.mode === Mode.Application && (
<UsagesButton onClick={jumpToUsage}>
{method.usages.length}
</UsagesButton>
)}
<ViewLink onClick={jumpToUsage}>View</ViewLink>
{props.modelingInProgress && <ProgressRing />}
</ApiOrMethodRow>
</VSCodeDataGridCell>
{props.modelingInProgress && (
<>
<VSCodeDataGridCell gridColumn={2}>
Expand All @@ -138,34 +152,46 @@ const ModelableMethodRow = forwardRef<HTMLElement | undefined, MethodRowProps>(
)}
{!props.modelingInProgress && (
<>
<VSCodeDataGridCell gridColumn={2}>
<ModelTypeDropdown
method={method}
modeledMethod={modeledMethod}
onChange={onChange}
/>
</VSCodeDataGridCell>
<VSCodeDataGridCell gridColumn={3}>
<ModelInputDropdown
method={method}
modeledMethod={modeledMethod}
onChange={onChange}
/>
</VSCodeDataGridCell>
<VSCodeDataGridCell gridColumn={4}>
<ModelOutputDropdown
method={method}
modeledMethod={modeledMethod}
onChange={onChange}
/>
</VSCodeDataGridCell>
<VSCodeDataGridCell gridColumn={5}>
<ModelKindDropdown
method={method}
modeledMethod={modeledMethod}
onChange={onChange}
/>
</VSCodeDataGridCell>
<MultiModelColumn gridColumn={2}>
{forEachModeledMethod(modeledMethods, (modeledMethod, index) => (
<ModelTypeDropdown
key={index}
method={method}
modeledMethod={modeledMethod}
onChange={onChange}
/>
))}
</MultiModelColumn>
<MultiModelColumn gridColumn={3}>
{forEachModeledMethod(modeledMethods, (modeledMethod, index) => (
<ModelInputDropdown
key={index}
method={method}
modeledMethod={modeledMethod}
onChange={onChange}
/>
))}
</MultiModelColumn>
<MultiModelColumn gridColumn={4}>
{forEachModeledMethod(modeledMethods, (modeledMethod, index) => (
<ModelOutputDropdown
key={index}
method={method}
modeledMethod={modeledMethod}
onChange={onChange}
/>
))}
</MultiModelColumn>
<MultiModelColumn gridColumn={5}>
{forEachModeledMethod(modeledMethods, (modeledMethod, index) => (
<ModelKindDropdown
key={index}
method={method}
modeledMethod={modeledMethod}
onChange={onChange}
/>
))}
</MultiModelColumn>
</>
)}
</DataGridRow>
Expand All @@ -178,7 +204,7 @@ const UnmodelableMethodRow = forwardRef<
HTMLElement | undefined,
MethodRowProps
>((props, ref) => {
const { method, mode, revealedMethodSignature } = props;
const { method, viewState, revealedMethodSignature } = props;

const jumpToUsage = useCallback(
() => sendJumpToUsageMessage(method),
Expand All @@ -191,17 +217,19 @@ const UnmodelableMethodRow = forwardRef<
ref={ref}
focused={revealedMethodSignature === method.signature}
>
<ApiOrMethodCell gridColumn={1}>
<ModelingStatusIndicator status="saved" />
<MethodName {...props.method} />
{mode === Mode.Application && (
<UsagesButton onClick={jumpToUsage}>
{method.usages.length}
</UsagesButton>
)}
<ViewLink onClick={jumpToUsage}>View</ViewLink>
<MethodClassifications method={method} />
</ApiOrMethodCell>
<VSCodeDataGridCell gridColumn={1}>
<ApiOrMethodRow>
<ModelingStatusIndicator status="saved" />
<MethodName {...props.method} />
{viewState.mode === Mode.Application && (
<UsagesButton onClick={jumpToUsage}>
{method.usages.length}
</UsagesButton>
)}
<ViewLink onClick={jumpToUsage}>View</ViewLink>
<MethodClassifications method={method} />
</ApiOrMethodRow>
</VSCodeDataGridCell>
<VSCodeDataGridCell gridColumn="span 4">
Method already modeled
</VSCodeDataGridCell>
Expand All @@ -218,3 +246,17 @@ function sendJumpToUsageMessage(method: Method) {
usage: method.usages[0],
});
}

function forEachModeledMethod(
Copy link
Copy Markdown
Contributor Author

@robertbrignull robertbrignull Oct 4, 2023

Choose a reason for hiding this comment

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

I'm not totally happy with this but it's the cleanest thing I could come up with right now. The idea is that when there are no modeled methods we still need to render one set of boxes, but passing undefined as the modeled method.

modeledMethods: ModeledMethod[],
renderer: (
modeledMethod: ModeledMethod | undefined,
index: number,
) => JSX.Element,
): JSX.Element | JSX.Element[] {
if (modeledMethods.length === 0) {
return renderer(undefined, 0);
} else {
return modeledMethods.map(renderer);
}
}
Loading