From fd54e43f9d784db6196c8b78891509f2fbfa2d84 Mon Sep 17 00:00:00 2001 From: Christopher Serr Date: Wed, 19 Jun 2024 19:31:07 +0200 Subject: [PATCH] Clean up some async race conditions & CSS fixes The dialogs are now asynchronous as opposed to the browser built-in ones that are synchronous, probably because they are incredibly old. The problem with the new dialogs being asynchronous is that for example the hotkeys, the server protocol and honestly any sort of async download that's still running in the background can all mess with the timer while the dialog is open. This could lead to all sorts of weird issues, like the timer being in a different state than what the dialog expects, the timer being reset twice and creating two dialogs that are open at the same time and possibly even memory corruption in the Rust code. This commit ensures that the timer can not be interacted with while it is "locked for interaction". In fact we already had such a notion where when you navigate to the settings, layout or run editor, the hotkeys would be disabled. Now this is concept is extended so the event sink also can prevent interaction with the timer. Navigating to these menus and opening dialogs will now trigger this locking mechanism. Additionally this improves various async race conditions in the splits editor related to downloading resources from speedrun.com or splits.io. This also touches the CSS of the dialogs again, adding some spacing around the dialog, so it doesn't touch the edges of the window and allowing the buttons to have more dynamic sizing. In particular I've noticed that the text sometimes doesn't fit into the buttons on iOS, so they are now allowed to grow a little to handle that situation. --- src/css/Dialog.scss | 91 +++++++++++++---------- src/css/HotkeyButton.scss | 6 +- src/css/LiveSplitServerButton.scss | 8 +- src/css/RunEditor.scss | 4 +- src/css/Sidebar.scss | 10 ++- src/css/Table.scss | 22 +++--- src/css/Toggle.scss | 10 +-- src/css/main.scss | 8 +- src/ui/Dialog.tsx | 95 ++++++++++++++---------- src/ui/LSOEventSink.ts | 98 ++++++++++++++++++++++++ src/ui/LiveSplit.tsx | 115 +++++++++++++++++++---------- src/ui/RunEditor.tsx | 57 +++++++++++--- src/ui/Settings.tsx | 9 ++- src/ui/SplitsSelection.tsx | 43 ++++++----- src/util/FileUtil.ts | 78 +++++++++++++------ src/util/SplitsIO.ts | 3 +- 16 files changed, 461 insertions(+), 196 deletions(-) diff --git a/src/css/Dialog.scss b/src/css/Dialog.scss index ff833b291..3884e8849 100644 --- a/src/css/Dialog.scss +++ b/src/css/Dialog.scss @@ -1,52 +1,65 @@ @import 'variables'; +.is-mobile+dialog>.dialog { + min-width: auto; +} + dialog { - color: #eee; - background: $main-background-color; - border: 2px solid $border-color; - border-radius: 10px; - min-width: 225px; - max-width: 400px; - padding: $ui-large-margin; - - h1 { - font-size: 20px; - margin: 5px 0; + background: none; + border: none; + outline: 0; + + &::backdrop { + background: rgba(0, 0, 0, 0.5); } - .buttons { - button { - font-size: 16px; - margin: 0; - min-width: 80px; + .dialog { + color: #eee; + background: $main-background-color; + border: 2px solid $border-color; + border-radius: 10px; + min-width: 225px; - &:focus { - border-color: #888; - } + max-width: 400px; + padding: $ui-large-margin; + + h1 { + font-size: 20px; + margin: 5px 0; } - display: flex; - flex-direction: row; - justify-content: flex-end; - column-gap: $ui-margin; - } + .buttons { + button { + font-size: 16px; + margin: 0; + min-width: 80px; + height: auto; - &::backdrop { - background: rgba(0, 0, 0, 0.5); - } + &:focus { + border-color: #888; + } + } + + display: flex; + flex-direction: row; + flex-wrap: wrap; + justify-content: flex-end; + gap: $ui-margin; + } + + input { + width: 100%; + border: none; + border-bottom: 1px solid hsla(0, 0%, 100%, 0.25); + background: transparent; + color: white; + text-overflow: ellipsis; + font-family: "fira", sans-serif; + font-size: 16px; - input { - width: 100%; - border: none; - border-bottom: 1px solid hsla(0, 0%, 100%, 0.25); - background: transparent; - color: white; - text-overflow: ellipsis; - font-family: "fira", sans-serif; - font-size: 16px; - - &:focus { - outline: 0; + &:focus { + outline: 0; + } } } } diff --git a/src/css/HotkeyButton.scss b/src/css/HotkeyButton.scss index 3ca626e44..607ea7d0d 100644 --- a/src/css/HotkeyButton.scss +++ b/src/css/HotkeyButton.scss @@ -2,9 +2,11 @@ .hotkey-box { button { - margin: 0px; + margin: 0; font-size: 16px; - height: 22px; + min-height: 22px; + padding-top: 0; + padding-bottom: 0; } .hotkey-button.focused { diff --git a/src/css/LiveSplitServerButton.scss b/src/css/LiveSplitServerButton.scss index c653a4270..e89f5be23 100644 --- a/src/css/LiveSplitServerButton.scss +++ b/src/css/LiveSplitServerButton.scss @@ -1,6 +1,10 @@ +@import 'variables'; + .livesplit-server-button { - margin: 0px; + margin: 0; font-size: 16px; - height: 22px; + min-height: 22px; + padding-top: 0; + padding-bottom: 0; // TODO: This is the same as the hotkey button. } diff --git a/src/css/RunEditor.scss b/src/css/RunEditor.scss index cfd3f178d..7a9d4ec11 100644 --- a/src/css/RunEditor.scss +++ b/src/css/RunEditor.scss @@ -270,7 +270,7 @@ $small-button-padding: 1px 3px 1px 3px; font-size: 15px; width: $button-width; margin-right: 0; - height: 30px; + min-height: 30px; @include mobile { margin-top: 0; @@ -389,7 +389,7 @@ $small-button-padding: 1px 3px 1px 3px; content: ""; height: 2px; width: 0; - bottom: 0px; + bottom: 0; position: absolute; background: hsla(50, 100%, 50%, 1); transition: 0.2s ease all; diff --git a/src/css/Sidebar.scss b/src/css/Sidebar.scss index cd8fcf2d4..50118e45c 100644 --- a/src/css/Sidebar.scss +++ b/src/css/Sidebar.scss @@ -18,9 +18,13 @@ $h2-font-size: 22px; @include toggle; - >div>div.small>button { - width: 50%; - font-size: 18px; + >div>div.small { + display: flex; + + >button { + width: 50%; + font-size: 18px; + } } .sidebar-buttons { diff --git a/src/css/Table.scss b/src/css/Table.scss index 44169d66f..9ffb0ba46 100644 --- a/src/css/Table.scss +++ b/src/css/Table.scss @@ -12,12 +12,12 @@ .table-row-even { display: table-row; - background: $dark-row-color !important; + background: $dark-row-color !important; } .table-row-odd { display: table-row; - background: $light-row-color !important; + background: $light-row-color !important; } } @@ -54,16 +54,16 @@ } .selected { - background: $selected-row-color !important; + background: $selected-row-color !important; } .tab-bar>button { font-size: 15px; - height: 30px; - border-bottom-right-radius: 0px; - border-bottom-left-radius: 0px; - margin-bottom: 0px; - border-bottom: 0px; + min-height: 30px; + border-bottom-right-radius: 0; + border-bottom-left-radius: 0; + margin-bottom: 0; + border-bottom: 0; } tr>td>input { @@ -144,9 +144,11 @@ grid-template-columns: 1fr 30px; button { - margin: 0px; + margin: 0; font-size: 12px; - height: $settings-row-height; + min-height: $settings-row-height; + padding-top: 0; + padding-bottom: 0; } } } diff --git a/src/css/Toggle.scss b/src/css/Toggle.scss index d50dcd2a4..0bf620e14 100644 --- a/src/css/Toggle.scss +++ b/src/css/Toggle.scss @@ -2,17 +2,17 @@ @mixin toggle { .toggle-left { - border-top-right-radius: 0px; - border-bottom-right-radius: 0px; + border-top-right-radius: 0; + border-bottom-right-radius: 0; } .toggle-right { - border-top-left-radius: 0px; - border-bottom-left-radius: 0px; + border-top-left-radius: 0; + border-bottom-left-radius: 0; } .toggle-middle { - border-radius: 0px; + border-radius: 0; } .button-pressed, diff --git a/src/css/main.scss b/src/css/main.scss index 944593828..e1c00716e 100644 --- a/src/css/main.scss +++ b/src/css/main.scss @@ -59,7 +59,7 @@ td#dif { } table { - border-spacing: 0px 2px; + border-spacing: 0 2px; } button:active, @@ -83,6 +83,7 @@ button { color: $button-text-color; cursor: pointer; font-size: 20px; + line-height: 1.1; border-width: 1px; border-style: solid; border-color: $border-color; @@ -90,7 +91,8 @@ button { border-radius: 5px; font-weight: bold; font-family: "fira", sans-serif; - height: $button-height; + min-height: $button-height; + padding: 5px $ui-margin; } a { @@ -124,7 +126,7 @@ button:disabled:active { ::-webkit-scrollbar-thumb { background-clip: padding-box; background-color: #303030; - border: 0px solid #0000; + border: 0 solid #0000; border-radius: 10px; } diff --git a/src/ui/Dialog.tsx b/src/ui/Dialog.tsx index e22ed91c9..1538a384a 100644 --- a/src/ui/Dialog.tsx +++ b/src/ui/Dialog.tsx @@ -2,6 +2,11 @@ import * as React from "react"; import "../css/Dialog.scss"; +export interface Props { + onShow: () => void, + onClose: () => void, +} + export interface Options { title: string | JSX.Element, description: string | JSX.Element, @@ -18,23 +23,32 @@ export interface State { let dialogElement: HTMLDialogElement | null = null; let setState: ((options: Options) => void) | undefined; let resolveFn: ((_: [number, string]) => void) | undefined; +let onCloseFn: (() => void) | undefined; +let alreadyClosed = false; export function showDialog(options: Options): Promise<[number, string]> { if (dialogElement) { dialogElement.showModal(); + alreadyClosed = false; + dialogElement.setAttribute("disabled", ""); const closeWith = options.buttons.length - 1; dialogElement.onclose = () => { - resolveFn?.([closeWith, ""]); + if (!alreadyClosed) { + resolveFn?.([closeWith, ""]); + onCloseFn?.(); + } }; } setState?.(options); return new Promise((resolve) => resolveFn = resolve); } -export default class DialogContainer extends React.Component { - constructor(props: unknown) { +export default class DialogContainer extends React.Component { + constructor(props: Props) { super(props); + onCloseFn = props.onClose; + this.state = { options: { title: "", @@ -46,15 +60,17 @@ export default class DialogContainer extends React.Component { } public componentDidMount(): void { - setState = (options) => this.setState({ - options, - input: options.defaultText ?? "", - }); + setState = (options) => { + this.props.onShow(); + this.setState({ + options, + input: options.defaultText ?? "", + }); + }; } public render() { return dialogElement = element} onKeyDown={(e) => { if (e?.key === "ArrowLeft") { @@ -66,38 +82,43 @@ export default class DialogContainer extends React.Component { } }} > -

{this.state.options.title}

-

{this.state.options.description}

- { - this.state.options.textInput && this.setState({ input: e.target.value })} - onKeyDown={(e) => { - if (e?.key === "Enter") { - e.preventDefault(); - dialogElement?.close(); - resolveFn?.([0, this.state.input]); - } - }} - /> - } -
+
+

