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

UI: Implement backend work for configurable storage locations for settings, profiles, and scene collections #11055

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

PatTheMav
Copy link
Member

Description

This PR implements backend changes required to make the storage locations of settings, profiles, and scene collections configurable and decoupled from OBS' current default storage location for its settings, log files, crash logs, and other generated files.

This is achieved in 3 major parts:

  1. The configuration is split up into an "app" configuration (mainly for information that pertains to the work of the app itself, e.g. actual location of user configuration, last time the app has checked for updates, etc.) and a "user" configuration (mainly for settings that are exposed to the user and can be changed according to their preference).
  2. Profile storage location is decoupled from any default path for settings and as such all code that uses the settings' path to access profile information has been changed
  3. Scene collection storage location is decoupled from any default path for settings, like for profiles, code that uses the settings' path to access them has been changed

The implementation also introduces additional changes:

  • Changes rely on C++17 functionality, such as availability of std::string_view, std::filesystem and others.
  • Reliance of using namespace std was avoided as much as possible as it's a bad design pattern, when functionality from std was used, it's made explicit
  • Use of pointers was avoided in favour of either std::optional (where it's use of copies were deemed good enough) or reference types to keep using value semantics as much as possible
  • Some names were changed to make their function more explicit, e.g. basicConfig was renamed to activeConfiguration because it represents the currently active configuration object in use by the application (it's a state-dependent variable).
  • In some cases (e.g. deletion of profiles or scene collections) copies are used on purpose as the underlying references are removed from the owning collection container
  • Exception handling is favoured over returning single error values or checking for validity of modified variables as favoured by modern C++ practices.

Architecturally an attempt was made to introduce some encapsulation to the management of profiles and scene collections. Available scene collections or profiles are discovered on disk and cached in memory (with refreshes from disk happening only when explicitly requested).

Handling of encoder configuration (which is stored in separate JSON files next to the profile settings file) has not been changed, but should be moved out of the encoder configuration and into the profile management, but was out of scope for this PR.

Motivation and Context

The long-term goal of this work is to enable placing configuration files as well as profiles and scene collections in arbitrary locations provided by the user, such as:

  • A network drive
  • A vendor-specific directory that is automatically synced with cloud services (e.g. iCloud Drive)
  • (Future) Non-file based storage (e.g. cloud storage APIs)

How Has This Been Tested?

Tested on macOS 14 with the following scenarios:

  • Launched OBS with existing configuration, settings were migrated to user.ini
  • Launched OBS with missing scene collection, new collection was created in-place
  • Launched OBS with missing profile, profile was created in-place
  • Created/duplicated/removed/renamed profiles with and without "invalid" characters (i.e. emoji)
  • Created/duplicated/removed/renamed scene collections with and without "invalid" characters
  • Checked that UUIDs of sources in duplicated scene collections were correctly changed
  • Checked that name of renamed/duplicated scene collections was correctly changed
  • Checked that currently active profiles or scene collections were saved before any change took place

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@PatTheMav PatTheMav added the Seeking Testers Build artifacts on CI label Jul 31, 2024
@PatTheMav
Copy link
Member Author

Will require obsproject/obs-websocket#1241 to be merged first to unbreak Linux compilation.

@PatTheMav PatTheMav force-pushed the relocatable-settings branch 4 times, most recently from 4e3b777 to c6353b8 Compare August 2, 2024 13:59
@WizardCM WizardCM added the New Feature New feature or plugin label Aug 3, 2024
@PatTheMav PatTheMav force-pushed the relocatable-settings branch 4 times, most recently from 67c4598 to c582855 Compare August 14, 2024 17:01
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Some quick general comments/questions.

UI/obs-app-theming.cpp Show resolved Hide resolved
UI/window-basic-main.hpp Outdated Show resolved Hide resolved
UI/obs-app.hpp Outdated Show resolved Hide resolved
Copy link
Member

@derrod derrod left a comment

Choose a reason for hiding this comment

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

There are a number of naïve conversion from std::string to std::filesystem::path, those should all use std::filesystem::u8path to ensure proper conversion from UTF-8. Otherwise a native encoding is assumed that - mainly on Windows - may result in invalid paths if non-ASCII characters are used.

Also see #9183 and obsproject/obs-websocket#1231

@PatTheMav
Copy link
Member Author

PatTheMav commented Sep 1, 2024

There are a number of naïve conversion from std::string to std::filesystem::path, those should all use std::filesystem::u8path to ensure proper conversion from UTF-8. Otherwise a native encoding is assumed that - mainly on Windows - may result in invalid paths if non-ASCII characters are used.

Also see #9183 and obsproject/obs-websocket#1231

Good point, if the std::string storage is UTF-8-based, the value needs to be converted to the correct encoding of the path value_type.

Unfortunately u8path has been deprecated in C++20 already and the approach suggested by the author of the deprecation proposals suggests the following:

std::string utf8_encoded_filename = ...;
auto char8_view =
    std::ranges::views::transform(utf8_encoded_filename,
                                  [](char c) { return (char8_t)c; });
std::filesystem::path p(char8_view.begin(), char8_view.end());

The C++ STL discourages use of UTF-8 encoded text when using char types and as such the language wants us to jump through these hoops if we do.

@derrod
Copy link
Member

derrod commented Sep 1, 2024

Good point, if the std::string storage is UTF-8-based, the value needs to be converted to the correct encoding of the path value_type.

Unfortunately u8path has been deprecated in C++20 already and the approach suggested by the author of the deprecation proposals suggests the following:

The C++ STL discourages use of UTF-8 encoded text when using char types and as such the language wants us to jump through these hoops if we do.

We disable char8_t as of #9194 because it's half-assed and the C++ standards committee should feel bad about introducing it. It's not supported in various STL functions and features (including std::format/std::cin/std::cout), there's no conversion features provided, and it doesn't solve anything while also breaking backwards compatibility, e.g. by changing the u8 prefix to be char8_t instead of char. And of course, many third-party libraries such as nlohmann_json simply do not support it (or u8strings) because nobody actually uses it in their codebases.

@PatTheMav
Copy link
Member Author

Pushed an update with the following changes:

  • Functions NewProfile, DuplicateProfile and their variants for scene collections have been renamed to CreateNewProfile and CreateDuplicateProfile respectively
  • File name composition and handling has been revised:
    • If a std::filesystem::path exists already, it is used directly
    • If a std::filesystem::path needs to be composed of two existing paths, they are both used directly
    • If a std::filesystem::path needs to be created of a std::string or char *, the u8path constructor is used
    • If a std::filesystem::path needs to be composed of an existing path and a std::string or char *, the latter are first converted to a path via u8path before being used in the composition
    • If a std::string or char * is required, the existing std::filesystem::path is converted into a UTF-8 character array via u8string and then either its c_str() value is used or it's composed into a new string with other tokens directly (it's faster to convert just the path back to UTF-8 than to convert all string tokens first from UTF-8 to the local file system encoding and then appending all parts)

This probably makes the code less efficient because of more temporary objects being created and (mostly on Windows) more character conversion taking place, but in the greater context of file system operations those losses are probably negligible.

@PatTheMav PatTheMav force-pushed the relocatable-settings branch 3 times, most recently from 5946ec4 to c471700 Compare September 5, 2024 15:19
@RytoEX
Copy link
Member

RytoEX commented Sep 5, 2024

Building this on Windows 11 and running with a fresh installation in Portable Mode results in a crash on startup:
image

 	KernelBase.dll!00007ffd125bfabc()	Unknown
 	vcruntime140d.dll!00007ffc2d8c9362()	Unknown
>	obs64.exe!std::filesystem::_Throw_fs_error(const char * _Op, const std::error_code & _Error, const std::filesystem::path & _Path1, const std::filesystem::path & _Path2) Line 1875	C++
 	obs64.exe!std::filesystem::copy(const std::filesystem::path & _From, const std::filesystem::path & _To, const std::filesystem::copy_options _Options) Line 4431	C++
 	obs64.exe!std::filesystem::copy(const std::filesystem::path & _From, const std::filesystem::path & _To) Line 4440	C++
 	obs64.exe!OBSApp::MigrateGlobalSettings() Line 885	C++
 	obs64.exe!OBSApp::InitGlobalConfig() Line 712	C++
 	obs64.exe!OBSApp::AppInit() Line 1272	C++
 	obs64.exe!run_program(std::basic_fstream<char,std::char_traits<char>> & logFile, int argc, char * * argv) Line 2107	C++
 	obs64.exe!main(int argc, char * * argv) Line 2854	C++
 	obs64.exe!qtEntryPoint() Line 50	C++
 	obs64.exe!WinMain(HINSTANCE__ * __formal, HINSTANCE__ * __formal, char * __formal, int __formal) Line 60	C++

Even copying in some test profiles and scene collections resulted in the same crash on startup.

@PatTheMav
Copy link
Member Author

Building this on Windows 11 and running with a fresh installation in Portable Mode results in a crash on startup: image

 	KernelBase.dll!00007ffd125bfabc()	Unknown
 	vcruntime140d.dll!00007ffc2d8c9362()	Unknown
>	obs64.exe!std::filesystem::_Throw_fs_error(const char * _Op, const std::error_code & _Error, const std::filesystem::path & _Path1, const std::filesystem::path & _Path2) Line 1875	C++
 	obs64.exe!std::filesystem::copy(const std::filesystem::path & _From, const std::filesystem::path & _To, const std::filesystem::copy_options _Options) Line 4431	C++
 	obs64.exe!std::filesystem::copy(const std::filesystem::path & _From, const std::filesystem::path & _To) Line 4440	C++
 	obs64.exe!OBSApp::MigrateGlobalSettings() Line 885	C++
 	obs64.exe!OBSApp::InitGlobalConfig() Line 712	C++
 	obs64.exe!OBSApp::AppInit() Line 1272	C++
 	obs64.exe!run_program(std::basic_fstream<char,std::char_traits<char>> & logFile, int argc, char * * argv) Line 2107	C++
 	obs64.exe!main(int argc, char * * argv) Line 2854	C++
 	obs64.exe!qtEntryPoint() Line 50	C++
 	obs64.exe!WinMain(HINSTANCE__ * __formal, HINSTANCE__ * __formal, char * __formal, int __formal) Line 60	C++

Even copying in some test profiles and scene collections resulted in the same crash on startup.

In obs-app.cpp on line 872, change resize into reserve - does that fix the crash?

@RytoEX
Copy link
Member

RytoEX commented Sep 5, 2024

Building this on Windows 11 and running with a fresh installation in Portable Mode results in a crash on startup:
Even copying in some test profiles and scene collections resulted in the same crash on startup.

In obs-app.cpp on line 872, change resize into reserve - does that fix the crash?

Yes, that fixes the crash.

@RytoEX
Copy link
Member

RytoEX commented Sep 5, 2024

obs-websocket has been updated, so the temporary commit here can be dropped.

UI/obs-app.cpp Outdated Show resolved Hide resolved
UI/obs-app.cpp Outdated Show resolved Hide resolved
@RytoEX
Copy link
Member

RytoEX commented Sep 6, 2024

Review comments notwithstanding, this seemed fine in my local testing.

Copy link
Member

@Warchamp7 Warchamp7 left a comment

Choose a reason for hiding this comment

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

Approval from my side pending code review from others.

@RytoEX
Copy link
Member

RytoEX commented Sep 12, 2024

It looks like the resize->reserve change got reverted and this again crashes on Windows.

This introduces a split of the current single ConfigFile instance for
all configuration into two separate instances.

The app config instance contains system-wide settings that mainly
concern the working of the app itself, whereas the user config instance
contains settings actually exposed to the user for the configuration.
This change enables loading profiles from locations different than
OBS' own configuration directory.

It also rewrites profile management in the app to work off an in-memory
collection of profiles found on disk and does not require iterating
over directory contents for most profile interactions by the app.
This change enables loading scene collections from locations different
than OBS' own configuration directory.

It also rewrites profile management in the app to work off an in-memory
collection of profiles found on disk and does not require iterating
over directory contents for most profile interactions by the app.
@PatTheMav
Copy link
Member Author

It looks like the resize->reserve change got reverted and this again crashes on Windows.

Pushed an update to fix that.

@RytoEX
Copy link
Member

RytoEX commented Sep 12, 2024

It looks like the resize->reserve change got reverted and this again crashes on Windows.

Pushed an update to fix that.

That fixed the crash again.

Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Seems okay.

@RytoEX RytoEX added this to the OBS Studio 31 milestone Sep 12, 2024
@RytoEX RytoEX merged commit 3e0592d into obsproject:master Sep 12, 2024
14 checks passed
@PatTheMav PatTheMav deleted the relocatable-settings branch September 12, 2024 21:58
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Feature New feature or plugin Seeking Testers Build artifacts on CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants