Skip to content

add new wayland cursors #17

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

m4rch3n1ng
Copy link

@m4rch3n1ng m4rch3n1ng commented Mar 7, 2025

added the new dnd-ask and all-resize as specified in wayland/wayland-protocols!294. one concern is, that the names might not be final (especially for dnd-ask), and changing them later would be a breaking change. i think that could potentially be mitigated by adding a new enum variant and deprecating the current, but i also think that it will probably take some time (if it will even happen) for wayland to change the naming, so it should probably be fine for a while.

mutter falls back to dnd-copy and dnd-move while kwin falls back to copy and move. since this repo consistently uses the dnd-less versions, i decided to fall back to copy and move respectively.

i don't know what description to add to those: breeze doesn't have them and the only ones i could find for the new dnd-ask icon were in the adwaita docs and from bibata who are drawing them very differently. (though bibata does some other strange decisions like having a different cursor for dnd-copy and copy). if you want me to i could add a description like /// Often drawn as an arrow with either a small question mark or three dots next to it, but i just left it for now.

src/lib.rs Outdated
Comment on lines 255 to 260

/// Indicates that the user will select the action that will be carried out.
DndAsk,

/// Indicates that something can be moved or resized in any direction.
AllResize,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how to be with this one. The repo follows the w3c css cursor spec, thus such cursors are not present in it and would confuse the impl.

Maybe we should separate them into the platform specific enum, so we say that they are generally may not be present, etc?

Copy link
Author

Choose a reason for hiding this comment

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

there's an open issue for this at w3c/csswg-drafts#10115, but it doesn't seem like there's happening a lot there. i think even without this being added to the css standard adding these is useful: cursor-icon is used by winit, smithay and the smithay-client-toolkit and i think it would be annoying to not be able to set your winit cursor to all of the available ones.

maybe a native and a css cursor enum might make sense? but i don't know if that would complicate things.

Copy link
Member

Choose a reason for hiding this comment

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

There is precedent in keyboard-types for non-standard variants. As long as it's clearly documented, I'm on board with it here as well.

Though I would somewhat prefer to wait with adding these 'till they're actually finalized in the Wayland protocol (unless there's a good reason not to?).

Moreso, I'd be interested in seeing how Chrome and/or Firefox decide to expose these to the Web, if they do?

Copy link
Author

Choose a reason for hiding this comment

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

not sure what you mean by "finalized in the wayland protocol", but the new cursors were released in wayland-protocols 1.42, released on march 27th.

Copy link
Member

@kchibisov kchibisov May 22, 2025

Choose a reason for hiding this comment

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

he means w3c spec, not wayland spec, since this crate more or less represent w3c spec.

Copy link
Member

@madsmtm madsmtm May 22, 2025

Choose a reason for hiding this comment

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

No I meant the Wayland spec, I wasn't sure it was actually released

@Enokilis
Copy link

Enokilis commented Mar 8, 2025

I wouldn't bet on 'move' ever having its common use case reinterpreted when all three major platforms treat it as 'all-resize' is described. It would be really nice if the CSS spec were less ambiguous and had some meaningful reference cursors.

@madsmtm madsmtm added the enhancement New feature or request label May 22, 2025
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.

I am fine with this with the comments below resolved. Again, while this library does follow the W3C spec, I do not believe we need to limit ourselves to that (as long as it's clearly stated so in docs).

Comment on lines +328 to +329
CursorIcon::DndAsk => "dnd-ask",
CursorIcon::AllResize => "all-resize",
Copy link
Member

Choose a reason for hiding this comment

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

Either the docs for this function should be updated, we should panic here, or we should return a standard Web name like copy/move.

Copy link
Author

@m4rch3n1ng m4rch3n1ng May 22, 2025

Choose a reason for hiding this comment

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

updated the docs of the function. i would personally be against a panic! as that makes it more annoying for a wayland compositor to load cursor icons to render, like i do with my own compositor here.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

Noting down my thoughts for why this is the better solution (rather than returning copy/move), and why it's fine as a non-breaking change:
On Wayland, a 1-to-1 mapping will be useful when importing cursors.
On Web, Winit and similar "consumer" libraries will set the cursor CSS property for e.g. CursorIcon::AllResize to all-resize. This isn't supported by any (current) browser, and so the browser will fall back to the default icon - just like it will on e.g. the macOS backend, which also doesn't handle this case.

@m4rch3n1ng m4rch3n1ng force-pushed the add-a-few-missing-shapes branch from feddad0 to f64d8c1 Compare May 22, 2025 19:05
@madsmtm madsmtm requested a review from kchibisov May 22, 2025 19:19
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

We should clearly indicate in the docs that those 2 new icons are not part of the w3c spec we refer to and that they may generally not be present as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants