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

Remove wx variable types from config code. Rewrite Portable mode. #4005

Closed
wants to merge 36 commits into from

Conversation

weirdbeardgame
Copy link
Contributor

@weirdbeardgame weirdbeardgame commented Dec 15, 2020

This PR is a total rewrite of AppUserMode and the start of a rewrite to Config.h pcsx2Config.cpp AppConfig. This clears a whole chunk of needless wx variable type usage in favor of standard std / filesystem ones. This casts all types back to their wx counterparts when needed.

@weirdbeardgame weirdbeardgame changed the title Add ghc filesystem. Remove wx variable types from config code. Rewrite Portable mode. Merge after 3928 Remove wx variable types from config code. Rewrite Portable mode. Merge after 3928 Dec 15, 2020
@weirdbeardgame weirdbeardgame marked this pull request as draft December 15, 2020 03:17
@weirdbeardgame weirdbeardgame force-pushed the removeWX branch 6 times, most recently from e066506 to c42e101 Compare December 17, 2020 20:15
@weirdbeardgame weirdbeardgame changed the title Remove wx variable types from config code. Rewrite Portable mode. Merge after 3928 Remove wx variable types from config code. Rewrite Portable mode. Dec 18, 2020
Copy link
Member

@xTVaser xTVaser left a comment

Choose a reason for hiding this comment

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

Hopefully someone more familiar with unicode path handling can chime in, I think we want to use std::wstring more to avoid issues, but I'm not 100% sure. That's really the only thing that jumps out at me as a concern.

The lion share of these changes are just patching up the file-path related usages around the codebase which i would expect. These changes shallowly touch the following "no-longer-plugins" configuration/logging so we'll probably want to do some brief testing in the future around those:

  • PAD
  • CDVD
  • DEV9
  • SPU2

Given the re-write to portable mode, it would be nice if we could detect and flag portable mode earlier as discovered here - #3991 (comment)

bin/portable.yaml Outdated Show resolved Hide resolved
common/include/Utilities/Path.h Outdated Show resolved Hide resolved
common/include/Utilities/PathUtils.h Outdated Show resolved Hide resolved
common/include/Utilities/PathUtils.h Outdated Show resolved Hide resolved
common/include/Utilities/PathUtils.h Outdated Show resolved Hide resolved
pcsx2/gui/AppUserMode.cpp Outdated Show resolved Hide resolved
pcsx2/gui/Dialogs/FirstTimeWizard.cpp Outdated Show resolved Hide resolved
pcsx2/gui/GlobalCommands.cpp Outdated Show resolved Hide resolved
pcsx2/gui/Panels/MemoryCardListPanel.cpp Show resolved Hide resolved
pcsx2/gui/Panels/MemoryCardListPanel.cpp Outdated Show resolved Hide resolved
@tadanokojin
Copy link
Member

Hopefully someone more familiar with unicode path handling can chime in, I think we want to use std::wstring more to avoid issues, but I'm not 100% sure. That's really the only thing that jumps out at me as a concern.

You want to use std::string and convert to and from wide strings as Win32 api needs. std::string supports utf8 with some caveats. Do not use std::wstring outside of the win32 stuff that requires it. It is not a magic type which makes i18n work.

We have a file:
pcsx2/common/include/PluginCompatibility.h

It can convert utf8 (string) to utf16 (wstring). If needed you can add a function which does the opposite.
Dolphin is a good reference

@weirdbeardgame
Copy link
Contributor Author

Hopefully someone more familiar with unicode path handling can chime in, I think we want to use std::wstring more to avoid issues, but I'm not 100% sure. That's really the only thing that jumps out at me as a concern.

You want to use std::string and convert to and from wide strings as Win32 api needs. std::string supports utf8 with some caveats. Do not use std::wstring outside of the win32 stuff that requires it. It is not a magic type which makes i18n work.

We have a file:
pcsx2/common/include/PluginCompatibility.h

It can convert utf8 (string) to utf16 (wstring). If needed you can add a function which does the opposite.
Dolphin is a good reference

Yeah that's fine I can do that. It's mostly in lieu of converting to wxString since it's a wchar_t type or UTF16 using fs::path's wstring
was a convince to avoid bad unicode errors and casts

@weirdbeardgame weirdbeardgame force-pushed the removeWX branch 2 times, most recently from f1ca945 to c4d4d3d Compare December 29, 2020 04:39
@weirdbeardgame weirdbeardgame marked this pull request as ready for review December 29, 2020 04:42
@weirdbeardgame weirdbeardgame force-pushed the removeWX branch 11 times, most recently from 32a4876 to 0db1a79 Compare January 3, 2021 21:19
@weirdbeardgame weirdbeardgame force-pushed the removeWX branch 3 times, most recently from b121323 to 0db1a79 Compare January 5, 2021 22:27
weirdbeardgame and others added 26 commits September 17, 2021 22:18
…struct stream. Fixed yaml output stream handling, will reset portable.yaml if it gets into a bad state
…factor pass, focused on cleaning up string conversions
…removed duplicate code. Fix wxConfigFile singleton initialization
… Construct fullpath to bios with dolder and filename
Moved Portable file to pcsx2/Docs. This prevents it being modified when it's set to false. Ignore bin portable file in git. VS: Post-build event to copy portable.yaml file into /bin with escape-hatch

If a /bin/portable.build.yaml file is detected, it will use that instead of the pcsx2/Docs/portable.yaml file.

This is to maintain parity with the CMake solution which is to always overwrite the file while still allowing a decent method to bypass that behaviour.
…it defined and will conflict AppUserMode.cpp: Added error checking to fix codacity error. AppConfig.cpp: Fix a codacity error
…efrences to plugins

AppConfig: Fix CurrentIso Save
@lightningterror
Copy link
Contributor

Closing as it's not being worked on, when work resumes pr can be reopened.

@weirdbeardgame weirdbeardgame deleted the removeWX branch January 7, 2024 04:24
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.