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

Integrate lefthk #682

Merged
merged 53 commits into from
Sep 10, 2022
Merged

Integrate lefthk #682

merged 53 commits into from
Sep 10, 2022

Conversation

AethanFoot
Copy link
Member

Pushing this as a discussion point. Until we rewrite the config code, do we want to make this a non-braking change? To do this we would need to parse the BaseCommands into external commands that lefthk can execute. This would block any lefthk commands being able to be used until the rewrite (Reload, Kill, Chord, and ExitChord). Meaning that the user won't see any change from this other than being able to build without the hotkey daemon.

This will be a large pr so any feedback and opinions will be much appreciated.

Thanks!

@0323pin
Copy link
Member

0323pin commented Jan 29, 2022

@AethanFoot Would this break current configurations and custom keybindings or, will lefthk read those? Are you guys planning on it to become a bit like bspwm and one could use lefthk with other wms or, will be integrated into leftwm?
I'm a bit curious of the way forward since, I'm running leftwm built from git-HEAD on NetBSD and, I'm considering it for my Void Linux laptop where I'd be using the release version. Will it be possible to have similar configurations on both machines?

As you can see, I'm rather curious about what this means in practice. Would you mind a slightly deeper walk through?
Thanks

@AethanFoot
Copy link
Member Author

So if this is going to be done before the config language change, it will likely not be a breaking change as we would be able to parse the current setup into something lefthk can run (e.g. convert it to leftwm-command ...). With lefthk I took the same approach we want for leftwm, that being the core functionality being a library which others can use to build their own version. So the integration with leftwm will use the core library of lefthk using its own implementation of the config and starting it, etc. This then would allow us to easily place the hotkey daemon shipped with leftwm behind a build flag allowing users to remove the hotkey functionality and use their own hotkey daemon, whether that be sxhkd or lefthk standalone (as the standalone version of lefthk will likely use and build upon more features than the integrated version).

There will be (and currently is, but is in very earlier but usable stages) a standalone implementation of lefthk like sxhkd, which as mentioned will likely implement more functionality of the core library and add more to it.

@0323pin
Copy link
Member

0323pin commented Jan 29, 2022

Thanks for the explanation.

@VuiMuich
Copy link
Member

This would block any lefthk commands being able to be used until the rewrite (Reload, Kill, Chord, and ExitChord). Meaning that the user won't see any change from this other than being able to build without the hotkey daemon.

Would Chords really break compatibility? Not if we don't insist on a default keybind for it, right? Blocking Kill and Reload absolutely makes sense.

@AethanFoot
Copy link
Member Author

We could add chords with toml, it would just take a bit more parsing trickery.

@VuiMuich
Copy link
Member

We could add chords with toml, it would just take a bit more parsing trickery.

Ahh riiight, TOML 🙄
scratch that idea 😂

@VuiMuich
Copy link
Member

VuiMuich commented Feb 2, 2022

Never really have checked what happens if you completely remove some of the default keybinds, will this break parsing?

If so we should add use_external_hotkey_daemon: bool as a config option, and ignore parsing errors for keybinds. Probably should throw a Warnung with leftwm-check like:

WARNING: ignoring checks on keybinds as you set for using an external hot key daemon.

@AethanFoot
Copy link
Member Author

Ok rebasing didn't give the effect I was expecting.

@AethanFoot AethanFoot marked this pull request as ready for review February 7, 2022 14:06
@AethanFoot
Copy link
Member Author

This seems to be mostly working. However I am uncertain how we should handle killing lefthk and if lefhk crashes. Also when killed lefthk seems to be leaving ghost processes, I am unsure what is causing this.

@AethanFoot
Copy link
Member Author

Also to test this you need to completely kill leftwm as there are changes in the binary.

Copy link
Member

@VuiMuich VuiMuich left a comment

Choose a reason for hiding this comment

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

Tiny review round 1:
Just a reminder, that we need to update the link to keysym.rs in the wiki.

leftwm/src/config/mod.rs Outdated Show resolved Hide resolved
leftwm/src/bin/leftwm.rs Outdated Show resolved Hide resolved
@VuiMuich
Copy link
Member

I might have been late to review #704 (all-family weekend 🤷🏼‍♂️), but I have been running on this PR all the last week and haven't found any hiccups or even major problems. (Despite the one thing, that stand-alone lefthk and this PR don't mix well for SotReloading leftwm due to using the same command.pipe file.
🚀

Copy link
Member

@VuiMuich VuiMuich left a comment

Choose a reason for hiding this comment

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

There is a bunch of places, where I stumbled from the simlarity of ̀ leftwmandlefthk and had to read twico to catch the differnce. This might be just my tired eyes and brains, but maybe we could also avoid some confusion for newcomers if we generalise it as much as possible to ̀ hotkeydaemon or something along that.

