Skip to content

Allow custom cursor caching#3276

Merged
daxpedda merged 3 commits intorust-windowing:masterfrom
daxpedda:custom-cursor-cache
Dec 22, 2023
Merged

Allow custom cursor caching#3276
daxpedda merged 3 commits intorust-windowing:masterfrom
daxpedda:custom-cursor-cache

Conversation

@daxpedda
Copy link
Copy Markdown
Member

@daxpedda daxpedda commented Dec 21, 2023 •
edited
Loading

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 Affects the Web backend (WebAssembly/WASM) 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
notgull
notgull approved these changes Dec 21, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

eero-lehtinen commented Dec 21, 2023 •
edited
Loading

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.

eero-lehtinen
eero-lehtinen reviewed Dec 21, 2023
View reviewed changes
@daxpedda
Copy link
Copy Markdown
Member Author

daxpedda commented Dec 22, 2023

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
madsmtm
madsmtm approved these changes Dec 22, 2023
View reviewed changes
Copy link
Copy Markdown
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

@daxpedda daxpedda force-pushed the custom-cursor-cache branch from 1f03bbc to e38ad41 Compare December 22, 2023 21:07
@daxpedda daxpedda force-pushed the custom-cursor-cache branch from e38ad41 to c07b701 Compare December 22, 2023 21:14
@daxpedda daxpedda merged commit 2c15de7 into rust-windowing:master Dec 22, 2023
@daxpedda daxpedda mentioned this pull request Dec 24, 2023
Open
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@madsmtm madsmtm madsmtm approved these changes

@notgull notgull notgull approved these changes

@jackpot51 jackpot51 Awaiting requested review from jackpot51

@msiglreith msiglreith Awaiting requested review from msiglreith

@kchibisov kchibisov Awaiting requested review from kchibisov

+1 more reviewer

@eero-lehtinen eero-lehtinen eero-lehtinen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

DS - web Affects the Web backend (WebAssembly/WASM) S - api Design and usability

Milestone

Version 0.30.0

Development

Successfully merging this pull request may close these issues.

4 participants

@daxpedda @eero-lehtinen @madsmtm @notgull