ipc: add query for mode bindings #5561
Conversation
|
Related #3839 |
Thanks for the link. I'll read up on the discussion and see what the i3 crowd thinks. I didn't mention it above, but for the record I do think there is a compelling reason to support this in sway even if it is rejected in i3. Only sway is actually capable of listing all configured global keybinds. i3 could never do this because of the nature of X11 – applications can insert their own global keybinds whenever they want without consulting i3, but that cannot happen in Wayland. Any application that wants a global keybind in sway would need to create it through the sway ipc, and it would be listed by this query. EDIT: From the discussions it seems i3 also doesn't support keybinds added at runtime, which reduces the utility of this query in i3 compared to sway. |
|
Alright here goes. After looking into the discussions linked in that thread, I think it's worth considering adding this feature to sway without adding it in i3, so I'll try to make that case here. Though they seemed open to the idea, the arguments that resulted in rejecting the feature in i3 appear to be:
Truthfully, I think if it were as simple to implement in i3 as it is in sway, I'd still say it's a worthwhile feature to add. But assuming that's not reason enough, I want to list differences between sway and i3 that are in favor of this addition.
The following creates a binding in sway, but errors in i3 Related to redefinition, the following also creates a binding in sway, but errors in i3: They're tiny differences, but the point is a client that wants to be perfectly compatible with both i3 and sway might already have to make code changes. With an ipc query yes that's a totally different code path but at least they can only worry about parsing one config syntax. In the case of sway, the ipc will have done all the work for them, and done it correctly all at little code complexity cost to sway. Comparatively, I have no idea what it would take to implement this in i3, so I don't know how that factored into their decision.
AFAICT, Reasons against adding this in sway are:
Which I understand is compelling, but for the reasons above, sway isn't i3. I could first try to get this feature in i3. However, I'm not inclined to bother a project I don't use, for a feature they don't need, that I probably won't implement, and that they've already rejected once. If i3 ever decides to add this feature down the line, I've tried to pick an implementation that, if it ends up not matching i3's implementation, is at least less trouble to deal with than a new ipc type. |
|
I'm personally not opposed to this functionality (and wasn't when the original PR was submitted last year either). Since @ddevault was the one to NACK this last time, he's the one you'll have to convince. Any change in stance on this with the new arguments? |
|
The third reason is because it's complex and not necessarily the right way to go about implementing the desired downstream feature. So far the only use-case which has been presented is some kind of UI to display the key bindings to the user, which is not very compelling. |
Do you mean a third reason it was rejected in i3 or a third reason it should not be implemented in sway? I'm not sure I agree. From the prior discussion I think their conclusion, and mine, is that i3 does not need this feature. The best way to implement a feature you don't need is of course to not implement it at all. I tried to describe why I think that's not true of sway in my post above. As for complexity, I take it you mean it's complex to extend the ipc, since the code here is pretty simple. I've mostly just moved around what was already there. That may be true. I only see it being complex in the case the i3 extends GET_BINDING_MODES in an incompatible way. If they introduce an equivalent feature with a new ipc type, we could implement that as well. At this time, I don't think either event is likely to happen in i3, and neither is so bad it couldn't be corrected. Realistically, if users find value in this feature, and i3 users end up wanting it in i3 after all, I imagine they would implement it whatever way sway does.
You don't think so? There are existing clients that do basically this though, such as the one I linked in the pull description. When i3 rejected this feature my understanding wasn't that they felt these clients weren't worthwhile, but that they are already adequately supported by i3, and therefore would not benefit from a new ipc query. Again, I tried to explain my reasoning of why that's not true of sway above. I understand you probably aren't interested in using such clients. Personally, I don't see myself using one either – I want to use this in my zsh completions as I said. From looking at the discussion surrounding those projects I get the impression most users like them as learning tools. I get that. Not every sway users was an i3 user for years prior. I wasn't. The way I see it i3 supports these clients and sway does not, and that's something worth fixing. |
|
In my comment, "downstream" is referring to your use-case, not to i3. Sorry for the confusion. For example: should this really be done with sway's IPC at all, or is a Wayland protocol better? In either case, is the use-case strong enough to be worth it? I think a better strategy would be to generate a reference image for the default keybindings. This is what I used to learn i3. Hell, you could make it your wallpaper. |
Oh, my bad. I don't mean to seriously propose changes just to support zsh completions for swaymsg. The points I listed originally were all meant to be a rebuttal in some form to comments I saw in the i3 discussion when applied to sway. They seemed particularly concerned that no one actually wanted to use the proposed query right now. My emphasis on binding-presentation type clients is also based on the fact that those are the type of clients that exist right now. IMO the interesting takeaway for sway is more that the sway ipc already does a good job of completely covering the running state of a sway session, with this notable exception. I understand your reservation in modifying the ipc, but I hope we're on the same page that this information should be made available to clients somehow. If binding-presentation or binding-manager type clients are not compelling, then perhaps a session-save/restore type client? Such clients exist for i3, and i3 additionally supports a type of layout restore feature for them. If an external client wanted to implement a session save/restore type feature for sway, they would need something like this query to save/restore mode bindings. I can also see it being useful to write your current bindings out to a file. Maybe tweak them until they feel right with the ipc, then save them somewhere.
If we ever allow this information to be extracted from sway, and I think we should, I believe an ipc query is the obvious choice. Right now in the sway ipc there are queries like get_tree from i3, and also new queries like get_inputs that exist to expose state that has no analogue in i3. If you don't mind my asking, why did you choose an ipc query for get_inputs instead of a Wayland protocol? There are already related structures for each input device defined in the core Wayland protocol so at first glance it seems like it might fit. I think bindings are less related to anything currently communicated through Wayland. I didn't originally consider a Wayland protocol because AFAIK the concept of a binding mode is fairly specific to sway/i3. I don't know of other desktops that are modal in this way (tbf I haven't tried all that many), so I suspect a protocol extension would never be supported by other Wayland desktops, even wlroots based ones. It doesn't seem right to introduce these ideas into wlroots. There is one desktop that might be interested in implementing this feature, and that is i3. The ipc we share with i3 is the i3ipc. I know you said you don't feel they are a compelling use-case, but the existing clients that care about bindings like the binding-presentation type client I originally linked don't know how to speak Wayland, only i3ipc. I considered an ipc query first because sway already has the code to emit a nearly identical ipc message format for the "binding" event. I implemented this in the way that was most obvious to me, and that required the least change: move that code into a function and call it in two places. I intentionally avoided introducing a new command type because I thought that would be undesired. In my view, the argument for get_binding_modes has no other obvious behavior than to return information about those binding modes, which is why I think this solution works. The only other way I see for exposing this information is by adding it into the reply of an existing query. For example get_inputs could also output input-specific bindings in the emitted json object for each input device. I can't think of anywhere that would be suitable to return general mode bindings information though, other than the get_binding_modes query. If i3 had chosen the get_binding_modes output format to emit an object for each mode, like is done for get_binding_state, I would have suggested adding a new key to those. Sadly that is not the case. |
|
I think having a way to visualize all the keybindings is compelling. There is the i3keys project, which supports both Sway and i3: https://github.com/RasmusLindroth/i3keys It works by implementing it's own config file parser in Go. I'm not sure whether the existence of this project should be seen as an endorsement of the proposed PR or as a counter-argument consider it solves one of the primary uses for the extra API. Before I found http://www.keyboard-layout-editor.com/#/gists/120d9e7b4440aa0ec6b010c77b7d7e3b However it's accomplished, a visual representation of the key bindings is a valuable tool! Another piece of related prior art is the Remontoire project, developed by Regolith: https://github.com/regolith-linux/remontoire They take a slightly different approach by requiring a specific comment format next to each keybinding, so a description of each keybinding can be parsed out along side. From reading the source code, it's actually not parsing the binding syyntax, just the comments: https://github.com/regolith-linux/remontoire/blob/master/src/config_parser.vala This project could also work with Sway except that The "regomontoire" approach of parsing "smart comments" is in some ways more compelling as it's the only approach that returns some custom documentation of what the key bindings are doing. An alternate resolution could be to have |
|
A bit more about Remontoire: It can be easily used with a Sway config file that puts all key bindings in a config file. The solution is simply to use it's option to reference a config file directly rather than use the IPC call. Then you can point it at the bindings file instead of the main config file: |
FWIW I think that's a reasonable change, but I don't think it addresses what this pull does in that it does not help reveal the runtime state of keybindings. |
rpigott
force-pushed
the
showsyms
branch
2 times, most recently
from
522395e
to
7cbc7f3
Compare
rpigott
changed the title
|
Do we want this? I'd say I'm still in favor for the reasons above, but if others don't agree this can be closed. |
|
I'm in favor. Note that the PR is considered WIP pending feedback and the docs haven't been updated for it yet. |
|
I personally don't think the added complexity and API surface is worth it. |
|
The Remontoire approach of parsing structured key binding comments is a decent alternative for those who who are primarily concerned with producing a report of all their key bindings. |
I added docs at some point, so they're there. That's why the #lines changed is so high. |
|
Another use-case that wouldn't be covered with the very specific fix to render an image is to generate something like a command palette (with something like wofi) that contains all key bindings and what they do. This would also allow learning/easing into using Sway as well as allow one to easily use their less often used bindings that they don't remember. Some examples of people looking for this (and often people coming up with pretty hacky solutions to parse the config) Obviously without some sort of description/comment field there's no way to present a human-readable description of the action, this might or might not be a deal-breaker for some. |
|
Another tool has been created to view Sway keybindings-- great for people learning a whole set of keybindings at once. That's Ilia. It will not just display the keybindings but also execute them, but only in "some cases". I presume it can can't function reliably now because it works by parsing comments for lack of a proper API. |
So I came across this project and thought there should really just be an ipc way to query all of sway's keybindings. Sway already has all the necessary code to record and jsonify those bindings in order to support existing functionality, so all that's really needed is a new ipc bit to hook it up.
Then projects like the one above could:
In this PR I just moved some code from the IPC_EVENT_BINDING bit into a new function to make it a little easier to share code between that event and the new query. I called the modifiers field "modifiers" for just the new query because the name "event_state_mask" doesn't make sense in that case. It also adds the field "input_device" to the response in both the query and event in order to more completely describe a sway binding. I figure since sway already extends the binding ipc event with new fields it shouldn't be trouble.
I used IPC_GET_BINDING_MODES in analogy to the way
swaymsg -t get_bar_configreturns a list of bars when invoked without a bar name. Nowswaymsg -t get_binding_modes resizewould return all the bindings (the "config") of the "resize" mode. For example,I made this quick PR to ask for thoughts. It does not modify the docs or handle the case where the user queries a non existent mode, so I've marked it WIP. I realize that with any addition to the ipc we should probably first see what the i3 folks think too. I can start the discussion over there if it seems appropriate.