glib: Remove ending NUL when converting Variant to OsString.

[rodrigorc]

Fixes #620.

[sdroege]

Makes sense to me but now I'm wondering if maybe we should add NULs when serializing those types into variants. I wonder what C code making use of paths in variants is expecting.

Might make sense bringing that up as a GLib issue if this is not documented anywhere.

[rodrigorc]

...but now I'm wondering if maybe we should add NULs when serializing those types into variants.

I was just thinking about that!

I wonder what C code making use of paths in variants is expecting. Might make sense bringing that up as a GLib issue if this is not documented anywhere.

I think it is documented if you look deep enough:
In GApplication:

• for G_OPTION_ARG_FILENAME, use ^&ay

With ay being an array of bytes, that would be read with g-variant-get-bytestring, that says:

If the array does not end with a nul terminator character, the empty string is returned. For this reason, you can always trust that a non-NULL nul-terminated string will be returned by this function.

So my guess is that the NUL byte is mandatory. Should I update this pull-request or create a new one, as this is technically a different issue?

What I don't quite understand here is why the Windows version stores WCHAR (u16) values instead of bytes. Nothing in the GLib source code that I saw suggests that an array of WCHAR should be used there. Maybe I'm missing something?

I don't have a Windows machine around to try, maybe I'll try with Wine and let you know.

[jf2048]

That does look like it should probably always get converted to utf8, to match what glib always does. Exposing the utf16 is a rust thing

[sdroege]

That does look like it should probably always get converted to utf8, to match what glib always does. Exposing the utf16 is a rust thing

The "local options" in GApplication are using a non-UTF-8 string for filename options though. It would be in local encoding (except on Windows where it's always UTF-8?).

What I don't quite understand here is why the Windows version stores WCHAR (u16) values instead of bytes. Nothing in the GLib source code that I saw suggests that an array of WCHAR should be used there. Maybe I'm missing something?

That was because OsString uses u16 on Windows, but probably does not make much sense considering the above, and should probably always use UTF-8 on Windows. (This means it would have to work similar to ToGlibPtr / FromGlibPtr for OsString / Path which also special-cases Windows).

With ay being an array of bytes, that would be read with g-variant-get-bytestring, that says:

ay is a byte array that doesn't have to end on NUL but get_bytestring() is a helper function that assumes that for "byte strings" (i.e. not byte arrays), AFAIU.

I think your analysis is correct in that regard and it should indeed be assuming / putting a NUL at the end for Path / OsString conversions.

[jf2048]

Hmm, yes it does look like the "local encoding" on windows is supposed to be utf8: g_win32_get_command_line

[rodrigorc]

Ok, I'm trying to fix everything in one go. I've just added a new commit and squashed the PR.

I've added changes to OsString::from_variant() and OsStr::to_variant(), both for Unix and Windows variants, pushing and popping the NUL byte appropriately.

I'm not totally sure about the lossy conversions in Windows... But then, if you have a Windows file name that is not UTF-8 valid, I think that you have already lost: Rust can handle that with the internal WTF-8 encoding of OsString, but AFAIK glib uses pure UTF-8. The alternative to lossy conversion would be to replace them with an empty string, as Variant does with the missing NUL.

I have tested my example from the original bug report, both in Linux and in Windows (tricky!) and now it seems to work great. (previously Windows failed with something about the type of variant being "aq" instead of "ay").

[sdroege]

I would go with how it's implemented in glib/src/translate.rs in c_to_path_buf and path_to_c. I mean, you can even call those functions from here after making them pub(crate) :)

[sdroege]

Otherwise this looks all good to me!

[rodrigorc]

I have force-pushed a new version of this PR.

Funnily, using translate.rs makes the code shorter: (23 insertions(+), 54 deletions(-)).

[sdroege]

The implementation looks all good to me, just needs a little bit of cleanup and then this can go in.

Funnily, using translate.rs makes the code shorter: (23 insertions(+), 54 deletions(-)).

That's nice :)

[rodrigorc]

I've just force-pushed a new version.
Also, now that these functions call into translate.rs I've fully separated the impls of Path/PathBuf and OsStr/OsString.