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.

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

Problem History #39

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
e53a0e9
Remove logical expression parsing
Elscrux Oct 25, 2023
9341dcd
Remove negated variable token and use separate tokens for negate and …
Elscrux Oct 25, 2023
60085dc
Add more logical expression validations
Elscrux Oct 25, 2023
5d550c8
Refactor text synchronization
Elscrux Nov 2, 2023
820b1f5
Add fetchSolution API call
Elscrux Nov 2, 2023
6b52dd6
Add history
Elscrux Nov 2, 2023
a220fbc
Give the ListItem key a proper value
Elscrux Nov 2, 2023
23b0da7
Add tooltip to be able to see the full text for long problems
Elscrux Nov 9, 2023
4d6fbe0
Add load spinner for loading solutions too
Elscrux Nov 9, 2023
59786da
Add cookie library
Elscrux Nov 9, 2023
dcc7725
Add cookie to store history
Elscrux Nov 9, 2023
98bc3c2
Fix using wrong variable for MaxCut text
Elscrux Nov 9, 2023
297877a
Use different cookies for different problem types and useEffect loop
Elscrux Nov 9, 2023
ce58623
Don't try to load an existing solution when clicking then Go button
Elscrux Nov 9, 2023
7b723a0
Revert "Add cookie library"
Elscrux Nov 14, 2023
4e254b8
Replace cookie approach with local storage
Elscrux Nov 14, 2023
1af6a83
Reduce History api to one function onRequestRollback
Elscrux Jan 23, 2024
b1d51b2
Rename content to problemInput
Elscrux Jan 23, 2024
a516efc
Rename setText to onTextChanged
Elscrux Jan 23, 2024
fb71ba4
Extract history storage to own file
Elscrux Jan 23, 2024
3be04be
Rename getSolution to startSolving
Elscrux Jan 23, 2024
53e60be
Rename ProblemTypeSolutionId to SolutionIds
Elscrux Jan 23, 2024
2f5b411
Remove code that is commented out
Elscrux Jan 23, 2024
de8cfb8
Remove unused import
Elscrux Jan 23, 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: 2 additions & 2 deletions __tests__/converter/dimacs/DimacsLogicalExpression.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ describe("Parsing", () => {
let dimacsParser = new DimacsParser();
let logicalExpressionParser = new LogicalExpressionParser();

each([
/*each([
[
"((1 or 2 or not 3) and (!4 and (not 5 and 6)) and 3 and (7 or 2))",
"c First comment\nc Some Comment\nc 1 => 1\np sat 7\n*(+( 1 2 -3 )*( -4 *( -5 6 )) 3 +( 7 2 ))",
Expand All @@ -48,7 +48,7 @@ describe("Parsing", () => {
logicalExpression
);
}
);
);*/

each([
[
Expand Down
38 changes: 26 additions & 12 deletions src/api/ToolboxAPI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,19 @@ import { SolveRequest } from "./data-model/SolveRequest";
*/
export const baseUrl = () => process.env.NEXT_PUBLIC_API_BASE_URL;

function invalidSolution(reason: string): Solution {
return {
id: -1,
status: SolutionStatus.INVALID,
solverName: "",
executionMilliseconds: 0,
solutionData: "",
debugData: "",
metaData: "",
error: `${reason}`,
};
}

export async function postProblem<T>(
problemType: string,
solveRequest: SolveRequest<T>
Expand All @@ -23,18 +36,19 @@ export async function postProblem<T>(
})
.then((response) => response.json())
.then((json) => json as Solution)
.catch((reason) => {
return {
id: -1,
status: SolutionStatus.INVALID,
solverName: "",
executionMilliseconds: 0,
solutionData: "",
debugData: "",
metaData: "",
error: `${reason}`,
};
});
.catch(invalidSolution);
}

export async function fetchSolution(problemType: string, solutionId: number) {
return fetch(`${baseUrl()}/solution/${problemType}?id=${solutionId}`, {
Copy link
Member

Choose a reason for hiding this comment

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

This code seems to throw a preflight CORS error when a solution is clicked

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed by this toolbox PR that I forgot to make
ProvideQ/toolbox-server#67

method: "GET",
headers: {
"Content-Type": "application/json",
},
})
.then((response) => response.json())
.then((json) => json as Solution)
.catch(invalidSolution);
}

export async function fetchSolvers(
Expand Down
54 changes: 54 additions & 0 deletions src/components/solvers/History.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import {
Button,
Divider,
Heading,
List,
ListItem,
Stack,
Tooltip,
} from "@chakra-ui/react";
import { ProblemState, ProblemTypeSolutionId } from "./ProgressHandler";

interface HistoryProps<T> {
problemStates: ProblemState<T>[];

setContent: (t: T) => void;
loadSolution: (id: ProblemTypeSolutionId) => void;
Copy link
Member

Choose a reason for hiding this comment

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

This naming is unconventional for React. I suggest using onRequestSolution for loadSolution to express the event-like character of passing data upwards. I'm unsure what to do with setContent, maybe onRequestContent or onContentChange?

Copy link
Member

Choose a reason for hiding this comment

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

maybe rather a joint event onRequestRollback(problemState) that can be handled by the containing element? Then, this element wouldn't have to deal with the data managed by another element.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea!

}

export const History = <T extends {}>(props: HistoryProps<T>) => {
if (props.problemStates.length == 0) return null;

function reloadState(state: ProblemState<T>) {
props.setContent(state.content);
props.loadSolution(state.solutionIds);
}

return (
<Stack>
<Heading as="h3" size="md" textAlign="center">
History
</Heading>
<List>
{props.problemStates.map((x) => {
let contentString = x.content.toString();
return (
<ListItem key={contentString}>
<Tooltip label={contentString}>
<Button
width="200px"
overflow="hidden"
variant="link"
onClick={(_) => reloadState(x)}
>
{contentString}
</Button>
</Tooltip>
<Divider />
</ListItem>
);
})}
</List>
</Stack>
);
};
83 changes: 77 additions & 6 deletions src/components/solvers/ProgressHandler.tsx
Original file line number Diff line number Diff line change
@@ -1,33 +1,70 @@
import { Box, Center, HStack, VStack } from "@chakra-ui/react";
import React, { useState } from "react";
import { Box, Center, HStack } from "@chakra-ui/react";
import React, { useEffect, useState } from "react";
import { History } from "./History";
import { GoButton } from "./buttons/GoButton";
import { postProblem } from "../../api/ToolboxAPI";
import { fetchSolution, postProblem } from "../../api/ToolboxAPI";
import { SolutionView } from "./SolutionView";
import { Container } from "../Container";
import { Solution } from "../../api/data-model/Solution";
import { SolveRequest } from "../../api/data-model/SolveRequest";
import { SolverPicker } from "./SolverPicker";

export interface ProblemState<T> {
/**
* Problem input
*/
content: T;
Copy link
Member

Choose a reason for hiding this comment

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

To me, this content name seems a bit unintuitive. Maybe use input?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's inspired by SolveRequest's naming which has a field called requestContent - probably not the best naming either so we might end up changing that too.
In any case, I've changed this to problemInput. What do you think?

/**
* Ids of the solutions of the problem input per problem type
*/
solutionIds: ProblemTypeSolutionId;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just use a Map here?

(or was there a problem with using maps in serialization? I can't quite remember)

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a serialization issue here but not the one we had when passing a map to the server.
When storing something in the local storage, we just store a string. When deserializing the string, the type information of the map won't persist because of TypeScript limitations which causes that you can't call any functions on the object, as only the primitive object data is loaded but not the type information which are lost.

}

export type ProblemTypeSolutionId = {
[problemTypeId: string]: number;
};
Copy link
Member

Choose a reason for hiding this comment

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

The name is a bit misleading. It seems like an object of this type is an ID. Please give this a clearer name (i.e., SolutionIds) or just use a Map


export interface ProgressHandlerProps<T> {
/**
* List of problem types that should be solved with the given input.
*/
problemTypes: string[];
problemInput: T;
setProblemInput: (t: T) => void;
}

export const ProgressHandler = <T extends {}>(
props: ProgressHandlerProps<T>
) => {
const historyItemName: string = `problemStates-${props.problemTypes}`;
Copy link
Member

Choose a reason for hiding this comment

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

This identification scheme breaks for our Feature Model Anomaly solver. If I run a solve request only for the DEAD anomaly type, this state is gone after refreshing the page.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe store results separately for separate problem types?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I think storying them separately would be good but it also creates some new questions. One in particular, that I don't know the answer to is should clicking on a history element should change the currently selected solvers as well. As an example: You have only the dead anomaly type selected and solve problem 1. Then you select the void anomaly type as well and click on the history. Will this deselect void anomaly type again or will it just show no solution for it?

Copy link
Member Author

@Elscrux Elscrux Jan 23, 2024

Choose a reason for hiding this comment

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

image
Also would the history be displayed like this or how would it all work? (both would be a problem that both had, void only void, dead only dead)


const [wasClicked, setClicked] = useState<boolean>(false);
const [finished, setFinished] = useState<boolean>(false);
const [solutions, setSolutions] = useState<Solution[]>();
const [problemStates, setProblemStates] = useState<ProblemState<T>[]>([]);
const [solveRequest, setSolveRequest] = useState<SolveRequest<T>>({
requestContent: props.problemInput,
requestedSubSolveRequests: {},
});

async function startSolving() {
useEffect(() => {
// Handle problem states from local storage
if (problemStates.length == 0) {
let historyItem = localStorage.getItem(historyItemName);
if (historyItem == null) return;

let storageStates = JSON.parse(historyItem);
if (storageStates.length > 0) {
setProblemStates(storageStates);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you extract the store / restore code to an external file? useEffect should remain within a component but the code called within may go somewhere else

}, [problemStates, historyItemName]);

useEffect(() => {
localStorage.setItem(historyItemName, JSON.stringify(problemStates));
}, [historyItemName, problemStates]);

async function getSolution() {
setClicked(true);
setFinished(false);

Expand All @@ -39,7 +76,35 @@ export const ProgressHandler = <T extends {}>(
setSolveRequest(newSolveRequest);
Promise.all(
props.problemTypes.map((problemType) =>
postProblem(problemType, newSolveRequest)
postProblem(problemType, newSolveRequest).then((s) => ({
problemType: problemType,
solution: s,
}))
)
).then((result) => {
let solutionIdMap = result.reduce((acc, item) => {
acc[item.problemType] = item.solution.id;
return acc;
}, {} as ProblemTypeSolutionId);

let newProblemState: ProblemState<T> = {
content: props.problemInput,
solutionIds: solutionIdMap,
};

setSolutions(result.map((x) => x.solution));
setProblemStates([...problemStates, newProblemState]);
setFinished(true);
});
}

async function loadSolution(ids: ProblemTypeSolutionId) {
setClicked(true);
setFinished(false);

Promise.all(
props.problemTypes.map((problemType) =>
fetchSolution(problemType, ids[problemType])
)
).then((solutions) => {
setSolutions(solutions);
Expand All @@ -66,7 +131,7 @@ export const ProgressHandler = <T extends {}>(
/>
))}
<Center>
<GoButton clicked={startSolving} />
<GoButton clicked={getSolution} />
Copy link
Member

Choose a reason for hiding this comment

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

Why did you rename this function? I think the getSolution name hides the fact that this function might invoke an expensive computation of a solution. Also, this getter doesn't return anything

</Center>
</HStack>
) : null}
Expand All @@ -86,6 +151,12 @@ export const ProgressHandler = <T extends {}>(
</Box>
))
: null}

<History
problemStates={problemStates}
loadSolution={loadSolution}
setContent={props.setProblemInput}
/>
Copy link
Member

Choose a reason for hiding this comment

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

Why is the history part of the progress handler? Shouldn't this be independent from each other? Similarly, I don't think problemState data should be managed by the progress handler

Copy link
Member Author

Choose a reason for hiding this comment

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

Iirc this was a compromise as I didn't find a good structure to be able to load solutions (which is handled by the ProgressHandler) from the History without requiring a lot of boilerplate code in all the problem masks.
I'm not sure what the best way to resolve these kinds of communication problems is in React. Do you know any patterns or techniques to do that properly there? Some event-driven thing or so?

</Container>
);
};
2 changes: 1 addition & 1 deletion src/components/solvers/SAT/TextArea.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { SAT_language } from "./prism-SAT";

interface TextAreaProps {
problemString: string;
setProblemString: React.Dispatch<React.SetStateAction<string>>;
setProblemString: (problemString: string) => void;
Elscrux marked this conversation as resolved.
Show resolved Hide resolved
}

export const TextArea = (props: TextAreaProps) => {
Expand Down
4 changes: 2 additions & 2 deletions src/components/solvers/SAT/prism-SAT.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { regexVariable } from "../../../converter/dimacs/Syntax/CommonSyntax";
import {
regexAND,
regexNOTVariable,
regexNOT,
regexOR,
} from "../../../converter/dimacs/Syntax/LogicalExpressionSyntax";

Expand All @@ -16,7 +16,7 @@ export const SAT_language = {
},
"SAT-punctuation": /[()]/,
negation: {
pattern: regexNOTVariable,
pattern: regexNOT,
alias: "string",
},
"SAT-variable": {
Expand Down
11 changes: 5 additions & 6 deletions src/components/solvers/TextInputMask.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,17 @@ import { EditorControls } from "./EditorControls";

export interface TextInputMaskProperties {
textPlaceholder: string;
onTextChanged: (text: string) => void;
Copy link
Member

Choose a reason for hiding this comment

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

why did you rename this? passing events is much more conventional than passing setters in react

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't remember. I changed it back.

text: string;
setText: (value: string) => void;
body?: ReactElement;
}

export const TextInputMask = (props: TextInputMaskProperties) => {
const [text, setText] = useState<string>("");
const [errorString, setErrorString] = useState("");

function onTextChanged(text: string): void {
try {
setText(text);
props.onTextChanged(text);
props.setText(text);

setErrorString("");
} catch (e: any) {
Expand All @@ -40,11 +39,11 @@ export const TextInputMask = (props: TextInputMaskProperties) => {
errorText={errorString}
idleText={props.textPlaceholder + " 👇"}
onUpload={onTextChanged}
editorContent={text}
editorContent={props.text}
/>
<Textarea
placeholder={props.textPlaceholder}
value={text}
value={props.text}
minHeight="10rem"
isInvalid={errorString != ""}
onChange={(x) => onTextChanged(x.target.value)}
Expand Down
Loading