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

Fix cross-platform mudabar, wire up better CI #75

Merged
merged 3 commits into from
Jun 20, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 86 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
name: CI

on:
pull_request:
push:
branches:
- main

name: CI
concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true

env:
RUSTDOCFLAGS: "-D warnings"
Expand Down Expand Up @@ -69,3 +73,84 @@ jobs:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
- run: cargo doc

# just cargo check for now
matrix_test:
runs-on: ${{ matrix.platform.os }}
if: github.event.pull_request.draft == false
env:
RUST_CARGO_COMMAND: ${{ matrix.platform.cross == true && 'cross' || 'cargo' }}
strategy:
matrix:
platform:
- {
target: x86_64-pc-windows-msvc,
os: windows-latest,
toolchain: "1.75.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why 1.75?

Copy link
Collaborator

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?

cross: false,
command: "test",
args: "--all --tests",
}
- {
target: x86_64-apple-darwin,
os: macos-latest,
toolchain: "1.75.0",
cross: false,
command: "test",
args: "--all --tests",
}
- {
target: x86_64-unknown-linux-gnu,
os: ubuntu-latest,
toolchain: "1.75.0",
cross: false,
command: "test",
args: "--all --tests",
}
- {
target: aarch64-apple-ios,
os: macos-latest,
toolchain: "1.75.0",
cross: false,
command: "build",
args: "--package dioxus-mobile",
Copy link
Collaborator

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?

Copy link
Collaborator

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

}
- {
target: aarch64-linux-android,
os: ubuntu-latest,
toolchain: "1.75.0",
cross: true,
command: "build",
args: "--package dioxus-mobile",
}

steps:
- uses: actions/checkout@v4
- name: install stable
uses: dtolnay/rust-toolchain@master
with:
toolchain: ${{ matrix.platform.toolchain }}
targets: ${{ matrix.platform.target }}
components: rustfmt

- name: Install cross
if: ${{ matrix.platform.cross == true }}
uses: taiki-e/install-action@cross

- name: Free Disk Space (Ubuntu)
if: ${{ matrix.platform.os == 'ubuntu-latest' }}
uses: jlumbroso/[email protected]
with: # speed things up a bit
large-packages: false
docker-images: false
swap-storage: false

- uses: Swatinem/rust-cache@v2
with:
key: "${{ matrix.platform.target }}"
cache-all-crates: "true"
save-if: ${{ github.ref == 'refs/heads/main' }}

- name: test
run: |
${{ env.RUST_CARGO_COMMAND }} ${{ matrix.platform.command }} ${{ matrix.platform.args }} --target ${{ matrix.platform.target }}
6 changes: 6 additions & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
Copy link
Collaborator

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?

Copy link
Contributor

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

