joshgoebel / keyszer

a smart, flexible keymapper for X11 (a fork/reboot of xkeysnail )
Other
69 stars 15 forks source link

(enh) Add environment awareness module #130

Closed RedBearAK closed 1 year ago

RedBearAK commented 1 year ago

@joshgoebel

I'm working on developing the functions necessary (well, largely done, actually) to make information about the environment (distro, desktop, session type) available to all parts of keyszer. It makes sense to me to start adding this into config_api so that the config file or other modules like keycontext can adapt themselves to different circumstances.

I already have this working with keycontext, but the logic is just sitting in keycontext itself, and keycontext needs to adapt not just to the session type (Wayland vs X11) but it needs to know the DE if it's Wayland, to connect to a handler module specific to the DE/Wayland combination.

Since the DE and/or distro info is needed in keycontext, but is also going to be very handy to auto-adapt shortcut behavior in the config file, I figured this should all just be part of the API, in a central location. Rather than repeating any part of this logic in different modules.

Do you have an opposition to adding the necessary components (some lists/dicts, and the functions to call on os.environ and read from /etc/os-release and so forth) to config_api? These would be called once at startup and then the info stored in some "constants" that any other module could draw on during normal operation. It's going to end up being at least a couple of "pages" worth of new code.

If this is OK, point to a spot in config_api where you'd like to see the "Environment" section added.

I'm thinking of putting it right after all the globals are defined, and right before the reset_configuration function. Technically I know it could go anywhere, but since it's about collecting basic information at startup, it seems logical to put it close to the beginning.

joshgoebel commented 1 year ago

I think I'd need to see a specific list. I'm not sure that conditionals need to know the WM or DE... they really just need to known "which window is this"... it would be far better if these differences could be abstracted away (to a list of regex?) - rather than forcing more and more of the configuration to care about those details.

Also, I think a lot of this is "config level" stuff not "key handler level" ... I don't see where individual key processing needs to know if you are on Debian or Ubuntu... rather the config file can learn this (using the existing os functions) and then setup appropriate keymaps for the OS/DE - if different keymaps are indeed needed.

For example much of what Kinto.sh does with comments could be done with just a smart config that probed a few details about the system and then built the appropriate keymaps dynamically. But that would largely be done while building the keycaps (startup time), not at runtime.


I'd step back here and try to first explain the problem rather than proposing a specific solution.

RedBearAK commented 1 year ago

It's not different keymaps that are needed, and I'm not talking about the conditionals. Rather, there are specific shortcuts that need to be enabled or disabled when you change DEs. At the moment you have to re-run the Kinto installer or manually un/comment certain lines if you log into a different DE that requires different mappings. I want to make that happen automatically, just based on the acquired DE info. By wrapping the input combo in a function that checks the DE. I already have this working. By returning "None" for the input combo, it disables that specific mapping if the current DE is not in a list that goes along with the input combo. Like Kinto's current comments, but automatic.

For example much of what Kinto.sh does with comments could be done with just a smart config that probed a few details about the system and then built the appropriate keymaps dynamically. But that would largely be done while building the keymaps (startup time), not at runtime.

Yeah, like that. But different. Not really sure what information you're imagining the dynamic keymaps to be built from. All that's typically necessary is to disable or enable a few different lines that need to be a different mapping for specific DEs. Like what I'm doing.

This may be one of those situations where we're saying the same thing in slightly different ways.

There are also a couple of lines in the Kinto config that are tagged with a distro name (ubuntu, fedora, eos, pop_os), although some or perhaps all are really just referring to the specific DE setup of the default desktop on those distros. Anyway, distro name is one of the easiest things to grab. So why not have it available without the user needing to jump through extra hoops?

To get KeyContext to adjust itself automatically to work with Wayland+GNOME vs X11 (and later maybe Wayland+sway, slight possibility of Wayland+Plasma via qdbus at some point), you have to have both the session type and the DE. It just seems like the most efficient way to make all this info available for any purpose is to put it in config_api.

The user's config could be minimal, or empty. I can put all kinds of things in my Kinto config, but KeyContext can't rely on being able to import that. Yet it needs that info, unless you just want to make the user select the correct window context module manually. That would be a pain. The config would need to be changed every time to user logged into a different session type. Why make the user go through that if we don't have to?

Or, here's an alternative. Keep keycontext simple, with no environment awareness logic. Don't put anything new in config_api. Move that logic into a connector module that detects the environment and then calls on functions specific to each type of environment to get the window info, returning it to keycontext when it calls a more generic function like get_window_context() rather than get_xorg_context().

The user config could import the environment constants/variables from this new module dedicated to the task of getting window context in different ways based on the environment. That seems like a good "central" place to deal with it.

Would that make more sense?

joshgoebel commented 1 year ago

