fix: change layout event's separator

[s1syph0s]

Describe your PR, what does it fix/add?

The layout event separator is changed from "," to ";" to improve parsing ability, since some keyboard can also have "," in its name. Alexays/Waybar#2137 Alexays/Waybar#3224

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

Breaks compability with waybar's hyprland/language module.

Is it ready for merging, or does it need work?

This is ready for merging

[MahouShoujoMivutilde]

I think adding another event, e.g. activelayoutv2 with ; as a separator would better, to not break compatibility with everything else that uses it.

Original activelayout could be then declared deprecated in the next version, if needed, and in the meantime a note in the wiki can be added to explain why v2 is the one that should be used now.

[s1syph0s]

Thanks for the input! I'll fix this soon

[MahouShoujoMivutilde]

Rereading a related issue, someone here #6298 (comment) said that some keyboards could contain ; in their names. I didn't see anything like that, but sure.

Maybe let's just use ;;;, or |, or something else extremely unlikely to appear in the name, but easy to split by?

Btw, see activewindow and activewindowv2, no need to EMIT_... twice, as it seems to be internal Hyprland's events, which aren't serialized in strings where splitting would matter, as opposed to the ones posted at socket2.

Hyprland/src/Compositor.cpp

Lines 1007 to 1010 in 4fdc0d5

	g_pEventManager->postEvent(SHyprIPCEvent{"activewindow", ","});
	g_pEventManager->postEvent(SHyprIPCEvent{"activewindowv2", ""});
	EMIT_HOOK_EVENT("activeWindow", (PHLWINDOW) nullptr);

or

Hyprland/src/Compositor.cpp

Lines 2031 to 2033 in 4fdc0d5

	g_pEventManager->postEvent(SHyprIPCEvent{"moveworkspace", PWORKSPACEB->m_szName + "," + pMonitorA->szName});
	g_pEventManager->postEvent(SHyprIPCEvent{"moveworkspacev2", std::format("{},{},{}", PWORKSPACEB->m_iID, PWORKSPACEB->m_szName, pMonitorA->szName)});
	EMIT_HOOK_EVENT("moveWorkspace", (std::vector<std::any>{PWORKSPACEB, pMonitorA}));

[vaxerski]

you're avoiding the problem instead of fixing it. Just replace ,s with a space or something?

[MahouShoujoMivutilde]

That would mean that IPC users would need to do that too, if they want to match a particular keyboard / layout, because now .name and .active_keymap won't match between hyprctl devices (or config) and events at socket2.

Having more unique separator character feels like a simpler change.

[vaxerski]

\,?

[MahouShoujoMivutilde]

As for escaping commas in keyboard / layouts - that would mean you still can't just split for it and get what you want (the same thing that hyprctl devices -j has).

❯ echo "activelayout>>some\,weird\,keyboard,Some\,Layout" | awk -F '>>' '{print $2}' | awk -F ',' '{print $2}'
weird\

You'll need to (pre/post)process it like e.g.

❯ echo "activelayout>>some\,weird\,keyboard,Some\,Layout" | awk -F '>>' '{print $2}' | sed 's#\\,#~#g' | awk -F ',' '{print $1}' | sed 's#~#,#g'
some,weird,keyboard

~ 
❯ echo "activelayout>>some\,weird\,keyboard,Some\,Layout" | awk -F '>>' '{print $2}' | sed 's#\\,#~#g' | awk -F ',' '{print $2}' | sed 's#~#,#g'
Some,Layout

As separator - it still has problem, at least for awk,

~ 
❯ echo "activelayout>>some,weird,keyboard\,Some,Layout" | awk -F '>>' '{print $2}' | awk -F '\,' '{print $1}' 
awk: warning: escape sequence `\,' treated as plain `,'
some

~ 
❯ echo "activelayout>>some,weird,keyboard\,Some,Layout" | awk -F '>>' '{print $2}' | awk -F '\\,' '{print $1}' 
awk: warning: regexp escape sequence `\,' is not a known regexp operator
some

but that's fixable in awk and isn't a problem in a saner choose

❯ echo "activelayout>>some,weird,keyboard\,Some,Layout" | awk -F '>>' '{print $2}' | awk -F '\\\\,' '{print $1}' 
some,weird,keyboard

~ 
❯ echo "activelayout>>some,weird,keyboard\,Some,Layout" | awk -F '>>' '{print $2}' | awk -F '\\\\,' '{print $2}'
Some,Layout

This could work, but is worse than just unique not escaped character.

Admittedly, || is also not as neat as I thought it would be:

 echo "activelayout>>some,weird,keyboard||Some,Layout" | awk -F '>>' '{print $2}' | awk -F '||' '{print $1}'
some,weird,keyboard||Some,Layout

~ 
❯ echo "activelayout>>some,weird,keyboard||Some,Layout" | awk -F '>>' '{print $2}' | awk -F '||' '{print $2}'


~ 
❯ echo "activelayout>>some,weird,keyboard||Some,Layout" | awk -F '>>' '{print $2}' | awk -F '\|\|' '{print $2}' 
awk: warning: escape sequence `\|' treated as plain `|'


