Monitor selection option #6261
Monitor selection option #6261
Conversation
9a490b6 to
a6ea490
Compare
| @@ -1026,6 +1034,32 @@ public void ApplyInputSettings() | |||
| /// </summary> | |||
| public void ApplyWindowSettings() | |||
| { | |||
| var screenId = -1; | |||
| bool isOldSelectionDifferent = false; | |||
| if (OperatingSystem.IsWindows()) | |||
There was a problem hiding this comment.
Why is this only checking for Windows? Shouldn't this work on any desktop OS?
There was a problem hiding this comment.
I thought the original issue mentioned Windows; it would be a Windows specific operation. I think it would be good to remove it too.
There was a problem hiding this comment.
The issue originally mentions Windows, because players on Mac and Linux already have features in their operating system to fix the problem of the game on the wrong screen themselves. Only people who still use Windows have the problem, so the "fix" is mainly aimed at them. However the feature should still be quite workable on Mac and Linux as well, as long as the default behaviour is "auto" and not changing the game monitor.
| if (OperatingSystem.IsWindows()) | ||
| { | ||
| var currentScreenId = DisplayServer.WindowGetCurrentScreen(); | ||
| if (oldDisplaySelectionIndex == -1) |
There was a problem hiding this comment.
What's the use for this old selection index? Cannot this just call WindowGetCurrentScreen and if the result is not what is desired, then call WindowSetCurrentScreen?
| /// Selected window index for the game window. | ||
| /// </summary> | ||
| [JsonProperty] | ||
| public SettingValue<int> SelectedDisplayIndex { get; set; } = new(DisplayServer.WindowGetCurrentScreen()); |
There was a problem hiding this comment.
I really think the default should be -1 and it should mean "auto" as on non-Windows platform it's going to be annoying if the game forces itself on a specific screen.
| @@ -33,6 +33,17 @@ public partial class OptionsMenu : ControlWithInput | |||
| .GetOutputDeviceList().Where(d => d != Constants.DEFAULT_AUDIO_OUTPUT_DEVICE_NAME) | |||
| .Prepend(Constants.DEFAULT_AUDIO_OUTPUT_DEVICE_NAME).ToList(); | |||
|
|
|||
| private static readonly Lazy<List<DisplayInfo>> DisplaysCache = new(() => | |||
There was a problem hiding this comment.
This should not be static because attaching new monitors should work without having to restart the game...
There was a problem hiding this comment.
I hadn't thought about that. Thanks.
| @@ -263,6 +265,7 @@ layout_mode = 2 | |||
| size_flags_horizontal = 3 | |||
| size_flags_vertical = 3 | |||
| follow_focus = true | |||
| scroll_vertical = 100 | |||
There was a problem hiding this comment.
This one line chagne seems suspicious to me...
There was a problem hiding this comment.
I know. Whenever I open the OptionsMenu scene, Godot just changes this line for me. I always revert it but looks like I missed this one.
|
Here's the documentation on working with text in the game: https://github.com/Revolutionary-Games/Thrive/blob/master/doc/working_with_translations.md |
Brief Description of What This PR Does
This PR should create a new option for Windows user in the options scene. It is under Graphics tab, called "Display". It should list all available displays for user to select and immediately move the game window to the selected display.
Maybe there is way but, I couldn't find a good way to list the name of the displays.
Also, I don't know and not sure about how translations are working. I will get back to the translation part later.
Related Issues
Closes #6110
Progress Checklist
Note: before starting this checklist the PR should be marked as non-draft.
break existing features:
https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
(this is important as to not waste the time of Thrive team
members reviewing this PR)
styleguide.
Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.