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

feat: handle unzip errors #249

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

Conversation

Jan200101
Copy link
Contributor

fixes #238
fixes #240

handles unzip failures gracefully with visual feedback in the UI

image

@0neGal
Copy link
Owner

0neGal commented Aug 4, 2024

As it stands, I'm not sure I'd agree that this fixes those issues, but were you to add something like "this may happens due to file permissions" or similar, it'd be a different story. But even so, those issues should be caught much earlier than the unzipping process imo

@0neGal 0neGal added the enhancement New feature or request label Aug 4, 2024
@Jan200101
Copy link
Contributor Author

Jan200101 commented Aug 4, 2024

sure, but this should be done regardless.
this is just a blanket solution that gracefully deals with all errors during unzipping, actually checking if we have the permission to put files in place could be done ahead of time though we should look to avoid what FlightCore does and not check for a hardcoded path

@0neGal
Copy link
Owner

0neGal commented Aug 4, 2024

sure, but this should be done regardless.
this is just a blanket solution that gracefully deals with all errors during unzipping

Totally agree

actually checking if we have the permission to put files in place could be done ahead of time though we should look to avoid what FlightCore does and not check for a hardcoded path

Also agree, however keep in mind, there was already some attempt to make this be a thing, and it did work in my testing on Linux, as mentioned in those issues, I've however not been able to get around to if it's just on Windows it breaks, and why...

@Jan200101
Copy link
Contributor Author

Also agree, however keep in mind, there was already some attempt to make this be a thing, and it did work in my testing on Linux, as mentioned in those issues, I've however not been able to get around to if it's just on Windows it breaks, and why...

Oh yeah thats kinda expected.
fs.accessSync doesn't really work with directories on windows, libuv does some minor checks and then pretends everything is a-okay since access isn't controlled by metadata but by a lot more complicated stuff that cannot be sanely put into the RWX format.

what could be added after the accessSync is an attempt to create a file and delete it.

@0neGal
Copy link
Owner

0neGal commented Aug 4, 2024

I figured Windows did some dumb stuff underneath or handled something different oh well, any fix is appreciated really... Whether it's recursively checking the files inside, or creating a file etc

@Jan200101
Copy link
Contributor Author

permission check for Windows was added.
I didn't make it check specifically for windows since this is such a minor thing.
image

@Jan200101
Copy link
Contributor Author

the last thing I want to propose here is making gamepath-lost-perms create a toast message that would call setpath on click.

Currently it creates an alert box and opens the setpath dialog on loop until you select a path with permissions or until the user terminates the app because gamepath.setting keeps being set to false when the file dialog is closed.

@0neGal
Copy link
Owner

0neGal commented Aug 5, 2024

The original intention was that the dialog would show, and it'd disable the buttons beside the gamepath related ones, letting you mount the drive or fix permissions etc without having to set the gamepath again. Just to clarify, are you saying the permission alert continuously show keeps popping up? Because that would be unintentional

@Jan200101
Copy link
Contributor Author

Just to clarify, are you saying the permission alert continuously show keeps popping up? Because that would be unintentional

yes, that is what is happening.

The loop goes like this:
setInterval(gamepath.js:L193) emits gamepath-lost-perms
gamepath-lost-perms emits setpath if gamepath.setting is false and sets it to true
setpath calls gamepath.set
gamepath.set opens the file dialog via dialog.showOpenDialog and sets gamepath.setting to false in its callback handler.
because gamepath.setting is false gamepath-lost-perms can start the dialog again.

@0neGal
Copy link
Owner

0neGal commented Aug 5, 2024

I see, I suppose you could do:

ipcMain.on("gamepath-lost-perms", async (e, selected_gamepath) => {
	if (! gamepath.setting && ! gamepath.lost_perms == selected_gamepath) {
		gamepath.lost_perms = selected_gamepath;

		// remaining code ...
	}
})

And then clear gamepath.lost_perms whenever a gamepath is set or the permissions have been restored.

src/modules/gamepath.js Outdated Show resolved Hide resolved
src/modules/gamepath.js Outdated Show resolved Hide resolved
@Jan200101
Copy link
Contributor Author

Anything still needed for this to be merged?

  • general unzip failures are handles and propagated up with the ability to print specific messages for certain failures
    • a message for permission denied was added, though it may be best to omit that for now since the existing permission check should handle this. a follow-up PR could add it and other checks in.
  • permissions check was updated to check for permissions and try a filesystem operation for maximum coverage
  • toast over IPC was fixed
  • race condition for permission check was fixed

@0neGal
Copy link
Owner

0neGal commented Aug 10, 2024

Besides localization, this should be able to be merged.

@Jan200101
Copy link
Contributor Author

Jan200101 commented Aug 10, 2024

reverted unneeded localization changes, added german translation.
Poked Alystrasz on Discord for a French translation, would need to ask @XNovaDelta for the spanish one.
I don't know if KenMizz is still away, so I'll try finding someone for the chinese translation again. EDIT: found one

@Jan200101
Copy link
Contributor Author

was asked to ping here so they are aware when they are back @Alystrasz

src/lang/fr.json Outdated Show resolved Hide resolved
Co-authored-by: Rémy Raes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: eperm: Operation not permitted bug: Error when Titanfall2 is installed on default EA App path.
3 participants