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

Allow custom cursor caching #3276

Merged
merged 3 commits into from
Dec 22, 2023

Conversation

daxpedda
Copy link
Member

@daxpedda daxpedda commented Dec 21, 2023

This is a proposal to change the API as discussed before in IRC and #3218.
The API now works like this:

let custom_cursor = CustomCursor::from_rgba(rgba, width, height, hotspot_x, hotspot_y)
    .unwrap()
    .build(&event_loop);
window.set_custom_cursor(&custom_cursor);

So now building a CustomCursor requires access to EventLoopWindowTarget, the cursor itself remains Send + Sync (the builder itself as well).

All backends work exactly the same as before, ergo they don't implement caching. I only implemented caching for Web.
I will make follow-up PRs for Windows and MacOS, where I know caching should be pretty straightforward and uncontroversial to implement.

I also made a big improvement in the Web implementation: we now wait for cursors to actually load before applying them. We had a really complex fallback cursor system in place beforehand.

Cc @kchibisov, @eero-lehtinen.

@daxpedda daxpedda added S - api Design and usability DS - web labels Dec 21, 2023
@daxpedda daxpedda force-pushed the custom-cursor-cache branch 2 times, most recently from ba4f5e2 to 5bb3dd4 Compare December 21, 2023 14:33
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

LGTM to me for X11 changes

@eero-lehtinen
Copy link
Contributor

eero-lehtinen commented Dec 21, 2023

Looks really nice and just like we talked about. Also implementing this shouldn't be too hard for x11 and wayland anymore because both of their cursor types already do cleanup with Drop. Wayland's SlotPool does the hard work for us.

@daxpedda
Copy link
Member Author

To summarize what I did for Web here because this was ridiculously complicated:

Before

  1. Used the object or user-supplied URL.
  2. URL still had to be loaded, even if we created the image and stored it as an object URL.
  3. During loading time the cursor switched back to the previous cursor, which we added as a fallback.

Unfortunately this isn't ideal, as we also didn't determine if the previous cursor has finished loading. It also complicated the code, storing and account for previous cursors.

After

  1. Loaded the object or user-supplied URL into an HTMLImageElement and waited for it to finish decoding.
  2. Store the HTMLImageElement to prevent the browser from cleaning up the newly decoded image.

This simplified the code a lot, but we also had to add a lot more code to handle all this correctly. Specifically the fact that HTMLImageElement can't be successfully dropped in another thread, so when we detect that we aren't on the main thread we send the HTMLImageElement to it to be cleaned up.

@daxpedda daxpedda added this to the Version 0.30.0 milestone Dec 22, 2023
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

API looks fine to me

src/cursor.rs Outdated Show resolved Hide resolved
@daxpedda daxpedda merged commit 2c15de7 into rust-windowing:master Dec 22, 2023
50 checks passed
@daxpedda daxpedda mentioned this pull request Dec 24, 2023
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DS - web S - api Design and usability
Development

Successfully merging this pull request may close these issues.

4 participants