Use more appropriate types for cross-platform unicode support

[vjoki]

Interally things are switched over to using widestring and W/Ex variants of the unrar_sys functions, as these seem to be preferred (unrar internally converts non-wchar strings). This is also required for unicode support on Windows (UTF-16), but also works on other platforms (by using UTF-32).

Breaking changes to public API:

• More permissive with the types accepted for archive filename,
  destination and password. (TBH could be more permissive if I didn't (pointlessly) try to avoid ownership inside Archive.)
• Return PathBufs rather than Strings when representing filenames.

One thing still missing is that passwords are still Strings, as the old RARSetPassword interface only
supports chars. It's going to necessary to add support for UCM_NEEDPASSWORDW first, which requires some more changes to the callback.

[muja]

Thank you very much for this PR! I believe this should also fix #4.

Some things to note:

• This is a breaking change, so I should release a patch version before including this in a minor version release.
• Why are you avoiding ownership inside Archive?

[muja]

Is this string non-utf8? How would this work? Aren't &strs UTF-8? Wouldn't you need to use a Vec<u8>?

[vjoki]

It's UTF-8, I haven't yet come up with any good way of crafting a suitable UTF-16 test case.

[vjoki]

The reason why I opted to borrowing over owning in Archive was because I didn't see much value in taking owned values, as they all get copied in the conversion to WideCString and dropped after. Also taking ownership would result in unnecessary (but probably cheap) cloning of borrowed types, while the tradeoff seemed more harmless.

To go in more detail, the options I considered were:

• Using values (String, PathBuf), but it forces the user to clone borrowed and do manual conversions so I ruled it out.
• Using AsRef<Path> + ?Sized + 'a stored as Cow<'a, Path> for filename, which is the most generic interface without unecessary cloning that I could come up with. It has the caveat of owned types needing to be passed as references. Accepting at least str, Path, OsStr, &PathBuf, &String, &OsString.
• Using Into<PathBuf>, which would probably be the most invisible to the user, but results in unnecessary copying when passing borrowed types (str, Path, OsStr). Accepting at least str, Path, OsStr, PathBuf, String, OsString plus references.

password could also be &'a str (almost equivalent to the AsRef?) or Into<String>, with essentially the same considerations as with filename. There's still minor differences right now as String/CString is used, but that changes when UCM_NEEDPASSWORDW gets added (probably to AsRef<OsStr> or Into<OsString> converted to WideCString).

For destination path in Archive::extract_to, AsRef<Path> seems like the best option as it ought to be as generic as it gets (no extra constraints like what filename has).

So the decisions here boil down to whether to accept the cost of extra copy or the need to pass owned by reference. Personally, I still prefer the latter but the final call is yours.

[muja]

I think it's okay to get this merged for now. However, I'd like to wait before doing a 0.5 release as I've been planning to restructure the library for a while now but have never gotten around to it.

I'm not sure if you're aware, but I've talked about it in PR #10, too. My idea is to give the user more control over what to do with each entry by separating the read_header call and the process from one another. I'm trying to use a streaming iterator to iterate over the archive's entries.

Maybe you have some thoughts about that, too. I opened an issue: #20

[muja]

However, would it be possible to not panic if CString contains nul characters?

[muja]

Also (sorry for spamming), would you like to be included in the authors file? :) I think you more than deserve to be mentioned.

[vjoki]

I can have it not panic, but I'm not quite sure what to do instead. Maybe return with UnrarError(Code::Unknown, ...) for now, until UnrarError gets refactored?

Also inclusion in the authors file is fine with me.

[muja]

I see what you're getting at, with the Code::Unknown and #22. A compromise in the meantime would be, before deciding on an error framework as a long term solution, one of these two options:

1. convert the password into a CString in the with_password function and return a Result there
2. same as 1) but panic instead of returning a Result (should be documented though).
3. split the password at NUL char and take the first part

Options 1) or 2) seem like good solutions and are more honest with the caller. 3) seems like black magic and would possibly contain some element of surprise.

The same I would do for the filename (this would be in the constructor of Archive).
As for destination in extract_to, option 1) would not be ergonomic as it already returns an UnrarResult. This is the reason why I'm personally inclined towards 2).

We could also 1) Archive::with_password and Archive::new and 2) Archive::extract_to and add another function Archive::extract_to_checked which adheres to 1)... What do you think?

Sorry for bringing this up only after you refactored your PR to remove the panics.

[vjoki]

No problem.

Doing 1) for password seems fine, but filename can get modified by as_first_part(). So the earliest point filename can be converted is in open().

I suppose it should be 1) for password, and 2) for the rest?

[muja]

Well if it really only panics if the String contains a nul character, we could just look for one and not wait for the open call to convert it? I'm not sure if I like inconsistent treatments of the same kind of error

[vjoki]

I'd be fine with that, but it would leave some room for mistakes (as in nul getting inserted in filename somewhere in Archive). Which means you'll want to check for nul twice then?

There is from_os_str_unchecked, if you dare. :-)

[vjoki]

Well, I went ahead and gave it a go, but I think it might be better to just revert the last 2 commits and panic on all cases for now. Because doing the conversions in OpenArchive::new and failing there seems to be the cleanest option, but it's your call.

[muja]

I think for now this looks fine, especially since we explicitly document where the function might panic. #22 should be implemented though in the future to be able to handle these higher level errors gracefully. Thank you for your time! :)

[muja]

@vjoki unrelated to this but do you prefer your full name or your alias for the authors list?

[vjoki]

Alias would be fine.