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 command to set keybaord layout #708

Closed
wants to merge 0 commits into from

Conversation

Leon-Plickat
Copy link
Member

No description provided.

@novakne
Copy link
Contributor

novakne commented Oct 17, 2022

Works as exoected for me.

That's not only about this PR since this the way commands works but finding wich value is wrong with an InvalidValue error is a bit annoying with a command with so many args in it, would be nicer if the error would print the wrong value. Just nitpicking though

@TAforever
Copy link

This is a very good idea. Could you create a waybar module in the future to display the keyboard layout?

@pmkap
Copy link
Contributor

pmkap commented Oct 17, 2022

Nice. Works well for me too. I think it would be nice if trailing _ could be left out. Lots of people probably only use the first argument.

Also, at this point, couldn't we make this keyboard specific very easily?

This is a very good idea. Could you create a waybar module in the future to display the keyboard layout?

No. It would need to be added to a protocol like river-status (and implemented).

@Leon-Plickat
Copy link
Member Author

Could you create a waybar module in the future to display the keyboard layout?

"You" as in me specifically? No. I don't use waybar and I generally am not interested in patching projects I don't use myself.

"You" in a general, "some random person" sense? Yes. I don't think we even need a custom protocol for that: Waybar should be able to just bind the wl_keyboard.

@Leon-Plickat
Copy link
Member Author

Also, at this point, couldn't we make this keyboard specific very easily?

No. There is nothing easy - or even sane - about per-keyboard layouts. It's basically a hack: It requires the compositor switching the layout live based on the last pressed key. It's overly complicated for very little return.

Unless I can see a real world, non-hypothetical use case that applies to more than just a single person, I see no reason to spend any time on that.

@TAforever
Copy link

Yes, by "you" I mean the developers of the river.
I like the river and I would like the river to develop and get better. Of the wayland compositors, we actually only have sway for now, but how can use manual tiling :) The fact is that waybar is the best bar for TWC and already supports tag and window title modules and I don’t think that these modules were made by the waybar developer. I also think that users need other layouts than English, and setting them using an environment variable is not the best solution.

@Leon-Plickat
Copy link
Member Author

Did anyone who tested this also experience the micro-stutter when loading a new layout? Wondering whether this is just a quirk of my system. I am not sure if anything can be done about it, since it seems to happen in xkbcommon.


That's not only about this PR since this the way commands works but finding wich value is wrong with an InvalidValue error is a bit annoying with a command with so many args in it, would be nicer if the error would print the wrong value.

I don't think xkbcommon is nice enough to tell us what wrong. Either way, I think proper error messages make sense for riverctl. Before we do that though, we probably want to change our protocol to send explicit requests instead of a string parsed by river, so that we can do all the verification and error messages client side.


I think it would be nice if trailing _ could be left out. Lots of people probably only use the first argument.

I generally prefer if things aren't too magic. Maybe we can use a configuration syntax that makes this more explicit, like maybe layout=de options=caps:swapescape variant=nodeadkeys. But that would be a pretty different to how other options work. Either way, that's bikeshedding and can always be done later.


Yes, by "you" I mean the developers of the river.

Well, I don't feel responsible for the general ecosystem around (our) Wayland compositor(s). The river modules available in waybar have been implemented by people who explicitly wanted to have them. It will be the same for a hypothetical keyboard layout module.

@TAforever
Copy link

TAforever commented Oct 21, 2022

I looked at the list of those who created the waybar modules and so
the river/tags module was created by Isaac Freund, and the hyprland modules were created by Vaxry, the main developer of hyprland. Therefore, if my request was excessive or impudent, then i asking you to forgive me.

@novakne
Copy link
Contributor

novakne commented Oct 21, 2022

Did anyone who tested this also experience the micro-stutter when loading a new layout?

Haven't experience something like this

@MaxVerevkin
Copy link
Contributor

I don't think we even need a custom protocol for that: Waybar should be able to just bind the wl_keyboard.

Are you sure that this is possible? I mean can a wayland client determine the active layout without receiving keyboard input? (I haven't tried this PR, but I don't think it is possible right now).

Copy link
Member

@ifreund ifreund left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the patch!

The implementation looks nice and clean, my only gripe is with the CLI design.

Comment on lines 333 to 336
*keyboard-layout* _layout_ _variant_ _model_ _options_ _rules_
Set the XKB layout for all keyboards. Use \_ for values you do not
want to set.

Copy link
Member

Choose a reason for hiding this comment

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

This feature looks good, but I'm not a huge fan of this CLI interface. It seems to me that most users will only want to specify only the layout. A few more will want to specify options, but I suspect very few users will set the model or rules for example.

Consider also the readability of

riverctl keyboard-layout de _ foo caps:swapesc _

vs

riverctl keyboard-layout -options caps:swapesc -model foo de

Copy link
Member Author

Choose a reason for hiding this comment

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

I was a bit hesitant to do something like this initially, considering command parsing is done server side and river feels like the wrong place for making the riverctl UX nice.

Anyway, I should have this done sometime this week.

@ifreund
Copy link
Member

ifreund commented Dec 2, 2022

To avoid any confusion, this PR was closed by accident and github has thwarted our attempts to re-open it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants