-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fix cross-platform mudabar, wire up better CI #75
Conversation
.vscode/settings.json
Outdated
@@ -0,0 +1,6 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committing this seems like a bad idea as it means that people won't be able to set their own settings. Maybe a "template" file with a different name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree here 👍 maybe .vscode.template
or similar? This was really helpful for me (I didn't know about allTargets
👀) so other people might find that nice too
packages/dioxus-blitz/src/window.rs
Outdated
// for now, forget the menu_bar so it doesn't get dropped | ||
// there's a bug in muda that causes this to segfault | ||
// todo: we should just store this somewhere | ||
std::mem::forget(menu_bar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just be a field of View
?
.github/workflows/ci.yml
Outdated
toolchain: "1.75.0", | ||
cross: false, | ||
command: "build", | ||
args: "--package dioxus-mobile", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building dioxus-mobile
in blitz CI probably doesn't make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just skip on android/ios for the time being
.github/workflows/ci.yml
Outdated
- { | ||
target: x86_64-pc-windows-msvc, | ||
os: windows-latest, | ||
toolchain: "1.75.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 1.75
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, does toolchain
need to be specified per-platform. Could just make global?
packages/dioxus-blitz/src/window.rs
Outdated
@@ -23,7 +25,6 @@ use winit::platform::unix::WindowExtUnix; | |||
use winit::platform::windows::WindowExtWindows; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like winit has wayland
and x11
platforms rather than unix
.
packages/dioxus-blitz/src/window.rs
Outdated
#[cfg(target_os = "windows")] | ||
{ | ||
use winit::platform::windows::WindowExtWindows; | ||
use winit::raw_window_handle; | ||
let id = window.window_handle_any_thread().unwrap(); | ||
if let raw_window_handle::RawWindowHandle::Win32(rwh) = rwh { | ||
let hwnd = id.hwnd; | ||
_ = menu.init_for_hwnd(hwnd as _); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it no longer compiles on Windows.
I just noticed my earlier change wasn't working either though, I think as long as we re-use the menu and make sure it isn't dropped we're OK.
#[cfg(target_os = "windows")] | |
{ | |
use winit::platform::windows::WindowExtWindows; | |
use winit::raw_window_handle; | |
let id = window.window_handle_any_thread().unwrap(); | |
if let raw_window_handle::RawWindowHandle::Win32(rwh) = rwh { | |
let hwnd = id.hwnd; | |
_ = menu.init_for_hwnd(hwnd as _); | |
} | |
} | |
#[cfg(target_os = "windows")] | |
{ | |
use winit::raw_window_handle::*; | |
if let RawWindowHandle::Win32(handle) = window.window_handle().unwrap().as_raw() { | |
menu.init_for_hwnd(handle.hwnd.get()).unwrap(); | |
} | |
} |
* Fix menu on windows * Refactor menu initialization * Fix missing gtk in CI * Create CONTRIBUTING.MD * Add info text
Needed to fix some issues regarding muda - looks like we never implemented some of it.
Had to allow deprecated on the closure approach for winit.
Stole our CI for multiplatform from dioxus
(todo) override opt-level for CI