Support settings keys and hotkey config embedded in settings INI for RA1#943
Support settings keys and hotkey config embedded in settings INI for RA1#943CO2-code wants to merge 41 commits intoCnCNet:developfrom
Conversation
Removed RA game type settings from UserINISettings.
…-cncnet-client into Ra1-Options" This reverts commit 1721273, reversing changes made to 7bf4645.
add KeyboardHotkeySection to the configs
|
Nightly build for this pull request:
|
Metadorius
left a comment
There was a problem hiding this comment.
The keyboard INI change is good and if you split it out it can be merged even now, the rest needs more refinement
| { | ||
| private readonly string HOTKEY_TIP_TEXT = "Press a key...".L10N("Client:DTAConfig:PressAKey"); | ||
| private const string HOTKEY_INI_SECTION = "Hotkey"; | ||
| private readonly string HOTKEY_TIP_TEXT = "Press a key...".L10N("Client:DTAConfig:PressAKey"); |
There was a problem hiding this comment.
what changed in this specific line? did line ending change? that should be reverted as it litters the history with unnecessary changes
There was a problem hiding this comment.
I deleted (private const string HOTKEY_INI_SECTION = "Hotkey";) only
| /* AUDIO */ | ||
| /*********/ | ||
|
|
||
| public DoubleSetting MultiplayerScoreVolume { get; private set; } |
There was a problem hiding this comment.
not sure what is this for, some other score volume?
There was a problem hiding this comment.
should be done in another way probably, not sure. @SadPencil @11EJDE11 opinions?
There was a problem hiding this comment.
should be done in another way probably, not sure. @SadPencil @11EJDE11 opinions?
can you re-review please
There was a problem hiding this comment.
not sure what is this for, some other score volume?
yes ScoreVolume and MultiplayerScoreVolume are the game's "Music volume"
Can we assume ScoreVolume and MultiplayerScoreVolume always have the same values?
I am 100% sure that they should always have the same values , according to Iran's RAConfigTool ,
double MusicVolume = ((double)this.Slider_MusicVolume.Value) / 1000;
Files.RedAlertINI.setStringValue("Options", "ScoreVolume", MusicVolume.ToString());
Files.RedAlertINI.setStringValue("Options", "MultiplayerScoreVolume", MusicVolume.ToString());
although keep in mind that this is a basic review of only changed files, I suspect there may be more places to handle this |
|
have you handled "reset to defaults" part of the keyboard INI to work with RA1 setup and your changes? |
I think it is already done as it uses WriteKeyboardINI() , right ? |
Metadorius
left a comment
There was a problem hiding this comment.
looks much better now! remind me to review in more details on weekend
for me the weekend is Saturday and Sunday , which starts tomorrow :) |
|
yea correct, I'll just forget by tomorrow likely 🤣 |
Removed call to HotkeyConfigurationWindow.WriteKeyboardINI() during settings save.
Reminder :) |
Added functionality to handle keyboard hotkey section and apply default settings.
Add hotkey section handling in SaveSettings method.
Refactor WriteKeyboardINI method to use UserINISettings for keyboard configuration.
| if (ClientConfiguration.Instance.ClientGameType == ClientType.RA) | ||
| { | ||
| _instance = new UserINISettings(userIni); | ||
| return; | ||
| } |
| using Rampastring.XNAUI.XNAControls; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using ClientCore.Enums; |
| // TODO should it be like this and main settings window will handle it, or should we flush even here, saving all settings? | ||
| if (!ClientConfiguration.Instance.SettingsIniAsKeyboardIni) | ||
| keyboardIni.WriteIniFile(SafePath.CombineFilePath(ProgramConstants.GamePath, ClientConfiguration.Instance.KeyboardINI)); |
There was a problem hiding this comment.
For consistency I'd say flush here. The hotkey window has its own save button.
if (ClientConfiguration.Instance.SettingsIniAsKeyboardIni)
keyboardIni.WriteIniFile();
else
keyboardIni.WriteIniFile(SafePath.CombineFilePath(ProgramConstants.GamePath, ClientConfiguration.Instance.KeyboardINI));There was a problem hiding this comment.
just as you said this I realized it's not gonna work. Imagine a situation when the user pressed save in the hotkey config window, then decided they don't want config changes they did in the options.
If we don't flush here, everything (including hotkeys) can be restored by hitting a cancel.
If we flush here - you can't cancel anymore at all.
The hotkeys erased/not is a discrepancy in behavior, though, but I'd argue it's not as critical and the behavior with unified INI might be more logical
There was a problem hiding this comment.
Makes sense, what a pain. I guess just add a comment explaining that for now.
| public bool SettingsIniAsKeyboardIni => SettingsIniName == KeyboardINI; | ||
|
|
||
| public string KeyboardHotkeySection => clientDefinitionsIni.GetStringValue(SETTINGS, "KeyboardHotkeySection", "Hotkey"); | ||
|
|
There was a problem hiding this comment.
Consider that we already introduced so many hard coded ini key names for RA, when providing the default value of KeyboardHotkeySection, provide "WinHotKeys" as the default value when it's RA.


Fixes an issue where hotkeys and client settings overwrite each other when using same INI file.
Updated UserINISettings key mapping to match RA1's expected keys.
Made the hotkey section configurable (allowing RA1 to use WinHotkeys while other games continue using Hotkey).