-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
FSUI: Add Network and HDD settings page #12938
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
base: master
Are you sure you want to change the base?
Conversation
c0b4728
to
3495a8e
Compare
1be676b
to
ad1374c
Compare
Tested only Windows, and haven't done a code review In Qt, the API is set only when a valid adapter is selected I cannot select the device when sockets is set. I cannot adjust the DNS settings under sockets when Internet DHCP has been disabled (for other API types) IP input boxes lack much of the styling styling that other popups have Min & default HDD size should be 40GB HDD input boxes lack much of the styling that other popups have The LBA48 setting is a fake setting (it isn't saved anywhere) The HDD section does not display the existing file (See Bios for example) The existing file size is not displayed either (differing from Qt) |
Okay I will look into that all for you right now and fix them 🫡 |
9061651
to
bd9be20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick code review of the changes in FullscreenUI only
As for (Windows) testing
Vertical spacing for the IP address isn't consistent between display and edit in the IP address popup
You could immediately show the edit box rather then having to click a button (given you don't spinner arrows that the OSD scale popup has)
otherwise, styling looks correct.
When switching to pcap or tap, you default to Auto
Auto isn't a valid setting (at least on windows) for these adapters
The Qt UI attempts to match to the same adapter if it can, but if that is too difficult you can just clear it.
Changing the HDD directory didn't seem to work.
The custom HDD size input popup is not styled correctly.
I had originally intended inis to be the default directory for Hdd
Core defaults to ini if only a filename is set, but it seems that the Qt's file picker defaults to documents/PCSX2 directory instead.
The default directory you've picked is probably the one that people actually end up using, so seems the better pick.
7e78dae
to
f5d031a
Compare
f5d031a
to
eff9ae2
Compare
Signed-off-by: SternXD <[email protected]> .
eff9ae2
to
47b21b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Selecting an existing HDD via browse doesn't close the file picker
The file picker probably should show only .raw
files
The text box for saving an input profile is a different colour in this PR vs master.
else if (!previous_device.empty() && index < static_cast<s32>(current_adapter_lists.size())) | ||
{ | ||
const auto& new_adapter_list = current_adapter_lists[index]; | ||
const auto& old_adapter_list = current_adapter_lists[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
old_adapter_list
doesn't appear to be used
{ | ||
for (const auto& adapter : new_adapter_list) | ||
{ | ||
if (adapter.name == old_adapter_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you aren't matching based on GUID?
// Get device display name | ||
std::string device_display = "Auto"; | ||
if (show_device_setting && !current_device.empty()) | ||
{ | ||
// Get the adapter list for current API type | ||
if (current_api_index < adapter_lists.size()) | ||
{ | ||
const auto& adapter_list = adapter_lists[current_api_index]; | ||
for (const auto& adapter : adapter_list) | ||
{ | ||
if (adapter.guid == current_device) | ||
{ | ||
device_display = adapter.name; | ||
break; | ||
} | ||
} | ||
} | ||
|
||
if (device_display == "Auto") | ||
device_display = current_device; | ||
} | ||
else if (show_device_setting && current_device.empty()) | ||
{ | ||
const std::string current_api = bsi->GetStringValue("DEV9/Eth", "EthApi", "Unset"); | ||
if (current_api == "PCAP Bridged" || current_api == "PCAP Switched" || current_api == "TAP") | ||
{ | ||
device_display = "Not Selected"; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, Sockets doesn't default to Auto when nothing is selected, instead, a GUID of Auto
needs to be in the config.
Thus, "Not Selected" should be displayed for all adapters when current_device
is an empty string.
@@ -2867,6 +3030,115 @@ void FullscreenUI::DrawPathSetting(SettingsInterface* bsi, const char* title, co | |||
} | |||
} | |||
|
|||
void FullscreenUI::DrawTextSetting(SettingsInterface* bsi, const char* title, const char* summary, const char* section, const char* key, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function used anywhere?
ImGui::PushFont(g_large_font.first, g_large_font.second); | ||
ImGui::PushStyleVar(ImGuiStyleVar_WindowRounding, LayoutScale(10.0f)); | ||
ImGui::PushStyleVar(ImGuiStyleVar_WindowPadding, LayoutScale(20.0f, 20.0f)); | ||
ImGui::PushStyleVar(ImGuiStyleVar_FrameBorderSize, 0.0f); | ||
ImGui::PushStyleVar(ImGuiStyleVar_FramePadding, | ||
LayoutScale(ImGuiFullscreen::LAYOUT_MENU_BUTTON_X_PADDING, ImGuiFullscreen::LAYOUT_MENU_BUTTON_Y_PADDING)); | ||
|
||
bool is_open = true; | ||
if (ImGui::BeginPopupModal(title, &is_open, ImGuiWindowFlags_NoCollapse | ImGuiWindowFlags_NoResize | ImGuiWindowFlags_NoMove)) | ||
{ | ||
BeginMenuButtons(); | ||
|
||
bool value_changed = false; | ||
char ip_str[16]; | ||
std::snprintf(ip_str, sizeof(ip_str), "%d.%d.%d.%d", ip_octets[0], ip_octets[1], ip_octets[2], ip_octets[3]); | ||
|
||
const float end = ImGui::GetCurrentWindow()->WorkRect.GetWidth(); | ||
ImGui::SetNextItemWidth(end); | ||
if (ImGui::InputText("##ip", ip_str, sizeof(ip_str), ImGuiInputTextFlags_CharsDecimal | ImGuiInputTextFlags_CallbackCharFilter, | ||
[](ImGuiInputTextCallbackData* data) -> int { | ||
// Only allow digits and dots | ||
return (data->EventChar >= '0' && data->EventChar <= '9') || data->EventChar == '.' ? 0 : 1; | ||
})) | ||
{ | ||
std::istringstream iss(ip_str); | ||
std::string segment; | ||
int i = 0; | ||
while (std::getline(iss, segment, '.') && i < 4) | ||
{ | ||
ip_octets[i] = std::clamp(std::atoi(segment.c_str()), 0, 255); | ||
i++; | ||
} | ||
value_changed = true; | ||
} | ||
ImGui::SetCursorPosY(ImGui::GetCursorPosY() + LayoutScale(10.0f)); | ||
|
||
if (value_changed) | ||
{ | ||
std::snprintf(ip_str, sizeof(ip_str), "%d.%d.%d.%d", ip_octets[0], ip_octets[1], ip_octets[2], ip_octets[3]); | ||
if (IsEditingGameSettings(bsi) && strcmp(ip_str, default_value) == 0) | ||
bsi->DeleteValue(section, key); | ||
else | ||
bsi->SetStringValue(section, key, ip_str); | ||
SetSettingsChanged(bsi); | ||
} | ||
|
||
if (MenuButtonWithoutSummary(FSUI_CSTR("OK"), true, LAYOUT_MENU_BUTTON_HEIGHT_NO_SUMMARY, g_large_font, ImVec2(0.5f, 0.0f))) | ||
{ | ||
ImGui::CloseCurrentPopup(); | ||
} | ||
EndMenuButtons(); | ||
ImGui::EndPopup(); | ||
} | ||
|
||
ImGui::PopStyleVar(4); | ||
ImGui::PopFont(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our existing code would have popup logic in ImGuiFullscreen
(via Open*
,Is*Open
,Close
, with Draw
being called ImGuiFullscreen::EndLayout()
I would prefer if this new code was consistent (i.e. creating *IPAddressDialog
functions)
ImGui::SetNextItemWidth(text_box_size.x - 10.0f); | ||
|
||
// Check if we're in an IP address or numeric input field | ||
bool is_ip_input = s_input_dialog_message.find("xxx.xxx.xxx.xxx") != std::string::npos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you are using OpenInputStringDialog() for IP addresses now
I now notice that you this was what you originally used for the IP dialogs.
This only got used for naming an input profile dialog was, which I didn't open when comparing dialogs back when I mentioned styling.
@@ -2172,7 +2172,7 @@ bool ImGuiFullscreen::IsFileSelectorOpen() | |||
} | |||
|
|||
void ImGuiFullscreen::OpenFileSelector(std::string_view title, bool select_directory, FileSelectorCallback callback, | |||
FileSelectorFilters filters, std::string initial_directory) | |||
FileSelectorFilters filters, std::string initial_directory, std::string default_filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new parameter isn't used
OpenInputStringDialog( | ||
FSUI_ICONSTR(ICON_FA_PEN_TO_SQUARE, "Custom HDD Size"), | ||
FSUI_STR("Enter custom HDD size in gigabytes (40-2000):"), | ||
"40", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
// Check if we're in an IP address or numeric input field | ||
bool is_ip_input = s_input_dialog_message.find("xxx.xxx.xxx.xxx") != std::string::npos; | ||
bool is_numeric_input = s_input_dialog_message.find("gigabytes") != std::string::npos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps OpenInputStringDialog()
could accept a callback or enum/bool for input filtering, rather then guessing based on the dialog title.
Will work on the fixes and stuff later taking a little break from PCSX2 dev |
Description of Changes
This PR adds a complete Network and HDD settings page to FullscreenUI, bringing feature parity with the Qt interface for DEV9. This includes:
Rationale behind Changes
Previously, users had to exit FullscreenUI and use the Qt interface to configure network and HDD settings for DEV9. This created an inconsistent user experience and made it a horrible experience for anyone couch gaming.
The changes solve this by:
Suggested Testing Steps
Network Settings:
HDD Settings:
Cross-Platform:
Did you use AI to help find, test, or implement this issue or feature?
I did not use it for my code but it did help me with grammar and better helping understand why this should be added.