feat: load cursor from data

[Decodetalkers]

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

[codecov]

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.

❗ 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 ⚠️

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%)	⬇️
test--	78.08% <ø> (-2.99%)	⬇️
test--server_system	61.08% <ø> (-3.59%)	⬇️
test-client_system-	68.92% <ø> (-3.27%)	⬇️
test-client_system-server_system	51.32% <ø> (-3.64%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[elinorbgr]

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?

[Decodetalkers]

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

[elinorbgr]

I was rather thinking of defining the fallback closure one and for all as a standalone method of CursorTheme, so that the rest of the API does not need to change at all.

Or is having an independent closure for each time you retrieve the cursor, with all that error-processing machinery something that you need for your uses?

[Decodetalkers]

Emm, what do you mean , make the function out of CursorTheme? I am a little difficult to understand.. sorry

[elinorbgr]

My idea was to add to CursorTheme a method like this:

pub fn set_fallback(&mut self, fb: impl FnMut(&str) -> Cow<'static, [u8]>) { /* ... */ }

that stores a fallback closure inside the CursorTheme, that would then be used internally by get_cursor() if the system theme does not provide the requested cursor.

[Decodetalkers]

ha, I see

[Shinyzenith]

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

[elinorbgr]

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 ?

[Decodetalkers]

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]

done @elinorbgr

[MarijnS95]

While patching up the docs, is it intentional that this is called set_callback() and not set_fallback() (where the fallback is a callback that provides a fallback value when called)?

This function name and docs exclusively speak of a callback, but the get_cursor() function "referencing it" exclusively speaks of fallback, which is confusing on the user end.

[Decodetalkers]

oh.. it is a mistake. can you make a pr for me? thanks

[MarijnS95]

Can you do it? I don't know what name you want to make this into.

[Decodetalkers]

set_fallback, it was the author suggested. but then it become a mistake

[MarijnS95]

@Decodetalkers done in #723!