touchHLE
touchHLE copied to clipboard
hikari-no-yume
Allow opening an IPA bundle file directly
Closes #20
It works, but I am not yet happy about all the code.
The error handling is kind of all over the place.
Also, maybe it's worth making a FileRef enum representing "a file that can potentially be opened" instead of two variants in FsNode enum
Oh right, the README and maybe the help/usage text should be updated after this. I'm happy to do that myself, I like writing documentation.
Regarding the FileRef: When I first added the IpaBundleFile enum variant, I completely forgot to update the places where matches are made over it (and the is_file even made it into git).
What I think could have helped is to add another level of indirection (as always): have a "this is a file" enum variant in the FsNode and an inner FileRef enum specifying which kind.
This way the is_file problem would not have happened (one enum variant for all files, no need to update)
In the places you actually need to look inside the file (like open), you would first match over the FsNode to check whether it's a file and then you would match exhaustively over FileRef (as you no longer need "it's a directory" error) and, therefore, changing the FileRef enum will give you some compiler errors to tell you what needs updating.
But I don't think it's a good idea to it in this PR
Yeah, that justification makes sense. I'll think about doing something like that if the enum gets any more complicated than it already is.
Compiling this on my machine right now, I'll tell you once I've tested it a bit. I'll also have a look at the handful of IPA files I have to see what compression methods they have.
cargo clippy has three suggestions for me now, maybe you'll want to address them. Otherwise I'll do it myself later.
I guess I have a newer Rust toolchain than you because there's one more lint for me:
warning: use of `ok_or` followed by a function call
--> src/bundle.rs:36:14
|
36 | .ok_or("plist root value is not a dictionary".to_string())?;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `ok_or_else(|| "plist root value is not a dictionary".to_string())`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call
= note: `#[warn(clippy::or_fun_call)]` on by default
warning: `touchHLE` (bin "touchHLE") generated 1 warning
Finished dev [unoptimized + debuginfo] target(s) in 10.52s
I've pushed the fixes for the lints
Just a note: in rust 1.66 a new uninlined_format_args lint was added, which is triggered in... almost the entire project :P (that's why I was kinda ignoring clippy here)
It's machine fixable, so probably want to update the toolchain & run cargo clippy --fix
I guess I have a newer Rust toolchain than you because there's one more lint for me:
I am on latest stable, 1.67. Still, will fix it
Ohh wow, I've stubbornly avoided using inlined format args, but maybe it's time for me to switch. Also, ah, for some reason I've never used cargo clippy --fix, I guess I've been wasting time :p
I've stubbornly avoided using inlined format args
Same, lol. Now that clippy is complaining I have no choice ig
macOS has an unzip utility, I assume the same or a similar tool is available on common Linux distros. I was able to write this one-liner to check a bunch of IPA files' compression methods:
for i in *.ipa; do unzip -v "$i" | cut -f 3,1 -w | grep -v 'Defl' | grep -v 'Stored' ; done
I ran this on all the IPAs I have on my drive: all the ones I redownloaded from the iTunes Store, and a number of others.
As expected, everything is either DEFLATE or uncompressed (aka “Stored”). I think the latter is maybe just for empty files?
So not supporting other compression methods seems pretty safe. 👍
Oops, I got distracted. Works great in my testing. I was a bit worried it'd be slower than loading a pre-extracted file, but I can't notice the difference in release builds.
I think I'm ready to merge this. My preference is a squash-and-rebase merge type. Do you want to squash all the commits into one yourself (if so, please use --reset-author so the author date is more accurate) or shall I do it?
Thanks a lot for this contribution!
I was a bit worried it'd be slower than loading a pre-extracted file, but I can't notice the difference in release builds.
The whole game is like 40 megabytes, really small for modern hardware
Do you want to squash all the commits into one yourself
Sure
Yeah I guess, but I know from one of my emulator developer friends that even small titles can chug if you have to decompress too often! Luckily Monkey Ball has loading screens.
Yeah, looks good to me! My preference is to remove the info in the commit message about which commits were squashed, since that's usually useless extra info in future when reading the commit history, so I'll remove that bit when I cherry-pick the commit, if that's okay.
Merged as 32d12cc697f8fc62770962b8d5a1fee44dfba677, thanks!
I added a changelog and credited you via your GitHub username in c407ef831b9f67ef0ced294dc30208108b16cf2c. If you'd prefer a different style of credit, please let me know.