Did I get this right that modmask_lookud.rs is the only keyboard related tie into ̀ xlib`? Could we feasibly use something display server independant here, already or should we keep this for later, when making the core display server agnostic?

All in all thouhg, another amazing 🪄 Wizzard Aethan PR 😀

leftwm/src/bin/leftwm.rs Outdated Show resolved Hide resolved
leftwm/src/bin/leftwm.rs Outdated Show resolved Hide resolved
leftwm-core/src/utils/command_pipe.rs Outdated Show resolved Hide resolved
@0323pin
Copy link
Member

0323pin commented Sep 6, 2022

Actually, something is wrong. I can not cycle through open windows with the keybindings 😢
Built from 6f5ead6
It works if all the windows are terminals but, not if one of them is a GUI app, with the exception of firefox.
The bar says I'm cycling through the open widows but, it doesn't focus the new window. Moreover, if I close the window it doesn't go away until I click another one. I can also focus the window by clicking on it but, not with the keybindings.


Edit by VuiMuich: omit config codeblock, as turned out to be not relevant.

@VuiMuich
Copy link
Member

VuiMuich commented Sep 6, 2022

Ooooh, good catch. I can reproduce. This affects focus and moving windows.
Also managed to get the tiled windows to an unmapped state so the were unfocussable with Driven focus mode and scratchpad window rendering behind them (we already had this issue some time ago, maybe it got 'refactored' in recently?).

@0323pin can you please test if reverting to a0a4e58? I'll have look at this tomorrow. Tbh I suspect more some side effects from some refactors that snuck in unnoticed then it being actually the fault of lefthk.

@0323pin
Copy link
Member

0323pin commented Sep 6, 2022

@VuiMuich I can try tomorrow, already very late here and I need to get up in 5 hours.
It was not present in 451f686

EDIT: Ops, tomorrow I'm away the whole day. Thursday if you don't find it before.

@AethanFoot
Copy link
Member Author

AethanFoot commented Sep 7, 2022

This seems to only happen in ClickTo and Driven, and appears to be a lack of update rather than it not happening. Also picom doesn't start when reloading but I suspect that is because we moved the up scripts to before the first loop (maybe we are too fast for it).

Edit: Looking over I think it is the EventResponse::Continue causing issues as it is preventing display servers being sent through, which isn't an issue in sloppy as the mouse events are causing us to get through.

@VuiMuich
Copy link
Member

VuiMuich commented Sep 7, 2022

🤔 I thought I put EventResponse::Continue in places, where the original code befor the evevnloob refactor continued out of the loop - but seems like the jumping through function calls messes with the order of things happening I guess.

@AethanFoot
Copy link
Member Author

AethanFoot commented Sep 7, 2022

I think the continues were related to the select macro rather than the loop

@0323pin
Copy link
Member

0323pin commented Sep 7, 2022

I'll try to kick another build when I'm home late this evening. Can't promise I'll manage, though. Else, early on Thursday morning, sorry, on mobile at the moment.

@VuiMuich
Copy link
Member

VuiMuich commented Sep 7, 2022

On my system all running fine now.

@0323pin
Copy link
Member

0323pin commented Sep 7, 2022

@AethanFoot, @VuiMuich All good, everything is working as it should but, ... you knew that already 😉
picom is also working after a soft-reload.
Time to 😴

@AethanFoot AethanFoot merged commit 9a680e1 into leftwm:main Sep 10, 2022
@AethanFoot AethanFoot deleted the implement_lefthk branch September 10, 2022 13:00
@0323pin
Copy link
Member

0323pin commented Sep 10, 2022

@AethanFoot ❤️ 🥳

@AethanFoot
Copy link
Member Author

@0323pin on another note, does zbus still fail to build on netbsd?

@0323pin
Copy link
Member

0323pin commented Sep 10, 2022

@AethanFoot haven't tried in a while. Easiest if I had something to test build. Would you like me to try building it stand-alone?

@AethanFoot
Copy link
Member Author

@0323pin Yeah that would be helpful.

@0323pin
Copy link
Member

0323pin commented Sep 10, 2022

@AethanFoot I'll give it a go after building the new git-head from leftwm and starship latest release.
Back soon!

@0323pin
Copy link
Member

0323pin commented Sep 10, 2022

@AethanFoot Ready to test a build of whatever you have in mind 😄

   Compiling zbus_macros v3.1.0 (/usr/pkgsrc/wip/zbus/work/zbus-zbus-3.1.0/zbus_macros)
   Compiling zvariant v3.6.0 (/usr/pkgsrc/wip/zbus/work/zbus-zbus-3.1.0/zvariant)
   Compiling zbus_names v2.2.0 (/usr/pkgsrc/wip/zbus/work/zbus-zbus-3.1.0/zbus_names)
   Compiling zbus v3.1.0 (/usr/pkgsrc/wip/zbus/work/zbus-zbus-3.1.0/zbus)
   Compiling zbus_polkit v2.0.2 (/usr/pkgsrc/wip/zbus/work/zbus-zbus-3.1.0/zbus_polkit)
   Compiling zbus_xmlgen v2.0.1 (/usr/pkgsrc/wip/zbus/work/zbus-zbus-3.1.0/zbus_xmlgen)
    Finished release [optimized] target(s) in 4m 30s

Honestly, I don't know how this is even possible, I don't have polkit installed on my system.

@0323pin
Copy link
Member

0323pin commented Sep 10, 2022

Oh, and I had to patch zvariant

--- zvariant/Cargo.toml.orig	2022-09-02 17:10:31.000000000 +0000
+++ zvariant/Cargo.toml
@@ -35,7 +35,7 @@ time = { version = "0.3.9", features = [
 doc-comment = "0.3.3"
 serde_json = "1.0"
 serde_repr = "0.1.5"
-glib = { git = "https://github.com/gtk-rs/glib", rev = "c9ee583cea0" }
+glib = "0.10.0"
 rand = "0.8.3"
 criterion = "0.3"

As already mentioned, we don't pull distfiles at compile time, our builds are all done in --offline mode.

@AethanFoot
Copy link
Member Author

AethanFoot commented Sep 10, 2022

I presume it is good now then, I remember before it wasn't compiling (which u mentioned on future plans issue). Just wanted to check as I was gonna dive into us using dbus for things.

Is that zvariant an issue?

@0323pin
Copy link
Member

0323pin commented Sep 10, 2022

I presume it is good now then

Yeah! Go ahead and dump something to build 🤣

Is that zvariant an issue?

It depends. If there's some functionality that is required, yes. They should release a new version, so we can fetch distfiles before compiling. If, it's just for development, no. I don't mind keeping a patch.

@0323pin
Copy link
Member

0323pin commented Sep 10, 2022

Oh, btw.
If you want, we don't need to keep this discussion here. Just drop me a line directly pin@NetBSD.org

@0323pin
Copy link
Member

0323pin commented Sep 10, 2022

@AethanFoot Hmm ... the latest changes should warrant a new release. It's miles ahead of 0.3.0.

We have a commit freeze coming-up soon. Is there a plan for when 0.4.0 should hit the wild? From our side, within the next few days or, on the last week of this month.

@0323pin
Copy link
Member

0323pin commented Sep 18, 2022

@AethanFoot there might still be issues with zbus, from a fellow dev https://gitlab.gnome.org/World/pika-backup/-/issues/269

I know it's not the same. In any case what's wrong with dbus-rs and why is zbus a prefered choice?

@hertg
Copy link
Member

hertg commented Sep 19, 2022

I know it's not the same. In any case what's wrong with dbus-rs and why is zbus a prefered choice?

To my knowledge zbus is completely written in rust whereas dbus-rs is "just" rust bindings to a C library. It also seems to me that going through the C library is less performant, but I can't link any benchmarks to back that up right now. AFAIK dbus-rs is probably more "stable" but zbus is more performant + rust native and it seems to still be well maintained. But maybe @AethanFoot has more info or can correct me if I'm wrong.

@0323pin
Copy link
Member

0323pin commented Sep 19, 2022

@hertg thanks for the feedback. Hopefully, we won't run into trouble with zbus.

@AethanFoot
Copy link
Member Author

Yeah, again I don't mind either or. To add to @hertg points, is that they have some nice abstractions that make the interfaces easy to setup and interact with through code. These abstractions obviously make it a bit easier on me, but have a greater impact for others working on the code, as the easier it is to understand the more can work on it. Their book shows nicely how to do it from a super low level point (not quite as low as the c-bindings), and how the abstractions are built up, https://dbus.pages.freedesktop.org/zbus/introduction.html . I am thinking of testing this with lefthk first which has lower demands than leftwm.

@0323pin
Copy link
Member

0323pin commented Sep 19, 2022

Thanks! Do let me know when there's something to test. Hope not to hit the same issue where the application is unable to find dbus, although it is running, just because it's looking in the wrong place.

@0323pin
Copy link
Member

0323pin commented Sep 20, 2022

@AethanFoot Go ahead and implement zbus. I think I found the reason for my fellow dev issue above after he sent me some debug data.

In case you are interested, here are my thoughts on the debug data. Please do say if I misunderstood it.

Let's see if I can make some sense of my thoughts.
Initially, your output looks just fine an it connects to dbus.

signal time=1663583063.914001 sender=org.freedesktop.DBus -> destination=:1.4 serial=2 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameAcquired
   string ":1.4"
method call time=1663583063.914245 sender=:1.4 -> destination=org.freedesktop.DBus serial=2 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=RequestName
   string "org.gnome.World.PikaBackup"
   uint32 4
signal time=1663583063.914257 sender=org.freedesktop.DBus -> destination=(null destination) serial=6 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameOwnerChanged
   string "org.gnome.World.PikaBackup"
   string ""
   string ":1.4"

But then, it looks like it's expecting a second connection.
After all, that's what dbus is supposed to do, intraprocess communication.

method call time=1663583064.131676 sender=:1.4 -> destination=org.freedesktop.DBus serial=3 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=AddMatch
   string "type='signal',sender='org.freedesktop.DBus',interface='org.freedesktop.DBus',member='NameOwnerChanged',path='/org/freedesktop/DBus',arg0='org.gnome.SessionManager'"
method return time=1663583064.131706 sender=org.freedesktop.DBus -> destination=:1.4 serial=5 reply_serial=3
method call time=1663583064.131787 sender=:1.4 -> destination=org.freedesktop.DBus serial=4 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=GetNameOwner
   string "org.gnome.SessionManager"
   
Here it's lookig for another process to connect from dbus.
In this case is looking for Gnome session.

When it doesn't find it, it goes on and removes the match attempt.

error time=1663583064.131795 sender=org.freedesktop.DBus -> destination=:1.4 error_name=org.freedesktop.DBus.Error.NameHasNoOwner reply_serial=4
   string "Could not get owner of name 'org.gnome.SessionManager': no such name"
method call time=1663583064.132059 sender=:1.4 -> destination=org.freedesktop.DBus serial=5 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=RemoveMatch
   string "type='signal',sender='org.freedesktop.DBus',interface='org.freedesktop.DBus',member='NameOwnerChanged',path='/org/freedesktop/DBus',arg0='org.gnome.SessionManager'"

And goes looking for another option, Xfce session.

method return time=1663583064.132068 sender=org.freedesktop.DBus -> destination=:1.4 serial=7 reply_serial=5
method call time=1663583064.132165 sender=:1.4 -> destination=org.freedesktop.DBus serial=6 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=AddMatch
   string "type='signal',sender='org.freedesktop.DBus',interface='org.freedesktop.DBus',member='NameOwnerChanged',path='/org/freedesktop/DBus',arg0='org.xfce.SessionManager'"
method return time=1663583064.132175 sender=org.freedesktop.DBus -> destination=:1.4 serial=8 reply_serial=6
method call time=1663583064.132290 sender=:1.4 -> destination=org.freedesktop.DBus serial=7 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=GetNameOwner
   string "org.xfce.SessionManager"
   
Which, it also doesn't find
   
error time=1663583064.132300 sender=org.freedesktop.DBus -> destination=:1.4 error_name=org.freedesktop.DBus.Error.NameHasNoOwner reply_serial=7
   string "Could not get owner of name 'org.xfce.SessionManager': no such name"
method call time=1663583064.132527 sender=:1.4 -> destination=org.freedesktop.DBus serial=8 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=RemoveMatch
   string "type='signal',sender='org.freedesktop.DBus',interface='org.freedesktop.DBus',member='NameOwnerChanged',path='/org/freedesktop/DBus',arg0='org.xfce.SessionManager'"

My guess is that after this it goes on and tries to connect to any Desktop,
there are repeated attempt to connect to 'org.freedesktop.portal.Desktop'
Until, it finally gives-up and drops the initial connection to pika-backup.


signal time=1663583064.243915 sender=org.freedesktop.DBus -> destination=(null destination) serial=8 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameOwnerChanged
   string "org.gnome.World.PikaBackup"
   string ":1.4"
   string ""
signal time=1663583064.243973 sender=org.freedesktop.DBus -> destination=:1.4 serial=9 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameLost
   string ":1.4"

In other words, I think zbus is working and the initial connection is established
between pika-backup and dbus.

But, this is only half of what it's needed, it requires a second connection.
When this can not be established, dbus drops the initial one and you get the error
that you have reported on your issue.
Although, I do agree that the error message could be better explained.

If we look at the error message with the above in mind it actually makes sense.

'Failed to create ZBus session connection'

It actually didn't fail to create a connection to zbus, it failed to connect to a session from it.

I start to question if this software is even supposed to run on it's own.
Apparently, it needs a full desktop session running.

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

Successfully merging this pull request may close these issues.

None yet

6 participants