wayland-rs icon indicating copy to clipboard operation
wayland-rs copied to clipboard

feat: load cursor from data

Open Decodetalkers opened this issue 1 year ago • 3 comments

I want to add a way to load cursor from raw data

Decodetalkers avatar Dec 07 '23 14:12 Decodetalkers

Codecov Report

Attention: Patch coverage is 0% with 39 lines in your changes are missing coverage. Please review.

Project coverage is 72.68%. Comparing base (ea03873) to head (610e93a). Report is 4 commits behind head on master.

:exclamation: Current head 610e93a differs from pull request most recent head 85585de. Consider uploading reports for the commit 85585de to get more accurate results

Files Patch % Lines
wayland-cursor/src/lib.rs 0.00% 39 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #680      +/-   ##
==========================================
- Coverage   76.16%   72.68%   -3.49%     
==========================================
  Files          45       48       +3     
  Lines        8153     7879     -274     
==========================================
- Hits         6210     5727     -483     
- Misses       1943     2152     +209     
Flag Coverage Δ
main 58.04% <0.00%> (-1.03%) :arrow_down:
test-- 78.08% <ø> (-2.99%) :arrow_down:
test--server_system 61.08% <ø> (-3.59%) :arrow_down:
test-client_system- 68.92% <ø> (-3.27%) :arrow_down:
test-client_system-server_system 51.32% <ø> (-3.64%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 07 '23 15:12 codecov[bot]

Thanks, that is interesting, however I'm not sure this design is the whole way we want to go.

As I understand, the goal is to allow an app to provide fallback cursor images by itself in case the system theme does not provide all the ones it needs.

The method you introduce directly creates a Cursor instance, storing its contents using the SHM pool of the theme, but without any other checks, nor inserting it into the internal state. As a result the user will have to keep the Cursor instance around themselves (as the CursorTheme does not store it) and check to use it whenever they need this particular cursor, to avoid reloading a new copy in memory every time.

I think a better approach would be to introduce a proper mechanism by which the user of wayland-cursor can specify a general fallback mechanism. A possible implementation of that would be to let the user provide a fallback Fn(&str) -> Cow<'static, [u8]> closure that would be invoked by load_cursor if the requested cursor was not found in the system theme. (The Cow<'static, [u8]> return type being used so that this closure could either return a reference to some cursor data bundled in the executable itself, or load the contents from disk in the app files.)

With such a fallback mechanism, later use of the CursorTheme would be transparent, and everything would "just work".

What do you think of this, would that cover your needs?

elinorbgr avatar Dec 08 '23 10:12 elinorbgr

Ok, I have made change. I also think that is bad.. so I am also thinking a better way..

Decodetalkers avatar Dec 08 '23 13:12 Decodetalkers

Hi gentle ping: @elinorbgr Can we get a review on this? 😄

Shinyzenith avatar Apr 06 '24 19:04 Shinyzenith

Sorry for the long time, this kinda slipped outside of my radar...

LGTM overall, but:

* the `Fallback` function should take a size as argument in addition to the cursor name, like the `load_cursor`, making it possible to have multiple sizes for the fallback cursor if desired

* Could you expand the documentation of `set_fallback()` to make it clearer what that callback is supposed to do, and how it's meant to be used ?

Ok, is that ok? I have pushed another commit

Decodetalkers avatar Apr 11 '24 11:04 Decodetalkers

done @elinorbgr

Decodetalkers avatar Apr 23 '24 12:04 Decodetalkers