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

warn when WinePrefixesBasePath is empty #3948

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

Conversation

pt8o
Copy link

@pt8o pt8o commented Aug 12, 2024

Resolves #2556

Screenshot from 2024-08-12 18-22-53

Since all of these settings are saved on edit (rather than waiting for the user to hit a "Save" button or something) I didn't think there was a great way UX-wise to prevent the user from saving an empty folder. Instead I opted to display a warning below the input, and a button to revert to the most recent non-empty path. But the user can still ignore it and save the empty path.

If the user has already saved an empty path and then opens Settings, then there is no recent non-empty path, so the warning still appears but the undo button doesnt.


Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@flavioislima
Copy link
Member

flavioislima commented Aug 12, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@pt8o pt8o changed the title warn whem WinePrefixesBasePath is empty warn when WinePrefixesBasePath is empty Aug 12, 2024
@pt8o
Copy link
Author

pt8o commented Aug 13, 2024

I have read the CLA Document and I hereby sign the CLA

@CommandMC CommandMC added the pr:ready-for-review Feature-complete, ready for the grind! :P label Aug 13, 2024

function handlePathChange(val: string) {
setDefaultWinePrefix(val)
if (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:

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?

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.

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).

@pt8o pt8o force-pushed the lh/warn-empty-wine-prefix-folder branch from 2c932f2 to 17fb14e Compare August 24, 2024 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default wine prefixes folder can be left empty
4 participants