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.

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

Jump to bottom

warn when WinePrefixesBasePath is empty #3948

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

warn when WinePrefixesBasePath is empty #3948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b1e1c7
TextInput: do not require value to display warning
pt8o Aug 12, 2024
17fb14e
warn on empty WinePrefixesBasePath
pt8o Aug 12, 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
1 change: 1 addition & 0 deletions public/locales/en/translation.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,7 @@
"default-install-path": "Default Installation Path",
"default-steam-path": "Default Steam path",
"defaultWinePrefix": "Set Folder for new Wine Prefixes",
"defaultWinePrefixEmpty": "Warning: empty path may cause installation problems.",
"disable_controller": "Disable Heroic navigation using controller",
"disable_logs": "Disable Logs",
"disablePlaytimeSync": "Disable playtime synchronization",
Expand Down
5 changes: 4 additions & 1 deletion src/frontend/components/UI/PathSelectionBox/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ interface Props {
label?: string
afterInput?: ReactNode
disabled?: boolean
warning?: ReactNode
}

const PathSelectionBox = ({
Expand All @@ -43,7 +44,8 @@ const PathSelectionBox = ({
htmlId,
label,
afterInput,
disabled = false
disabled = false,
warning
}: Props) => {
const { t } = useTranslation()
// We only send `onPathChange` updates when the user is done editing, so we
Expand Down Expand Up @@ -89,6 +91,7 @@ const PathSelectionBox = ({
htmlId={htmlId}
label={label}
afterInput={afterInput}
warning={warning}
/>
)
}
Expand Down
2 changes: 1 addition & 1 deletion src/frontend/components/UI/TextInputField/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const TextInputField = ({
{label && <label htmlFor={htmlId}>{label}</label>}
{inputIcon}
<input type="text" id={htmlId} value={value} {...inputProps} />
{value && warning}
{warning}
pt8o marked this conversation as resolved.
Show resolved Hide resolved
{afterInput}
</div>
)
Expand Down
1 change: 1 addition & 0 deletions src/frontend/components/UI/TextInputWithIconField/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ interface TextInputWithIconFieldProps {
disabled?: boolean
extraClass?: string
onBlur?: (event: FocusEvent<HTMLInputElement>) => void
warning?: ReactNode
}

const TextInputWithIconField = ({
Expand Down
49 changes: 46 additions & 3 deletions src/frontend/screens/Settings/components/WinePrefixesBasePath.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import React, { useContext } from 'react'
import React, { useContext, useMemo, useState } from 'react'
import { useTranslation } from 'react-i18next'
import ContextProvider from 'frontend/state/ContextProvider'
import useSetting from 'frontend/hooks/useSetting'
import { PathSelectionBox } from 'frontend/components/UI'
import { PathSelectionBox, SvgButton } from 'frontend/components/UI'
import SettingsContext from 'frontend/screens/Settings/SettingsContext'
import { Undo } from '@mui/icons-material'
import { Box } from '@mui/material'

const WinePrefixesBasePath = () => {
const { t } = useTranslation()
Expand All @@ -19,20 +21,61 @@ const WinePrefixesBasePath = () => {
'defaultWinePrefix',
''
)
const [lastValidPrefix, setLastValidPrefix] = useState(defaultWinePrefix)

function handlePathChange(val: string) {
setDefaultWinePrefix(val)
if (val) {
setLastValidPrefix(val)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure whether a simple truthiness check makes sense here. Sure, if the user highlights the entire path and then deletes it, this'll work. More likely however (especially considering virtual keyboards, e.g. on the Deck), they're gonna backspace one-by-one, causing a restore operation to just re-insert the /. This of course won't work as a prefix path either

The easiest way to resolve this would be to not store the "last valid" value at all, instead just restoring it to the same path a new install would use:

HeroicGamesLauncher/src/frontend/screens/Library/components/InstallModal/WineSelector/index.tsx

Lines 70 to 73 in 47d3075

const dirName =
removeSpecialcharacters(title) || removeSpecialcharacters(appName)
const suggestedWinePrefix = `${prefixFolder}/${dirName}`
setWinePrefix(suggestedWinePrefix)

Copy link
Author

Choose a reason for hiding this comment

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

Sorry if this is a silly question but I don't see where to find the original defaultWinePrefix? In the code you linked the suggestedWinePrefix is built partly the title or appName from WineSelector's props, which we don't have in the component we're working on... Is there something like a constants file that the initial value comes from?

}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of storing lastValidPrefix (which the name can be a bit misleading, it doesn't mean this is valid) I imagine this logic for handlePathChange:

function handlePathChange(newPath: string) {
  if (newPath) {
    // not empty, all good
    setDefaultWinePrefix(newPath)
  }  else {
    // empty, show an alert/warning saying it will be reverted to the previous value
    // reset the value to the original config value
  }
}

But I actually think we have a problem with the PathSelectionBox component because we have no simple way of resetting the value (we would have to re-render the component cause it uses the internal tmpPath state).

I imagine the onPathChange function that we pass with the props could be updated to return a string (instead of void) that the component should set the field to, and instead of simply calling that function in the onBlur callback, we could wrap it to call it and use the return value to update with setTmpPath(theReturnValueOfOnPathChange).

function handleRevert() {
setDefaultWinePrefix(lastValidPrefix)
}

const warning = useMemo(() => {
if (defaultWinePrefix !== '') return undefined
return (
<Box
sx={{
display: 'flex',
alignItems: 'center',
paddingBlockStart: 'var(--space-sm)',
gap: 1,
color: 'var(--status-warning)'
}}
>
{t(
'setting.defaultWinePrefixEmpty',
'Warning: empty path may cause installation problems.'
)}
{lastValidPrefix && (
<SvgButton
className="button button-icon-flex is-danger"
onClick={handleRevert}
>
<Undo />
</SvgButton>
)}
</Box>
)
}, [])

return (
<PathSelectionBox
htmlId="selectDefaultWinePrefix"
label={t('setting.defaultWinePrefix', 'Set Folder for new Wine Prefixes')}
path={defaultWinePrefix}
onPathChange={setDefaultWinePrefix}
onPathChange={handlePathChange}
type="directory"
pathDialogTitle={t(
'toolbox.settings.wineprefix',
'Select a Folder for new Wine Prefixes'
)}
noDeleteButton
pathDialogDefaultPath={defaultWinePrefix}
warning={warning}
/>
)
}
Expand Down
Loading