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

Replace winapi with windows crate bindings shared with WRY #206

Merged
merged 31 commits into from
Sep 22, 2021
Merged

Replace winapi with windows crate bindings shared with WRY #206

merged 31 commits into from
Sep 22, 2021

Conversation

wravery
Copy link
Contributor

@wravery wravery commented Sep 13, 2021

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Documentation
  • Build-related changes
  • Other, please describe:

By itself, this change should not have much impact on TAO. I've replaced all of the winapi crate references with the windows crate. The generated bindings are in the webview2-com-sys crate, which is important to be able to share types with WRY later in a separate PR. That's already working in my fork of WRY,

Mostly that's a matter of 1:1 replacements, where the types and functions in winapi matched what the windows crate generated, although there are a few less obvious changes. For one thing, the winapi crate seems to declare enums for most of the #define macros in the Windows headers, and the windows crate declares them as const values. That means you can't refer to them in a pattern (i.e. match arms), because that just shadows the const with a local variable using the same name. So that's why I had to replace so many patterns with something like _ if msg == WM_FOO =>, using a pattern guard expression.

There were a few macros like GET_X_LPARAM/GET_Y_LAPARAM and PRIMARYLANGID that did bit manipulation which winapi reimplemented as public functions, and the windows crate doesn't. I ended up reimplementing those inline where I found them, but I might go back and replace some of those with const fns. On the bright side, while researching those macros I also found a few places where TAO was not quite passing the correct values. For instance, drag_window passed a pointer to a POINTS struct instead of encoding the x/y coordinates as i16 values in a u32. I think I saw the same bug in at least one other place.

One thing the windows crate does much better is argument coercions, so for example you can pass false and it will automatically convert that to a BOOL(0) with a conversion trait. It also implements the Default trait for many of the structs, so you don't need to use std::mem::zeroed in an unsafe block to initialize them.

The windows crate also wins hands down when dealing with COM, especially if you need to implement them. For instance, TAO benefits from this in the FileDropHandler and when dealing with ITaskbarList2. I was able to remove a lot of code in FileDropHandler dealing with ref-counts and v-tables and replace it with #[windows::implement(Windows::Win32::System::Com::IDropTarget)].

Does this PR introduce a breaking change? (check one)

  • Yes. Issue #___
  • No

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

This is related to tauri-apps/wry#133, which was later removed because it only worked for Windows 10 and it required redistributing DLLs along with the app. Both of those have been addressed since then, plus the windows crate now supports the #[implement] macro. I think it's worth another try.

@wusyong wusyong changed the base branch from dev to next September 13, 2021 11:14
@wravery
Copy link
Contributor Author

wravery commented Sep 14, 2021

Marking this ready for review. Now that it's targeting the next branch I don't think there's any reason to hold it back.

@wravery wravery marked this pull request as ready for review September 14, 2021 04:06
@wravery wravery requested a review from a team as a code owner September 14, 2021 04:06
@wravery wravery requested review from a team September 14, 2021 04:06
@lemarier
Copy link
Member

Personally, it looks AMAZING.
I'll not tell you how the Windows API work :P

We'll run different tests/scenarios on different platforms, and we'll wait for the complete review from @amrbashir (he's away for few days) as it's our Windows specialist ;)

Meanwhile, there is a little fmt error, would be great if you can fix it.

This is the kind of gift we didn't expect and we do appreciate it a lot.
It'll change a lot of things.

Thanks again from the whole Tauri community.

@wravery
Copy link
Contributor Author

wravery commented Sep 14, 2021

We'll run different tests/scenarios on different platforms, and we'll wait for the complete review from @amrbashir (he's away for few days) as it's our Windows specialist ;)

Sounds great, LMK if I can help. 👍🏼

Meanwhile, there is a little fmt error, would be great if you can fix it.

It looks like the errors came from clippy running on macOS. I have a Mac (and Linux machine), so I can take a look, but I wonder if it would make sense to run the clippy checks on Windows and Linux as well. When I ran clippy on Windows I found a couple of other pre-existing errors that I fixed on Windows and the workflow doesn't seem to have caught those.

@lemarier
Copy link
Member

but I wonder if it would make sense to run the clippy checks on Windows and Linux as well

Yes totally! I was sure it was checking all platforms already. We can add it in another PR, you don't have to deal with the CI fun. You've already done enough hehehe ;)

