QtHost: fix bigpicture mode not starting correctly when fullscreen mode is on

[aldee]

Description of Changes

This fixes the issue described here: #13228, where when users enable both Start Fullscreen and Start Big Picture UI options, PCSX2 app will stay in windowed mode while showing the Big Picture UI, whereas it should be showing the Big Picture UI in Fullscreen mode.

Rationale behind Changes

The changes are rather simple. The issue was caused by the variable s_start_fullscreen_ui_fullscreen currently always resolves to false when the app is not ran with command line argument -fullscreen, so it will always start big picture UI in windowed mode, regardless whether the fullscreen option in Settings is enabled. So we need to fetch the StartFullscreen flag to make sure that the correct setting is passed to big picture UI logic.

Suggested Testing Steps

Just need to run the app, test with following scenarios:

1. Start Fullscreen on & Start Big Picture UI on -> should start big picture UI in fullscreen mode (currently in windowed mode)
2. Start Fullscreen on & Start Big Picture UI off -> should start desktop UI in fullscreen mode (currently working as intended)
3. Start Fullscreen off & Start Big Picture UI off -> should start big picture UI in windowed mode (currently working as intended)

Did you use AI to help find, test, or implement this issue or feature?

Nope.

[github-actions]

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!

[SternXD]

Works fine

[aldee]

@SternXD thanks! since i can't merge the PR, what's the next step to have this included in the latest nightly build?

[SternXD]

Wait for a maintainer to review/merge it

[TheTechnician27]

This would leave us at s_start_fullscreen_ui, s_start_fullscreen_ui_fullscreen, and start_fullscreen alongside a StartFullscreen setting floating around in the same short conditional. As the const bool declaration is already long, this is worth just sending into startFullscreenUI directly like:

if (s_start_fullscreen_ui || Host::GetBaseBoolSettingValue("UI", "StartBigPictureMode", false))
	g_emu_thread->startFullscreenUI(s_start_fullscreen_ui_fullscreen || Host::GetBaseBoolSettingValue("UI", "StartFullscreen", false);

I'd also suggest leaving out the comment, as it's fairly self-evident that's what the code is doing – especially if you keep the named const bool start_fullscreen. Logically, the PR checks out.

[aldee]

Thank you for the feedback.

As the const bool declaration is already long, this is worth just sending into startFullscreenUI directly like:

I respectfully disagree, i think it's better to have just one long line of declaration instead of two long redundant statements. I believe it's more readable that way and the program doesn't have to execute a function twice to get the same exact value.

I'd also suggest leaving out the comment, as it's fairly self-evident that's what the code is doing – especially if you keep the named const bool start_fullscreen

This one also I disagree, as the existing code references s_start_fullscreen_ui_fullscreen, which is implying that the variable would hold the correct state of whether the app needs to run with fullscreen_ui in fullscreen mode, but turns out it only hold the state of the flag from command argument --fullscreen, making it not that obvious. So I added that comment to clarify, in case somebody assumed s_start_fullscreen_ui_fullscreen already hold the correct state, they would understand that it doesn't, hence that's what the code below the comment is doing.

[TheTechnician27]

There would be no redundancy, as the full code would look like:

if (s_start_fullscreen_ui || Host::GetBaseBoolSettingValue("UI", "StartBigPictureMode", false))
		g_emu_thread->startFullscreenUI(s_start_fullscreen_ui_fullscreen || Host::GetBaseBoolSettingValue("UI", "StartFullscreen", false);

opposed to:

if (s_start_fullscreen_ui || Host::GetBaseBoolSettingValue("UI", "StartBigPictureMode", false))
{
	// Get start fullscreen flag to make sure whether we want the big picture mode to run with full screen mode
	const bool start_fullscreen = s_start_fullscreen_ui_fullscreen || Host::GetBaseBoolSettingValue("UI", "StartFullscreen", false);
	g_emu_thread->startFullscreenUI(start_fullscreen);
}

Both the comment line and the start_fullscreen declaration are long enough to wrap the window I'm typing this into (pictured below), so it's not any more readable by declaring a variable first; it just introduces four lines that aren't necessary. s_start_fullscreen_ui_fullscreen is in the same file, so when a modestly competent programmer sees you're getting the StartFullscreen setting and suspects redundancy, they could trivially look at s_start_fullscreen_ui_fullscreen and see it doesn't account for that.

And the proposed comment doesn't even help with what you're suggesting it does:

Get start fullscreen flag to make sure whether we want the big picture mode to run with full screen mode

gives zero indication that s_start_fullscreen_ui_fullscreen doesn't include the setting; it's just an overly verbose and confusing way of saying that we're determining whether to start in fullscreen, which is trivially deducible from just looking at the proceeding statement.

[aldee]

Apologies, you're right, I misread the if statement as being the same as the parameter for the function call below it.

When I tried to fix the issue, I was referencing this code from duckstation's repo https://github.com/stenzek/duckstation/blob/62054ba97cd56b91b2578a9ebf39c9a7ae5c66c6/src/duckstation-qt/qthost.cpp#L783 and I couldn't find any formatting guide on pcsx2 repo, so I didn't saw any problem with it.

As for the comment, I think the point was so that people don't have to deduce too much, or wrongly, like how the static constant was named (and without comment explaining what it holds) and I deduced that it would hold the fullscreen state from both the command argument and the settings, but I deduced wrongly.

And by saying "get the fullscreen flag", which refers to the second part of the boolean evaluation, it implies that the s_start_fullscreen_ui_fullscreen variable doesn't include the setting.

Anyway, I honestly think what we're discussing here is more about individual preference, especially the comment, as I don't think it's misleading. As for the how long the line becomes on start_fullscreendeclaration, I'd be more than happy to change it if you can point me to the formatting guide here.

Thanks!

[F0bes]

Apologies, you're right, I misread the if statement as being the same as the parameter for the function call below it.

When I tried to fix the issue, I was referencing this code from duckstation's repo https://github.com/stenzek/duckstation/blob/62054ba97cd56b91b2578a9ebf39c9a7ae5c66c6/src/duckstation-qt/qthost.cpp#L783 and I couldn't find any formatting guide on pcsx2 repo, so I didn't saw any problem with it.

As for the comment, I think the point was so that people don't have to deduce too much, or wrongly, like how the static constant was named (and without comment explaining what it holds) and I deduced that it would hold the fullscreen state from both the command argument and the settings, but I deduced wrongly.

And by saying "get the fullscreen flag", which refers to the second part of the boolean evaluation, it implies that the s_start_fullscreen_ui_fullscreen variable doesn't include the setting.

Anyway, I honestly think what we're discussing here is more about individual preference, especially the comment, as I don't think it's misleading. As for the how long the line becomes on start_fullscreendeclaration, I'd be more than happy to change it if you can point me to the formatting guide here.

Thanks!

... you can't do that. DuckStation is not license compatible with the GPL. Look at the license of the code you linked. Did you do that for this entire PR?

[aldee]

Hmm interesting. I didn't exactly copy whatever's in there to this PR, but I did look at DuckStation's code and see why did their code work and pcsx2 didn't, since it's very similar in nature, at least in how both of them handles the QtHost and how they store and retrieve the settings.

So then I look at the pcsx2 code, and see that the s_start_fullscreen_ui_fullscreen variable doesn't store the intended state, and then I saw how the state of the settings are retrieved, and reference it for the StartFullscreen part of the change.

[F0bes]

Superseded by #13533