~
❯ echo "activelayout>>some,weird,keyboard||Some,Layout" | awk -F '>>' '{print $2}' | awk -F '\\|\\|' '{print $2}' 
Some,Layout

~ 
❯ echo "activelayout>>some,weird,keyboard||Some,Layout" | awk -F '>>' '{print $2}' | awk -F '\\|\\|' '{print $1}'
some,weird,keyboard

But that's, again, purely awk's problem. For waybar it wouldn't matter.

But ;;; as a separator would work everywhere, and is more intuitive

❯ echo "activelayout>>some,weird,keyboard;;;Some,Layout" | awk -F '>>' '{print $2}' | awk -F ';;;' '{print $1}' 
some,weird,keyboard

~ 
❯ echo "activelayout>>some,weird,keyboard;;;Some,Layout" | awk -F '>>' '{print $2}' | awk -F ';;;' '{print $2}'
Some,Layout

You get what you split for, no need to learn any escaping.

[s1syph0s]

Should I just replace the separator from || to ;;?

[cnt0]

I'd propose something like

activelayoutv2>>KEYBOARDNAME,LAYOUTNAME,LAYOUTNAMEPOS

where integer LAYOUTNAMEPOS points to the beginning of LAYOUTNAME in the event string.

This allows us to 1) correctly parse the event regardless of layout and keyboard name contents 2) maintain the backwards compatibility with activelayout, so we actually don't need to introduce the new event at all.

[s1syph0s]

This sounds interesting! However, using , as a separator won't solve the problem, since the KEYBOARDNAME and LAYOUTNAME could contain , (Alexays/Waybar#3224 (comment)).

A more extreme solution is to output the event string as a JSON, which requires us to change all event outputs.

[vaxerski]

This could work, but is worse than just unique not escaped character.

dumb statement, what is there to make you so sure there is any character that will forever be unique?

[MahouShoujoMivutilde]

Practical experience of how humans make up names for things meant to be read by other humans?

; is rather common part of written language, so to see it in some Gaymer Keyboard (uwu edition; international) is more likely than ;;; or || or a \t (tab).

Maybe someone who has a keyboard with a fully programmable controller in it can mess this up.

But if you expect a completely arbitrary text in the name and layout, sure, escaping the separator character is the way.

If we're being this pedantic, escaping the separator during the serialization and unescaping on user side should always be done, for all events with arbitrary output, and maybe all for the sake of consistency / simpler maintainability should the expected text change later.

I just think that the less unescaping the user needs to do, the better.

[vaxerski]

escaping is a common practice and I still believe \, is the way. It comes with a free bonus of a) consistency and b) no breaking of the api

[s1syph0s]

I tried to use \, as the separator, but it is an unknown escape sequence. If I build this and gets the event output, I will receive this, which has no difference:

activelayout>>wl_keyboard,English (US)
activelayoutv2>>wl_keyboard,English (US)

If I remove the v2 for the second output and put both in a diffcheck tool, the tool won't find any difference.

Using rust to parse the event output, trying to read \, will result in an error, since the escape sequence is not known. Do you mean to use a literal \,, which leads to activelayoutv2>>wl_keyboard\,English (US), or escape the comma?

[vaxerski]

you need to put \\, in the source

[MahouShoujoMivutilde]

Sure, you're the boss anyway.

It will still break the API if you had keyboard with a comma defined and now event has keyboard-(something\,-international),Layout(with\, commas).

I personally think adding v2 is better, regardless of the solution.

@s1syph0s I'm pretty sure he means replace all , with \, in keyboard's name, do the same for layout, and only then join them with ,.

EDIT:

Or not...?

Needing to split by \, instead of , definitely breaks api (because the keyboard now has extra \ at the end if you use old splitting method by ,, so you no longer get the same result).

If you join with literal \,, the same argument about escaping applies, now you just need to care about \, in the keyboard/layout instead of plain , tho

[s1syph0s]

Hi, is there any update to this issue? I'll be happy to adjust the PR

[vaxerski]

well I am still convinced escaping the commas with \ is the way to go.

[s1syph0s]

I created a commit that escapes the comma. Could you review the code?

[vaxerski]

it's the opposite of what we want. We want to escape the commas in the layout name