Thanks for the clippy fix!!

Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

Thanks for the awesome PR and I apologize for the nit-picks.

There is 2 concerns I have:

  1. DWORD, UINT and similar types are replaced with rust types, I think I prefer to stick with win32 types if possible, so it is consistent with original c++ win32 api.
  2. There is a lot of Anonymous fields on structs in this PR which tbh kinda scary and I don't understand why they exist and how much maintenance it will require.

A totally useless request is can we rename webview2-com-sys, seems like win32-com-sys might be a better name, as my OCD is triggered 😅 (A window management crate depending on a webview crate) .

src/platform/windows.rs Outdated Show resolved Hide resolved
src/platform/windows.rs Outdated Show resolved Hide resolved
src/platform_impl/windows/accelerator.rs Show resolved Hide resolved
src/platform_impl/windows/accelerator.rs Show resolved Hide resolved
src/platform_impl/windows/event_loop.rs Outdated Show resolved Hide resolved
src/platform_impl/windows/event_loop.rs Show resolved Hide resolved
src/platform_impl/windows/menu.rs Outdated Show resolved Hide resolved
src/platform_impl/windows/menu.rs Show resolved Hide resolved
src/platform_impl/windows/monitor.rs Show resolved Hide resolved
src/platform_impl/windows/window.rs Outdated Show resolved Hide resolved
@wravery
Copy link
Contributor Author

wravery commented Sep 20, 2021

Couple of remaining issues that I haven't really addressed, hopefully neither of them is a blocker. 😄

  1. DWORD, UINT and similar types are replaced with rust types, I think I prefer to stick with win32 types if possible, so it is consistent with original c++ win32 api.

The windows-rs crate takes a somewhat different approach by preferring Rust types as much as possible with automatic conversions. It keeps the same function/struct names to make it easier to find documentation even though they don't use snake-case, but where there's an equivalent simple type it will use them. It doesn't include any structs for DWORD or UINT, so we'd need to include our own. I think that would just make it a little harder to consume from the Rust side though, and it wouldn't apply to any other platforms.

A totally useless request is can we rename webview2-com-sys, seems like win32-com-sys might be a better name, as my OCD is triggered 😅 (A window management crate depending on a webview crate) .

Maybe... but then I think we should probably create a TAO-specific bindings crate for it, under the tauri-apps org. The thing that worries me about that is the types wouldn't be the same as the bindings consumed by WRY, but if all of the types from webview2-com-sys are removed from the TAO public interface (e.g. putting back *mut libc::c_void), I guess that should be OK. In the worst case, it might define 2 different implementations of any associated functions that are referenced in each crate and bloat the binary a little bit.

@amrbashir
Copy link
Member

amrbashir commented Sep 20, 2021

No these are not exactly blockers, it was just a mere preference.

I will try to do a final review of this PR soon and make sure it is all fine before merge.

src/platform/windows.rs Outdated Show resolved Hide resolved
src/platform/windows.rs Outdated Show resolved Hide resolved
src/platform_impl/windows/event_loop.rs Outdated Show resolved Hide resolved
src/platform_impl/windows/event_loop.rs Outdated Show resolved Hide resolved
src/platform_impl/windows/event_loop.rs Outdated Show resolved Hide resolved
src/platform_impl/windows/event_loop.rs Outdated Show resolved Hide resolved
src/platform_impl/windows/keycode.rs Outdated Show resolved Hide resolved
Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

Thanks again for the awesome work, the PR looks fantastic.

Copy link
Member

@wusyong wusyong left a comment

Choose a reason for hiding this comment

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

Thanks both of your works! Let's move to wry and make this happen!

@wusyong wusyong merged commit e1e9b61 into tauri-apps:next Sep 22, 2021
wusyong pushed a commit that referenced this pull request Feb 5, 2022
* refactor(windows): `begin_resize_drag` now similar to gtk's (#200)

* refactor(windows): `begin_resize_drag` now similart to gtk's

* fix

