-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
Some small GUI improvements #337
base: main
Are you sure you want to change the base?
Conversation
merge changes
the template gen script is possibly broken and totally erased my rc2 files so this script is just the isolated hash gen function. Run with cmd like so ```Powershell -File hasher.ps1 "message string"```
add local var for screenshot name to not overwrite user setting
? "%Date% %Time%_%TimeMS%" | ||
: _screenshot_name; | ||
// This will remove the appname and leading whitespace if people have not changed their target path but select this option. Also updates to new default | ||
// if (_screenshot_name == "%AppName% %Date% %Time%_%TimeMS%" || _screenshot_name == "%AppName% %Date% %Time) _screenshot_name = "%Date% %Time%_%TimeMS%"; |
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 think it would be even more powerful if instead of adding this special case, we just add the ability to add macros to _screenshot_path
, just like _screenshot_name
. Then the user can just set the "Screenshot path" to something like .\%AppName%
and the "Screenshot name" to %Date% %Time%_%TimeMS%
to achieve the same thing you have done here, but they can also use completely different naming schemes, e.g. a path like .\Screenshots\%AppName\%Date%\
.
Doing this would also make the changes to the localization resources unneeded.
if (ImGui::IsItemHovered(ImGuiHoveredFlags_ForTooltip)) | ||
{ | ||
ImGui::SetItemTooltip(_("Split save paths into folders by App Name.\nUseful if you have multiple games sharing the same screenshot directory")); | ||
} |
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.
With the above proposal this option would go away and instead would just add the same tooltip on the "Screenshot name" widget to the "Screenshot path" widget too.
@@ -362,6 +362,7 @@ void reshade::runtime::load_config_gui(const ini_file &config) | |||
config_get("INPUT", "KeyFPS", _fps_key_data); | |||
config_get("INPUT", "KeyFrameTime", _frametime_key_data); | |||
config_get("INPUT", "InputProcessing", _input_processing_mode); | |||
config_get("INPUT", "EscClosesOverlay", _esc_closes_overlay); |
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 feels like an option that should be in the OVERLAY
section, since it's not really a hotkey, more like [OVERLAY] EscapeToClose=<0,1>
.
@@ -2168,8 +2173,8 @@ void reshade::runtime::draw_gui_settings() | |||
"Block all input when overlay is visible\n"); | |||
std::replace(input_processing_mode_items.begin(), input_processing_mode_items.end(), '\n', '\0'); | |||
modified |= ImGui::Combo(_("Input processing"), reinterpret_cast<int *>(&_input_processing_mode), input_processing_mode_items.c_str()); | |||
|
|||
modified |= imgui::key_input_box(_("Overlay key"), _overlay_key_data, *_input); | |||
modified |= ImGui::Checkbox(_("Escape Closes Overlay"),&_esc_closes_overlay); |
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'm not convinced this needs to be a GUI option, since it's very niche seems sufficient to just be a config file option only.
These changes add a couple small usability settings to the GUI including machine translated localizations. There may also be a few unchanged lines that are just reformatted by the godawful intellisense features in VS, sorry if those snuck through, but I believe all changes are merged and I've tested everything thoroughly.
New checkbox item, "Escape closes overlay", enabled by default to preserve usage. When disabled you can only close the overlay with the open/close key, as there can be cases where you want the overlay to stay open but you need to press escape to close a game menu. I didn't want to mess with the input blocking modes so I just made it a separate option. It takes some getting used to and honestly is a bit annoying not to be able to press escape but I think its a necessary option to have. Possible improvement could be to add an option to hold escape to close but I think this is fine for now.
The other option sorts screenshots into folders by AppName which is great for people who save everything to a unified screenshot directory. Previously I used a python script to do this post save but I think this is useful enough to add to the runtime and I've seen a few requests for it. I thought about also making Before/After folders if that option is selected and could go ahead and add that if desirable but I think for now this makes sense for a core runtime feature.
Users can put %AppName% in their screenshot name still (e.g. "Date Time AppName" works), however I made it so if they have either the new default or the old default option "AppName Date Time" it will remove the %AppName% and leading whitespace from the name whenever a screenshot is saved to ensure they don't get screenshots like "AppName/AppName" which is redundant and bad for organization (instead they get "AppName\Date Time Ms").
Maybe this feels a bit too heavy handed to change the user's setting, but I think most people won't mind. While writing this I went ahead and changed it to use a new local variable so the actual config value won't be changed. This looks a bit awkward to have two local vars for screenshot name and there is a downside that it may not convey the change properly to users since it doesn't save, but it does ensure that the default setting will be preserved if they disable the option. If you like the feature enough to make it default to on we could also just have the setup tool change everyone's default screenshot path. Although in that case we might need to do the reverse and add the AppName to the screenshot name if this setting gets disabled. Lots of ways to do it and none of them feel quite right so its up to your judgement.
I could add something to the tutorial text or the macro string tooltip to explain this, actually I also tried to add a tooltip to this option but it appears blank. I'm sure I calculated the CRC hash correctly but maybe you just can't have a tooltip on a checkbox? It still shows a tooltip but its blank. The option is probably self explanatory enough so its not a big deal and if its not fixable we can just remove the tooltip.
Last change here is to the post save command feature. I made it accept file extensions for runnable scripts with specific handling for python and powershell scripts and anything else being run by calling "cmd /C ". Truthfully users could already do this themselves by running "C:\Windows\System32\cmd.exe" as their post save command and "/C <script path>" as their arguments but this makes it a bit more intuitive and clean. Up to you if you want it but I don't really see a reason not to since it doesn't actually add any new feature.