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

Add Support for ydotool #212

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

Add Support for ydotool #212

wants to merge 5 commits into from

Conversation

relma2
Copy link

@relma2 relma2 commented Dec 22, 2024

🪛 ydotool

Added support for ydotool as a "typer" for rofimoji.

This typer requires a service called ydotoold to be running, but gets arround the limitations on Wayland of wtype not supporting the compositor. Should probably update the documentation to instruct users as to how to use ydotool.

Also updated the is_supported() condition for wtype to account for the compositor issue.

I tested my changes with all the selectors for both the clipboard and type actions.

Resolves issue GH-211

KNOWN ISSUES:

  • The X-ray emoji 🩻 and Screwdriver emoji 🪛 keeps typing as Ǻ for some reason on the "type" action.
    • This issue could be on other emojis, please help. Smiley emojis seem to be unaffected
  • Spaghetti code everywhere

@relma2
Copy link
Author

relma2 commented Dec 23, 2024

Ok I fixed the issue with typing that A with the circle on top

@relma2
Copy link
Author

relma2 commented Dec 23, 2024

@fdw Please take a look at my PR 🥰

@fdw
Copy link
Owner

fdw commented Dec 27, 2024

Thanks for the PR! 🙂
Since this is quite a large pull request doing many things, I have a few comments:

The wtype warning
I'm not a fan of the new warning for wtype - there is already a warning in these circumstances. Granted, it's not an ideal one, but it's the official one from wtype and people can solve that problem themselves.
But adding that check first adds a performance penalty every time rofimoji needs to find a typer (so probably at every start), and it adds a hard dependency on that exact error string. If wtype ever changes it, rofimoji is suddenly broken.
And finally, how many people have wtype installed even though it doesn't work for them? In that case, I think it's okay to ask them to specify a typer themselves.

The numerical input
This seems to be not specific to ydotool, but is supported by at least GTK and QT. So I would like to move that to a new action numerical-input that is supported by all typers.
The way to implement this is in action.py, using the existing method __get_codepoints that we can hopefully re-use (with minor modifications). Checking some Linux header file is not needed and adds a hard dependency that I would really like to avoid.
Also, each typer probably needs a new method to send the ctrl and shift keycodes.

ydotool typer
After the keycode stuff is moved out, it looks quite well already. But I think it still tries to do too much to find out whether ydotool is supported. Do we really need to pass the socket? Is it not able to find that itself?
Also, what happens if ydotoold is not started with systemd? Should it really fail then? I'd rather let ydotool take care of that problem itself - if it can't connect to its daemon, let it report its own problem. Trying to wrap other program's error messages forces us to re-implement their logic, i.e. a hard dependency.

One final request:
Could you please split your changes into separate commits so that each is independent of the others? In this case, each of the topics above should be its own commit.
This way, it's easy to later find out why a change happened. If everything is in one commit, it's harder to disentangle them.

Have I overlooked something? Do you disagree somewhere?

@relma2
Copy link
Author

relma2 commented Dec 27, 2024

Oh! So you want to implement the Ctrl+Shift+U<your unicode code> thing for all the typers? That's interesting, but I don't think that one is necessary since the other typers can directly "type" emojis without the "numerical_input" workaround.

As for the hard errors on wtype and ydotoold, I can move those out of the code; but I'd want to update the documentation so that users of rofimoji can troubleshoot if they get the error message from the typer.

Changes:
  - Added ydotool.py
  - updated README
  - added ydotool to possible typer arguments
  - added helper methods to convert to/from unicode and ydotool keycodes
  - added ydotool to the list of supported typers
  - Changed the supported() method of wtype to return False if the compositor does not support virtual keyboard
@relma2
Copy link
Author

relma2 commented Dec 27, 2024

Also, as for the socket thing; ydotoold selects a socket path depending on how it is running. If run without sudo or with systemd, it will select /run/user/1000/.ydotool_socket; and if run with sudo, it will select /tmp/.ydotool_socket, but since ydotool by default looks in /run/user/1000/.ydotool_socket, it will fail if the daemon is running independently with sudo. That is why I pass in the socket to the environment because it may sometimes need help finding it

This removes the checking for specific error messages on wtype and
ydotool to determine compatibility. Added a "Troubleshooting" section
to the README to help users with common issues with the typers.
@fdw
Copy link
Owner

fdw commented Dec 27, 2024

Oh! So you want to implement the Ctrl+Shift+U thing for all the typers? That's interesting, but I don't think that one is necessary since the other typers can directly "type" emojis without the "numerical_input" workaround.

I don't think it's necessary for the other typers, but it should work with them and there is no harm in implementing it like that, I think.

As for the hard errors on wtype and ydotoold, I can move those out of the code; but I'd want to update the documentation so that users of rofimoji can troubleshoot if they get the error message from the typer.

Adding it to the documentation is better than adding it to the code, but the Readme is already very long. I would have expected people to search for the error message and then finding the tool's own documentation, which is more extensive and up-to-date than rofimoji's can be.

Also, as for the socket thing [...]

But all the current code is doing is checking whether YDOTOOL_SOCKET is set, and if so, passing the value on in the same variable. So, nothing is changed. Or am I missing something?

Changes implemented by adding a new method to actions.py which gets
the "event code" of a key. This method is used in the ydotool.py
script to get the event code of a key and then use it to send the
key press event to the system.
Has a helper dictionary to map key names to event codes in
input_event_codes.py

Also I cleaned up some code; removed the socket thing.
@relma2
Copy link
Author

relma2 commented Dec 27, 2024

I would have expected people to search for the error message [...]

Do not underestimate people's capacity for being dumb.

Also, you were right about the socket thing. I don't need to pass in an env variable. Fixed.

Lastly, a numerical-input action should be a separate PR.

@fdw
Copy link
Owner

fdw commented Dec 27, 2024

I would have expected people to search for the error message [...]

Do not underestimate people's capacity for being dumb.

True, but that doesn't mean I have to take responsibility for all possible problems. If they open an issue here, I always try to help, but I think it's also fair to expect everyone to solve their problems on their own first. If I wanted to collect all possible errors and weird circumstances, I'd be quite busy.
Also, rofimoji has a rather "niche" audience, with Linux and not one of the usual DMs. These are not average users 😉
Finally, I believe in educating people: If I solve your problem, you learn nothing except asking me for help. If I show you how to solve a problem, you can hopefully fix the next one yourself.

Also, you were right about the socket thing. I don't need to pass in an env variable. Fixed.

Thank you 🙂

Lastly, a numerical-input action should be a separate PR.

As you want 🙂 But then we should do that first, as ydotool depends on it, and I don't want to merge code that's gonna be removed soon.

@relma2
Copy link
Author

relma2 commented Dec 27, 2024

Added Pull Request #213 to implement numerical action

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