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

implement cursor-shape-v1 #844

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Conversation

m4rch3n1ng
Copy link
Contributor

@m4rch3n1ng m4rch3n1ng commented Sep 8, 2024

when a cursor is requested it first renders any Surface cursors, then it renders the named cursors giving priority to the cursors set by the set_shape function.

surprising behaviour to me at least is, that it gives priority to Hidden and Surface cursors over the cursors set by set_shape, but since that also was the old behaviour i didn't change it without getting feedback first.

i replaced the manual cursor cache insertion on startup (because i thought it was silly to repeat this for 34 cursors) and replaced it with a function that loads the cursors on demand.

i also changed the behaviour of Cursor::load, so that it first tries to load the default cursor of the theme instead of using the fallback cursor immediately.

and last i also changed two calls to Duration::from(time).as_millis() with a direct call to Time::as_millis() (as of Smithay/smithay@c700970).

as the actual last point i also didn't know how cursors work in xwayland, so i left that largely unchanged as well.

a lot of the noise in the commit is because i changed the set_shape() function to take a CursorIcon instead of a CursorShape and removed the CursorShape enum entirely.

@Drakulix
Copy link
Member

Drakulix commented Sep 8, 2024

Looks good on first glance, thanks for working on this!

when a cursor is requested it first renders any Surface cursors, then it renders the named cursors giving priority to the cursors set by the set_shape function.

surprising behaviour to me at least is, that it gives priority to Hidden and Surface cursors over the cursors set by set_shape, but since that also was the old behaviour i didn't change it without getting feedback first.

Yeah, I am not sure this is correct, as obviously shapes set by grabs should take precedence. And they might already, because grabs make the surface loose focus, so smithay should unset the CursorImageState. But that is just a coincidence, so it still might make sense to prefer explicitly set shapes by the compositor. Could you switch that around?

as the actual last point i also didn't know how cursors work in xwayland, so i left that largely unchanged as well.

Xwayland also uses Surface cursors and renders the cursor for us. We only need to provide it with a rendered default cursor to fallback to, so what you have done is totally fine.

@m4rch3n1ng
Copy link
Contributor Author

m4rch3n1ng commented Sep 8, 2024

done, but i'm not super happy with the .or(match, do you want me to move the stuff to it's seperate function and just call that twice?

Xwayland also uses Surface cursors and renders the cursor for us.

ah nice, that makes my job significantly easier

Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

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

done, but i'm not super happy with the .or(match, do you want me to move the stuff to it's seperate function and just call that twice?

Nah, that is fine.

Thanks! This looks good to go. :)

@Drakulix Drakulix merged commit 0ffe6ae into pop-os:master Sep 9, 2024
1 check passed
@m4rch3n1ng m4rch3n1ng deleted the cursor-shape-v1 branch September 9, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants