Add RGBA cursor icon support on Windows/macOS/Linux #2117

rofferom · 2021-12-28T16:25:38Z

This MR aims to be a shorter version of #1617. Note this is currently a draft to discuss if the global approach is valid from the maintainers point of view.

The goal is to have a lower level API (compared to #1617) that only provides a RGBA bitmap, its size, and the cursor hotspot, with empty implementation easy to add for other platforms.

However, I have some remarks about my patch

I'm quite noobish in Rust, and it is my first contribution attempt to winit. I'm pretty sure my patch is a mess :)
The naming is dirty, but I'm confident we can find something better
I can also provide a X11 implementation if requested
The Windows implementation is basically a port of SDL2 code
The whole CursorRgba content must be confirmed

Checklist

Tested on all platforms changed
Compilation warnings were addressed
cargo fmt has been run on this branch
cargo doc builds successfully
Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
Created or updated an example program if it would help users understand this functionality
Updated feature matrix, if new features were added or implemented

msiglreith

Sorry for the silence! Some first feedback from my side.

msiglreith · 2022-01-23T12:03:50Z

src/platform_impl/windows/window_state.rs

+        }
+    }
+
+    pub fn display(&self) {


this could be moved to CursorHandle, which removes the duplicate logic in WM_SETCURSOR and set_cursor_ api.

msiglreith · 2022-01-23T12:05:21Z

src/platform_impl/windows/window_state.rs

@@ -42,9 +48,100 @@ pub struct SavedWindow {
    pub placement: winuser::WINDOWPLACEMENT,
 }

-#[derive(Clone)]
+pub struct RgbaHandle {
+    pub hcursor: AtomicPtr<HICON__>,


It would be good to reuse the RaiiIcon struct here imo, mirroring WinIcon

Actually HICON__ is a raw pointer that can't be sent, I can't use RaiiIcon as is :

error[E0277]: `*mut HICON__` cannot be sent between threads safely --> src/platform_impl/windows/window.rs:556:30 | 556 | self.thread_executor.execute_in_thread(move || { | ^^^^^^^^^^^^^^^^^ `*mut HICON__` cannot be sent between threads safely | = help: within `WindowState`, the trait `Send` is not implemented for `*mut HICON__`

My remark is outdated because of the switch to windows_sys that uses integer instead of raw pointers. I'll try to update it.

msiglreith · 2022-01-23T12:09:21Z

src/window.rs

@@ -961,6 +966,15 @@ impl Default for CursorIcon {
    }
 }

+#[derive(Clone)]
+pub struct CursorRgba {
+    pub xhot: u16,


Extent and offset related parts of the API should use dpi::{PhysicalPosition, PhysicalSize} or dpi::{LogicalPosition, LogicalSize} to make it dpi aware. (I haven't checked so far how scaling logic applies in this case)

rofferom · 2022-10-17T09:36:26Z

I have worked recently on a macOS implementation too, but it is based on the same old winit version used by this merge request. I'm going to rebase it to update my changes.

The initial version of the pull request is quite old, and I didn't take enough time to improve it, sorry about that.

rofferom · 2022-10-18T08:13:01Z

I have worked recently on a macOS implementation too, but it is based on the same old winit version used by this merge request. I'm going to rebase it to update my changes.

Update pushed with support of Windows, Linux and macOS (the name of the source branche is now wrong 🙈).

Some remarks

This is still a draft/proof of concept. I think a lot of things should be improved, like the CursorRgba API
Android/iOS/Web build need to be fixed. I will add an initial commit with the public API with empty implementations for all platforms once the public API will be correctly defined
Windows : Because windows_sys is now used by winit, some issues I had previously about HICON__ being raw pointer are not valid anymore. It will be easier to make a cleaner version
Wayland: Not implemented
macOS:
- I'm not really used to Objective C, I just imported the SDL2 way to setup custom cursors. The integration in the project is dirty in the current version
- I had a weird issue with the old winit version, I couldn't work on my Vec<> in the callback. I haven't retested it with the new version

madsmtm

Yeah, the macOS code is a bit dirty, but workable, so don't worry much about that.

Perhaps if you made or extended an example it would help to showcase the feature (and to make it easier to test).

A fair warning, I don't think anyone really has the time nor drive to push this feature to completion, so apologies in advance for that!

src/platform_impl/macos/appkit/bitmap_image.rs

madsmtm · 2022-11-22T08:26:53Z

src/platform_impl/macos/appkit/cursor.rs

+        unsafe {
+            std::ptr::copy_nonoverlapping(
+                cursor.data.as_ptr() as *const u8,
+                pixels,
+                cursor.width as usize * cursor.height as usize * std::mem::size_of::<u32>(),
+            )
+        };


Note: It is only safe to modify the pixels pointer derived from an immutable reference to NSBitmapImageRep because it contains UnsafeCell internally.

Not sure to understand what you mean 😶

madsmtm · 2022-11-22T08:29:46Z

src/window.rs

@@ -1310,6 +1315,15 @@ impl Default for CursorIcon {
    }
 }

+#[derive(Clone)]
+pub struct CursorRgba {


We already have Icon struct, could these perhaps be merged somehow? Unsure how exactly though.

Yes this is a remark on my initial comment: the API is ugly. I wanted to make a basic initial implementation at first to have a something to discuss about.

I think adding a Rgba struct-like entry in Icon could be a good idea too. Maybe the other maintainers have an opinion about this ?

I think an entry in Icon would be a good idea, that is also basically what's proposed in the original PR

src/platform_impl/macos/window.rs

madsmtm · 2022-11-22T08:38:14Z

src/window.rs

+    pub width: u32,
+    pub height: u32,


How does DPI work wrt. cursors? Would it perhaps make sense to do:

pub hotspot: PhysicalPosition<u32>, pub size: PhysicalSize<u32>,

I think that Windows change the cursor size, depending of the scaling. I should make some tests on various platforms to check the behavior.

madsmtm · 2022-11-22T08:45:51Z

src/window.rs

+    pub yhot: u32,
+    pub width: u32,
+    pub height: u32,
+    pub data: Vec<u32>,


While it may be efficient, I quite dislike the opaqueness of this data. Rather, let's use a struct Rgba { red: u8, green: u8, blue: u8, alpha: u8 } or similar?

Or maybe if we're venturing this far, we should just straight up start using the rgb crate, or integrate with the image crate directly?

Yes this is ugly, I must clean this part.

Use a crate to avoid to redefine a custom struct could be a good idea. I think image is a big one, rgb could be a better choice. However the crate to use should be decided by the maintainers.

daxpedda · 2023-08-23T13:46:32Z

Closing due to inactivity.

Tracked in #3005.

maroider added the DS - windows label Dec 28, 2021

rofferom changed the title ~~Add custom cursor icon support on Windows~~ Add RGBA cursor icon support on Windows Dec 28, 2021

rofferom force-pushed the windows-custom-cursor-icon branch from 5e712be to 78999c9 Compare January 6, 2022 11:05

rofferom force-pushed the windows-custom-cursor-icon branch from 78999c9 to 24919b1 Compare January 21, 2022 09:56

msiglreith reviewed Jan 23, 2022

View reviewed changes

msiglreith added the S - api Design and usability label Jan 23, 2022

Herschel mentioned this pull request May 22, 2022

avm2: Start of Mouse Cursor APIs ruffle-rs/ruffle#7055

Closed

rofferom force-pushed the windows-custom-cursor-icon branch from 11a7e4f to 5430e42 Compare October 18, 2022 08:04

rofferom requested review from madsmtm and kchibisov as code owners October 18, 2022 08:04

rofferom changed the title ~~Add RGBA cursor icon support on Windows~~ Add RGBA cursor icon support on Windows/macOS/Linux Oct 18, 2022

rofferom added 2 commits October 18, 2022 10:19

Add custom cursor icon support on Windows

a1e9b27

Add custom cursor icon support on Linux

aa301c8

rofferom force-pushed the windows-custom-cursor-icon branch from 5430e42 to fd5d844 Compare October 18, 2022 08:19

Add custom cursor icon support on macOS

881d7c7

rofferom force-pushed the windows-custom-cursor-icon branch from fd5d844 to 881d7c7 Compare October 18, 2022 09:22

madsmtm requested changes Nov 22, 2022

View reviewed changes

rofferom added 2 commits November 23, 2022 16:31

fixup! Add custom cursor icon support on macOS

a25d817

Add two RGBA cursor in cursor example

e015307

madsmtm mentioned this pull request Aug 5, 2023

Custom cursor icons #3005

Closed

daxpedda closed this Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RGBA cursor icon support on Windows/macOS/Linux #2117

Add RGBA cursor icon support on Windows/macOS/Linux #2117

rofferom commented Dec 28, 2021 •

edited

Loading

msiglreith left a comment

msiglreith Jan 23, 2022

rofferom Jan 24, 2022

msiglreith Jan 23, 2022

rofferom Jan 24, 2022

rofferom Nov 23, 2022

msiglreith Jan 23, 2022

rofferom Jan 24, 2022

rofferom commented Oct 17, 2022

rofferom commented Oct 18, 2022 •

edited

Loading

madsmtm left a comment

madsmtm Nov 22, 2022

rofferom Nov 23, 2022

madsmtm Nov 22, 2022

rofferom Nov 23, 2022

madsmtm Nov 23, 2022

madsmtm Nov 22, 2022

rofferom Nov 23, 2022

madsmtm Nov 22, 2022

rofferom Nov 23, 2022

daxpedda commented Aug 23, 2023

Add RGBA cursor icon support on Windows/macOS/Linux #2117

Add RGBA cursor icon support on Windows/macOS/Linux #2117

Conversation

rofferom commented Dec 28, 2021 • edited Loading

msiglreith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rofferom commented Oct 17, 2022

rofferom commented Oct 18, 2022 • edited Loading

madsmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxpedda commented Aug 23, 2023

rofferom commented Dec 28, 2021 •

edited

Loading

rofferom commented Oct 18, 2022 •

edited

Loading