Skip to content

Conversation

WarmUpTill
Copy link
Contributor

@WarmUpTill WarmUpTill commented Sep 3, 2025

Description

Fix obs_frontend_set_preview_enabled not working when used repeatedly.

Motivation and Context

An initial call to obs_frontend_set_preview_enabled to enable or disable the preview works as expected.
However, any subsequent calls to obs_frontend_set_preview_enabled to change the preview enable state would not work, because previewEnabled was not updated within OBSStudioAPI::obs_frontend_set_preview_enabled.
This PR sets previewEnabled to the appropriate value.

How Has This Been Tested?

Tested by toggling the preview in a plugin via obs_frontend_set_preview_enabled.
Enabling and disabling the preview via the UI also seems to work as expected still.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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.

@Warchamp7
Copy link
Member

I feel like this value should be set in EnablePreviewDisplay() itself to prevent these kinds of mistakes

@gxalpha
Copy link
Member

gxalpha commented Sep 3, 2025

I feel like this value should be set in EnablePreviewDisplay() itself to prevent these kinds of mistakes

Objection, EnablePreviewDisplay itself is the method that enables/disables the preview display on the technical side. It intentionally doesn't always match previewEnabled, which effectively is the state of the user setting. So in cases where OBS for example is minimized, previewEnable=true together with a disabled preview display would be valid.

That said the correct thing to do here is to call EnablePreview() or DisablePreview() which take care of the user setting as well as the display state, and would also match the hotkey behavior.

@WarmUpTill WarmUpTill force-pushed the fix-obs_frontend_set_preview_enabled branch from 5ce432e to 2d3f2f1 Compare September 4, 2025 15:08
@WarmUpTill
Copy link
Contributor Author

WarmUpTill commented Sep 4, 2025

Thanks for the feedback!

correct thing to do here is to call EnablePreview() or DisablePreview() which take care of the user setting as well as the display state, and would also match the hotkey behavior.

I have updated the PR accordingly.
Seems to behave as expected.

I am not sure if this is really needed any more after switching to EnablePreview and DisablePreview:

	if (main->previewEnabled == enable) {
		return;
	}

Let me know if you want me to remove it.

@gxalpha
Copy link
Member

gxalpha commented Sep 4, 2025

I am not sure if this is really needed any more after switching to EnablePreview and DisablePreview:

	if (main->previewEnabled == enable) {
		return;
	}

Let me know if you want me to remove it.

Yeah I agree this seems unnecessary there at first glance, feel free to remove it everything still works correctly without it. Otherwise, the PR looks good to me.

@WarmUpTill WarmUpTill force-pushed the fix-obs_frontend_set_preview_enabled branch from 2d3f2f1 to 8e80e02 Compare September 4, 2025 15:30
@WarmUpTill
Copy link
Contributor Author

Yeah I agree this seems unnecessary there at first glance, feel free to remove it everything still works correctly without it.

Done.
Still seems to work as expected with obs_frontend_set_preview_enabled as well as using the UI.

@WizardCM WizardCM added the Bug Fix Non-breaking change which fixes an issue label Sep 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants