hyprwm / Hyprland

Hyprland is an independent, highly customizable, dynamic tiling Wayland compositor that doesn't sacrifice on its looks.
https://hyprland.org
BSD 3-Clause "New" or "Revised" License
21.83k stars 908 forks source link

IPC: improve activelayout event #6298

Open cnt0 opened 5 months ago

cnt0 commented 5 months ago

Description

Hello. Hyprland IPC has the following event emitted on the layout change

activelayout>>KEYBOARDNAME,LAYOUTNAME

KEYBOARDNAME and LAYOUTNAME are separated by comma. The problem is that KEYBOARDNAME can contain commas as well. At least it does in my case, it starts with mistel-co.,ltd.-.

Such behavior complicates parsing, for example, shellevents can't correctly handle it right now.

I believe that instead of polluting IPC clients with workarounds we can fix the IPC itself. For example we can introduce the activelayoutv2>>LAYOUTNAME event, because who cares about keyboard names anyway. But I'd listen to suggestion first.

MahouShoujoMivutilde commented 5 months ago

~ 
❯ event_data='some,strange,name,layout' 

~ 
❯ echo "${event_data%,*}"                      
some,strange,name

~ 
❯ echo "${event_data#*,}" 
strange,name,layout

~ 
❯ echo "${event_data##*,}"
layout

But if we're talking about this - there are apparently layouts with commas too e.g.:

https://gist.github.com/MahouShoujoMivutilde/8a12c0d40e7b262f2c78ef6e01193542

because who cares about keyboard names anyway

I care*, because wtype, power button, my bluetooth headphones and possibly other unexpected things are "keyboards".

Here is wtype working:

activelayout>>cvirtualkeyboard,English (US)
activelayout>>cvirtualkeyboard,English (US)
activelayout>>cvirtualkeyboard,none
activelayout>>cvirtualkeyboard,error

* but it's obviously not mutually exclusive thing where we all have to settle to some ONE event to rule 'em all.

zjeffer commented 4 months ago

This is indeed a very annoying issue for IPC clients. I see the event is posted in InputManager.cpp:

https://github.com/hyprwm/Hyprland/blob/22138ac259b2f4253be29311f6b60fbd675074b4/src/managers/input/InputManager.cpp#L873-L874

and two other places in this file.

We could maybe replace the ',' in this line with something like a ;, but I feel this is not a very robust as some keyboard names or layout could contain a ; as well.

Maybe a better solution could be to escape the existing commas in the keyboard name and layout with a \? Does that make sense to do?

Vladimir-csp commented 3 months ago

Since issue title is quite generic, i'll add another improvement request: also output layout index. Otherwise it is harder to translate name to index for use in hyprctl switchxkblayout: activelayoutv2>>KEYBOARD_NAME;LAYOUT_INDEX;LAYOUT_NAME

ENDERZOMBI102 commented 3 months ago

encountered this , issue, but with window titles ;w;

I'd rather make separators globally use a value which can't be found in text, like \xFD, which can't be used as a character, so any conflict is avoided and its easy enough to parse, but with the possible problem which it is not a valid character, so not sure if would be handled correctly everywhere...

MahouShoujoMivutilde commented 3 months ago

I think it's better to rewrite the way IPC events are handled in general, to never run into the same issue ever again with any other event.

How?

For example

  1. Make SHyprIPCEvent.data a vector of strings, and change all its mentions accordingly
  2. Join these strings in CEventManager::formatEvent() with some separator, (hopefully) escaping it with some replaceAll(string, 'sep', '\sep') before that.

What separator?

Considering it's going to be used in scripts too - something reasonable, but rare. I think tab (\t) works well for this purpose. It's clean, it's easy to input ($'\t' in shell), and you're unlikely to see it anywhere unintentionally.

But, since this obviously breaks the API, might as well make some "socket3" with JSON, like waybar does.

JSON is awesome, because all the (un)escaping (can be) done for you, and typically there is some library lets you parse it automatically in some native object, bypassing the need to deal with (de)serialization entirely.

zjeffer commented 2 months ago

I like your second proposal: printing the data as JSON. It basically works with everything and it's very easy to parse.

cnt0 commented 2 months ago

I'm actually surprised that this whole IPC is not a json in the first place (like in i3/sway)

Vladimir-csp commented 2 months ago

Json isn't all that handy to deal with in shell (unless you just string a bunch of external tools on a pipe and call it a day). Radical notion: unit separator rules:

Oct   Dec   Hex   Char                        Oct   Dec   Hex   Char
───────────────────────────────────────────────────────────────────────
034   28    1C    FS  (file separator)        134   92    5C    \  '\\'
035   29    1D    GS  (group separator)       135   93    5D    ]
036   30    1E    RS  (record separator)      136   94    5E    ^
037   31    1F    US  (unit separator)        137   95    5F    _
zjeffer commented 2 months ago

Json isn't all that handy to deal with in shell

But the goal is to use it in IPC, I don't see any problems with JSON in that context? As far as external tools go for handling it in a shell, simply using jq can get you most of the way there.

Unit seperators are pretty cool but I feel like JSON is much more universally supported and understood.

Vladimir-csp commented 2 months ago

Unit seperators are pretty cool but I feel like JSON is much more universally supported and understood.

But, since this obviously breaks the API, might as well make some "socket3" with JSON

Unit separators could be an [initially] optional upgrade to socket2.

MahouShoujoMivutilde commented 2 months ago

Btw, as a workaround, you could use

Details

```bash #!/usr/bin/env bash event_data="some-weird,keyboard,Some,Layout" # so like # hyprctl devices -j | jq -r '.keyboards.[] | select(.main == true).name' kbd1="some-weird,keyboard" case "$event_data" in "$kbd1"*) echo "matched keyboard" echo "Layout is '${event_data##"$kbd1",}'" ;; *) echo "not our keyboard, skipping" ;; esac ```

to get

matched keyboard
Layout is 'Some,Layout'

This could be expanded to something generic, like

Details

```bash #!/usr/bin/env bash attached_keyboards=('some,kbd,(1)' 'some,kbd,(2)' 'some,kbd,(3)' 'some-kbd-4') parse_layout() { local event_data="$1" for kbd in "${attached_keyboards[@]}"; do case "$event_data" in "$kbd"*) echo "-> keyboard = '$kbd'" echo "-> layout = '${event_data##"$kbd",}'" ;; esac done } ## TEST VALUES event_datas=( 'some,kbd,(1),Aawdmkawdm,(Phonetic)' 'some,kbd,(2),Atrhjhyt Bryjytjjy,,wemrmmggr 4t43 t34t43 t3t54' 'some,kbd,(2),English (US)' 'some,kbd,(3),error' 'some-kbd-4,AAAAAA,AAAAAAAAA' 'some-kbd-4,YES' ) for data in "${event_datas[@]}"; do echo "$data" parse_layout "$data" done ```

Resulting in

Details

``` some,kbd,(1),Aawdmkawdm,(Phonetic) -> keyboard = 'some,kbd,(1)' -> layout = 'Aawdmkawdm,(Phonetic)' some,kbd,(2),Atrhjhyt Bryjytjjy,,wemrmmggr 4t43 t34t43 t3t54 -> keyboard = 'some,kbd,(2)' -> layout = 'Atrhjhyt Bryjytjjy,,wemrmmggr 4t43 t34t43 t3t54' some,kbd,(2),English (US) -> keyboard = 'some,kbd,(2)' -> layout = 'English (US)' some,kbd,(3),error -> keyboard = 'some,kbd,(3)' -> layout = 'error' some-kbd-4,AAAAAA,AAAAAAAAA -> keyboard = 'some-kbd-4' -> layout = 'AAAAAA,AAAAAAAAA' some-kbd-4,YES -> keyboard = 'some-kbd-4' -> layout = 'YES' ```

attached_keyboards could be populated like so

attached_keyboards=($(hyprctl devices -j | jq -r '.keyboards.[].name'))

And updated on ....device(added|removed) events that doesn't exist. Well. Updated on every function call then.