-
Notifications
You must be signed in to change notification settings - Fork 2
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
base: develop
Are you sure you want to change the base?
Problem History #39
Changes from 16 commits
e53a0e9
9341dcd
60085dc
5d550c8
820b1f5
6b52dd6
a220fbc
23b0da7
4d6fbe0
59786da
dcc7725
98bc3c2
297877a
ce58623
7b723a0
4e254b8
1af6a83
b1d51b2
a516efc
fb71ba4
3be04be
53e60be
2f5b411
de8cfb8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This naming is unconventional for React. I suggest using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe rather a joint event There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
); | ||
}; |
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To me, this There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
/** | ||
* Ids of the solutions of the problem input per problem type | ||
*/ | ||
solutionIds: ProblemTypeSolutionId; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe just use a (or was there a problem with using maps in serialization? I can't quite remember) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
export type ProblemTypeSolutionId = { | ||
[problemTypeId: string]: number; | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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., |
||
|
||
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}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe store results separately for separate problem types? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
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); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you extract the store / restore code to an external file? |
||
}, [problemStates, historyItemName]); | ||
|
||
useEffect(() => { | ||
localStorage.setItem(historyItemName, JSON.stringify(problemStates)); | ||
}, [historyItemName, problemStates]); | ||
|
||
async function getSolution() { | ||
setClicked(true); | ||
setFinished(false); | ||
|
||
|
@@ -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); | ||
|
@@ -66,7 +131,7 @@ export const ProgressHandler = <T extends {}>( | |
/> | ||
))} | ||
<Center> | ||
<GoButton clicked={startSolving} /> | ||
<GoButton clicked={getSolution} /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you rename this function? I think the |
||
</Center> | ||
</HStack> | ||
) : null} | ||
|
@@ -86,6 +151,12 @@ export const ProgressHandler = <T extends {}>( | |
</Box> | ||
)) | ||
: null} | ||
|
||
<History | ||
problemStates={problemStates} | ||
loadSolution={loadSolution} | ||
setContent={props.setProblemInput} | ||
/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
</Container> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,18 +8,17 @@ import { EditorControls } from "./EditorControls"; | |
|
||
export interface TextInputMaskProperties { | ||
textPlaceholder: string; | ||
onTextChanged: (text: string) => void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
@@ -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)} | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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