Skip to content

Code Quality: Reorganze namespaces #11552

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

Closed
wants to merge 12 commits into from

Conversation

0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Mar 4, 2023

Description

Reorganize namespaces. For more information, see Details of Changes

Motivation and Context

I found that there're a lot of namespaces that need to be reorganized and there're namespaces that do not need to be separated and I merged them into the same.

Details of Changes

Previous New Rationale
Files.App.Controllers.IJson Remove Unused
Files.App.ValueConverters Files.App.Converters Same thing
Files.App.AppModels Files.App.DataModels Same thing
Files.App.CommandLine Files.Backend.CommandLine Be able to be moved to Backend
UserControls/Pane/PreviewPane UserControls/PreviewPane No need to be another folder
UserControls' ViewModels(Files.App.ViewModels.*) Files.App.ViewModels.UserControls Organized

Validation

  • Built and ran the app
  • Tested the changes for accessibility

@0x5bfa 0x5bfa marked this pull request as ready for review March 5, 2023 04:23
@0x5bfa
Copy link
Member Author

0x5bfa commented Mar 5, 2023

Ready to review :)

Copy link
Contributor

@QuaintMako QuaintMako left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@QuaintMako
Copy link
Contributor

@onein528 could you look into the conflicts?

QuaintMako
QuaintMako previously approved these changes Mar 8, 2023
@yaira2
Copy link
Member

yaira2 commented Mar 8, 2023

@d2dyno1 are you okay with these changes?

@@ -264,12 +264,12 @@ public DynamicDialogButtons DynamicButtonsEnabled
/// </summary>
public Action HideDialog { get; set; }

private Action<DynamicDialogViewModel, ContentDialogButtonClickEventArgs> primaryButtonAction;
private Action<DynamicViewModel, ContentDialogButtonClickEventArgs> primaryButtonAction;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be confusing when using multiple namespaces

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As well not include Settings, Properties, Page like AboutSettingsViewModel. This is not accepted(AboutViewModel will be okay).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of this approach. While I'd agree to shorten longer class names, in this example, the full name is actually necessary.

Someone looking at the code might question themselves: "DynamicViewModel of what? How is it dynamic? Oh, it's for a dialog..."

Copy link
Member Author

@0x5bfa 0x5bfa Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then how about Settings.AboutViewModel? I think we should avoid duplication because you can hover on them and look for where it from and what it represents if they have the XAML comment. That's why I changed Properties.PropertiesGeneral to Properties.GeneralPage(although Page is necessary, they didn't have it) and I added ViewModels.UserControls.* instead of putting directly UserControl's ViewModel into ViewModels.*

@0x5bfa 0x5bfa closed this Mar 26, 2023
@0x5bfa 0x5bfa deleted the 5bfa/organize-viewmodels branch March 26, 2023 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants