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

Update cursor on wl, x11 and gtk4 #605

Merged
merged 5 commits into from
Oct 23, 2023
Merged

Conversation

joantolo
Copy link
Contributor

@joantolo joantolo commented Oct 3, 2023

These changes allow the cursor to be updated when hovering on links or on input text fields. And the rest with the default pointer.

From a small conversation with @aperezdc on matrix, the idea was to create a signal at WebKitWebView and connect to it. However, the 'mouse-target-changed' signal already gives some hovering mouse info, but not all. For instance, that signal doesn't inform when hovering over text.

As a first step, I've used 'mouse-target-changed' signal.
A better signal could be emitted from PageClientImpl::setCursor.
But when debugging cog, I see that setCursor isn't used, I don't know why.

The next step would be to use the setCursor to expose a signal with all cursor changes and add more cursor names.

Fixes the cursor part of #594.

@joantolo joantolo force-pushed the update-cursor branch 2 times, most recently from 9cb484e to 934f99b Compare October 6, 2023 17:37
@aperezdc aperezdc self-requested a review October 17, 2023 13:53
@aperezdc aperezdc self-assigned this Oct 17, 2023
@aperezdc aperezdc added the enhancement New feature or request label Oct 17, 2023
Copy link
Member

@aperezdc aperezdc left a comment

Choose a reason for hiding this comment

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

@joantolo I have a couple of comments, could you please take a look? While at it, please use the data/check-style script to ensure the patch follows the same coding style as the rest of the source code.

Regarding the usage of the signal, I agree that it would be ideal to plumb WebKit's PageClientImpl::setCursor() instead to make it work for WPE; but that will need either new public API either in libwpe or in WPE WebKit itself, which would be way more effort.

In the meantime, I would be more than happy to have this implementation merged in Cog, because it's better than nothing. Thanks for working on this!

platform/gtk4/cog-platform-gtk4.c Outdated Show resolved Hide resolved
platform/gtk4/cog-platform-gtk4.c Outdated Show resolved Hide resolved
Copy link
Member

@aperezdc aperezdc left a comment

Choose a reason for hiding this comment

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

@joantolo This is looking good, there's only one last detail before we can merge this: we need to add libxcb-cursor-dev in the list of packages to install during the CI builds, in .github/workflows/ci-native.yml.

If you could have a moment to rebase the patch and do that small change, then we can merge this. Thanks again!

This definitions are used when creating cursors for the different
platforms using the names.
For each cursor type there is an array of names sorted by preference.
The cursor is updated from mouse-target-changed signal.

It can be: hand, text or left_ptr.

To get each cursor it is used the cursor_theme_get_cursor func with the
cursor_names definitions.
The cursor is updated from mouse-target-changed signal.

It can be: hand, text or left_ptr.

This is done using the xcb cursor library.

To get each cursor it is using the xcb_cursor_load_cursor func with the
cursor_names definitions.
The cursor is updated from mouse-target-changed signal.

It can be: hand, text or left_ptr.

To get each cursor it is using the gdk_cursor_new_from_name func with the
cursor_names definitions.
@joantolo
Copy link
Contributor Author

Added libxcb-cursor-dev and rebased.

Copy link
Member

@aperezdc aperezdc left a comment

Choose a reason for hiding this comment

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

Thanks @joantolo, this is now ready to merge!

@aperezdc aperezdc merged commit f2af3c0 into Igalia:master Oct 23, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants