-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Fix: Fixed formatting bug when entering commands into the Address Bar #15448
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
Conversation
| private static string NormalizePathInput(string currentInput, bool isFtp) | ||
| { | ||
| // Check if input is a command and not a path | ||
| if (currentInput.Length > 1 && currentInput.ElementAt(1) != ':') |
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.
Is : always the second character in a valid path?
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.
Couldn't the paths be relative? ./Child ../Sibling etc? It won't have a drive letter then
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'm not concerned since we don't have plans on supporting that, but worst case we can come up with a new fix if things change.
|
I think it will be a big hassle to individually blacklist/whitelist each case. My approach would be to pass the original
Then after finding the Files/src/Files.App/ViewModels/UserControls/AddressToolbarViewModel.cs Lines 821 to 828 in 4b77a5a
|
|
Files/src/Files.App/ViewModels/UserControls/AddressToolbarViewModel.cs Lines 821 to 828 in 4b77a5a
After looking a bit more into this part of the code and doing some testing, it seems like if there is spaces in the path of the application (and there is parameters to the program), the program fails to run I propose to additionally check if |
That's a good approach. Should I disregard #15448 (comment)? |
FieryRMS
left a comment
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.
lgtm. built and tested it
That bug I mentioned should persist even after the current fix. This issue comes from I suppose I should make a separate issue for this. It seems unrelated to the issue at hand. |
|
Thanks! |
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.
git clone https://github.com/files-community/Filesinto address bar