From 0314a709f507063de746ad579cfe657d307828f1 Mon Sep 17 00:00:00 2001 From: Shawn Yuan <128874481+shuaiyuanxx@users.noreply.github.com> Date: Fri, 9 Jan 2026 13:06:50 +0800 Subject: [PATCH] Optimize the module list logic (#44628) ## Summary of the Pull Request This pull request refactors how module enable/disable state changes are handled in the settings UI, centralizing and simplifying the logic for updating and notifying these changes. The main improvements include unifying the callback mechanism for enable state changes, reducing code duplication, and making the update process more robust against circular updates. **Refactoring and Simplification of Enable State Handling:** * Introduced a unified `EnabledChangedCallback` and `UpdateStatus` method in the base `ModuleListItem` class to handle enable/disable state changes and notifications, replacing redundant logic in derived classes like `FlyoutMenuItem` and `DashboardListItem`. [[1]](diffhunk://#diff-23ab2cc13a1098a6071b3e12ce0919b7eba451d7683f6f62e5ec2cf661778a4cR21-R36) [[2]](diffhunk://#diff-23ab2cc13a1098a6071b3e12ce0919b7eba451d7683f6f62e5ec2cf661778a4cR99-R103) [[3]](diffhunk://#diff-5033dabc0e3ec7d01509b9d58878b9ee5745378d5e3a7fa92779bf9c111bcffcL25-L39) [[4]](diffhunk://#diff-9c93f68ee87a48d8affd140224601da95d7fe9642ad24350c7527d0f5773ec7dL29-L43) * Updated all usages to call `UpdateStatus` instead of directly setting `IsEnabled`, ensuring callbacks are managed consistently and preventing unnecessary notifications during programmatic updates. [[1]](diffhunk://#diff-734ba1b4b3044eb540bba08334bd141c968a113625be2d92c831f3cc3debc62fL108-R109) [[2]](diffhunk://#diff-aea3404667e7a3de2750bf9ab7ee8ff5e717892caa68ee1de86713cf8e21b44cL261-R260) **Improvements to Callback and Update Logic:** * Changed the signature of UI event handlers to use the base `ModuleListItem` type, improving code maintainability and reducing casting and duplication. [[1]](diffhunk://#diff-734ba1b4b3044eb540bba08334bd141c968a113625be2d92c831f3cc3debc62fL160-R161) [[2]](diffhunk://#diff-aea3404667e7a3de2750bf9ab7ee8ff5e717892caa68ee1de86713cf8e21b44cL278-R287) * Removed obsolete or redundant flags and logic for tracking update state, further simplifying the codebase. [[1]](diffhunk://#diff-aea3404667e7a3de2750bf9ab7ee8ff5e717892caa68ee1de86713cf8e21b44cL53) [[2]](diffhunk://#diff-aea3404667e7a3de2750bf9ab7ee8ff5e717892caa68ee1de86713cf8e21b44cL328) [[3]](diffhunk://#diff-aea3404667e7a3de2750bf9ab7ee8ff5e717892caa68ee1de86713cf8e21b44cL343-L346) **Project and Namespace Adjustments:** * Updated the project file to include all localized resource files recursively, improving localization support. * Added a missing namespace import to ensure proper type resolution. * Added a missing `using System;` directive for the `Action` delegate. ## PR Checklist - [ ] Closes: #xxx - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [ ] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx ## Detailed Description of the Pull Request / Additional comments ## Validation Steps Performed --- .../ViewModels/AllAppsViewModel.cs | 86 +++++++++---------- .../ViewModels/FlyoutMenuItem.cs | 15 ---- .../ModuleList/ModuleListItem.cs | 22 +++++ .../ViewModels/DashboardListItem.cs | 15 ---- .../ViewModels/DashboardViewModel.cs | 20 ++--- 5 files changed, 67 insertions(+), 91 deletions(-) diff --git a/src/settings-ui/QuickAccess.UI/ViewModels/AllAppsViewModel.cs b/src/settings-ui/QuickAccess.UI/ViewModels/AllAppsViewModel.cs index 198deff34f..0461c3e59e 100644 --- a/src/settings-ui/QuickAccess.UI/ViewModels/AllAppsViewModel.cs +++ b/src/settings-ui/QuickAccess.UI/ViewModels/AllAppsViewModel.cs @@ -10,6 +10,7 @@ using global::PowerToys.GPOWrapper; using ManagedCommon; using Microsoft.PowerToys.QuickAccess.Helpers; using Microsoft.PowerToys.QuickAccess.Services; +using Microsoft.PowerToys.Settings.UI.Controls; using Microsoft.PowerToys.Settings.UI.Library; using Microsoft.PowerToys.Settings.UI.Library.Helpers; using Microsoft.PowerToys.Settings.UI.Library.Interfaces; @@ -26,6 +27,7 @@ public sealed class AllAppsViewModel : Observable private readonly SettingsUtils _settingsUtils; private readonly ResourceLoader _resourceLoader; private readonly DispatcherQueue _dispatcherQueue; + private readonly List _allFlyoutMenuItems = new(); private GeneralSettings _generalSettings; public ObservableCollection FlyoutMenuItems { get; } @@ -58,9 +60,28 @@ public sealed class AllAppsViewModel : Observable _resourceLoader = Helpers.ResourceLoaderInstance.ResourceLoader; FlyoutMenuItems = new ObservableCollection(); + BuildFlyoutMenuItems(); RefreshFlyoutMenuItems(); } + private void BuildFlyoutMenuItems() + { + _allFlyoutMenuItems.Clear(); + foreach (ModuleType moduleType in Enum.GetValues()) + { + if (moduleType == ModuleType.GeneralSettings) + { + continue; + } + + _allFlyoutMenuItems.Add(new FlyoutMenuItem + { + Tag = moduleType, + EnabledChangedCallback = EnabledChangedOnUI, + }); + } + } + private void OnSettingsChanged(GeneralSettings newSettings) { _dispatcherQueue.TryEnqueue(() => @@ -82,63 +103,37 @@ public sealed class AllAppsViewModel : Observable private void RefreshFlyoutMenuItems() { - var desiredItems = new List(); - - foreach (ModuleType moduleType in Enum.GetValues()) + foreach (var item in _allFlyoutMenuItems) { - if (moduleType == ModuleType.GeneralSettings) - { - continue; - } - + var moduleType = item.Tag; var gpo = Helpers.ModuleGpoHelper.GetModuleGpoConfiguration(moduleType); var isLocked = gpo is GpoRuleConfigured.Enabled or GpoRuleConfigured.Disabled; var isEnabled = gpo == GpoRuleConfigured.Enabled || (!isLocked && Microsoft.PowerToys.Settings.UI.Library.Helpers.ModuleHelper.GetIsModuleEnabled(_generalSettings, moduleType)); - var existingItem = FlyoutMenuItems.FirstOrDefault(x => x.Tag == moduleType); + item.Label = _resourceLoader.GetString(Microsoft.PowerToys.Settings.UI.Library.Helpers.ModuleHelper.GetModuleLabelResourceName(moduleType)); + item.IsLocked = isLocked; + item.Icon = Microsoft.PowerToys.Settings.UI.Library.Helpers.ModuleHelper.GetModuleTypeFluentIconName(moduleType); - if (existingItem != null) + if (item.IsEnabled != isEnabled) { - existingItem.Label = _resourceLoader.GetString(Microsoft.PowerToys.Settings.UI.Library.Helpers.ModuleHelper.GetModuleLabelResourceName(moduleType)); - existingItem.IsLocked = isLocked; - existingItem.Icon = Microsoft.PowerToys.Settings.UI.Library.Helpers.ModuleHelper.GetModuleTypeFluentIconName(moduleType); - - if (existingItem.IsEnabled != isEnabled) - { - var callback = existingItem.EnabledChangedCallback; - existingItem.EnabledChangedCallback = null; - existingItem.IsEnabled = isEnabled; - existingItem.EnabledChangedCallback = callback; - } - - desiredItems.Add(existingItem); - } - else - { - desiredItems.Add(new FlyoutMenuItem - { - Label = _resourceLoader.GetString(Microsoft.PowerToys.Settings.UI.Library.Helpers.ModuleHelper.GetModuleLabelResourceName(moduleType)), - IsEnabled = isEnabled, - IsLocked = isLocked, - Tag = moduleType, - Icon = Microsoft.PowerToys.Settings.UI.Library.Helpers.ModuleHelper.GetModuleTypeFluentIconName(moduleType), - EnabledChangedCallback = EnabledChangedOnUI, - }); + item.UpdateStatus(isEnabled); } } var sortedItems = DashboardSortOrder switch { - DashboardSortOrder.ByStatus => desiredItems.OrderByDescending(x => x.IsEnabled).ThenBy(x => x.Label).ToList(), - _ => desiredItems.OrderBy(x => x.Label).ToList(), + DashboardSortOrder.ByStatus => _allFlyoutMenuItems.OrderByDescending(x => x.IsEnabled).ThenBy(x => x.Label).ToList(), + _ => _allFlyoutMenuItems.OrderBy(x => x.Label).ToList(), }; - for (int i = FlyoutMenuItems.Count - 1; i >= 0; i--) + if (FlyoutMenuItems.Count == 0) { - if (!sortedItems.Contains(FlyoutMenuItems[i])) + foreach (var item in sortedItems) { - FlyoutMenuItems.RemoveAt(i); + FlyoutMenuItems.Add(item); } + + return; } for (int i = 0; i < sortedItems.Count; i++) @@ -146,20 +141,17 @@ public sealed class AllAppsViewModel : Observable var item = sortedItems[i]; var oldIndex = FlyoutMenuItems.IndexOf(item); - if (oldIndex < 0) - { - FlyoutMenuItems.Insert(i, item); - } - else if (oldIndex != i) + if (oldIndex != -1 && oldIndex != i) { FlyoutMenuItems.Move(oldIndex, i); } } } - private void EnabledChangedOnUI(FlyoutMenuItem item) + private void EnabledChangedOnUI(ModuleListItem item) { - if (_coordinator.UpdateModuleEnabled(item.Tag, item.IsEnabled)) + var flyoutItem = (FlyoutMenuItem)item; + if (_coordinator.UpdateModuleEnabled(flyoutItem.Tag, flyoutItem.IsEnabled)) { _coordinator.NotifyUserSettingsInteraction(); } diff --git a/src/settings-ui/QuickAccess.UI/ViewModels/FlyoutMenuItem.cs b/src/settings-ui/QuickAccess.UI/ViewModels/FlyoutMenuItem.cs index 32243ceed0..b0ce5a9512 100644 --- a/src/settings-ui/QuickAccess.UI/ViewModels/FlyoutMenuItem.cs +++ b/src/settings-ui/QuickAccess.UI/ViewModels/FlyoutMenuItem.cs @@ -22,21 +22,6 @@ public sealed class FlyoutMenuItem : ModuleListItem set => base.Tag = value; } - public override bool IsEnabled - { - get => base.IsEnabled; - set - { - if (base.IsEnabled != value) - { - base.IsEnabled = value; - EnabledChangedCallback?.Invoke(this); - } - } - } - - public Action? EnabledChangedCallback { get; set; } - public bool Visible { get => _visible; diff --git a/src/settings-ui/Settings.UI.Controls/ModuleList/ModuleListItem.cs b/src/settings-ui/Settings.UI.Controls/ModuleList/ModuleListItem.cs index ef92e3e592..3b6bf6a0bb 100644 --- a/src/settings-ui/Settings.UI.Controls/ModuleList/ModuleListItem.cs +++ b/src/settings-ui/Settings.UI.Controls/ModuleList/ModuleListItem.cs @@ -2,6 +2,7 @@ // The Microsoft Corporation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System; // For Action using System.ComponentModel; using System.Runtime.CompilerServices; using System.Windows.Input; @@ -17,6 +18,22 @@ namespace Microsoft.PowerToys.Settings.UI.Controls private bool _isLocked; private object? _tag; private ICommand? _clickCommand; + private bool _isUpdating; + + public Action? EnabledChangedCallback { get; set; } + + public void UpdateStatus(bool isEnabled) + { + _isUpdating = true; + try + { + IsEnabled = isEnabled; + } + finally + { + _isUpdating = false; + } + } public virtual string Label { @@ -79,6 +96,11 @@ namespace Microsoft.PowerToys.Settings.UI.Controls { _isEnabled = value; OnPropertyChanged(); + + if (!_isUpdating) + { + EnabledChangedCallback?.Invoke(this); + } } } } diff --git a/src/settings-ui/Settings.UI/ViewModels/DashboardListItem.cs b/src/settings-ui/Settings.UI/ViewModels/DashboardListItem.cs index c292ec5011..aad7b9536b 100644 --- a/src/settings-ui/Settings.UI/ViewModels/DashboardListItem.cs +++ b/src/settings-ui/Settings.UI/ViewModels/DashboardListItem.cs @@ -26,21 +26,6 @@ namespace Microsoft.PowerToys.Settings.UI.ViewModels set => base.Tag = value; } - public Action EnabledChangedCallback { get; set; } - - public override bool IsEnabled - { - get => base.IsEnabled; - set - { - if (base.IsEnabled != value) - { - base.IsEnabled = value; - EnabledChangedCallback?.Invoke(this); - } - } - } - public bool Visible { get => _visible; diff --git a/src/settings-ui/Settings.UI/ViewModels/DashboardViewModel.cs b/src/settings-ui/Settings.UI/ViewModels/DashboardViewModel.cs index 3c35e9279d..ed0babe364 100644 --- a/src/settings-ui/Settings.UI/ViewModels/DashboardViewModel.cs +++ b/src/settings-ui/Settings.UI/ViewModels/DashboardViewModel.cs @@ -50,7 +50,6 @@ namespace Microsoft.PowerToys.Settings.UI.ViewModels // Flag to prevent circular updates when a UI toggle triggers settings changes. private bool _isUpdatingFromUI; - private bool _isUpdatingFromSettings; private AllHotkeyConflictsData _allHotkeyConflictsData = new AllHotkeyConflictsData(); @@ -258,7 +257,7 @@ namespace Microsoft.PowerToys.Settings.UI.ViewModels // Only update if there's an actual change to minimize UI notifications. if (item.IsEnabled != newEnabledState) { - item.IsEnabled = newEnabledState; + item.UpdateStatus(newEnabledState); } if (item.IsLocked != newLockedState) @@ -275,19 +274,17 @@ namespace Microsoft.PowerToys.Settings.UI.ViewModels /// Sets the _isUpdatingFromUI flag to prevent circular updates, then updates /// settings, re-sorts if needed, and refreshes dependent collections. /// - private void EnabledChangedOnUI(DashboardListItem dashboardListItem) + private void EnabledChangedOnUI(ModuleListItem item) { - if (_isUpdatingFromSettings) - { - return; - } + var dashboardListItem = (DashboardListItem)item; + var isEnabled = dashboardListItem.IsEnabled; _isUpdatingFromUI = true; try { - Views.ShellPage.UpdateGeneralSettingsCallback(dashboardListItem.Tag, dashboardListItem.IsEnabled); + Views.ShellPage.UpdateGeneralSettingsCallback(dashboardListItem.Tag, isEnabled); - if (dashboardListItem.Tag == ModuleType.NewPlus && dashboardListItem.IsEnabled == true) + if (dashboardListItem.Tag == ModuleType.NewPlus && isEnabled == true) { var settingsUtils = SettingsUtils.Default; var settings = NewPlusViewModel.LoadSettings(settingsUtils); @@ -325,7 +322,6 @@ namespace Microsoft.PowerToys.Settings.UI.ViewModels return; } - _isUpdatingFromSettings = true; try { RefreshModuleList(); @@ -340,10 +336,6 @@ namespace Microsoft.PowerToys.Settings.UI.ViewModels { Logger.LogError($"Updating active/disabled modules list failed: {ex.Message}"); } - finally - { - _isUpdatingFromSettings = false; - } } ///