Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redundant ICommand and INotifyPropertyChanged implementations #8146

Closed
davidegiacometti opened this issue Nov 19, 2020 · 9 comments
Closed
Labels
Area-Quality Stability, Performance, Etc. Help Wanted We encourage anyone to jump in on these and submit a PR. Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.

Comments

@davidegiacometti
Copy link
Collaborator

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
@ghost ghost added the Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams label Nov 19, 2020
@microsoft microsoft deleted a comment from Ziyadsho Nov 19, 2020
@crutkas
Copy link
Member

crutkas commented Nov 19, 2020

Agreed this would be a good thing to unify.

@crutkas crutkas added Area-Quality Stability, Performance, Etc. and removed Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams labels Nov 19, 2020
@crutkas
Copy link
Member

crutkas commented Nov 19, 2020

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

@crutkas crutkas added the Help Wanted We encourage anyone to jump in on these and submit a PR. label Nov 19, 2020
@enricogior
Copy link
Contributor

@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
Copy link
Member

crutkas commented Nov 20, 2020

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
Copy link
Contributor

enricogior commented Nov 20, 2020

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
Copy link
Member

crutkas commented Nov 20, 2020

i don't think this is urgent

@microsoft microsoft deleted a comment from Ziyadsho Nov 20, 2020
@davidegiacometti
Copy link
Collaborator Author

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

@crutkas
Copy link
Member

crutkas commented Jan 4, 2021

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

@crutkas
Copy link
Member

crutkas commented Jan 4, 2021

closing in favor of #1083

@crutkas crutkas closed this as completed Jan 4, 2021
@crutkas crutkas added the Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. label Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Quality Stability, Performance, Etc. Help Wanted We encourage anyone to jump in on these and submit a PR. Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.
Projects
Status: No status
Development

No branches or pull requests

3 participants