Davejkane / riv

Riv - The Rust Image Viewer
MIT License
99 stars 10 forks source link

Normalize relative paths #61

Closed NickHackman closed 5 years ago

NickHackman commented 5 years ago

So in the previous PR you guys merged while I was on lunch and was fixing this issue :joy:

I fixed the issue with the README.md and reverted to the previous design for the ./keep file

Issue:

Paths aren't normalized, so paths on the infobar could have unnecessary bloat.

newglob had this issue and so did passing paths from command line. I also moved the commonly used functions like cli.rs's push_image_path and normalize_paths into paths.rs since they're directly related and both are utility functions.

This made it really difficult to navigate using newglob to move around.

Issues with current implementation:

For instance if you cd into a symlink to another directory, typing cd ..afterwards won't bring you back up to the directory the symlink is contained in, but just the higher directory of what the symlink points to. It's impossible to implement the proper reverse following without severely hurting performance. Read more here

Examples:

~/Pictures/././././../Pictures/* => ~/Pictures/*

NickHackman commented 5 years ago

@Davejkane @gurgalex I'm going to close this PR, I like what you guys are saying and that really should have a

if cfg!(unix) {
    // split on '\ '
}

because windows that's completely unnecessary, my apologizes on that :slightly_frowning_face:

I think separating these issues into multiple PRs would make the most sense, I shouldn't have tried to cram them all together into the one.

  1. Fix README changes from the end of #58
  2. Remove .keep implementation I added on and fix the '\ ' issue as mentioned above.
  3. Normalize paths, since this isn't truly necessary at the time being, but a nice thing we should have in the future, while the other PRs would be based around issues that either make it harder for users or deviates from the intended nature of riv. It should use std::fs::canonicalize but solving that for allowing *s gets to be a bit tricky.

Closing note, sorry for the mess that was PR #58 it was truly a nightmare I didn't properly test every feature, but handled common cases along with the poor handling of cross platform, it was truly a, pardon my language, fuck up on my behalf and you won't see anything of that sort in the future.