{this.state.options.title}

+

{this.state.options.description}

{ - this.state.options.buttons.map((button, i) => { - return ; - }) + this.state.options.textInput && this.setState({ input: e.target.value })} + onKeyDown={(e) => { + if (e?.key === "Enter") { + e.preventDefault(); + this.close(0); + } + }} + /> } +
+ { + this.state.options.buttons.map((button, i) => { + return ; + }) + } +
; } + + private close(i: number) { + alreadyClosed = true; + dialogElement?.close(); + resolveFn?.([i, this.state.input]); + this.props.onClose(); + } } diff --git a/src/ui/LSOEventSink.ts b/src/ui/LSOEventSink.ts index 7e9a9f245..355c39906 100644 --- a/src/ui/LSOEventSink.ts +++ b/src/ui/LSOEventSink.ts @@ -1,9 +1,16 @@ import { EventSink, EventSinkRef, ImageCacheRefMut, LayoutEditorRefMut, LayoutRefMut, LayoutStateRefMut, Run, RunRef, TimeSpan, TimeSpanRef, Timer, TimerPhase, TimingMethod } from "../livesplit-core"; import { WebEventSink } from "../livesplit-core/livesplit_core"; +import { assert } from "../util/OptionUtil"; import { showDialog } from "./Dialog"; export class LSOEventSink { private eventSink: EventSink; + // We don't want to the timer to be interacted with while we are in menus + // where the timer is not visible or otherwise meant to be interacted with, + // nor do we want it to to be interacted with while dialogs are open. + // Multiple of these conditions can be true at the same time, so we count + // them. + private locked = 0; constructor( private timer: Timer, @@ -27,7 +34,24 @@ export class LSOEventSink { return this.eventSink; } + public isLocked(): boolean { + return this.locked > 0; + } + + public lockInteraction() { + this.locked++; + } + + public unlockInteraction() { + this.locked--; + assert(this.locked >= 0, "The lock count should never be negative."); + } + public start(): void { + if (this.locked) { + return; + } + this.timer.start(); this.currentPhaseChanged(); @@ -36,6 +60,10 @@ export class LSOEventSink { } public split(): void { + if (this.locked) { + return; + } + this.timer.split(); this.currentPhaseChanged(); @@ -43,6 +71,10 @@ export class LSOEventSink { } public splitOrStart(): void { + if (this.locked) { + return; + } + this.timer.splitOrStart(); this.currentPhaseChanged(); @@ -51,6 +83,10 @@ export class LSOEventSink { } public async reset(): Promise { + if (this.locked) { + return; + } + let updateSplits = true; if (this.timer.currentAttemptHasNewBestTimes()) { const [result] = await showDialog({ @@ -72,6 +108,10 @@ export class LSOEventSink { } public undoSplit(): void { + if (this.locked) { + return; + } + this.timer.undoSplit(); this.currentPhaseChanged(); @@ -79,12 +119,20 @@ export class LSOEventSink { } public skipSplit(): void { + if (this.locked) { + return; + } + this.timer.skipSplit(); this.currentSplitChanged(); } public togglePauseOrStart(): void { + if (this.locked) { + return; + } + this.timer.togglePauseOrStart(); this.currentPhaseChanged(); @@ -93,39 +141,67 @@ export class LSOEventSink { } public pause(): void { + if (this.locked) { + return; + } + this.timer.pause(); this.currentPhaseChanged(); } public resume(): void { + if (this.locked) { + return; + } + this.timer.resume(); this.currentPhaseChanged(); } public undoAllPauses(): void { + if (this.locked) { + return; + } + this.timer.undoAllPauses(); this.currentPhaseChanged(); } public switchToPreviousComparison(): void { + if (this.locked) { + return; + } + this.timer.switchToPreviousComparison(); this.currentComparisonChanged(); } public switchToNextComparison(): void { + if (this.locked) { + return; + } + this.timer.switchToNextComparison(); this.currentComparisonChanged(); } public setCurrentComparison(comparison: string): void { + if (this.locked) { + return; + } + this.timer.setCurrentComparison(comparison); this.currentComparisonChanged(); } public toggleTimingMethod(): void { + if (this.locked) { + return; + } + this.timer.toggleTimingMethod(); this.currentTimingMethodChanged(); } @@ -136,6 +212,9 @@ export class LSOEventSink { } public setGameTimeInner(timeSpan: TimeSpanRef): void { + if (this.locked) { + return; + } this.timer.setGameTime(timeSpan); } @@ -154,22 +233,37 @@ export class LSOEventSink { } public pauseGameTime(): void { + if (this.locked) { + return; + } this.timer.pauseGameTime(); } public resumeGameTime(): void { + if (this.locked) { + return; + } this.timer.resumeGameTime(); } public setCustomVariable(name: string, value: string): void { + if (this.locked) { + return; + } this.timer.setCustomVariable(name, value); } public initializeGameTime(): void { + if (this.locked) { + return; + } this.timer.initializeGameTime(); } public setLoadingTimesInner(timeSpan: TimeSpanRef): void { + if (this.locked) { + return; + } this.timer.setLoadingTimes(timeSpan); } @@ -190,6 +284,10 @@ export class LSOEventSink { } public markAsUnmodified(): void { + if (this.locked) { + return; + } + this.timer.markAsUnmodified(); this.splitsModifiedChanged(); } diff --git a/src/ui/LiveSplit.tsx b/src/ui/LiveSplit.tsx index 7f6016aa1..8eec519e5 100644 --- a/src/ui/LiveSplit.tsx +++ b/src/ui/LiveSplit.tsx @@ -5,7 +5,7 @@ import { Timer, HotkeyConfig, LayoutState, LayoutStateJson, TimingMethod, TimerPhase, } from "../livesplit-core"; -import { convertFileToArrayBuffer, convertFileToString, exportFile, openFileAsString } from "../util/FileUtil"; +import { FILE_EXT_LAYOUTS, convertFileToArrayBuffer, convertFileToString, exportFile, openFileAsString } from "../util/FileUtil"; import { Option, assertNull, expect, maybeDisposeAndThen, panic } from "../util/OptionUtil"; import * as SplitsIO from "../util/SplitsIO"; import { LayoutEditor as LayoutEditorComponent } from "./LayoutEditor"; @@ -42,6 +42,16 @@ export enum MenuKind { About, } +function isMenuLocked(menuKind: MenuKind) { + switch (menuKind) { + case MenuKind.Timer: + case MenuKind.Layout: + return false; + default: + return true; + } +} + type Menu = { kind: MenuKind.Timer } | { kind: MenuKind.Splits } | @@ -410,7 +420,10 @@ export class LiveSplit extends React.Component { return <> {view} - + this.lockTimerInteraction()} + onClose={() => this.unlockTimerInteraction()} + /> { ); } + private changeMenu(menu: Menu) { + const wasLocked = isMenuLocked(this.state.menu.kind); + const isLocked = isMenuLocked(menu.kind); + + this.setState({ menu, sidebarOpen: false }); + + if (!wasLocked && isLocked) { + this.lockTimerInteraction(); + } else if (wasLocked && !isLocked) { + this.unlockTimerInteraction(); + } + } + public openTimerView() { - this.setState({ - menu: { kind: MenuKind.Timer }, - sidebarOpen: false, - }); - this.state.hotkeySystem.activate(); + this.changeMenu({ kind: MenuKind.Timer }); } public openSplitsView() { - this.setState({ - menu: { kind: MenuKind.Splits }, - sidebarOpen: false, - }); - this.state.hotkeySystem.deactivate(); + this.changeMenu({ kind: MenuKind.Splits }); } public openLayoutView() { - this.setState({ - menu: { kind: MenuKind.Layout }, - }); + this.changeMenu({ kind: MenuKind.Layout }); } public openAboutView() { - this.setState({ - menu: { kind: MenuKind.About }, - sidebarOpen: false, - }); - this.state.hotkeySystem.deactivate(); + this.changeMenu({ kind: MenuKind.About }); + } + + private lockTimerInteraction() { + if (!this.state.eventSink.isLocked()) { + // We need to schedule this to happen in the next micro task, + // because the hotkey system itself may be what triggered this + // function, so the hotkey system might still be in use, which would + // result in a deadlock acquiring the internal state of the hotkey + // system. + setTimeout(() => this.state.hotkeySystem.deactivate()); + } + this.state.eventSink.lockInteraction(); + } + + private unlockTimerInteraction() { + this.state.eventSink.unlockInteraction(); + if (!this.state.eventSink.isLocked()) { + // We need to schedule this to happen in the next micro task, + // because the hotkey system itself may be what triggered this + // function, so the hotkey system might still be in use, which would + // result in a deadlock acquiring the internal state of the hotkey + // system. + setTimeout(() => this.state.hotkeySystem.activate()); + } } public async importSplitsFromFile(file: File) { const splits = await convertFileToArrayBuffer(file); + if (splits instanceof Error) { + toast.error(`Failed to read the file: ${splits.message}`); + return; + } this.importSplitsFromArrayBuffer(splits); } @@ -500,10 +540,14 @@ export class LiveSplit extends React.Component { } public async importLayout() { - const maybeFile = await openFileAsString(); + const maybeFile = await openFileAsString(FILE_EXT_LAYOUTS); if (maybeFile === undefined) { return; } + if (maybeFile instanceof Error) { + toast.error(`Failed to read the file: ${maybeFile.message}`); + return; + } const [file] = maybeFile; try { this.importLayoutFromString(file); @@ -513,7 +557,12 @@ export class LiveSplit extends React.Component { } public async importLayoutFromFile(file: File) { - const [fileString] = await convertFileToString(file); + const maybeFile = await convertFileToString(file); + if (maybeFile instanceof Error) { + toast.error(`Failed to read the file: ${maybeFile.message}`); + return; + } + const [fileString] = maybeFile; this.importLayoutFromString(fileString); } @@ -532,10 +581,7 @@ export class LiveSplit extends React.Component { RunEditor.new(run), "The Run Editor should always be able to be opened.", ); - this.setState({ - menu: { kind: MenuKind.RunEditor, editor, splitsKey }, - sidebarOpen: false, - }); + this.changeMenu({ kind: MenuKind.RunEditor, editor, splitsKey }); } public closeRunEditor(save: boolean) { @@ -569,17 +615,12 @@ export class LiveSplit extends React.Component { } public openLayoutEditor() { - this.state.hotkeySystem.deactivate(); - const layout = this.state.layout.clone(); const editor = expect( LayoutEditor.new(layout), "The Layout Editor should always be able to be opened.", ); - this.setState({ - menu: { kind: MenuKind.LayoutEditor, editor }, - sidebarOpen: false, - }); + this.changeMenu({ kind: MenuKind.LayoutEditor, editor }); } public closeLayoutEditor(save: boolean) { @@ -597,13 +638,9 @@ export class LiveSplit extends React.Component { } public openSettingsEditor() { - this.state.hotkeySystem.deactivate(); - this.setState({ - menu: { - kind: MenuKind.SettingsEditor, - config: this.state.hotkeySystem.config(), - }, - sidebarOpen: false, + this.changeMenu({ + kind: MenuKind.SettingsEditor, + config: this.state.hotkeySystem.config(), }); } diff --git a/src/ui/RunEditor.tsx b/src/ui/RunEditor.tsx index ec3d4706d..dfade43ef 100644 --- a/src/ui/RunEditor.tsx +++ b/src/ui/RunEditor.tsx @@ -1,7 +1,7 @@ import * as React from "react"; import { ContextMenu, ContextMenuTrigger, MenuItem } from "react-contextmenu"; import * as LiveSplit from "../livesplit-core"; -import { openFileAsArrayBuffer } from "../util/FileUtil"; +import { FILE_EXT_IMAGES, FILE_EXT_SPLITS, openFileAsArrayBuffer } from "../util/FileUtil"; import { TextBox } from "./TextBox"; import { toast } from "react-toastify"; import { @@ -42,6 +42,7 @@ export interface State { attemptCountIsValid: boolean, rowState: RowState, tab: Tab, + abortController: AbortController, } interface Callbacks { @@ -106,6 +107,7 @@ export class RunEditor extends React.Component { splitTimeChanged: false, }, tab: state.timing_method === "RealTime" ? Tab.RealTime : Tab.GameTime, + abortController: new AbortController(), }; if (props.generalSettings.speedrunComIntegration) { @@ -261,6 +263,11 @@ export class RunEditor extends React.Component { ); } + private close(save: boolean) { + this.state.abortController.abort(); + this.props.callbacks.closeRunEditor(save); + } + private renderSidebarContent() { return (
@@ -269,13 +276,13 @@ export class RunEditor extends React.Component {