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

New option to hide cursor for DEs other than GNOME and KDE #3607

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

berkentekin
Copy link

@berkentekin berkentekin commented May 13, 2024

  • This PR adds an option to hide cursor for screenshots taken in DEs other than GNOME and KDE. The checkbox is therefore disabled in GNOME, KDE and non-UNIX systems.
  • In screenGrabber.cpp, the application decides to use either a DBus interface or the wlroots-based grim program depending on the desktop environment. Unfortunately, from what I understand, KDE and GNOME do not support wlr protocols, and the DBus interface does not provide an option to show or hide the cursor. Therefore, my changes (should) only apply to DEs that can utilize grim.
  • grim includes the cursor in screenshots with -c argument. This PR basically just allows passing this option.
  • The cursor is hidden by default.
  • For now, no CLI options to set this behaviour. If requested, I can mark this PR as WIP and work on that.

Partially resolves #3582, #604

GRIM provides a -c option to display cursor in screenshots. I added a new option to make us of this functionality. However, technically this option needs to be hidden/disabled under GNOME and KDE. I may address this in a later commit.
@berkentekin berkentekin changed the title New option to hide cursor for wlroots-based grim New option to hide cursor for DEs other than GNOME and KDE May 13, 2024
@mmahmoudian
Copy link
Member

mmahmoudian commented May 14, 2024

Thanks for the PR. I believe the correct course UX is to have only and only one checkbox that works universally on all DEs and OSs. What I mean is that the user should not care which feature they should toggle based on what software stack they are using. So if we cannot turn off cursor in Gnome Wayland or KDE or Windows, we should somehow show a message to the user and inform them why the checkbox is disabled.

In this implementation the Windows, Mac, and BSD users will just get a disabled checkbox without any explanation, and the Gnome and KDE users will get different tooltip.

I believe it is better if

  • we can show a red text above this checkbox to notify the user
  • include Mac and BSD in the mix by reversing the if statement condition to if linux -> if wayland -> if not gnome or not kde -> ...

This PR also addresses #604 in a way (giving control over showing cursor in Wayland)

@mmahmoudian
Copy link
Member

mmahmoudian commented May 14, 2024

Aesthetically I like it, but the point is that users rarely check the tooltip for more info. How about adding a ℹ️ or something similar at the end of such items that kind of invite/attract the user to click/hover to get the reason on why this is red.

Btw, thanks for your efforts and responsiveness 👍

Now red text will show above the cursor hiding option if the desktop environment does not support setting the behaviour
@berkentekin
Copy link
Author

berkentekin commented May 14, 2024

@mmahmoudian Thank you for your kind words.

On second thought I think adding a text is a better idea precisely because of the reason you've mentioned. I just deleted my comment because my design was such a minor difference that I didn't think it'd be worth mentioning... Besides, whether an unsupported environment actually grabs the cursor or not is outside our control, so informing the user about the issue on first sight resolves the ambiguity of a disabled-but-checked box.

In the end this is what it looks like:

Screenshot_20240514_190908

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.

Add an option to hide cursor on screenshots
2 participants