Fix 80% battery charge limit for Legion Go resetting #1075
Conversation
not needed, SettingsManager will iterate through all settings on startup anyway
WalkthroughA new case Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
moyogii
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
HandheldCompanion/Views/Pages/DevicePage.xaml.cs (2)
Line range hint
157-243: Consider adding error handling for type conversions inSettingsManager_SettingValueChanged. Currently, if a conversion fails (e.g.,Convert.ToBoolean(value)wherevalueis not a valid boolean), it could throw an exception and crash the application.+ try + { switch (name) { case "ConfigurableTDPOverride": Toggle_cTDP.IsOn = Convert.ToBoolean(value); break; // other cases + } + catch (Exception ex) + { + // Log error or handle exception appropriately + }
Line range hint
36-36: Ensure that event subscriptions in the constructor are paired with unsubscriptions to prevent memory leaks. Consider adding these unsubscriptions in an appropriate method likePage_ClosedorDispose.+ protected override void OnClosed(EventArgs e) + { + base.OnClosed(e); + SettingsManager.SettingValueChanged -= SettingsManager_SettingValueChanged; + MainWindow.uiSettings.ColorValuesChanged -= OnColorValuesChanged; + ControllerManager.ControllerSelected -= ControllerManager_ControllerSelected; + DeviceManager.UsbDeviceArrived -= GenericDeviceUpdated; + DeviceManager.UsbDeviceRemoved -= GenericDeviceUpdated; + }Also applies to: 44-44
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- HandheldCompanion/Views/Pages/DevicePage.xaml.cs (1 hunks)
Additional comments not posted (2)
HandheldCompanion/Views/Pages/DevicePage.xaml.cs (2)
168-170: The addition of the case for "LegionBatteryChargeLimit" inSettingsManager_SettingValueChangedmethod is correctly implemented to update the toggle based on the setting value.
Line range hint
157-243: Good use ofIsLoadedchecks to prevent operations before the page is fully loaded. This is a good practice to ensure that UI elements are ready before they are manipulated.
* Add 80% battery charge limit for Legion Go * removed UpdateSettings() not needed, SettingsManager will iterate through all settings on startup anyway * Fix Legion Go battery charge limit toggle not saving --------- Co-authored-by: Plasmid <[email protected]> Co-authored-by: Lesueur Benjamin <[email protected]>
This MR fixes a bug reported where if you toggled the 80% battery charge limit for the Legion Go it would reset back to off after restarting HandheldCompanion.
Summary by CodeRabbit