feat(settings): add validation for volume and brightness (#42)#61
Open
VIDYANKSHINI wants to merge 1 commit into
Open
feat(settings): add validation for volume and brightness (#42)#61VIDYANKSHINI wants to merge 1 commit into
VIDYANKSHINI wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds input validation and warning logging around non-registry system settings so invalid brightness/volume values are rejected (with logs) rather than being applied, aligning with the bounds requirements in Issue #42.
Changes:
- Injected
ILoggerintoSystemSettingsServiceand replacedConsole.WriteLinewarnings withILogger.LogWarning. - Added bounds checking (0–100) and invalid-format handling for
brightnessandvolumeinApplyNonRegistrySettingsAsync. - Added unit tests covering valid bounds, out-of-range values, and invalid formats for
brightness/volume.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Services/System/SystemSettingsService.cs |
Adds logger integration and validates brightness/volume before issuing PowerShell commands. |
tests/WinHome.Tests/SystemSettingsServiceTests.cs |
Adds tests for new validation and warning-logging behavior. |
Comments suppressed due to low confidence (1)
src/Services/System/SystemSettingsService.cs:215
userSetting.Value.ToString()will throw if the config contains an explicit null forvolume. SinceConfiguration.SystemSettingsis aDictionary<string, object>populated from YAML/JSON, null values are possible and should be handled by logging an invalid-format warning instead of throwing (use a null-safe conversion).
case "volume":
if (int.TryParse(userSetting.Value.ToString(), out int volume))
{
if (volume < 0 || volume > 100)
{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+80
to
+84
| [Theory] | ||
| [InlineData(0)] | ||
| [InlineData(100)] | ||
| [InlineData(50)] | ||
| public async Task ApplyNonRegistrySettingsAsync_Should_Accept_Valid_Brightness_Boundaries(int brightness) |
Comment on lines
193
to
+197
| case "brightness": | ||
| if (int.TryParse(userSetting.Value.ToString(), out int brightness)) | ||
| { | ||
| string command = $"(Get-WmiObject -Namespace root/WMI -Class WmiMonitorBrightnessMethods).WmiSetBrightness(1, {brightness})"; | ||
| _processRunner.RunCommand("powershell", $"-Command \"{command}\"", dryRun); | ||
| if (brightness < 0 || brightness > 100) | ||
| { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Closes #42
Proposed Changes
volumeandbrightnesssettings inSystemSettingsService.cs.ILoggerservice for invalid/out-of-range settings.GetTweaksAsyncto useILogger.LogWarning.SystemSettingsServiceTests.cscovering valid boundaries (0, 100), out-of-range values (-1, 101), and invalid format inputs (like strings and decimals).Type of Change
Testing & Verification
dotnet testand all 60+ cross-platform tests passed.GSSOC 2026 Checklist
dotnet format).gssoc:approvedlabel for this PR to count for points.