-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Feature: Added support for bulk rename #16228
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code quality issues
!string.IsNullOrEmpty(fileName) && !IsNameValid; | ||
|
||
|
||
private string fileName = string.Empty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private string fileName = string.Empty; | |
private string _FileName = string.Empty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the '_' prefix against the code style rules for C#? I think it's IDE1006
https://learn.microsoft.com/en-gb/dotnet/fundamentals/code-analysis/style-rules/naming-rules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a private field. Using PascalCase is guided by our coding guideline, Section 2, Number 2.
Use PascalCase prefixed with _ for properties’ backing fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I forgot why that seemed so familiar...
} | ||
|
||
// Commands | ||
public IAsyncRelayCommand CommitRenameCommand { get; private set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public IAsyncRelayCommand CommitRenameCommand { get; private set; } | |
public IAsyncRelayCommand CommitRenameCommand { get; private set; } |
false | ||
); | ||
})); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private FrameworkElement RootAppElement | ||
=> (FrameworkElement)MainWindow.Instance.Content; | ||
|
||
public BulkRenameDialogViewModel ViewModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been a bad practice and I was going to delete.
Can you use binding instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that works for IDialog
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions, probably all wrong...
InitializeComponent(); | ||
} | ||
|
||
public new async Task<DialogResult> ShowAsync() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be public override async
? Or is this intended?
|
||
<StackPanel Width="440" Spacing="4"> | ||
|
||
<!-- Name --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<!-- Name -->
What does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment for the block named "Name" in the Bulk Rename dialog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will you add error handling? Or is it automatic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be handled by RenameAsync
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering code style in other files, here are some more advisories.
mc:Ignorable="d"> | ||
|
||
<StackPanel Width="440" Spacing="4"> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already suggested changes for all of them here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are different changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a preference to have spaces between xaml blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I know, this is just not the same as in other files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style has evolved over the years
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are different changes.
I misunderstood, sorry.
</TextBox> | ||
|
||
</Grid> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
} | ||
|
||
// Commands |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Commands | |
// Commands | |
Resolved / Related Issues
To prevent extra work, all changes to the Files codebase must link to an approved issue marked as
Ready to build
. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.Steps used to test these changes
Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.