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

Manually project IVector and IVectorView #406

Merged
merged 41 commits into from May 3, 2022
Merged

Manually project IVector and IVectorView #406

merged 41 commits into from May 3, 2022

Conversation

halildurmus
Copy link
Member

@halildurmus halildurmus commented Apr 15, 2022

Currently, the only supported types are String and WinRT types such as IHostName.

I found the types that IVector and IVectorView support in here.

From the link above, the types that IVector and IVectorView support are WinRT types, primitive types like String, int, bool etc., and also some structs like GUID, Point, Size, DateTime, Uri etc.

I looked into the WinMD files using ILSpy and the majority of the APIs returns IVector<String> or IVector<T extends IInspectable>. This is also the case for IVectorView. That's why I didn't add support for the remaining types and some of them were structs like GUID, Point which I don't know how to project them. We can add support for them in the future.

While adding support for generic types, I had to find a way to create constructors from generic T types. In the beginning, I was storing the constructor tear-off mappings for T types inside a Map like this:

  static const map = <Type, IInspectable Function(Pointer<COMObject>)>{
    IHostName: IHostName.new,
    IStorageFile: IStorageFile.new,
    ...
  };

After learning the number of types that IVectorView supports -- which is more than 300, I abandoned this plan and added a creator parameter to constructors of IVector and IVectorView.

This parameter is only required for WinRT types like IVector<IHostName> and as I assume we will convert methods or properties that return Pointer<COMObject> to their respective types such as IVector or IVectorView(we're gonna expose this as a List, but first we're gonna turn this into an IVectorView in the generated code), the consumers won't have to deal with this which is great.

I'm looking forward to hearing your thoughts on this.

EDIT: I just figured out how to use IIterable and IIterator. I won't include them in this since this PR has grown quite a bit already.

@halildurmus halildurmus changed the title Project Manually project IVector and IVectorView Apr 15, 2022
@Sunbreak
Copy link
Contributor

Good job. #259 depend on this!

@halildurmus halildurmus marked this pull request as ready for review April 20, 2022 05:56
@halildurmus halildurmus marked this pull request as draft April 20, 2022 11:38
@timsneath
Copy link
Contributor

Been playing around with this, with a couple of suggestions here: https://github.com/halildurmus/win32/pull/1

@halildurmus halildurmus marked this pull request as ready for review April 27, 2022 15:27
@timsneath
Copy link
Contributor

My goodness: this is impressive. You've taken this a whole level further.

I'll review the PR, but it'll take me a little while given its length.

Would you mind signing a CLA, by the way? https://cla.developers.google.com/about/google-individual

@halildurmus
Copy link
Member Author

My goodness: this is impressive. You've taken this a whole level further.

I'll review the PR, but it'll take me a little while given its length.

Thanks.

Would you mind signing a CLA, by the way? https://cla.developers.google.com/about/google-individual

I signed this last year.

Copy link
Contributor

@timsneath timsneath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a few relatively minor comments; if you can address these, I think we'll be in good shape to merge and then iterate further.

lib/src/extensions/iinspectable_query.dart Outdated Show resolved Hide resolved
lib/src/extensions/comobject_pointer.dart Outdated Show resolved Hide resolved
lib/src/extensions/hstring_array.dart Show resolved Hide resolved
lib/src/combase.dart Outdated Show resolved Hide resolved
lib/win32.dart Show resolved Hide resolved
test/winrt_collections_test.dart Show resolved Hide resolved
test/winrt_collections_test.dart Outdated Show resolved Hide resolved
tool/inputs/interfaces.dart Outdated Show resolved Hide resolved
@halildurmus
Copy link
Member Author

Hey @timsneath, this test is flaky:
https://github.com/timsneath/win32/blob/cc9933f2216b5da247227a050aa3e2f21421abaf/test/bstr_test.dart#L23-L26

If you run this test several times on your computer, you will notice that it sometimes fails.

@timsneath timsneath mentioned this pull request May 3, 2022
@timsneath
Copy link
Contributor

Are we good to merge, do you think? (Aside from my own test flake...)

@halildurmus
Copy link
Member Author

Yes, I think we're good.

@timsneath timsneath merged commit 007ce5b into dart-windows:main May 3, 2022
@timsneath
Copy link
Contributor

Thank you for this amazing pull request, Halil! This is awesome 🥳

@halildurmus halildurmus deleted the vector branch May 3, 2022 18:39
@halildurmus halildurmus mentioned this pull request Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants