Add an option to use seperated TDP when device power on Battery Mode #833
Conversation
WalkthroughThe software has been updated to manage Thermal Design Power (TDP) settings more effectively, especially when devices are running on battery power. New UI elements and event handlers have been added to allow users to toggle TDP settings and adjust power limits for battery operation. The application now responds to power mode changes, ensuring that TDP settings are appropriately applied based on whether the device is plugged in or on battery. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (8)
- HandheldCompanion/Managers/PerformanceManager.cs (4 hunks)
- HandheldCompanion/Misc/Profile.cs (1 hunks)
- HandheldCompanion/Properties/Resources.Designer.cs (1 hunks)
- HandheldCompanion/Properties/Resources.resx (1 hunks)
- HandheldCompanion/Views/Pages/ProfilesPage.xaml (1 hunks)
- HandheldCompanion/Views/Pages/ProfilesPage.xaml.cs (4 hunks)
- HandheldCompanion/Views/QuickPages/QuickProfilesPage.xaml (1 hunks)
- HandheldCompanion/Views/QuickPages/QuickProfilesPage.xaml.cs (8 hunks)
Files skipped from review due to trivial changes (1)
- HandheldCompanion/Properties/Resources.Designer.cs
Additional comments: 17
HandheldCompanion/Properties/Resources.resx (1)
- 2360-2365: The new entries for TDP settings on battery are correctly added to the
.resxfile. These will be used to display the appropriate UI text for the new feature. Make sure that the corresponding keys are used in the code to fetch these values for localization purposes.HandheldCompanion/Views/QuickPages/QuickProfilesPage.xaml (2)
235-294: The addition of the TDP on battery feature is well implemented with a clear UI design that includes a toggle switch and a slider for user interaction. The use of data binding and event handlers is consistent with the rest of the file. However, ensure that the visibility of the TDP on battery settings is correctly managed based on the state of the
TDPToggle. If theTDPToggleis off, the TDP on battery settings should not be visible to the user.231-291: The
TickPlacementandValueChangedevent are correctly set for theTDPOnBatterySlider. It's important to ensure that theValueChangedevent handlerTDPOnBatterySlider_ValueChangedis implemented in the code-behind to handle the user's input and update the TDP settings accordingly.HandheldCompanion/Views/Pages/ProfilesPage.xaml (1)
- 657-724: The implementation of the "TDP On Battery limit" feature in the UI seems consistent with the rest of the application's design and functionality. The use of
x:Staticfor resource strings ensures localization support, and the data bindings for the toggle switch and slider appear to be set up correctly to respond to user interactions.However, there are a few points to consider:
- Ensure that the
TDPOnBatteryToggletoggle switch correctly reflects the state of theTDPOnBatteryEnabledproperty from theProfileclass.- Verify that the
TDPOnBatterySliderslider is properly bound to theTDPOnBatteryValuesproperty and that the value changes are being saved to the profile settings.- Confirm that the
TDPOnBatterySlider_ValueChangedevent handler is implemented to handle the slider's value changes.- Check that the
TDPOnBatteryToggle_Toggledevent handler is implemented to handle the toggle switch state changes.- Make sure that the
IsEnabledbindings for theTDPOnBatteryToggleand the content grid are working as expected, enabling and disabling the UI elements based on theTDPTogglestate.The code is well-structured, and the use of styles and resources should make it easy to maintain and ensure a consistent look and feel across the application.
HandheldCompanion/Managers/PerformanceManager.cs (5)
6-14: The import of
Microsoft.Win32andSystem.Windows.Formsis necessary for handling system events related to power mode changes. This is a good use of system libraries to extend functionality.102-110: The addition of an event handler for
SystemEvents.PowerModeChangedis a good practice to dynamically adjust settings based on the power source. It's important to ensure that the application properly unsubscribes from this event when it's closed or no longer needed to prevent memory leaks.121-122: Setting
MaxDegreeOfParallelismto half of the processor count is a reasonable default for parallel operations, but it should be verified that this does not negatively impact performance on systems with a high number of processor cores.124-127: The
SystemEvents_PowerModeChangedmethod correctly triggers a UI update throughProfilesPage.RequestUpdate(). It's important to ensure that the UI update method is thread-safe sincePowerModeChangedcan be raised from a non-UI thread.151-174: The logic to determine
TDPOverrideValuesbased on whether the device is plugged in or on battery is sound. However, it's important to ensure thatSystemInformation.PowerStatus.PowerLineStatusis reliable across different devices and that theTDPOnBatteryValuesare validated before use.HandheldCompanion/Views/Pages/ProfilesPage.xaml.cs (2)
139-145: The use of
ScopedLockonupdateLockis good for thread safety when updating shared resources. This ensures that the TDP (Thermal Design Power) settings are not concurrently modified, which could lead to inconsistent states. However, it's important to verify thatupdateLockis properly released in all scenarios to avoid deadlocks.403-419: The code correctly sets the minimum and maximum values for the TDP sliders based on the device's capabilities. It's important to ensure that
SettingsManager.GetIntis a thread-safe operation since it's being called from within the UI thread's context. If it involves shared resources or complex logic, consider caching the values or ensuring thread safety within theSettingsManager.HandheldCompanion/Views/QuickPages/QuickProfilesPage.xaml.cs (6)
11-14: The addition of the
Microsoft.Win32namespace is appropriate for handling system events related to power mode changes. This is a necessary import for the functionality being implemented.271-285: The conditional adjustments to
TDPSliderandTDPOnBatterySlidervalues based on power status and profile settings are a key part of the feature. This logic ensures that the TDP settings are adjusted correctly when the power mode changes. The use ofPluggedInStatusto determine which slider to adjust is correct and aligns with the feature's requirements.308-314: The use of
ScopedLockonupdateLockis good for thread safety, ensuring that the UI updates do not conflict with each other. This is important when dealing with asynchronous operations that may affect shared resources.391-403: The initialization of
currentProfile.TDPOnBatteryValuesin theCreateProfilefunction is correct and necessary for the new feature. This ensures that the TDP settings for battery mode are set up when a new profile is created.499-505: The code correctly initializes the
TDPOnBatteryValuesfor the new profile based on the current device's normal TDP (nTDP). This is consistent with the initialization ofTDPOverrideValuesand is necessary for the new feature to work correctly.601-631: The event handlers for
TDPOnBatteryToggleandTDPOnBatterySliderare implemented correctly. They update thecurrentProfilewith the new TDP settings when the user interacts with the UI elements. This is a crucial part of the feature, ensuring that user preferences are captured and applied.
| // power | ||
| public bool TDPOverrideEnabled { get; set; } | ||
| public double[] TDPOverrideValues { get; set; } | ||
| public bool TDPOnBatteryEnabled { get; set; } | ||
| public double[] TDPOnBatteryValues { get; set; } |
There was a problem hiding this comment.
The addition of TDPOnBatteryEnabled and TDPOnBatteryValues properties is consistent with the feature being implemented. However, ensure that these properties are properly serialized and deserialized when the Profile objects are saved and loaded. Also, consider initializing TDPOverrideValues and TDPOnBatteryValues to prevent null reference exceptions when they are accessed before being set.
+ public double[] TDPOverrideValues { get; set; } = new double[0];
+ public double[] TDPOnBatteryValues { get; set; } = new double[0];Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // power | |
| public bool TDPOverrideEnabled { get; set; } | |
| public double[] TDPOverrideValues { get; set; } | |
| public bool TDPOnBatteryEnabled { get; set; } | |
| public double[] TDPOnBatteryValues { get; set; } | |
| // power | |
| public bool TDPOverrideEnabled { get; set; } | |
| public double[] TDPOverrideValues { get; set; } = new double[0]; | |
| public bool TDPOnBatteryEnabled { get; set; } | |
| public double[] TDPOnBatteryValues { get; set; } = new double[0]; |
| RequestUpdate(); | ||
| } | ||
|
|
||
| private void TDPOnBatteryToggle_Toggled(object sender, RoutedEventArgs e) | ||
| { | ||
| // wait until lock is released | ||
| if (updateLock) | ||
| return; | ||
|
|
||
| selectedProfile.TDPOnBatteryEnabled = TDPOnBatteryToggle.IsOn; | ||
| RequestUpdate(); | ||
| } | ||
|
|
||
| private void TDPOnBatterySlider_ValueChanged(object sender, RoutedPropertyChangedEventArgs<double> e) | ||
| { | ||
| if (!TDPOnBatterySlider.IsInitialized) | ||
| return; | ||
|
|
||
| // wait until lock is released | ||
| if (updateLock) | ||
| return; | ||
|
|
||
| selectedProfile.TDPOnBatteryValues = new double[3] | ||
| { | ||
| (int)TDPOnBatterySlider.Value, | ||
| (int)TDPOnBatterySlider.Value, | ||
| (int)TDPOnBatterySlider.Value | ||
| }; | ||
| RequestUpdate(); | ||
| } |
There was a problem hiding this comment.
The RequestUpdate method is called after setting the TDP values, which is good for ensuring that changes are queued for submission. However, the cast to (int) may not be necessary if the slider values are already double. If the intention is to store integers, it's fine, but if the underlying TDP settings can accept double values, consider removing the cast to avoid unnecessary precision loss.
- (int)TDPSlider.Value,
- (int)TDPSlider.Value,
- (int)TDPSlider.Value
+ TDPSlider.Value,
+ TDPSlider.Value,
+ TDPSlider.ValueCommitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| RequestUpdate(); | |
| } | |
| private void TDPOnBatteryToggle_Toggled(object sender, RoutedEventArgs e) | |
| { | |
| // wait until lock is released | |
| if (updateLock) | |
| return; | |
| selectedProfile.TDPOnBatteryEnabled = TDPOnBatteryToggle.IsOn; | |
| RequestUpdate(); | |
| } | |
| private void TDPOnBatterySlider_ValueChanged(object sender, RoutedPropertyChangedEventArgs<double> e) | |
| { | |
| if (!TDPOnBatterySlider.IsInitialized) | |
| return; | |
| // wait until lock is released | |
| if (updateLock) | |
| return; | |
| selectedProfile.TDPOnBatteryValues = new double[3] | |
| { | |
| (int)TDPOnBatterySlider.Value, | |
| (int)TDPOnBatterySlider.Value, | |
| (int)TDPOnBatterySlider.Value | |
| }; | |
| RequestUpdate(); | |
| } | |
| private void TDPOnBatterySlider_ValueChanged(object sender, RoutedPropertyChangedEventArgs<double> e) | |
| { | |
| if (!TDPOnBatterySlider.IsInitialized) | |
| return; | |
| // wait until lock is released | |
| if (updateLock) | |
| return; | |
| selectedProfile.TDPOnBatteryValues = new double[3] | |
| { | |
| TDPOnBatterySlider.Value, | |
| TDPOnBatterySlider.Value, | |
| TDPOnBatterySlider.Value | |
| }; | |
| RequestUpdate(); | |
| } |
| private void HotkeysManager_CommandExecuted(string listener) | ||
| { | ||
| // UI thread (async) | ||
| bool PluggedInStatus = System.Windows.Forms.SystemInformation.PowerStatus.PowerLineStatus == System.Windows.Forms.PowerLineStatus.Online; |
There was a problem hiding this comment.
The variable PluggedInStatus is used to check the power line status, which is essential for the feature being implemented. However, it's important to note that System.Windows.Forms.SystemInformation.PowerStatus is a part of the Windows Forms namespace, which is generally not used in WPF applications. If the application is primarily WPF, it would be better to use a more consistent approach for checking power status that aligns with WPF conventions.
// Instead of using System.Windows.Forms, consider using System.Management to query the power status
bool PluggedInStatus = new System.Management.ManagementObjectSearcher("root\\CIMV2",
"SELECT * FROM Win32_Battery").Get().Count > 0;
|
Added UI components |
|
UI when disabled/turn off |
|
Maybe align the translations better to the existing TDP limit option. |
|
Hey thank you very much for your involvement here. To be fair I'd suggest we implement support for two separate power profiles based on plugged or unplugged scenario. What do you think? |
Do you mean the Power Profiles of windows, those Balanced & Performance? Isn't that part windows handle? |
|
Yes, when I first thought about this feature, my plan was to have two power profiles associated to each profile. A plugged and unplugged one. |
So what you plan is Profiles > Plug & Unplug Profile > each has App-specific profiles under? And what I did is Profiles > App-specific Profiles contain Plug & Unplug TDP slider? I did try to did that first but need to change quite alot more than this one as It needs to change upper code and may messing up UI :D |
|
But if you already have plan the job and that has additional feature or usecase better then just ignore this one and close this PR as I think it may conflict your work plan. I'm happy with that |
|
I'm pretty sure what Valkirie meant is that he had the initial idea as he explained. It's not an "actual" plan, it's an idea that you could use to reshape your PR so it could potentially be integrated. |
Ohh, I think I can deal with it, just need more details from him |
|
Glad you're on board. I'm still ensure what's the cleanest UI approach but from a backoffice perspective I'd just make it so a profile has two power profiles bounds to it within a dictionary maybe. Key could be a bool (plugged, unplugged) and value would be PowerProfile. Either we duplicate the power profile selection UI and have one named plugged and the other unplugged. Either we need to think of something else.. |
|
Ok so you meant we should have all those profile settings like (CPU Count, TDP, AutoTDP, EPP, GPU Clock) should all separated between Plug & Unplug. The issue I can first see is we make it's nest 1 deeper dictionary but then it's bad for backward compatibility. The other option is having additional OnBattery for each settings in dictionary and it'll be used by a Toggle UseOnBattery settings (Placed above "Use per-game profile" toggle) and when On Battery just like I did with TDP but it adds up a lot more code than the nest deeper dictionary. What's your opinion? As it's your app I don't want to messing it up lol Regarding UI, I think adding a Tab under "Power settings" Expandable Panel but I could not find any Tab in your app that I can reuse (I'm bad at UI) |
|
@quangmach the layout editor has tabs, we can even add an icon with a power plug and a battery. See below. As for my thoughts on this feature. I think we should jut have two tabs that both have the same list of options. What I'm not sure about is how to handle the default state so to speak (not the Default profile). Do we want users to have to configure both tabs of settings, or if the plugged in settings are disabled, use the battery one or is it really disabled and what if you just want everything the same as "the other" tab?
|
|
My bad, used the Github mobile app to review something and ended up merging this. Reverted it. |
I think if the Togggle "Use On Battery different settings" On, it will show up the same panel for battery settings. Otherwise it will be the same as currently. User can choose to set & apply it for global or per-game like now too. I will try to utilise thay tab, draft it and show how it might look in here |
I add an option to use seperated TDP when device power on Battery Mode. I tested and it works nicely on my device, reusing all current UI component, UI still clean
Summary by CodeRabbit
New Features
Enhancements
Documentation
Bug Fixes