microsoft / PowerToys

Windows system utilities to maximize productivity
MIT License
109.89k stars 6.48k forks source link

Redundant ICommand and INotifyPropertyChanged implementations #8146

Closed davidegiacometti closed 3 years ago

davidegiacometti commented 3 years ago

ICommand and INotifyPropertyChanged are implemented a lot of time in different mode. I think where possible would be nice to have a single implementation shared across the WPF projects: maybe in ManagedCommon.

src\modules\fancyzones\editor\FancyZonesEditor\Utils\RelayCommand.cs(10):    public class RelayCommand : ICommand
src\modules\fancyzones\editor\FancyZonesEditor\Utils\RelayCommand`1.cs(10):    public class RelayCommand<T> : ICommand
src\modules\imageresizer\ui\Helpers\RelayCommand.cs(10):    public class RelayCommand : ICommand
src\modules\imageresizer\ui\Helpers\RelayCommand.cs(36):    public class RelayCommand<T> : ICommand
src\modules\launcher\PowerLauncher\ViewModel\RelayCommand.cs(10):    public class RelayCommand : ICommand
src\core\Microsoft.PowerToys.Settings.UI\Helpers\RelayCommand.cs(10):    public class RelayCommand : ICommand
src\core\Microsoft.PowerToys.Settings.UI\Helpers\RelayCommand.cs(36):    public class RelayCommand<T> : ICommand
src\core\Microsoft.PowerToys.Settings.UI\ViewModels\Commands\ButtonClickCommand.cs(10):    public class ButtonClickCommand : ICommand
src\core\Microsoft.PowerToys.Settings.UI.Library\ViewModels\Commands\ButtonClickCommand.cs(10):    public class ButtonClickCommand : ICommand
src\core\Microsoft.PowerToys.Settings.UI.Library\ViewModels\Commands\RelayCommand.cs(10):    public class RelayCommand : ICommand
src\core\Microsoft.PowerToys.Settings.UI.Library\ViewModels\Commands\RelayCommand`1.cs(10):    public class RelayCommand<T> : ICommand
src\modules\colorPicker\ColorPickerUI\Common\RelayCommand.cs(10):    public class RelayCommand : ICommand
src\modules\fancyzones\editor\FancyZonesEditor\Models\LayoutModel.cs(16):    public abstract class LayoutModel : INotifyPropertyChanged
src\modules\fancyzones\editor\FancyZonesEditor\Models\MainWindowSettingsModel.cs(18):    public class MainWindowSettingsModel : INotifyPropertyChanged
src\modules\fancyzones\editor\FancyZonesEditor\Models\MonitorInfoModel.cs(10):    public class MonitorInfoModel : INotifyPropertyChanged
src\modules\fancyzones\editor\FancyZonesEditor\ViewModels\MonitorViewModel.cs(12):    public class MonitorViewModel : INotifyPropertyChanged
src\modules\imageresizer\ui\Helpers\Observable.cs(10):    public class Observable : INotifyPropertyChanged
src\modules\launcher\Wox.Plugin\BaseModel.cs(11):    public class BaseModel : INotifyPropertyChanged
src\core\Microsoft.PowerToys.Settings.UI\Helpers\Observable.cs(10):    public class Observable : INotifyPropertyChanged
src\core\Microsoft.PowerToys.Settings.UI.Library\Helpers\Observable.cs(10):    public class Observable : INotifyPropertyChanged
src\core\Microsoft.PowerToys.Settings.UI.Library\ImageSize.cs(13):    public class ImageSize : INotifyPropertyChanged
src\modules\colorPicker\ColorPickerUI\Common\ViewModelBase.cs(13):    public abstract class ViewModelBase : INotifyPropertyChanged
src\modules\colorPicker\ColorPickerUI\Settings\SettingItem`1.cs(9):    public sealed class SettingItem<T> : INotifyPropertyChanged
crutkas commented 3 years ago

Agreed this would be a good thing to unify.

crutkas commented 3 years ago

I think managed common is a wrapper for common. Maybe a new lib would be smarter. .net standard 2.0 since some of these are .net core, others are .net framework 4.7.2 still

enricogior commented 3 years ago

@crutkas

Maybe a new lib would be smarter. .net standard 2.0 since some of these are .net core, others are .net framework 4.7.2 still

What about continuing the effort to move to .NET Core and not start a new common lib with the .Net Standard 2.0 limitations?

crutkas commented 3 years ago

I'm seeing FancyZones and Color Picker in here which are on framework. Until those get migrated to .net core, we have to do Standard 2.0.

enricogior commented 3 years ago

FZ Editor will be moved to .NET Core next week. Having a common implementation for ICommand and INotifyPropertyChanged is a nice thing but is not urgent, if we introduce a new common lib it would be better to not limit it to .Net Standard 2.0. The project that can start using will adopt it, the project that are still on .Net framework will wait until they are moved to .NET Core.

crutkas commented 3 years ago

i don't think this is urgent

davidegiacometti commented 3 years ago

If we want to use Windows Community Toolkit I think we can close this in favour of #1083

crutkas commented 3 years ago

i'm fine with that but to consolidate / migrate, it is a large effort.

crutkas commented 3 years ago

closing in favor of #1083