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

Migrate to windows-rs 0.37 #102

Merged
merged 3 commits into from Jul 19, 2022
Merged

Conversation

str4d
Copy link
Contributor

@str4d str4d commented May 21, 2022

No description provided.

windows-rs 0.36 introduced its own Event type, which ends up in the
glob imports and would collide without this rename.
platforms/windows/src/node.rs Outdated Show resolved Hide resolved
@mwcampbell
Copy link
Contributor

@str4d Please update the test harness as you did for the example. I'm aware that our CI doesn't actually run the Windows tests at the moment, but I tried to do so, and they didn't build.

This is in preparation for windows-rs 0.32 which moves to a trait-based
`implement` macro.

Closes AccessKit#104.
@str4d
Copy link
Contributor Author

str4d commented May 21, 2022

Ahh, I missed this because cargo test passes, and I thought that ran everything, but if I run cargo test -p accesskit_windows I now see the failures. Fixing 😄

@str4d
Copy link
Contributor Author

str4d commented May 21, 2022

Okay, this should now be working, and also resolves #104.

@mwcampbell
Copy link
Contributor

@str4d Just wondering if you've had a chance to look at the two pending code review comments I left shortly after you opened the PR.

@str4d
Copy link
Contributor Author

str4d commented Jul 10, 2022

I don't see any unresolved issues on this PR. Are they in a review that hasn't been published?

@str4d
Copy link
Contributor Author

str4d commented Jul 10, 2022

The only action items I'm aware of were about the patterns! macro and the update to the test harness, both of which I had resolved as of my last comment on May 21st.

@DataTriny
Copy link
Member

Hello,

Version 0.38.0 was recently published. Is it worth directly upgrading to it? By reading the changelog, it seems to me that this would just be a matter of updating Cargo.toml.

Thanks again @str4d for your contribution.

enum FragmentRootResult {
This,
This(PlatformNode),
Copy link
Contributor

Choose a reason for hiding this comment

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

The spirit of the old implementation can be maintained by having PlatformNode derive Clone, then using self.clone().into() instead of self.into(). Please either do this or remove the enum altogether; it's safe to do the latter, since in your current implementation, both match arms do the same thing.

@@ -668,6 +683,8 @@ macro_rules! properties {
macro_rules! patterns {
($(($base_pattern_id:ident, $is_supported:ident, (
$(($base_property_id:ident, $getter:ident, $com_type:ident)),*
), (
$($extra_trait_method:item),*
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Putting whole function definitions inside a macro invocation seems likely to be unfriendly to IDEs and other tools.

Did you verify that Rust doesn't allow multiple impl blocks for a trait as it does for the struct itself?

@mwcampbell
Copy link
Contributor

@str4d I'm really sorry; I wrote my review comments on May 21, but since this is the first time I've used the GitHub code review feature as a reviewer, I didn't realize I had to submit the review after adding the comments.

@mwcampbell
Copy link
Contributor

@str4d Just wanted to confirm that you received the review comments that I belatedly submitted on Sunday.

@mwcampbell
Copy link
Contributor

I've decided that the two review comments I posted aren't blockers. I'll go ahead and merge this, then address those issues myself when I have a chance.

@mwcampbell mwcampbell merged commit a3eaec6 into AccessKit:main Jul 19, 2022
mwcampbell added a commit that referenced this pull request Jul 19, 2022
mwcampbell added a commit that referenced this pull request Jul 19, 2022
…#106)

This reverts commit a3eaec6.

I got the merge wrong. I screwed up the conventional commit message, and I should probably include the bit about renaming `Event to `QueuedEvent`. I will re-merge next.
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