Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding more Variant types #2983

Open
Guiguiprim opened this issue Apr 11, 2024 · 11 comments
Open

Adding more Variant types #2983

Guiguiprim opened this issue Apr 11, 2024 · 11 comments
Labels
enhancement New feature or request

Comments

@Guiguiprim
Copy link
Contributor

Suggestion

Hey,

Using windows-rs 0.52.0 I had my own Variant helper.
In windows-rs 0.54.0 I see that windows::core::Variant was added, which is nice.
With that change, all Windows::Win32::System::Variant::VARIANT* where removed (which completely breaks my Variant implementation).

I would be happy to migrate to windows::core::Variant but it is missing types support that I used:

  • fn null() -> Variant to construct a VT_NULL variant
  • TryFrom<&[String]> for Variant to construct VT_ARRAY_BSTR variant
  • TryFrom<&[u8]> for Variant to construct VT_ARRAY_UI1 variant

If I can, I'm going to have a try at it.

@Guiguiprim Guiguiprim added the enhancement New feature or request label Apr 11, 2024
@kennykerr
Copy link
Collaborator

Happy to review additions. I assume you mean VT_ARRAY | VT_BSTR and so on.

@hcldan
Copy link

hcldan commented May 2, 2024

We also need VT_ARRAY and VT_BSTR support.

@kennykerr
Copy link
Collaborator

VT_BSTR is already supported. It's really just VT_ARRAY that I hope to get to soon.

@hcldan
Copy link

hcldan commented May 2, 2024

Excellent!

@kennykerr
Copy link
Collaborator

VARIANT support is quite unwieldy and I haven't yet found a practical way to split it out from the windows-core crate, but I'm working on finding such a solution. #3151 just reduces the boilerplate code by using a macro to generate much of it, but the next step is to pull it into its own standalone windows-variant crate that the other windows-rs crates can depend on as needed.

@seritools
Copy link
Contributor

seritools commented Jul 26, 2024

To add to this request - I have to access VT_CLSID (GUID) and VT_LPWSTR variants, currently falling back to .as_raw() for those.

@PaulDance
Copy link

@kennykerr Did you find time to look at this again? If not, would you be willing to take a look at a redux of my previous PR? The addition of the desired APIs should not exactly need to come necessarily after the wanted refactor, but can be achieved in parallel, no?

@kennykerr
Copy link
Collaborator

kennykerr commented Aug 8, 2024

Sorry I've not had any time to return to this yet. I did have a quick look at your old PR and I'm not sure that's the right sustainable approach in general. We probably need a generic SAFEARRAY and then provide convertibility between VARIANT and SAFEARRAY but I've not given it much more thought yet.

@PaulDance
Copy link

But then again, this and that could be done separately, no? For example, implementing the variants first would mean having TryFrom<&[T]> for VARIANT implementations, then adding SAFEARRAY would mean a TryFrom<SAFEARRAY> for VARIANT implementation and the TryFrom<&[T]> for VARIANTs would simply be refactored to use the TryFrom<&[T]> for SAFEARRAY instead without having to change the API, right?

@kennykerr
Copy link
Collaborator

I'd just rather not rush into a solution. If you're blocked, I'd suggest you write a helper function to cover the missing functionality for the time being.

@kennykerr
Copy link
Collaborator

I finally settled on moving the VARIANT support into the windows crate. It's just too complex and costly to carry around in the windows-core crate. I'd considered a dedicated crate but so much of what makes VARIANT interesting lives in the windows crate so I couldn't really justify the additional complexity of a dedicated crate. In so far as it is practical to add additional conversion support in the new extension, we can certainly talk about that. #3282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants