Update windows to 0.58

[BenjaminBrienen]

No description provided.

[mwcampbell]

@BenjaminBrienen Thank you for your contribution. Unfortunately, this needs more work. The accesskit_windows crate doesn't compile. Also, looking at the diff of Cargo.lock, how did libloading's windows_targets dependency end up getting downgraded?

[BenjaminBrienen]

@BenjaminBrienen Thank you for your contribution. Unfortunately, this needs more work. The accesskit_windows crate doesn't compile. Also, looking at the diff of Cargo.lock, how did libloading's windows_targets dependency end up getting downgraded?

No idea, haha!

I'll fix the compile errors soon.

[BenjaminBrienen]

I'm trying to figure out what's going wrong with the implement macro, so I used cargo expand and cleaned up the output. The only real errors are related to PlatformNode missing impls:

error[E0277]: the trait bound `PlatformRange_Impl: Accessibility::ITextRangeProvider_Impl` is not satisfied
    --> platforms\test_output\src\text.rs:203:73
     |
203  |         <ITextRangeProvider as windows::core::Interface>::Vtable::new::<Self, -1>(),
     |                                                                         ^^^^ the trait `Accessibility::ITextRangeProvider_Impl` is not implemented for `PlatformRange_Impl`
     |
     = help: the trait `Accessibility::ITextRangeProvider_Impl` is implemented for `PlatformRange`
note: required by a bound in `ITextRangeProvider_Vtbl::new`
    --> C:\Users\BenjaminBrienen\.cargo\registry\src\index.crates.io-6f17d22bba15001f\windows-0.58.0\src\Windows\Win32\UI\Accessibility\impl.rs:3246:19
     |
3244 |     pub const fn new<Identity: windows_core::IUnknownImpl, const OFFSET: isize>() -> ITextRangeProvider_Vtbl
     |                  --- required by a bound in this associated function
3245 |     where
3246 |         Identity: ITextRangeProvider_Impl,
     |                   ^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `ITextRangeProvider_Vtbl::new`

[DataTriny]

Hello @BenjaminBrienen,

Looking at the changelog for the windows crate, I see that there is at least one breaking change in the COM interface authoring process. See microsoft/windows-rs#3065.

[BenjaminBrienen]

I took a step back to look at the compile error that @mwcampbell mentioned, and I'm not actually getting one:

PS C:\Users\BenjaminBrienen\source\accesskit> cargo clean
     Removed 0 files
PS C:\Users\BenjaminBrienen\source\accesskit> cargo build -p windows
   Compiling proc-macro2 v1.0.66
   Compiling unicode-ident v1.0.8
   Compiling windows_x86_64_msvc v0.52.6
   Compiling windows-targets v0.52.6
   Compiling windows-result v0.2.0
   Compiling windows-strings v0.1.0
   Compiling quote v1.0.33
   Compiling syn v2.0.32
   Compiling windows-implement v0.58.0
   Compiling windows-interface v0.58.0
   Compiling windows-core v0.58.0
   Compiling windows v0.58.0
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 15.15s
PS C:\Users\BenjaminBrienen\source\accesskit> git status      
On branch update-windows-dep
Your branch is up to date with 'origin/update-windows-dep'.

nothing to commit, working tree clean

[mwcampbell]

@BenjaminBrienen You need to compile the accesskit_windows crate, not the windows one.

[BenjaminBrienen]

I need some help with how to fix the error with HWND not being Send.

[DataTriny]

Relevant upstream issue: microsoft/windows-rs#3093

If we are sure that our use of HWND is thread-safe, then we'll probably have to wrap it in a new type which is Send, for internal use.

[BenjaminBrienen]

It compiles, but the test fails. I think I'm a bit out of my depth.

[DataTriny]

Tests pass on my machine. We always recommend to launch them from a regular command line window, otherwise they may fail.

I can see these warnings though:

warning: unused `BOOL` that must be used
   --> platforms\windows\examples\hello_world.rs:355:14
    |
355 |     unsafe { ShowWindow(window.0, SW_SHOW) };
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(unused_must_use)]` on by default
help: use `let _ = ...` to ignore the resulting value
    |
355 |     unsafe { let _ = ShowWindow(window.0, SW_SHOW); };
    |              +++++++                              +

warning: unused `BOOL` that must be used
   --> platforms\windows\examples\hello_world.rs:359:18
    |
359 |         unsafe { TranslateMessage(&message) };
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
help: use `let _ = ...` to ignore the resulting value
    |
359 |         unsafe { let _ = TranslateMessage(&message); };
    |                  +++++++                           +

warning: unused `Foundation::BOOL` that must be used
   --> platforms\windows\src\tests\mod.rs:176:18
    |
176 |         unsafe { ShowWindow(self.window.0, SW_SHOW) };
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(unused_must_use)]` on by default
help: use `let _ = ...` to ignore the resulting value
    |
176 |         unsafe { let _ = ShowWindow(self.window.0, SW_SHOW); };
    |                  +++++++                                   +

warning: unused `Foundation::BOOL` that must be used
   --> platforms\windows\src\tests\mod.rs:177:18
    |
177 |         unsafe { SetForegroundWindow(self.window.0) };
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
help: use `let _ = ...` to ignore the resulting value
    |
177 |         unsafe { let _ = SetForegroundWindow(self.window.0); };
    |                  +++++++                                   +

warning: unused `Foundation::BOOL` that must be used
   --> platforms\windows\src\tests\mod.rs:217:26
    |
217 |                 unsafe { TranslateMessage(&message) };
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
help: use `let _ = ...` to ignore the resulting value
    |
217 |                 unsafe { let _ = TranslateMessage(&message); };
    |                          +++++++                           +

warning: `accesskit_windows` (example "hello_world") generated 2 warnings
warning: `accesskit_windows` (lib test) generated 3 warnings

Also, I don't think we need to change the public API and expose WindowHandle. We don't want to explicitly allow convertion between WindowHandle and *mut c_void or vice versa, *mut c_void could be anything.

[BenjaminBrienen]

Also, I don't think we need to change the public API and expose WindowHandle.

The hello_world example needs to use Adapter::new, so how should we handle that interface?

[DataTriny]

I think Adapter::new should expect an HWND. The convertion to WindowHandle should probably be done inside Adapter::with_wrapped_action_handler.

[BenjaminBrienen]

I think Adapter::new should expect an HWND. The convertion to WindowHandle should probably be done inside Adapter::with_wrapped_action_handler.

And what about the handler parameter? It expects + Send and the struct the example uses isn't Send (I added an unsafe impl for now)

If my assumption is correct, then this branch should be ready for another round of review.

[DataTriny]

Remember that other parts of the project depend on accesskit_windows. Right now accesskit_winit is broken.

[DataTriny]

I'm not sure if SimpleActionHandler should be Send or if it should store a WindowHandle. The later would be more correct I guess but then we have to export the type just for the example.

[BenjaminBrienen]

The whole crate builds for me now. Sorry for not checking before.

[DataTriny]

@BenjaminBrienen I have pushed further changes to your branch, I hope it is OK with you?

@mwcampbell The only open question for me now is what do we want to do with the action handler in the example.

[BenjaminBrienen]

@BenjaminBrienen I have pushed further changes to your branch, I hope it is OK with you?

@mwcampbell The only open question for me now is what do we want to do with the action handler in the example.

Awesome, thanks for the help!

[mwcampbell]

In the example, I think manually implementing Send on SimpleActionHandler is the best we can do. We shouldn't need to implement Sync though.

Also, because the change to windows-rs's HWND crate is an API-breaking change (as shown by the required change to accesskit_winit), we're going to need to commit this as a semver-breaking release of accesskit_windows. But we don't want to do a semver-breaking release of accesskit_winit, since the API of that crate didn't break, and such a release would be more disruptive for our users. So unfortunately, what we'll need to do is withdraw the accesskit_winit changes from this PR, temporarily break the CI, and do another PR with those changes. @DataTriny and I have done this kind of thing before to work around the limitations of release-please, the release management tool we're using.

[DataTriny]

We're trying something different this time to work around the limitations of the tool we use to manage releases. As part of this I had to rebase your work @BenjaminBrienen. Your name disappeared from the second commit because of this. Sorry for hijacking your work, this is not my intention.

[BenjaminBrienen]

We're trying something different this time to work around the limitations of the tool we use to manage releases. As part of this I had to rebase your work @BenjaminBrienen. Your name disappeared from the second commit because of this. Sorry for hijacking your work, this is not my intention.

It doesn't bother me. 😄 Recognition isn't why I want to help! Thanks for bringing it to the finish line.