Skip to content

SettingsWindow: Make 'Recommended Value' an optional parameter#13472

Open
TheTechnician27 wants to merge 1 commit intoPCSX2:masterfrom
TheTechnician27:settingswidget
Open

SettingsWindow: Make 'Recommended Value' an optional parameter#13472
TheTechnician27 wants to merge 1 commit intoPCSX2:masterfrom
TheTechnician27:settingswidget

Conversation

@TheTechnician27
Copy link
Copy Markdown
Contributor

@TheTechnician27 TheTechnician27 commented Nov 1, 2025

Description of Changes

Changes 'Recommended Value" in the settings window from a mandatory parameter to an optional one. This has the consequence of flipping the argument order, where the recommended value is now the rightmost argument with a default of "" (taken by the function to mean "no recommended value"). This should've been built this way from the start, and this is just ripping off a bandage. There are ways to bodge this so I don't have to change all of these lines – the easiest way being to make no default value but to still accept "" as no value and have that logic in the function body – but this is the right way that will make expanding tooltips to many more non-settings objects more elegant.

Before:
Image of a tooltip with Recommended Value: None

After:
Image of the same tooltip as before but without an unnecessary recommended value

Rationale behind Changes

Currently, 'Recommended Value' is a required parameter which is sent into SettingsWindow::registerWidgetHelp() alongside the title and body. Because tooltip text can and should be used for buttons that strictly perform an action – i.e. you don't assign a value to them – it's dumb and inelegant to both the programmer and the end user to have something like "Recommended Value: N/A". This is presently the case for six options, but I plan to expand tooltip text to all – likely over 100 – non-setting buttons.

This was part of a major PR that got split into parts, the first part being #13388. Like that PR, most of this PR is just mindless repetition across many files.

Suggested Testing Steps

  • Make sure that all of the registerWidgetHelp() calls have had their arguments flipped.
    • You'll know immediately when they haven't been because the tooltip will have an obscenely long "Recommended Value".
  • Make sure that I haven't missed any "N/A", "None", etc. existing recommended values.

Edit: Here's a manufactured example of what a flipped argument would look like:
image

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

Nahp.

@github-actions github-actions bot added the GUI/Qt label Nov 1, 2025
@JordanTheToaster JordanTheToaster added this to the Release 2.6 milestone Nov 1, 2025
@TheTechnician27 TheTechnician27 force-pushed the settingswidget branch 3 times, most recently from 630d7b6 to 056942e Compare November 1, 2025 15:49
@F0bes
Copy link
Copy Markdown
Member

F0bes commented Nov 7, 2025

Some of these probably shouldn't have a recommended value at all. Like the "User preference" ones.

Or the achievement enable.

I don't know if you want to tackle that here? I see you removed some for other options.

@TheTechnician27
Copy link
Copy Markdown
Contributor Author

TheTechnician27 commented Nov 7, 2025

Some of these probably shouldn't have a recommended value at all. Like the "User preference" ones.

Fair point. I only removed the "None" and "N/A" ones where it's an obvious placeholder for an option where a rec makes no sense. I opted not to change weird ones like "Enable Achievements" –> "Unchecked" – that probably wouldn't have been added had the field been optional – in order to keep this a strictly technical change.

This PR is setting up a later one where I'm going through and checking tooltip descriptions, so changing the contents of the recs might be better-suited to that one.

Copy link
Copy Markdown
Member

@F0bes F0bes left a comment

Choose a reason for hiding this comment

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

It's a lot to review, but nothing sticks out as completely wrong.

@F0bes
Copy link
Copy Markdown
Member

F0bes commented Nov 29, 2025

Oh, you need to rebase.

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.

6 participants