-
-
Notifications
You must be signed in to change notification settings - Fork 54
feat: customize delay before the controls automatically hide #670
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: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR introduces a customizable delay setting for automatically hiding player controls, organizing player control settings under a new "Overlay controls" SettingsExpander.
- Adds a new setting to configure how long the player controls remain visible after user interaction stops
- Consolidates player control settings (show controls, show chapters, hide delay) into a single expandable settings group
- Implements the hide delay functionality with a dropdown offering 1-5 second options
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
Screenbox/Strings/en-US/Resources.resw | Adds localized strings for the new overlay controls section and hide delay options |
Screenbox/Pages/SettingsPage.xaml | Restructures player control settings into a SettingsExpander and adds hide delay ComboBox |
Screenbox.Core/ViewModels/SettingsPageViewModel.cs | Implements hide delay property with index-to-seconds conversion logic |
Screenbox.Core/ViewModels/PlayerPageViewModel.cs | Updates control hiding logic to use configurable delay instead of hardcoded values |
Screenbox.Core/Services/SettingsService.cs | Adds storage and default value for the new hide delay setting |
Screenbox.Core/Services/ISettingsService.cs | Adds interface property for the hide delay setting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
_playerControlsHideDelay = _settingsService.PlayerControlsHideDelay switch | ||
{ | ||
1 => 0, | ||
2 => 1, | ||
3 => 2, | ||
4 => 3, | ||
5 => 4, | ||
_ => 2 // Default to 3 seconds (Index 2) | ||
}; |
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.
The switch statement for converting seconds to index contains magic numbers and duplicates the conversion logic. Consider extracting this to a helper method or using a formula like seconds - 1
for the index conversion to improve maintainability.
_playerControlsHideDelay = _settingsService.PlayerControlsHideDelay switch | |
{ | |
1 => 0, | |
2 => 1, | |
3 => 2, | |
4 => 3, | |
5 => 4, | |
_ => 2 // Default to 3 seconds (Index 2) | |
}; | |
// Convert seconds (1-5) to index (0-4), default to 2 if out of range | |
_playerControlsHideDelay = (_settingsService.PlayerControlsHideDelay >= 1 && _settingsService.PlayerControlsHideDelay <= 5) | |
? _settingsService.PlayerControlsHideDelay - 1 | |
: 2; // Default to 3 seconds (Index 2) |
Copilot uses AI. Check for mistakes.
_settingsService.PlayerControlsHideDelay = value switch | ||
{ | ||
// Index => Seconds | ||
0 => 1, | ||
1 => 2, | ||
2 => 3, | ||
3 => 4, | ||
4 => 5, | ||
_ => 3 | ||
}; |
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.
The switch statement for converting index to seconds also contains magic numbers and duplicates conversion logic. This could be simplified to value + 1
to convert index to seconds, making the code more maintainable and less error-prone.
_settingsService.PlayerControlsHideDelay = value switch | |
{ | |
// Index => Seconds | |
0 => 1, | |
1 => 2, | |
2 => 3, | |
3 => 4, | |
4 => 5, | |
_ => 3 | |
}; | |
// Index (0-4) => Seconds (1-5), default to 3 if out of range | |
_settingsService.PlayerControlsHideDelay = (value >= 0 && value <= 4) ? value + 1 : 3; |
Copilot uses AI. Check for mistakes.
private void DelayHideControls() | ||
{ | ||
if (PlayerVisibility != PlayerVisibilityState.Visible || AudioOnly) return; | ||
|
||
int delayInSeconds = _settingsService.PlayerControlsHideDelay; | ||
_controlsVisibilityTimer.Debounce(() => TryHideControls(), TimeSpan.FromSeconds(delayInSeconds)); | ||
} |
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.
private void DelayHideControls(int extraSeconds = 0)
{
if (PlayerVisibility != PlayerVisibilityState.Visible || AudioOnly) return;
int delayInSeconds = _settingsService.PlayerControlsHideDelay + extraSeconds;
_controlsVisibilityTimer.Debounce(() => TryHideControls(), TimeSpan.FromSeconds(delayInSeconds));
}
We could add a parameter to the DelayHideControls()
so that we can increment the delay on the FocusManagerOnFocusChanged()
like this:
DelayHideControls(1);
6f1397f
to
172eec7
Compare
Okay, I've streamlined the index switching logic and improved the ComboBox localization. Unfortunately, I had to add a converter (which is probably more complex than necessary) since I couldn’t get it working with a basic method like this on the ItemTemplate: public string GetSecondsCountString(int value)
{
return Strings.Resources.SecondsCount(value);
} If you prefer using SelectedIndex, I can revert it. |
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.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Seconds = 4, | ||
Items = 11, |
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.
The enum values use non-sequential numbers (1, 2, 4, 11) which creates magic numbers without clear purpose. Consider using sequential values starting from 0 or add comments explaining the significance of these specific numbers.
Seconds = 4, | |
Items = 11, | |
Seconds = 3, | |
Items = 4, |
Copilot uses AI. Check for mistakes.
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.
Yeah, the gaps are there for time related units, like Millisecond, Minutes, Hours, etc.
|
||
public List<Models.Language> AvailableLanguages { get; } | ||
|
||
public int[] PlayerControlsHideDelayOptions { get; } = { 1, 2, 3, 4, 5 }; |
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.
The hardcoded array of options should be moved to a configuration file or made configurable. This makes it difficult to modify the available delay options without code changes.
Copilot uses AI. Check for mistakes.
Combines the player controls settings into its own SettingsExpander and introduces an option to configure its hide delay (partially resolves #548).
There is definitely room for improvement. Not a fan of the Index switch statements, and I'd wish if we could use ReswPlus for the ComboBox values.