From 9b811f09f2e832372745092006cb33b613358308 Mon Sep 17 00:00:00 2001 From: Neta London <67196883+netalondon@users.noreply.github.com> Date: Sat, 6 Apr 2024 20:02:21 +0300 Subject: [PATCH] Assembler UI fixes (#260) * Display filename in page title instead of panel title * Change compare panel title * Change comparison failure message * Fix jump-on-highlight behavior * Lock and recolor highlight on comparison error * Default rom format to bin when opening file from assembler * Make symbol table always visible --- components/src/stores/asm.store.ts | 23 +++++++++++++--- web/src/App.context.ts | 12 ++++++--- web/src/pages/asm.scss | 9 ++++--- web/src/pages/asm.tsx | 26 +++++++++++++----- web/src/pages/cpu.tsx | 4 +-- web/src/pico/pico.scss | 1 + web/src/shell/editor.scss | 4 +++ web/src/shell/editor.tsx | 43 ++++++++++++++++++++++++++---- 8 files changed, 98 insertions(+), 24 deletions(-) diff --git a/components/src/stores/asm.store.ts b/components/src/stores/asm.store.ts index fbde735f3..ea209db5f 100644 --- a/components/src/stores/asm.store.ts +++ b/components/src/stores/asm.store.ts @@ -180,6 +180,7 @@ export interface AsmPageState { compareName: string | undefined; lineNumbers: number[]; error?: CompilationError; + compareError: boolean; } export type AsmStoreDispatch = Dispatch<{ @@ -193,7 +194,7 @@ export function makeAsmStore( dispatch: MutableRefObject ) { const translator = new Translator(); - const highlightInfo = { + const highlightInfo: HighlightInfo = { resultHighlight: undefined, sourceHighlight: undefined, highlightMap: new Map(), @@ -202,6 +203,7 @@ export function makeAsmStore( let animate = true; let compiled = false; let translating = false; + let failure = false; const reducers = { setAsm( @@ -236,6 +238,7 @@ export function makeAsmStore( state.lineNumbers = Array.from(translator.lineNumbers); state.sourceHighlight = highlightInfo.sourceHighlight; state.resultHighlight = highlightInfo.resultHighlight; + state.compareError = failure; }, compare(state: AsmPageState) { @@ -245,6 +248,7 @@ export function makeAsmStore( .filter((line) => line.trim() != ""); if (resultLines.length != compareLines.length) { + failure = true; setStatus("Comparison failed - different lengths"); return; } @@ -252,8 +256,10 @@ export function makeAsmStore( for (let i = 0; i < compareLines.length; i++) { for (let j = 0; j < compareLines[i].length; j++) { if (resultLines[i][j] !== compareLines[i][j]) { - setStatus(`Comparison failed at ${i}:${j}`); - state.resultHighlight = { + setStatus(`Comparison failure: Line ${i}`); + + failure = true; + highlightInfo.resultHighlight = { start: i * 17, end: (i + 1) * 17, line: -1, @@ -341,7 +347,16 @@ export function makeAsmStore( return translator.done; }, + compare() { + dispatch.current({ action: "compare" }); + this.updateHighlight(highlightInfo.resultHighlight?.start ?? 0, false); + dispatch.current({ action: "update" }); + }, + updateHighlight(index: number, fromSource: boolean) { + if (failure) { + return; + } for (const [sourceSpan, resultSpan] of highlightInfo.highlightMap) { if ( (fromSource && @@ -363,6 +378,7 @@ export function makeAsmStore( }, reset() { + failure = false; setStatus("Reset"); translator.reset(); this.resetHighlightInfo(); @@ -399,6 +415,7 @@ export function makeAsmStore( compare: "", compareName: undefined, lineNumbers: [], + compareError: false, }; return { initialState, reducers, actions }; diff --git a/web/src/App.context.ts b/web/src/App.context.ts index 89474138b..9c563f281 100644 --- a/web/src/App.context.ts +++ b/web/src/App.context.ts @@ -1,6 +1,6 @@ import { FileSystem } from "@davidsouther/jiffies/lib/esm/fs.js"; import { AsmPageState } from "@nand2tetris/components/stores/asm.store"; -import { MemoryAdapter } from "@nand2tetris/simulator/cpu/memory"; +import { Format, MemoryAdapter } from "@nand2tetris/simulator/cpu/memory"; import { createContext, useCallback, useState } from "react"; import { useDialog } from "./shell/dialog"; import { useFilePicker } from "./shell/file_select"; @@ -32,19 +32,22 @@ export function useMonaco() { export function useToolStates() { const [rom, setRom] = useState(); const [cpuPath, setCpuPath] = useState(); + const [cpuFormat, setCpuFormat] = useState("asm"); const [asmState, setAsmState] = useState(); const setCpuState = ( path: string | undefined, - rom: MemoryAdapter | undefined + rom: MemoryAdapter | undefined, + format: Format ) => { setCpuPath(path); setRom(rom); + setCpuFormat(format); }; return { - cpuState: { rom: rom, path: cpuPath }, + cpuState: { rom: rom, path: cpuPath, format: cpuFormat }, setCpuState, asmState, setAsmState, @@ -123,7 +126,7 @@ export const AppContext = createContext>({ return undefined; }, toolStates: { - cpuState: { rom: undefined, path: undefined }, + cpuState: { rom: undefined, path: undefined, format: "asm" }, setCpuState() { return undefined; }, @@ -139,6 +142,7 @@ export const AppContext = createContext>({ compare: "", compareName: undefined, lineNumbers: [], + compareError: false, }, setAsmState() { return undefined; diff --git a/web/src/pages/asm.scss b/web/src/pages/asm.scss index 7e06135dd..879abe003 100644 --- a/web/src/pages/asm.scss +++ b/web/src/pages/asm.scss @@ -7,13 +7,12 @@ margin: 0px; gap: 0; - grid-template-areas: "source result compare"; + grid-template-areas: "source result compare" "source sym compare"; grid-template-columns: 2.5fr 1fr 1fr; - grid-template-rows: 1fr; + grid-template-rows: 2fr 1fr; .source { grid-area: source; - // min-height: calc(var(--line-height) * 10rem); } .result { @@ -23,4 +22,8 @@ .compare { grid-area: compare; } + + .sym { + grid-area: sym; + } } diff --git a/web/src/pages/asm.tsx b/web/src/pages/asm.tsx index 5677064ad..7fad889c0 100644 --- a/web/src/pages/asm.tsx +++ b/web/src/pages/asm.tsx @@ -19,7 +19,7 @@ import "./asm.scss"; export const Asm = () => { const { state, actions, dispatch } = useAsmPageStore(); - const { toolStates, filePicker } = useContext(AppContext); + const { toolStates, filePicker, setTitle } = useContext(AppContext); const sourceCursorPos = useRef(0); const resultCursorPos = useRef(0); @@ -30,6 +30,9 @@ export const Asm = () => { useEffect(() => { if (toolStates.asmState) { actions.overrideState(toolStates.asmState); + if (toolStates.asmState.path) { + setTitle(toolStates.asmState.path.split("/").pop() ?? ""); + } } }, []); @@ -67,6 +70,7 @@ export const Asm = () => { requestAnimationFrame(async () => { await actions.loadAsm(path); setStatus(""); + setTitle(path.split("/").pop() ?? ""); }); }; @@ -95,7 +99,7 @@ export const Asm = () => { }; const compare = () => { - dispatch.current({ action: "compare" }); + actions.compare(); }; const onSpeedChange = (speed: number) => { @@ -104,7 +108,11 @@ export const Asm = () => { const loadToCpu = async () => { const bytes = await loadHack(state.result); - toolStates.setCpuState(state.path, new ROM(new Int16Array(bytes))); + toolStates.setCpuState( + state.path?.replace(".asm", ".hack"), + new ROM(new Int16Array(bytes)), + "bin" + ); redirectRef.current?.click(); }; @@ -116,7 +124,6 @@ export const Asm = () => { <>
Source - {state.path && `: ${state.path.split("/").pop()}`}
{runnerAssigned && runner.current && ( @@ -153,6 +160,7 @@ export const Asm = () => { { actions.setAsm(source); }} @@ -230,10 +238,12 @@ export const Asm = () => { }} grammar={undefined} language={""} - dynamicHeight={true} + alwaysRecenter={false} lineNumberTransform={(n) => (n - 1).toString()} /> - {state.symbols.length > 0 && state.translating && "Symbol Table"} + + Symbol Table}> + {/* {state.symbols.length > 0 && state.translating && "Symbol Table"} */} {state.translating && } { header={ <>
- Compare + Compare Code {state.compareName && `: ${state.compareName}`}
@@ -255,6 +265,8 @@ export const Asm = () => { { action: "replaceROM", payload: toolStates.cpuState.rom, }); + setRomFormat(toolStates.cpuState.format); if (toolStates.cpuState.path) { const name = toolStates.cpuState.path.split("/").pop() ?? ""; setTitle(name); setFileName(name); - if (toolStates.cpuState.path.endsWith(".hack")) setRomFormat("bin"); onUpload(toolStates.cpuState.path); } } }, []); useEffect(() => { - toolStates.setCpuState(fileName, state.sim.ROM); + toolStates.setCpuState(fileName, state.sim.ROM, romFormat); }); useEffect(() => { diff --git a/web/src/pico/pico.scss b/web/src/pico/pico.scss index 1063ce2dd..1e4d3a5fd 100644 --- a/web/src/pico/pico.scss +++ b/web/src/pico/pico.scss @@ -29,6 +29,7 @@ --card-border-color: black; --text-color: black; --mark-background-color: rgb(255, 230, 121); + --mark-error-color: rgb(252, 169, 154); --light-grey: rgb(170, 170, 170); --file-picker-width: 400px; } diff --git a/web/src/shell/editor.scss b/web/src/shell/editor.scss index 1baea9d4d..5d73d6949 100644 --- a/web/src/shell/editor.scss +++ b/web/src/shell/editor.scss @@ -14,6 +14,10 @@ background-color: var(--mark-background-color); } + .error-highlight { + background-color: var(--mark-error-color); + } + .red { color: rgb(190, 16, 16); } diff --git a/web/src/shell/editor.tsx b/web/src/shell/editor.tsx index 567800177..ed7afba29 100644 --- a/web/src/shell/editor.tsx +++ b/web/src/shell/editor.tsx @@ -68,12 +68,28 @@ export interface Decoration { cssClass: string; } +const isRangeVisible = ( + editor: monacoT.editor.IStandaloneCodeEditor | undefined, + range: monacoT.Range +) => { + for (const visibleRange of editor?.getVisibleRanges() ?? []) { + if (visibleRange.containsRange(range)) { + return true; + } + } + return false; +}; + +export type HighlightType = "highlight" | "error"; + const makeDecorations = ( monaco: typeof monacoT | null, editor: monacoT.editor.IStandaloneCodeEditor | undefined, highlight: Span | undefined, additionalDecorations: Decoration[], - decorations: string[] + decorations: string[], + type: HighlightType = "highlight", + alwaysCenter = true ): string[] => { if (!(editor && highlight)) return decorations; const model = editor.getModel(); @@ -85,15 +101,20 @@ const makeDecorations = ( if (range) { nextDecoration.push({ range, - options: { inlineClassName: "highlight" }, + options: { + inlineClassName: type == "error" ? "error-highlight" : "highlight", + }, }); if (highlight.start != highlight.end) { - editor.revealRangeInCenter(range); + if (alwaysCenter || !isRangeVisible(editor, range)) { + editor.revealRangeInCenter(range); + } } } for (const decoration of additionalDecorations) { const range = monaco?.Range.fromPositions( model.getPositionAt(decoration.span.start), + // editor.getSc model.getPositionAt(decoration.span.end) ); if (range) { @@ -114,8 +135,10 @@ const Monaco = ({ error, disabled = false, highlight: currentHighlight, + highlightType = "highlight", customDecorations: currentCustomDecorations = [], dynamicHeight = false, + alwaysRecenter = true, lineNumberTransform, }: { value: string; @@ -125,8 +148,10 @@ const Monaco = ({ error?: CompilationError; disabled?: boolean; highlight?: Span | number; + highlightType?: HighlightType; customDecorations?: Decoration[]; dynamicHeight?: boolean; + alwaysRecenter?: boolean; lineNumberTransform?: (n: number) => string; }) => { const { theme } = useContext(AppContext); @@ -177,9 +202,11 @@ const Monaco = ({ // either. newHighlight ?? { start: 0, end: 0, line: 0 }, customDecorations.current, - decorations.current + decorations.current, + highlightType, + alwaysRecenter ); - }, [decorations, monaco, editor, highlight]); + }, [decorations, monaco, editor, highlight, highlightType]); const calculateHeight = () => { if (dynamicHeight) { @@ -304,8 +331,10 @@ export const Editor = ({ grammar, language, highlight, + highlightType = "highlight", customDecorations = [], dynamicHeight = false, + alwaysRecenter = true, lineNumberTransform, }: { className?: string; @@ -318,8 +347,10 @@ export const Editor = ({ grammar?: ohm.Grammar; language: string; highlight?: Span | number; + highlightType?: HighlightType; customDecorations?: Decoration[]; dynamicHeight?: boolean; + alwaysRecenter?: boolean; lineNumberTransform?: (n: number) => string; }) => { const { monaco } = useContext(AppContext); @@ -338,8 +369,10 @@ export const Editor = ({ error={error} disabled={disabled} highlight={highlight} + highlightType={highlightType} customDecorations={customDecorations} dynamicHeight={dynamicHeight} + alwaysRecenter={alwaysRecenter} lineNumberTransform={lineNumberTransform} /> ) : (