Again KeyContext (in most cases) doesn't need to know about this - and it should be doing as LITTLE as possible. Even the X11 probing we do is already way too heavy which is why there is a TODO to replace it with X telling us which window is on top vs us having to ask all the time.

The config body is where these high level changes should be handled...

basics = keymap()
if ubuntu():
  # basics.addSomeExtraKeys()
else if arch:
  # basics.addSomeExtraKeys()  

And it's easy for someone to write ubuntu as a Python function if they need that - I'm not yet convinced we need to ship a bunch of functions like this. And as I said DE's should hopefully be abstracted to the point where users don't need to care - they just think in terms of window names, etc... so figuring out which DE is active might be a concern, but it's not a USER concern...

I'm not even sure it needs to be an active concern. I think switching back and forth regularly between X11 and Weyland is an edge case and that in that case it might be better to just have a startup script send a signal to Keyser to tell it to "reprobe" the working environment once rather than trying to do that every single time a key is pressed.

joshgoebel commented 1 year ago

Also nothing is stopping you from using the os or sys modules (or your own special functions) in your callbacks - just import them into your config, it's all just Python... you can find out what the OS is whether or not that's built-in to Keyser - you don't need anything special in KeyContext to do that...

RedBearAK commented 1 year ago

Again KeyContext (in most cases) doesn't need to know about this - and it should be doing as LITTLE as possible.

I mean, that's what I just proposed at the end of the last comment. But, I think I made a mistake referring to KeyContext (the class) rather than keytcontext the file/module.

Even the X11 probing we do is already way too heavy which is why there is a TODO to replace it with X telling us which window is on top vs us having to ask all the time.

Yes, I understand the concept that it would be much better to got a notification of the focus change and then only do the re-evaluation of the window attributes when that happens. I'm trying to look into that as well, for both X and the Wayland methods.

I'm not even sure it needs to be an active concern. I think switching back and forth regularly between X11 and Weyland is an edge case and that in that case it might be better to just have a startup script send a signal to Keyser to tell it to "reprobe" the working environment once rather than trying to do that every single time a key is pressed.

I don't know where you got the idea that I'm proposing this would be something that needs to be run more than once. Right now the environment checks I'm testing are sitting outside the KeyContext class, which as far as I understand means they are only run once at startup. Then the context checker (like get_xorg_context) inside KeyContext uses the info stored in the resulting variables to decide which module to talk to, to actually get the attributes. But I was suggesting even that one "if" fork be taken out of KeyContext and have it call a more generic context function from another module that would be like xorg, but expanded to also know about the Wayland methods to get the window attributes. KeyContext then wouldn't need to know anything, it would be up to the new module to know about the environment. By checking it once at startup and putting the info in some "constants".

That's why I keep using the word "constants". They would be dynamically filled in at startup rather than statically defined, but still meant to be only filled in with a real value once, at startup. So the user doesn't have to think about which environment they are in. We are literally flinging around the same concepts for the same reasons.

basics = keymap()
if ubuntu():
  # basics.addSomeExtraKeys()
else if arch:
  # basics.addSomeExtraKeys()  

I would like to see a simple example of how you would use this. I think I know where you're going with this, but I need to see it in use. Right now what I'm doing is more "in-line" you might say.

Again, my understanding with what I'm doing (a function that returns "None" to disable individual mapping lines) and what you're proposing is that they both are things that only happen once at startup, and generate a static output that never gets reevaluated during a single run of the app.

Also nothing is stopping you from using the os or sys modules (or your own special functions) in your callbacks - just import them into your config, it's all just Python... you can find out what the OS is whether or not that's built-in to Keyser - you don't need anything special in KeyContext to do that...

That's literally what I'm doing right now. Both in config, and in keycontext, but outside of KeyContext, with KeyContext making a single decision about whether to talk to the function in xorg or the function in wayland. I only put it in keycontext for testing, since that's where keyszer already "decides" to talk to xorg for window attributes.

The thing is the gathering of info about the DE is not super straightforward. The different DEs and distros make different decisions about what to show in the relevant environment variables, and that variation needs to be funneled down into a limited set of DE keywords that are easier to base decisions on. Like figuring out that the DE is actually GNOME, despite seeing "Ubuntu" in XDG_CURRENT_DESKTOP. I might even need to add some checks of "ps" output to look for things like "gnome-shell". I think the Kinto installer does something like that, to verify the DE.

RedBearAK commented 1 year ago

Have a pretty robust environment module (env.py) as part of the Wayland support branch now.

Should be able to reliably identify X11/Xorg or Wayland+GNOME or future compatible environments under most normal circumstances, without the user needing to resort to manual injection with the API function in #138.

RedBearAK commented 1 year ago

Moved this environment module into my own project.