"rust-analyzer.check.features": "all",
"rust-analyzer.cargo.features": "all",
"rust-analyzer.check.allTargets": true,
"rust-analyzer.cargo.allTargets": true
}
2 changes: 1 addition & 1 deletion packages/dioxus-blitz/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ edition = "2021"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
winit = "0.30.2"
winit = { version = "0.30.2", features = ["rwh_06"] }
muda = { version = "0.11.5", features = ["serde"] }
tokio = { workspace = true, features = ["full"] }
dioxus = { workspace = true }
Expand Down
5 changes: 4 additions & 1 deletion packages/dioxus-blitz/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use documents::DioxusDocument;
use muda::{MenuEvent, MenuId};
use std::collections::HashMap;
use url::Url;
use winit::event_loop::{EventLoop, EventLoopBuilder};
use winit::event_loop::EventLoop;
use winit::window::WindowId;
use winit::{
event::{Event, WindowEvent},
Expand Down Expand Up @@ -108,6 +108,9 @@ fn launch_with_window<Doc: DocumentLike + 'static>(window: View<'static, Doc>) {
#[cfg(not(any(target_os = "android", target_os = "ios")))]
let mut initial = true;

// the move to winit wants us to use a struct with a run method instead of the callback approach
// we want to just keep the callback approach for now
#[allow(deprecated)]
event_loop
.run(move |event, event_loop| {
event_loop.set_control_flow(ControlFlow::Wait);
Expand Down
65 changes: 40 additions & 25 deletions packages/dioxus-blitz/src/window.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
use crate::waker::UserWindowEvent;
use blitz::{RenderState, Renderer, Viewport};
use blitz_dom::DocumentLike;
use wgpu::rwh::HasWindowHandle;
use winit::keyboard::PhysicalKey;
use winit::window::WindowAttributes;

#[allow(unused)]
use wgpu::rwh::HasWindowHandle;

use std::sync::Arc;
use std::task::Waker;
use vello::Scene;
use winit::dpi::LogicalSize;
use winit::event::{ElementState, MouseButton};
use winit::event_loop::{ActiveEventLoop, EventLoopProxy};

#[cfg(any(
target_os = "linux",
target_os = "dragonfly",
Expand All @@ -23,7 +25,6 @@ use winit::platform::unix::WindowExtUnix;
use winit::platform::windows::WindowExtWindows;
Copy link
Collaborator

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.

use winit::{event::WindowEvent, keyboard::KeyCode, keyboard::ModifiersState, window::Window};

#[cfg(not(target_os = "macos"))]
use muda::{AboutMetadata, Menu, MenuId, MenuItem, PredefinedMenuItem, Submenu};

pub(crate) struct View<'s, Doc: DocumentLike> {
Expand Down Expand Up @@ -224,7 +225,7 @@ impl<'a, Doc: DocumentLike> View<'a, Doc> {
};

if let RenderState::Active(state) = &self.renderer.render_state {
state.window.set_cursor_icon(tao_cursor);
state.window.set_cursor(tao_cursor);
self.request_redraw();
}
}
Expand All @@ -248,7 +249,6 @@ impl<'a, Doc: DocumentLike> View<'a, Doc> {
winit::event::MouseScrollDelta::PixelDelta(offsets) => {
self.renderer.scroll_by(offsets.y)
}
_ => {}
};
self.request_redraw();
}
Expand Down Expand Up @@ -290,26 +290,14 @@ impl<'a, Doc: DocumentLike> View<'a, Doc> {
}))
.unwrap();

#[cfg(target_os = "windows")]
{
use winit::raw_window_handle::*;
if let RawWindowHandle::Win32(handle) = window.window_handle().unwrap().as_raw() {
build_menu().init_for_hwnd(handle.hwnd.get()).unwrap();
}
}
#[cfg(target_os = "linux")]
{
build_menu()
.init_for_gtk_window(window.gtk_window(), window.default_vbox())
.unwrap();
}
let menu_bar = build_menu();

// !TODO - this may not be the right way to do this, but it's a start
// #[cfg(target_os = "macos")]
// {
// menu_bar.init_for_nsapp();
// build_menu().set_as_windows_menu_for_nsapp();
// }
init_menu_bar(&menu_bar, &window);

// 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);
Copy link
Collaborator

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?


let size: winit::dpi::PhysicalSize<u32> = window.inner_size();
let mut viewport = Viewport::new((size.width, size.height));
Expand All @@ -334,7 +322,34 @@ impl<'a, Doc: DocumentLike> View<'a, Doc> {
}
}

#[cfg(not(target_os = "macos"))]
#[allow(unused)]
pub fn init_menu_bar(menu: &Menu, window: &Window) {
#[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 _);
}
}
Copy link
Contributor

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.

Suggested change
#[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();
}
}


// todo: menu on linux
// #[cfg(target_os = "linux")]
// {
// use winit::platform::unix::WindowExtUnix;
// menu.init_for_gtk_window(window.gtk_window(), window.default_vbox())
// .unwrap();
// }

#[cfg(target_os = "macos")]
{
use winit::platform::macos::WindowExtMacOS;
menu.init_for_nsapp();
}
}

fn build_menu() -> Menu {
let menu = Menu::new();

Expand Down
Loading