Add support for reading files to file-explorer #47
Conversation
Just for fun, display file mode as well as size, and simplify code with `fs::read_to_string()`, which works with the corresponding libc changes.
| @@ -74,8 +75,37 @@ impl<'a> FileExplorer<'a> { | |||
| } | |||
|
|
|||
| fn print_menu(&mut self) { | |||
| println!("Viewing {}", self.path.display()); | |||
| match std::fs::metadata(&self.path) { | |||
There was a problem hiding this comment.
I see this error after reading romfs:/test-file.txt:
Failed to read romfs:: Illegal byte sequence (os error 138)
This is probably due to the weird path handling? Maybe we should handle this case in the code until the underlying path issue is fixed?
There was a problem hiding this comment.
Yes, this is because of Meziu/rust-horizon#13 as far as I know. I didn't want it to block this PR because I think it was an issue before but got more exposed by this PR.
There was a problem hiding this comment.
Yeah, not blocking, but maybe we can do something like in run with self.path.components().count() > 1 to avoid popping the path in this case. Or maybe we just need to reset it manually. Up to you.
| match std::fs::read_to_string(&self.path) { | ||
| Ok(contents) => { | ||
| println!("File contents:\n{0:->80}", ""); | ||
| println!("{contents}"); |
There was a problem hiding this comment.
Interestingly, for a really long file this takes a noticeably long amount of time (30 seconds on my /retroarch/retroarch.cfg), during which you see the lines scroll past pretty quickly. I wonder if the libctru console implementation is slow or something?
There was a problem hiding this comment.
How big is that file, out of curiosity? I only tested with this small one but anything on the order of 30s seems worth investigating. In particular I would not expect romfs to be slow, but I haven't looked too close at the io implementations for it...
There was a problem hiding this comment.
It's 104 KB. I think it's just the default config, but here is the exact file (compressed so GitHub accepts it):
retroarch_config.zip
There was a problem hiding this comment.
Yep, it looks like that they really needed a scroll effect, but since the console cooperative thread doesn’t yield the scroll blocks the whole app, and takes quite a while, too.
There was a problem hiding this comment.
Oh, the scroll effect is semi-intentional? I guess fixing that would require libctru changes...
| match std::fs::read_to_string(&self.path) { | ||
| Ok(contents) => { | ||
| println!("File contents:\n{0:->80}", ""); | ||
| println!("{contents}"); |
There was a problem hiding this comment.
Maybe we should paginate the output here too? I wonder if we should move the pagination message to the lower screen to avoid interrupting the top screen's output.
There was a problem hiding this comment.
Oops, good point, I think when I had BufReader here it made more sense but using read_to_string is probably questionable when trying to paginate. Using the bottom screen to prompt for it probably takes a little work but sounds like a potentially nicer UX for this example.
There was a problem hiding this comment.
I think the prompt move should be as making a new temporary console and selecting it.
Ref Meziu/rust-horizon#11
edit: linked wrong issue because #11 on this repo is also related to filesystem, lol
Issues I've found so far:
PathBuf::parent/PathBuf::popseems to have strange behavior with theromfs:/prefix. See my comment on File not readable Meziu/rust-horizon#11fs::read_to_stringdoesn't work. Perhaps ideally, we'd have a test for both instdor in this library.Tasks to probably address before merging:
parent(),components(),pop()? (romfs:/and friends) – probably not actually needed before mergingfs::read_to_string? Or at least add a test for it somewhere?@Meziu @AzureMarker