* feat(linux): skipping taskbar will now also skip pager (#198)

* refactor(linux): clean dummy device_id (#195)

* refactor(linux): clean dummy device_id

* fmt

* feat(linux): allow resizing undecorated window using touch (#199)

* refactor(windows): only skip taskbar if needed when `set_visible` is called (#196)

* fix: increase borderless resizing inset (#202)

* fix: increase borderless resizing inset

* update some comments

* Replace winapi with windows crate bindings shared with WRY (#206)

* fix(deps): update rust crate libayatana-appindicator to 0.1.6 (#190)

Co-authored-by: Renovate Bot <bot@renovateapp.com>

* Add Windows crate and webview2-com-sys bindings

* Initial port to webview2-com-sys

* Finish conversion and remove winapi

* Fix renamed lint warning

* Fix all match arms referencing const variables

* Put back the assert instead of expect

* Point to the published version of webview2-com-sys

* Cleanup slightly weird BOOL handling

* Replace mem::zeroed with Default::default

* Add a summary in .changes

* Remove extra projects not in config.json

* Fix clippy warnings

* Update to 32-bit compatible webview2-com-sys

* Better fix for merge conflict

* Fix clippy errors on Windows

* Use path prefix to prevent variable shadowing

* Fix Windows clippy warnings with nightly toolchain

* Fix Linux nightly/stable clippy warnings

* Fix macOS nightly/stable clippy warnings

* Put back public *mut libc::c_void for consistency

* Re-run cargo fmt

* Move call_default_window_proc to util mod

* Remove unnecessary util::to_wstring calls

* Don't repeat LRESULT expression in match arms

* Replace bitwise operations with util functions

* Cleanup more bit mask & shift with util fns

* Prefer from conversions instead of as cast

* Implement get_xbutton_wparam

* Use *mut libc::c_void for return types

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Renovate Bot <bot@renovateapp.com>

* fix(keyboard): add mapping for space key on Windows (#209)

* fix(keyboard): add mapping for space key on Windows

* change file

* feat: impl Clone for EventLoopWindowTarget (#211)

* chore: add `on_issue_closed.yml` (#214)

* Update tray dependency version (#217)

* Delete on_issue_closed.yml (#221)

* refactor(linux): event loop (#233)

* Use crossbeam::channel

* Fix crossbeam channel import

* Add check on poll event

* Fix deadlock when unregistering shortcut on Linux (#230)

* Add fullscreen monitor selection support on Linux (#235)

* Add fullscreen monitor support on Linux

* Add change file

* Remove todo on videomode

* Fix clippy

* Update to 2021 edition (#236)

* Update to 2021 edition

* Fix clippy

* Add run_return on Linux (#237)

* Add run_return on Linux

* Add main context

* Add run_return trait on Linux (#238)

* Fix: #239 Update webview2-com and windows crates (#240)

* Replace webivew2-com-sys with prebuilt windows

* Use windows utility instead of direct GetLastError

* Bump windows version and add changelog

* Run cargo fmt

* Restore inverted matches macro

* Scope constants in match arms

* Fix inverted null check

* Update src/platform_impl/windows/util.rs

Co-authored-by: Amr Bashir <48618675+amrbashir@users.noreply.github.com>

* Use env_logger instead of simple_logger (#241)

* Use env_logger instead of simple_logger

* Make clippy happy

* Cherry pick commits to next (#244)

* feat(macos): Add `unhide_application` method, closes #182 (#231)

* feat(macos): Add `unhide_application` method

* Update src/platform/macos.rs

Co-authored-by: Amr Bashir <48618675+amrbashir@users.noreply.github.com>

* Reanme to `show_application()`

* Remove broken doc link

Co-authored-by: Amr Bashir <48618675+amrbashir@users.noreply.github.com>

* feat: Allow more strings to parse to keycode (#229)

* feat: support accelerator key strings `,` `-` `.` `Space` `Tab` and `F13`-`F24` (#228)

* feat(macOS): support more accelerator key strings

* Move function keys together

* Add `,` `-` `.` `Space` `F20-F24` for Windows

* Remove support for accelerators not found in `winapi`

* Add `,` `-` `.` `Space` `F13-F24` for Linux

* Update .changes

* Add the rest for Windows

* Add the rest for Linux

* Add the rest on macOS

* Update accelerator-strings.md

* Fix git comments

Co-authored-by: Kasper <kasperkh.kh@gmail.com>
Co-authored-by: Amr Bashir <48618675+amrbashir@users.noreply.github.com>

* Add redraw events on Linux (#245)

* Add redraw events on Linux

* Update doc of RequestRedraw Event

* Add change file

* Fix missing menu bar on borderless window (#247)

Credit goes to irh's work on winit commit f2de8475fc4703d03a2ecc2cda627b017202e623

* refactor: improve `set_skip_taskbar` impl on Windows (#250)

* fix: emit errors on parsing an invalid accelerator for string, closes #135 (#252)

* chore: update comment

* fix(linux): fix focus events not firing properly (#253)

* fix(linux): fix focus events not firing properly

* add changelog

* chore: update focus events error message

* chore: fmt

* fix: revert windows-rs 0.28 version bump

* fix(linux): fix native menu items (#256)

* chore: remove examples commited by accident

* Update `ReceivedImeText` (#251)

* Allow receiving text without Ime on Windows

* Avoid panic todo

* Receive text without ime on mac

* Fix CursorMoved event on Linux

* Add ReceivedImeText on Linux

This only add Simple IME from GTK for now. We should add the actual IME
from system in the future.

* Fix redraw event that causes inifinite loop (#260)

* Fix redraw event that causes inifinite loop

* Refactor event loop

* Remove unused function

* Add doc comment on linux's run_return

* Ignore doc test on run_return

* Add non blocking iteration on Linux (#261)

* Docs: SystemTrayExtWindows::remove() is gone (#262)

Fix docs following #153

* Fix busy loop on Linux (#265)

* Update windows crate to 0.29.0 (#266)

* Update to windows 0.29.0

* Add change description

* Remove clippy check (#267)

* refactor(windows): align util function with win32 names

* chore: update PR template

* fix(linux): fire resized & moved events on min/maximize, closes #219 (#254)

* feat(linux): implement `raw_window_handle()` (#269)

* chore(deps): update to raw-window-handle 0.4

* add linux raw-window-handle support

* update macos/ios/android

* fix ios

* Fix core-video-sys dependency (#274)

* The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (#275)

However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.

* Fix some invalid msg_send! usage (#276)

* Revert "Fix some invalid msg_send! usage (#276)" (#277)

This reverts commit a3a2e0c.

* Revert "The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (#275)" (#279)

This reverts commit 6f9c468.

* The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (#280)

However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.

Co-authored-by: madsmtm <mads@marquart.dk>

* Fix some invalid msg_send! usage (#278)

Co-authored-by: madsmtm <mads@marquart.dk>

* Add exit code to ControlFlow::Exit (#281)

* Add exit code to ControlFlow::Exit

* Cargo fmt

* Add change files

Co-authored-by:  multisn8 <contact@multisamplednight.com>

* Add new_any_thread to Unix event loop (#282)

* Update windows crate to 0.30.0 (#283)

* Update windows crate to 0.30.0

* Simplify new-type usage

* Fix boxing in GWL_USERDATA

* Make sure everyone is using Get/SetWindowLongPtrW

* build the system_tray module when "ayatana" feature is enabled (#285)

Without those cfg feature checks, the "ayatana" feature does
actually not enable anything.

* Fix click events missing whe tray has menu (#291)

* Fix click events missing whe tray has menu

* Add change file

* Fix crash when tray has no menu (#294)

* chore: update pull request commit exmple

* fix(windows): send correct position for system tray events, closes #295 (#300)

* fix(windows): revert maximized state handling to winit impl, closes #193 (#299)

* fix(windows): revet maximized state handling to winit impl, closes #193

* add chanefile [skip ci]

* fix: `MenuItem::Quit` on Windows (#303)

* fix: `MenuItem::Close` on Windows

* use `PostQuitMessage` instead

Co-authored-by: amrbashir <amr.bashir2015@gmail.com>

* feat: v1 audit by Radically Open Security (#304)

* Update to gtk 0.15 (#288)

* Update to gtk 0.15

* Fix picky none on set_geometry_hint

* Fix CursorMoved position

Co-authored-by: Amr Bashir <48618675+amrbashir@users.noreply.github.com>
Co-authored-by: Bill Avery <wravery@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: Lucas Fernandes Nogueira <lucasfernandesnog@gmail.com>
Co-authored-by: Kasper <kasperkh.kh@gmail.com>
Co-authored-by: amrbashir <amr.bashir2015@gmail.com>
Co-authored-by: Jay Oster <jay@kodewerx.org>
Co-authored-by: madsmtm <mads@marquart.dk>
Co-authored-by: multisn8 <contact@multisamplednight.com>
Co-authored-by: Aurélien Jacobs <aurel@gnuage.org>
Co-authored-by: Lucas Fernandes Nogueira <lucas@tauri.studio>
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.

4 participants