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

BugFix: Error popup on insufficient permissions during memcard conversion #11825

Merged
merged 1 commit into from
Sep 22, 2024

Conversation

thethunderturner
Copy link
Contributor

@thethunderturner thethunderturner commented Sep 17, 2024

Description of Changes

Added a permissions check during the conversion of a memory card. It occurred to me that if somehow the permissions change during the setup of the conversion, then it will finish successfully, but wont be copied over to the new directory.

Furthermore, I also made a slight modification to the build-dependencies.sh, as I had an error when using curl.
More specifically: curl: (33) HTTP server does not seem to support byte ranges. Cannot resume.

Rationale behind Changes

Its just a bug fix. Helps user understand what went wrong.

Suggested Testing Steps

  • Have permissions to the memcard directory
  • Select a memcard for conversion
  • When the popup is displayed, change permissions of the directory to nobody (sudo chmod 000 ./memory_save)
  • Start conversion

This time you should get an error popup.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for submitting a contribution to PCSX2

As this is your first pull request, please be aware of the contributing guidelines.

Additionally, as per recent changes in GitHub Actions, your pull request will need to be approved by a maintainer before GitHub Actions can run against it. You can find more information about this change here.

Please be patient until this happens. In the meantime if you'd like to confirm the builds are passing, you have the option of opening a PR on your own fork, just make sure your fork's master branch is up to date!

@thethunderturner thethunderturner changed the title Feature/perms error BugFix: Receive error popup on insufficient permissions during memcard conversion Sep 17, 2024
@thethunderturner thethunderturner changed the title BugFix: Receive error popup on insufficient permissions during memcard conversion BugFix: Error popup on insufficient permissions during memcard conversion Sep 17, 2024
Copy link
Member

@F0bes F0bes left a comment

Choose a reason for hiding this comment

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

Thanks for doing the Windows side too.
You have a bunch of lines with spaces where tabs should be.
Could you also mark variables such as canRead or fileAttributes as const?

pcsx2-qt/Settings/MemoryCardConvertDialog.cpp Outdated Show resolved Hide resolved
pcsx2-qt/Settings/MemoryCardConvertDialog.cpp Outdated Show resolved Hide resolved
pcsx2-qt/Settings/MemoryCardConvertDialog.cpp Outdated Show resolved Hide resolved
pcsx2-qt/Settings/MemoryCardConvertDialog.cpp Outdated Show resolved Hide resolved
pcsx2-qt/Settings/MemoryCardConvertDialog.cpp Outdated Show resolved Hide resolved
pcsx2-qt/Settings/MemoryCardConvertDialog.cpp Outdated Show resolved Hide resolved
pcsx2-qt/Settings/MemoryCardConvertDialog.cpp Outdated Show resolved Hide resolved
pcsx2-qt/Settings/MemoryCardConvertDialog.cpp Outdated Show resolved Hide resolved
pcsx2-qt/Settings/MemoryCardConvertDialog.cpp Outdated Show resolved Hide resolved
pcsx2-qt/Settings/MemoryCardConvertDialog.cpp Outdated Show resolved Hide resolved
pcsx2-qt/Settings/MemoryCardConvertDialog.cpp Outdated Show resolved Hide resolved
@F0bes
Copy link
Member

F0bes commented Sep 21, 2024

Would you be able to squash the commits into two and prefix the resulting commit?

Using git CLI you can interactive rebase all of your commits and then force push the result back to this branch..

@thethunderturner
Copy link
Contributor Author

Would you be able to squash the commits into two and prefix the resulting commit?

Using git CLI you can interactive rebase all of your commits and then force push the result back to this branch..

Hey, could you check if everything is ok now? Lemme know if I missed something.

@thethunderturner
Copy link
Contributor Author

Hmm, I will check why its failing

@F0bes F0bes force-pushed the feature/perms-error branch from fd781a6 to 37ac41f Compare September 22, 2024 15:10
@F0bes
Copy link
Member

F0bes commented Sep 22, 2024

I've already taken care of it for you. I've just asked some people to test the Windows portion and it will be good to merge.

@thethunderturner
Copy link
Contributor Author

I've already taken care of it for you. I've just asked some people to test the Windows portion and it will be good to merge.

It still failed. I just tested it locally with the new commit, and everythign should work.
Apologies

@F0bes F0bes force-pushed the feature/perms-error branch 2 times, most recently from c70b3f3 to 064678d Compare September 22, 2024 15:44
@thethunderturner
Copy link
Contributor Author

Why is the #ifdef at 0 indentation? 🤔

@F0bes
Copy link
Member

F0bes commented Sep 22, 2024

If you want to use an LLM to help you program, that's fine, it's another tool that can sometimes be valuable. Please don't blindly copy and paste from it though. I was suspicious before, but now I'm 90% confident. The pointless commenting, the completely busted formatting (that I just fixed for you). With all respect I don't want to review a machines code.

Why is the #ifdef at 0 indentation? 🤔

It's configured that way in our clang-format.

@F0bes F0bes force-pushed the feature/perms-error branch from 064678d to 5592143 Compare September 22, 2024 15:51
@F0bes
Copy link
Member

F0bes commented Sep 22, 2024

I'll switch to windows and figure this out..

@F0bes F0bes force-pushed the feature/perms-error branch 2 times, most recently from 1d4beb6 to 1d973a8 Compare September 22, 2024 17:15
@F0bes
Copy link
Member

F0bes commented Sep 22, 2024

There, tested on Windows and Linux and it works.

@F0bes F0bes force-pushed the feature/perms-error branch from 1d973a8 to 44aaa7e Compare September 22, 2024 17:46
@F0bes F0bes merged commit ac4d827 into PCSX2:master Sep 22, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants