Skip to content

Conversation

@Trouffman
Copy link
Collaborator

@Trouffman Trouffman commented May 9, 2025

Built on the great work by @kaechele :

This properly splits settings for this plugin into the new split configuration stores for app and user settings.

All user-editable settings go into the user store, all functional, non-user facing settings go in app store (e.g. timestamp of last update check).

On OBS versions prior to 31 there is no change in behaviour and all settings continue to be read from and stored to the global store only.

Fixes #1121

This was originally dismissed as the backward support was under discussion. With DistroAV 6.1: OBS 31 is required. So that backward support is no longer required.

As to grant @kaechele the praise for this great piece (especially the migration settings) this is a revival of the PR #1152 that was originally closed.

Things to address:

  • Remove complexity of backward compatibility with pre-OBS 31
  • Update / Add a few missing config entries
PARAM_SKIP_UPDATE_VERSION
Can be set & influenced by the user, the decision is to make it a "User" config

The other update elements are not influenced / presented to the User and will be in the App Config:
PARAM_LAST_UPDATE_CHECK
PARAM_MIN_AUTO_UPDATE_CHECK_INTERVAL_SECONDS
PARAM_AUTO_CHECK_FOR_UPDATES

Note : this PR was re-created with a branch that can be modified.

kaechele and others added 9 commits November 20, 2024 12:09
This properly splits settings for this plugin into the new split
configuration stores for app and user settings.

All user-editable settings go into the user store, all functional,
non-user facing settings go in app store (e.g. timestamp of last update
check).

On OBS versions prior to 31 there is no change in behaviour and all
settings continue to be read from and stored to the global store only.

Signed-off-by: Felix Kaechele <felix@kaechele.ca>
Signed-off-by: Felix Kaechele <felix@kaechele.ca>
@Trouffman Trouffman self-assigned this May 9, 2025
@Trouffman Trouffman added the obs label May 9, 2025
@Trouffman Trouffman added this to the 6.1.0 milestone May 9, 2025
ProcessCommandLine();
SetDefaultsToGlobalStore();
SetDefaultsToUserStore();
if (obs_get_version() >= MAKE_SEMANTIC_VERSION(31, 0, 0))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe add a check/config entry when that migration is done so it does not trigger at every plugin launch

@Trouffman Trouffman marked this pull request as ready for review May 9, 2025 02:26
@Trouffman Trouffman requested review from BitRate27 and Copilot May 9, 2025 02:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes backward compatibility for pre-OBS 31 setups by splitting configuration settings between the user and app stores and updating default values accordingly. Key changes include:

  • Changing all relevant configuration references from the global store to the appropriate app or user store.
  • Removing deprecated functions for backward compatibility and simplifying config accessor functions.
  • Introducing configuration migration logic and updating default parameters and names.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

File Description
src/obs-support/shared-update.cpp Switched InstallGUID retrieval/set from global to app config.
src/obs-support/obs-app.hpp Simplified and streamlined config accessor functions.
src/config.h Updated default values and added new migration function declarations.
src/config.cpp Revised config store usage, added migration logic, and updated parameter handling.

MigrateSetting(app_config, user_config, SECTION_NAME, PARAM_TALLY_PROGRAM_ENABLED);
MigrateSetting(app_config, user_config, SECTION_NAME, PARAM_TALLY_PREVIEW_ENABLED);

MigrateSetting(app_config, user_config, SECTION_NAME, PARAM_SKIP_UPDATE_VERSION);
Copy link

Copilot AI May 9, 2025

Choose a reason for hiding this comment

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

PARAM_SKIP_UPDATE_VERSION is not included in ConfigTypeMap, causing the migration for this setting to be silently skipped. Consider adding an appropriate mapping for it in ConfigTypeMap so that its migration is executed as intended.

Copilot uses AI. Check for mistakes.
@Trouffman Trouffman added enhancement critical Seeking Testers PRs with this label will package the plugin so that others can test Dependencies Relate to a dependency (package / version) labels May 9, 2025
@Trouffman Trouffman requested a review from paulpv May 9, 2025 15:21
@BitRate27 BitRate27 merged commit 22f2082 into master May 9, 2025
6 checks passed
@BitRate27 BitRate27 deleted the obs-31-config-split branch May 9, 2025 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

critical Dependencies Relate to a dependency (package / version) enhancement obs Seeking Testers PRs with this label will package the plugin so that others can test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: OBS log reports obs_frontend_get_global_config is deprecated.

4 participants